Skip to content

Commit d8ee3cf

Browse files
stefano-garzarellamstsirkin
authored andcommitted
vhost/vsock: improve RCU read sections around vhost_vsock_get()
vhost_vsock_get() uses hash_for_each_possible_rcu() to find the `vhost_vsock` associated with the `guest_cid`. hash_for_each_possible_rcu() should only be called within an RCU read section, as mentioned in the following comment in include/linux/rculist.h: /** * hlist_for_each_entry_rcu - iterate over rcu list of given type * @pos: the type * to use as a loop cursor. * @Head: the head for your list. * @member: the name of the hlist_node within the struct. * @cond: optional lockdep expression if called from non-RCU protection. * * This list-traversal primitive may safely run concurrently with * the _rcu list-mutation primitives such as hlist_add_head_rcu() * as long as the traversal is guarded by rcu_read_lock(). */ Currently, all calls to vhost_vsock_get() are between rcu_read_lock() and rcu_read_unlock() except for calls in vhost_vsock_set_cid() and vhost_vsock_reset_orphans(). In both cases, the current code is safe, but we can make improvements to make it more robust. About vhost_vsock_set_cid(), when building the kernel with CONFIG_PROVE_RCU_LIST enabled, we get the following RCU warning when the user space issues `ioctl(dev, VHOST_VSOCK_SET_GUEST_CID, ...)` : WARNING: suspicious RCU usage 6.18.0-rc7 torvalds#62 Not tainted ----------------------------- drivers/vhost/vsock.c:74 RCU-list traversed in non-reader section!! other info that might help us debug this: rcu_scheduler_active = 2, debug_locks = 1 1 lock held by rpc-libvirtd/3443: #0: ffffffffc05032a8 (vhost_vsock_mutex){+.+.}-{4:4}, at: vhost_vsock_dev_ioctl+0x2ff/0x530 [vhost_vsock] stack backtrace: CPU: 2 UID: 0 PID: 3443 Comm: rpc-libvirtd Not tainted 6.18.0-rc7 torvalds#62 PREEMPT(none) Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.17.0-7.fc42 06/10/2025 Call Trace: <TASK> dump_stack_lvl+0x75/0xb0 dump_stack+0x14/0x1a lockdep_rcu_suspicious.cold+0x4e/0x97 vhost_vsock_get+0x8f/0xa0 [vhost_vsock] vhost_vsock_dev_ioctl+0x307/0x530 [vhost_vsock] __x64_sys_ioctl+0x4f2/0xa00 x64_sys_call+0xed0/0x1da0 do_syscall_64+0x73/0xfa0 entry_SYSCALL_64_after_hwframe+0x76/0x7e ... </TASK> This is not a real problem, because the vhost_vsock_get() caller, i.e. vhost_vsock_set_cid(), holds the `vhost_vsock_mutex` used by the hash table writers. Anyway, to prevent that warning, add lockdep_is_held() condition to hash_for_each_possible_rcu() to verify that either the caller is in an RCU read section or `vhost_vsock_mutex` is held when CONFIG_PROVE_RCU_LIST is enabled; and also clarify the comment for vhost_vsock_get() to better describe the locking requirements and the scope of the returned pointer validity. About vhost_vsock_reset_orphans(), currently this function is only called via vsock_for_each_connected_socket(), which holds the `vsock_table_lock` spinlock (which is also an RCU read-side critical section). However, add an explicit RCU read lock there to make the code more robust and explicit about the RCU requirements, and to prevent issues if the calling context changes in the future or if vhost_vsock_reset_orphans() is called from other contexts. Fixes: 834e772 ("vhost/vsock: fix use-after-free in network stack callers") Cc: stefanha@redhat.com Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20251126133826.142496-1-sgarzare@redhat.com> Message-ID: <20251126210313.GA499503@fedora> Acked-by: Jason Wang <jasowang@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
1 parent 7f81878 commit d8ee3cf

File tree

1 file changed

+11
-4
lines changed

1 file changed

+11
-4
lines changed

drivers/vhost/vsock.c

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -66,14 +66,15 @@ static u32 vhost_transport_get_local_cid(void)
6666
return VHOST_VSOCK_DEFAULT_HOST_CID;
6767
}
6868

69-
/* Callers that dereference the return value must hold vhost_vsock_mutex or the
70-
* RCU read lock.
69+
/* Callers must be in an RCU read section or hold the vhost_vsock_mutex.
70+
* The return value can only be dereferenced while within the section.
7171
*/
7272
static struct vhost_vsock *vhost_vsock_get(u32 guest_cid)
7373
{
7474
struct vhost_vsock *vsock;
7575

76-
hash_for_each_possible_rcu(vhost_vsock_hash, vsock, hash, guest_cid) {
76+
hash_for_each_possible_rcu(vhost_vsock_hash, vsock, hash, guest_cid,
77+
lockdep_is_held(&vhost_vsock_mutex)) {
7778
u32 other_cid = vsock->guest_cid;
7879

7980
/* Skip instances that have no CID yet */
@@ -709,9 +710,15 @@ static void vhost_vsock_reset_orphans(struct sock *sk)
709710
* executing.
710711
*/
711712

713+
rcu_read_lock();
714+
712715
/* If the peer is still valid, no need to reset connection */
713-
if (vhost_vsock_get(vsk->remote_addr.svm_cid))
716+
if (vhost_vsock_get(vsk->remote_addr.svm_cid)) {
717+
rcu_read_unlock();
714718
return;
719+
}
720+
721+
rcu_read_unlock();
715722

716723
/* If the close timeout is pending, let it expire. This avoids races
717724
* with the timeout callback.

0 commit comments

Comments
 (0)