Re: [External] Re: [PATCH v2] vduse: Fix race in vduse_dev_msg_sync and vduse_dev_read_iter

[Zhang Tianci <zhangtianci.1997@bytedance.com>]

Hi, Jason,

On Tue, Feb 3, 2026 at 11:27 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Tue, Feb 3, 2026 at 11:23 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Mon, Feb 2, 2026 at 11:13 AM Zhang Tianci
> > <zhangtianci.1997@bytedance.com> wrote:
> > >
> > > There is one race case in vduse_dev_msg_sync and vduse_dev_read_iter:
> > >
> > > vduse_dev_read_iter():
> > >     lock(msg_lock);
> > >     dequeue_msg(send_list);
> > >     unlock(msg_lock);
> > > vduse_dev_msg_sync():
> > >     wait_timeout() finish
> > >     lock(msg_lock);
> > >     check msg->complete is false
> > >         list_del(msg);   <- double list_del() crash!
> > >
> > > To fix this case, we shall ensure vduse_msg is on send_list or recv_list
> > > outside the msg_lock critical section.
> > >
> > > Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Zhang Tianci <zhangtianci.1997@bytedance.com>
> > > Reviewed-by: Xie Yongji <xieyongji@bytedance.com>
> > > ---
> > > v2:
> > >  - Rewrite commit message.                        [Michael]
> > >  - Add Fixes tag and cc stable email list.        [Eugenio]
> > >  - Rewrite one comment.                           [Michael]
> > >
> > > v1: https://lkml.org/lkml/2026/1/30/323
> > >
> > >  drivers/vdpa/vdpa_user/vduse_dev.c | 30 ++++++++++++++++++++++--------
> > >  1 file changed, 22 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > index ae357d014564c..a70d0580d54e8 100644
> > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > @@ -325,6 +325,7 @@ static ssize_t vduse_dev_read_iter(struct kiocb *iocb, struct iov_iter *to)
> > >         struct file *file = iocb->ki_filp;
> > >         struct vduse_dev *dev = file->private_data;
> > >         struct vduse_dev_msg *msg;
> > > +       struct vduse_dev_request req;
> > >         int size = sizeof(struct vduse_dev_request);
> > >         ssize_t ret;
> > >
> > > @@ -339,7 +340,7 @@ static ssize_t vduse_dev_read_iter(struct kiocb *iocb, struct iov_iter *to)
> > >
> > >                 ret = -EAGAIN;
> > >                 if (file->f_flags & O_NONBLOCK)
> > > -                       goto unlock;
> > > +                       break;
> > >
> > >                 spin_unlock(&dev->msg_lock);
> > >                 ret = wait_event_interruptible_exclusive(dev->waitq,
> > > @@ -349,17 +350,30 @@ static ssize_t vduse_dev_read_iter(struct kiocb *iocb, struct iov_iter *to)
> > >
> > >                 spin_lock(&dev->msg_lock);
> > >         }
> > > +       if (!msg) {
> > > +               spin_unlock(&dev->msg_lock);
> > > +               return ret;
> > > +       }
> >
> > Nit: this check seems to be redundant, I'd suggest to
> >
> > 1) move the spin_unlock() before the check of file->f_flags & O_NONBLOCK
> > 2) then we can simply do "return ret" when it's a nonblocking read.
> >
> > > +
> > > +       memcpy(&req, &msg->req, sizeof(req));
> > > +       /*
> > > +        * We must ensure vduse_msg is on send_list or recv_list before unlock
> > > +        * dev->msg_lock. Because vduse_dev_msg_sync() may be timeout when we
> > > +        * copy data to userspace, and will call list_del() for this msg.
> > > +        */
> > > +       vduse_enqueue_msg(&dev->recv_list, msg);
> > >         spin_unlock(&dev->msg_lock);
> > > -       ret = copy_to_iter(&msg->req, size, to);
> > > -       spin_lock(&dev->msg_lock);
> > > +
> > > +       ret = copy_to_iter(&req, size, to);
> > >         if (ret != size) {
>
> Btw, I would like to explain why it's still safe if a (malicious)
> userspace writes in this window in either commit log or here.

Do you mean we should document in a comment here why the potential read/write
race is safe?

Thanks,
Tianci