patches for DPDK stable branches
 help / color / mirror / Atom feed
* [PATCH] vhost: fix read vs write lock mismatch
@ 2024-11-18 16:24 Stephen Hemminger
  2024-11-25 11:14 ` David Marchand
  0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2024-11-18 16:24 UTC (permalink / raw)
  To: dev
  Cc: Stephen Hemminger, david.marchand, stable, Maxime Coquelin,
	Chenbo Xia, Eelco Chaudron

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);
 
 		virtio_dev_vring_translate(dev, vq);
 		goto out_no_unlock;
-- 
2.45.2


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] vhost: fix read vs write lock mismatch
  2024-11-18 16:24 [PATCH] vhost: fix read vs write lock mismatch Stephen Hemminger
@ 2024-11-25 11:14 ` David Marchand
  2024-11-25 16:20   ` Maxime Coquelin
  0 siblings, 1 reply; 6+ messages in thread
From: David Marchand @ 2024-11-25 11:14 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: dev, stable, Maxime Coquelin, Chenbo Xia, Eelco Chaudron

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).


-- 
David Marchand


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] vhost: fix read vs write lock mismatch
  2024-11-25 11:14 ` David Marchand
@ 2024-11-25 16:20   ` Maxime Coquelin
  2024-11-25 16:28     ` David Marchand
  0 siblings, 1 reply; 6+ messages in thread
From: Maxime Coquelin @ 2024-11-25 16:20 UTC (permalink / raw)
  To: David Marchand, Stephen Hemminger; +Cc: dev, stable, Chenbo Xia, Eelco Chaudron



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.

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>

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] vhost: fix read vs write lock mismatch
  2024-11-25 16:20   ` Maxime Coquelin
@ 2024-11-25 16:28     ` David Marchand
  2024-11-25 16:31       ` Maxime Coquelin
  0 siblings, 1 reply; 6+ messages in thread
From: David Marchand @ 2024-11-25 16:28 UTC (permalink / raw)
  To: Maxime Coquelin
  Cc: Stephen Hemminger, dev, stable, Chenbo Xia, Eelco Chaudron

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] vhost: fix read vs write lock mismatch
  2024-11-25 16:28     ` David Marchand
@ 2024-11-25 16:31       ` Maxime Coquelin
  2024-11-25 16:44         ` Stephen Hemminger
  0 siblings, 1 reply; 6+ messages in thread
From: Maxime Coquelin @ 2024-11-25 16:31 UTC (permalink / raw)
  To: David Marchand; +Cc: Stephen Hemminger, dev, stable, Chenbo Xia, Eelco Chaudron



On 11/25/24 17:28, David Marchand wrote:
> 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.

Great, thanks for that!

> 
>>
>> 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.
> 
> 

Ack, will change while applying.

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] vhost: fix read vs write lock mismatch
  2024-11-25 16:31       ` Maxime Coquelin
@ 2024-11-25 16:44         ` Stephen Hemminger
  0 siblings, 0 replies; 6+ messages in thread
From: Stephen Hemminger @ 2024-11-25 16:44 UTC (permalink / raw)
  To: Maxime Coquelin; +Cc: David Marchand, dev, stable, Chenbo Xia, Eelco Chaudron

On Mon, 25 Nov 2024 17:31:26 +0100
Maxime Coquelin <maxime.coquelin@redhat.com> wrote:

> On 11/25/24 17:28, David Marchand wrote:
> > 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.  
> 
> Great, thanks for that!
> 
> >   
> >>
> >> 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.
> > 
> >   
> 
> Ack, will change while applying.

Yes better title would be great.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-11-25 16:44 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-11-18 16:24 [PATCH] vhost: fix read vs write lock mismatch Stephen Hemminger
2024-11-25 11:14 ` David Marchand
2024-11-25 16:20   ` Maxime Coquelin
2024-11-25 16:28     ` David Marchand
2024-11-25 16:31       ` Maxime Coquelin
2024-11-25 16:44         ` Stephen Hemminger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).