lkml.org 
[lkml]   [2024]   [Mar]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 1/2] vhost: Add smp_rmb() in vhost_vq_avail_empty()
    On Wed, Mar 27, 2024 at 09:38:45AM +1000, Gavin Shan wrote:
    > A smp_rmb() has been missed in vhost_vq_avail_empty(), spotted by
    > Will Deacon <will@kernel.org>. Otherwise, it's not ensured the
    > available ring entries pushed by guest can be observed by vhost
    > in time, leading to stale available ring entries fetched by vhost
    > in vhost_get_vq_desc(), as reported by Yihuang Yu on NVidia's
    > grace-hopper (ARM64) platform.
    >
    > /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
    > -accel kvm -machine virt,gic-version=host -cpu host \
    > -smp maxcpus=1,cpus=1,sockets=1,clusters=1,cores=1,threads=1 \
    > -m 4096M,slots=16,maxmem=64G \
    > -object memory-backend-ram,id=mem0,size=4096M \
    > : \
    > -netdev tap,id=vnet0,vhost=true \
    > -device virtio-net-pci,bus=pcie.8,netdev=vnet0,mac=52:54:00:f1:26:b0
    > :
    > guest# netperf -H 10.26.1.81 -l 60 -C -c -t UDP_STREAM
    > virtio_net virtio0: output.0:id 100 is not a head!
    >
    > Add the missed smp_rmb() in vhost_vq_avail_empty(). Note that it
    > should be safe until vq->avail_idx is changed by commit 275bf960ac697
    > ("vhost: better detection of available buffers").
    >
    > Fixes: 275bf960ac697 ("vhost: better detection of available buffers")
    > Cc: <stable@kernel.org> # v4.11+
    > Reported-by: Yihuang Yu <yihyu@redhat.com>
    > Signed-off-by: Gavin Shan <gshan@redhat.com>
    > ---
    > drivers/vhost/vhost.c | 11 ++++++++++-
    > 1 file changed, 10 insertions(+), 1 deletion(-)
    >
    > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
    > index 045f666b4f12..00445ab172b3 100644
    > --- a/drivers/vhost/vhost.c
    > +++ b/drivers/vhost/vhost.c
    > @@ -2799,9 +2799,18 @@ bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq)
    > r = vhost_get_avail_idx(vq, &avail_idx);
    > if (unlikely(r))
    > return false;
    > +
    > vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
    > + if (vq->avail_idx != vq->last_avail_idx) {
    > + /* Similar to what's done in vhost_get_vq_desc(), we need
    > + * to ensure the available ring entries have been exposed
    > + * by guest.
    > + */

    A slightly clearer comment:

    /* Since we have updated avail_idx, the following call to
    * vhost_get_vq_desc will read available ring entries.
    * Make sure that read happens after the avail_idx read.
    */

    Pls repost with that, and I will apply.

    Also add suggested-by for will.


    > + smp_rmb();
    > + return false;
    > + }
    >
    > - return vq->avail_idx == vq->last_avail_idx;
    > + return true;
    > }
    > EXPORT_SYMBOL_GPL(vhost_vq_avail_empty);

    As a follow-up patch, we should clean out code duplication that
    accumulated with 3 places reading avail idx in essentially
    the same way - this duplication is what causes the mess in
    the 1st place.






    > --
    > 2.44.0


    \
     
     \ /
      Last update: 2024-05-27 16:11    [W:6.623 / U:0.068 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site