Re: [PATCH] vsock/virtio: prevent workers from using deleted virtqueues

[Weiming Shi]

Stefano Garzarella <sgarzare@xxxxxxxxxx> 于2026年7月29日周三 22:47写道：
>
> On Tue, Jul 28, 2026 at 10:17:55AM -0700, Bobby Eshleman wrote:
> >On Sun, Jul 26, 2026 at 08:58:01PM -0700, Weiming Shi wrote:
> >> The RX, TX and event workers read their virtqueue pointers before taking
> >> the mutex that protects the queue and its run flag. A work item delayed
> >> across freeze and restore can therefore retain a pointer deleted by
> >> virtio_vsock_vqs_del(), observe the run flag for the replacement queues,
> >> and use the freed pointer.
> >>
> >> RX has an additional path: when rx_run is clear, the common exit still
> >> refills the RX queue. A queued worker can consequently call
> >> virtqueue_add_sgs() immediately after freeze deletes the virtqueues.
> >>
> >> BUG: KASAN: slab-use-after-free in virtqueue_add_sgs
> >> Read of size 4 by task kworker/2:1
> >> Workqueue: virtio_vsock virtio_transport_rx_work
> >> Call Trace:
> >>  virtqueue_add_sgs (drivers/virtio/virtio_ring.c:2796)
> >>  virtio_vsock_rx_fill (net/vmw_vsock/virtio_transport.c:332)
> >>  virtio_transport_rx_work (net/vmw_vsock/virtio_transport.c:701)
> >>  process_one_work (kernel/workqueue.c:3314)
> >>  worker_thread (kernel/workqueue.c:3478)
> >>  kthread (kernel/kthread.c:436)
> >>  ret_from_fork (arch/x86/kernel/process.c:158)
> >>  ret_from_fork_asm (arch/x86/entry/entry_64.S:245)
> >> ...
> >> Freed by task 141:
> >>  kfree (mm/slub.c:6566)
> >>  vp_del_vq (drivers/virtio/virtio_pci_common.c:259)
> >>  vp_del_vqs (drivers/virtio/virtio_pci_common.c:285)
> >>  virtio_vsock_freeze (net/vmw_vsock/virtio_transport.c:912)
> >>  virtio_device_freeze (drivers/virtio/virtio.c:658)
> >>  virtio_pci_freeze (drivers/virtio/virtio_pci_common.c:601)
> >>  pci_pm_freeze (drivers/pci/pci-driver.c:1098)
> >>  device_suspend (drivers/base/power/main.c:1968)
> >> Kernel panic - not syncing: KASAN: panic_on_warn set ...
> >>
> >> Read each worker's virtqueue under its mutex after confirming that the
> >> queue is running, and only refill RX while RX is running.
> >>
> >> Fixes: b917507e5ad9 ("vsock/virtio: stop workers during the .remove()")
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> Reported-by: Xiang Mei <xmei5@xxxxxxx>
> >> Assisted-by: OpenAI-Codex:gpt-5
> >> Signed-off-by: Weiming Shi <bestswngs@xxxxxxxxx>
> >> ---
> >>  net/vmw_vsock/virtio_transport.c | 14 ++++++++------
> >>  1 file changed, 8 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> >> index 57f2d6ec3ffc..79cf19f58943 100644
> >> --- a/net/vmw_vsock/virtio_transport.c
> >> +++ b/net/vmw_vsock/virtio_transport.c
> >> @@ -346,12 +346,13 @@ static void virtio_transport_tx_work(struct work_struct *work)
> >>      struct virtqueue *vq;
> >>      bool added = false;
> >>
> >> -    vq = vsock->vqs[VSOCK_VQ_TX];
> >>      mutex_lock(&vsock->tx_lock);
> >>
> >>      if (!vsock->tx_run)
> >>              goto out;
> >>
> >> +    vq = vsock->vqs[VSOCK_VQ_TX];
> >> +
> >>      do {
> >>              struct sk_buff *skb;
> >>              unsigned int len;
> >> @@ -451,13 +452,13 @@ static void virtio_transport_event_work(struct work_struct *work)
> >>              container_of(work, struct virtio_vsock, event_work);
> >>      struct virtqueue *vq;
> >>
> >> -    vq = vsock->vqs[VSOCK_VQ_EVENT];
> >> -
> >>      mutex_lock(&vsock->event_lock);
> >>
> >>      if (!vsock->event_run)
> >>              goto out;
> >>
> >> +    vq = vsock->vqs[VSOCK_VQ_EVENT];
> >> +
> >>      do {
> >>              struct virtio_vsock_event *event;
> >>              unsigned int len;
> >> @@ -634,13 +635,13 @@ static void virtio_transport_rx_work(struct work_struct *work)
> >>              container_of(work, struct virtio_vsock, rx_work);
> >>      struct virtqueue *vq;
> >>
> >> -    vq = vsock->vqs[VSOCK_VQ_RX];
> >> -
> >>      mutex_lock(&vsock->rx_lock);
> >>
> >>      if (!vsock->rx_run)
> >>              goto out;
> >>
> >> +    vq = vsock->vqs[VSOCK_VQ_RX];
> >> +
> >>      do {
> >>              virtqueue_disable_cb(vq);
> >>              for (;;) {
> >> @@ -689,7 +690,8 @@ static void virtio_transport_rx_work(struct work_struct *work)
> >>      } while (!virtqueue_enable_cb(vq));
> >>
> >>  out:
> >> -    if (vsock->rx_buf_nr < vsock->rx_buf_max_nr / 2)
> >> +    if (vsock->rx_run &&
> >> +        vsock->rx_buf_nr < vsock->rx_buf_max_nr / 2)
> >>              virtio_vsock_rx_fill(vsock);
> >>      mutex_unlock(&vsock->rx_lock);
> >>  }
> >> --
> >> 2.55.0
> >
> >Not a strong opinion from me, but since this last hunk is the one that
> >fixes the bug and the other hunks are moreso hardening, maybe break
> >these out into two patches?
>
> I think also the other hunks fix an issue, but I agree on the split
> since IMO we are fixing 2 different commits. Commit b917507e5ad9
> ("vsock/virtio: stop workers during the .remove()") was before
> freeze/resume added by commit bd50c5dc182b ("vsock/virtio: add support
> for device suspend/resume"). Only after that one we can have the false
> -> true transition of *_run variables.
>
> So IMO hunks 1-3 should have Fixes: bd50c5dc182b ... and hunk 4 should
> have Fixes: b917507e5ad9 ...
>
> That said, it's not a strong opinion here too, but if you prefer a
> single patch, please add both Fixes.
>
> Thanks,
> Stefano
>
> >
> >Besides, that all looks good to me.
> >
> >Reviewed-by: Bobby Eshleman <bobbyeshleman@xxxxxxxx>
> >
>

Thanks for your review. v2 sent.

Best,
Weiming Shi