[PATCH] vhost: fix read vs write lock mismatch

Maxime Coquelin maxime.coquelin at redhat.com
Mon Nov 25 17:20:10 CET 2024



On 11/25/24 12:14, David Marchand wrote:
> On Mon, Nov 18, 2024 at 5:24 PM Stephen Hemminger
> <stephen at 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 at redhat.com
>> Cc: stable at dpdk.org
>>
>> Signed-off-by: Stephen Hemminger <stephen at 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.

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 at redhat.com>

Thanks,
Maxime



More information about the stable mailing list