Re: [PATCH] vhost: fix read vs write lock mismatch

[David Marchand <david.marchand@redhat.com>]

On Mon, Nov 25, 2024 at 5:20 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> On 11/25/24 12:14, David Marchand wrote:
> > On Mon, Nov 18, 2024 at 5:24 PM Stephen Hemminger
> > <stephen@networkplumber.org> wrote:
> >>
> >> If lock is acquired for write, it must be released for write
> >> or a deadlock is likely.
> >>
> >> Bugzilla ID: 1582
> >> Fixes: 9fc93a1e2320 ("vhost: fix virtqueue access check in datapath")
> >> Cc: david.marchand@redhat.com
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >> ---
> >>   lib/vhost/virtio_net.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> >> index 298a5dae74..d764d4bc6a 100644
> >> --- a/lib/vhost/virtio_net.c
> >> +++ b/lib/vhost/virtio_net.c
> >> @@ -2538,7 +2538,7 @@ virtio_dev_rx_async_submit(struct virtio_net *dev, struct vhost_virtqueue *vq,
> >>
> >>          if (unlikely(!vq->access_ok)) {
> >>                  vhost_user_iotlb_rd_unlock(vq);
> >> -               rte_rwlock_read_unlock(&vq->access_lock);
> >> +               rte_rwlock_write_unlock(&vq->access_lock);
> >
> > A write lock is taken earlier, because virtio_dev_rx_async_submit_*
> > need it for access to vq->async (as opposed to the sync code that only
> > takes read lock).
> >
> > Here, no need to release/take again all locks.
> > A simpler fix is to directly call vring_translate(dev, vq).
> >
> >
>
> Ok, so both solutions are correct.
>
> David's one is more optimized, but this is a corner case in the async
> datapath so not really critical.
>
> On the other hand, Stephen's solution keeps the same pattern as the
> other datapaths.

Ok, it avoids having a special case, and I prefer limiting special cases too.

I'll send a RFC for new lock annotations (for catching such bug) for
the next release.


>
> I'd go with Stephen's solution, but would change the commit title to:
>
> vhost: fix deadlock in Rx async path
>
> With this change:
>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Yes, the title needed some work.


-- 
David Marchand