[PATCH v3 1/4] vhost: fix vq use after free on NUMA reallocation

[PATCH v3 1/4] vhost: fix vq use after free on NUMA reallocation

[David Marchand]

translate_ring_addresses (via numa_realloc) may change a virtio device and
virtio queue.
The virtqueue object must be refreshed before accessing the lock.

Fixes: 04c27cb673b9 ("vhost: fix unsafe vring addresses modifications")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/vhost/vhost_user.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index 4ad28bac45..91d40e32fc 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -2596,6 +2596,7 @@ vhost_user_iotlb_msg(struct virtio_net **pdev,
 			if (is_vring_iotlb(dev, vq, imsg)) {
 				rte_spinlock_lock(&vq->access_lock);
 				*pdev = dev = translate_ring_addresses(dev, i);
+				vq = dev->virtqueue[i];
 				rte_spinlock_unlock(&vq->access_lock);
 			}
 		}
-- 
2.36.1

Re: [PATCH v3 1/4] vhost: fix vq use after free on NUMA reallocation

[Maxime Coquelin]

On 7/25/22 22:32, David Marchand wrote:
> translate_ring_addresses (via numa_realloc) may change a virtio device and
> virtio queue.
> The virtqueue object must be refreshed before accessing the lock.
> 
> Fixes: 04c27cb673b9 ("vhost: fix unsafe vring addresses modifications")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>   lib/vhost/vhost_user.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
> index 4ad28bac45..91d40e32fc 100644
> --- a/lib/vhost/vhost_user.c
> +++ b/lib/vhost/vhost_user.c
> @@ -2596,6 +2596,7 @@ vhost_user_iotlb_msg(struct virtio_net **pdev,
>   			if (is_vring_iotlb(dev, vq, imsg)) {
>   				rte_spinlock_lock(&vq->access_lock);
>   				*pdev = dev = translate_ring_addresses(dev, i);
> +				vq = dev->virtqueue[i];
>   				rte_spinlock_unlock(&vq->access_lock);
>   			}
>   		}

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [PATCH v3 1/4] vhost: fix vq use after free on NUMA reallocation

[Maxime Coquelin]

Hi,

On 7/26/22 09:55, Maxime Coquelin wrote:
> 
> 
> On 7/25/22 22:32, David Marchand wrote:
>> translate_ring_addresses (via numa_realloc) may change a virtio device 
>> and
>> virtio queue.
>> The virtqueue object must be refreshed before accessing the lock.
>>
>> Fixes: 04c27cb673b9 ("vhost: fix unsafe vring addresses modifications")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>> ---
>>   lib/vhost/vhost_user.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
>> index 4ad28bac45..91d40e32fc 100644
>> --- a/lib/vhost/vhost_user.c
>> +++ b/lib/vhost/vhost_user.c
>> @@ -2596,6 +2596,7 @@ vhost_user_iotlb_msg(struct virtio_net **pdev,
>>               if (is_vring_iotlb(dev, vq, imsg)) {
>>                   rte_spinlock_lock(&vq->access_lock);
>>                   *pdev = dev = translate_ring_addresses(dev, i);
>> +                vq = dev->virtqueue[i];
>>                   rte_spinlock_unlock(&vq->access_lock);
>>               }
>>           }
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Thanks,
> Maxime

The bug this patch is fixing is being reproduced downstream.
It would be great it gets merged in main branch rapidly so that we can
perform the backport.

Chenbo, are you planning a pull request for vhost/virtio in the next few
days? If not, should the main branch maintainer pick this single patch
directly and let the rest of the series more time for reviews?

Thanks,
Maxime

RE: [PATCH v3 1/4] vhost: fix vq use after free on NUMA reallocation

[Xia, Chenbo]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, September 13, 2022 11:03 PM
> To: David Marchand <david.marchand@redhat.com>; Xia, Chenbo
> <chenbo.xia@intel.com>; Thomas Monjalon <thomas@monjalon.net>
> Cc: stable@dpdk.org; dev@dpdk.org
> Subject: Re: [PATCH v3 1/4] vhost: fix vq use after free on NUMA
> reallocation
> 
> Hi,
> 
> On 7/26/22 09:55, Maxime Coquelin wrote:
> >
> >
> > On 7/25/22 22:32, David Marchand wrote:
> >> translate_ring_addresses (via numa_realloc) may change a virtio device
> >> and
> >> virtio queue.
> >> The virtqueue object must be refreshed before accessing the lock.
> >>
> >> Fixes: 04c27cb673b9 ("vhost: fix unsafe vring addresses modifications")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: David Marchand <david.marchand@redhat.com>
> >> ---
> >>   lib/vhost/vhost_user.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
> >> index 4ad28bac45..91d40e32fc 100644
> >> --- a/lib/vhost/vhost_user.c
> >> +++ b/lib/vhost/vhost_user.c
> >> @@ -2596,6 +2596,7 @@ vhost_user_iotlb_msg(struct virtio_net **pdev,
> >>               if (is_vring_iotlb(dev, vq, imsg)) {
> >>                   rte_spinlock_lock(&vq->access_lock);
> >>                   *pdev = dev = translate_ring_addresses(dev, i);
> >> +                vq = dev->virtqueue[i];
> >>                   rte_spinlock_unlock(&vq->access_lock);
> >>               }
> >>           }
> >
> > Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >
> > Thanks,
> > Maxime
> 
> The bug this patch is fixing is being reproduced downstream.
> It would be great it gets merged in main branch rapidly so that we can
> perform the backport.
> 
> Chenbo, are you planning a pull request for vhost/virtio in the next few
> days? If not, should the main branch maintainer pick this single patch
> directly and let the rest of the series more time for reviews?

Based on the status of all patches in the list, I guess PR will not happen
this week. So it will be good if David/Thomas can directly pick up this.

Thanks,
Chenbo

> 
> Thanks,
> Maxime

Re: [PATCH v3 1/4] vhost: fix vq use after free on NUMA reallocation

[Maxime Coquelin]

Hi Chenbo,

On 9/14/22 03:05, Xia, Chenbo wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Tuesday, September 13, 2022 11:03 PM
>> To: David Marchand <david.marchand@redhat.com>; Xia, Chenbo
>> <chenbo.xia@intel.com>; Thomas Monjalon <thomas@monjalon.net>
>> Cc: stable@dpdk.org; dev@dpdk.org
>> Subject: Re: [PATCH v3 1/4] vhost: fix vq use after free on NUMA
>> reallocation
>>
>> Hi,
>>
>> On 7/26/22 09:55, Maxime Coquelin wrote:
>>>
>>>
>>> On 7/25/22 22:32, David Marchand wrote:
>>>> translate_ring_addresses (via numa_realloc) may change a virtio device
>>>> and
>>>> virtio queue.
>>>> The virtqueue object must be refreshed before accessing the lock.
>>>>
>>>> Fixes: 04c27cb673b9 ("vhost: fix unsafe vring addresses modifications")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>>> ---
>>>>    lib/vhost/vhost_user.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
>>>> index 4ad28bac45..91d40e32fc 100644
>>>> --- a/lib/vhost/vhost_user.c
>>>> +++ b/lib/vhost/vhost_user.c
>>>> @@ -2596,6 +2596,7 @@ vhost_user_iotlb_msg(struct virtio_net **pdev,
>>>>                if (is_vring_iotlb(dev, vq, imsg)) {
>>>>                    rte_spinlock_lock(&vq->access_lock);
>>>>                    *pdev = dev = translate_ring_addresses(dev, i);
>>>> +                vq = dev->virtqueue[i];
>>>>                    rte_spinlock_unlock(&vq->access_lock);
>>>>                }
>>>>            }
>>>
>>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>
>>> Thanks,
>>> Maxime
>>
>> The bug this patch is fixing is being reproduced downstream.
>> It would be great it gets merged in main branch rapidly so that we can
>> perform the backport.
>>
>> Chenbo, are you planning a pull request for vhost/virtio in the next few
>> days? If not, should the main branch maintainer pick this single patch
>> directly and let the rest of the series more time for reviews?
> 
> Based on the status of all patches in the list, I guess PR will not happen
> this week. So it will be good if David/Thomas can directly pick up this.

OK, sounds good to me.

Thomas/David, is that good on your side?

Thanks,
Maxime

> Thanks,
> Chenbo
> 
>>
>> Thanks,
>> Maxime
> 

Re: [PATCH v3 1/4] vhost: fix vq use after free on NUMA reallocation

[Thomas Monjalon]

14/09/2022 09:14, Maxime Coquelin:
> On 9/14/22 03:05, Xia, Chenbo wrote:
> > From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> On 7/26/22 09:55, Maxime Coquelin wrote:
> >> The bug this patch is fixing is being reproduced downstream.
> >> It would be great it gets merged in main branch rapidly so that we can
> >> perform the backport.
> >>
> >> Chenbo, are you planning a pull request for vhost/virtio in the next few
> >> days? If not, should the main branch maintainer pick this single patch
> >> directly and let the rest of the series more time for reviews?
> > 
> > Based on the status of all patches in the list, I guess PR will not happen
> > this week. So it will be good if David/Thomas can directly pick up this.
> 
> OK, sounds good to me.
> 
> Thomas/David, is that good on your side?

Is there a blocker to merge the whole series?

Re: [PATCH v3 1/4] vhost: fix vq use after free on NUMA reallocation

[Maxime Coquelin]

On 9/14/22 11:15, Thomas Monjalon wrote:
> 14/09/2022 09:14, Maxime Coquelin:
>> On 9/14/22 03:05, Xia, Chenbo wrote:
>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> On 7/26/22 09:55, Maxime Coquelin wrote:
>>>> The bug this patch is fixing is being reproduced downstream.
>>>> It would be great it gets merged in main branch rapidly so that we can
>>>> perform the backport.
>>>>
>>>> Chenbo, are you planning a pull request for vhost/virtio in the next few
>>>> days? If not, should the main branch maintainer pick this single patch
>>>> directly and let the rest of the series more time for reviews?
>>>
>>> Based on the status of all patches in the list, I guess PR will not happen
>>> this week. So it will be good if David/Thomas can directly pick up this.
>>
>> OK, sounds good to me.
>>
>> Thomas/David, is that good on your side?
> 
> Is there a blocker to merge the whole series?

No, I reviewed the whole series.

But other patches are an enhancement, so it would not hurt to have more
reviews.

I am fine either way.

Thanks,
Maxime

Re: [PATCH v3 1/4] vhost: fix vq use after free on NUMA reallocation

[Thomas Monjalon]

14/09/2022 11:34, Maxime Coquelin:
> 
> On 9/14/22 11:15, Thomas Monjalon wrote:
> > 14/09/2022 09:14, Maxime Coquelin:
> >> On 9/14/22 03:05, Xia, Chenbo wrote:
> >>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>> On 7/26/22 09:55, Maxime Coquelin wrote:
> >>>> The bug this patch is fixing is being reproduced downstream.
> >>>> It would be great it gets merged in main branch rapidly so that we can
> >>>> perform the backport.
> >>>>
> >>>> Chenbo, are you planning a pull request for vhost/virtio in the next few
> >>>> days? If not, should the main branch maintainer pick this single patch
> >>>> directly and let the rest of the series more time for reviews?
> >>>
> >>> Based on the status of all patches in the list, I guess PR will not happen
> >>> this week. So it will be good if David/Thomas can directly pick up this.
> >>
> >> OK, sounds good to me.
> >>
> >> Thomas/David, is that good on your side?
> > 
> > Is there a blocker to merge the whole series?
> 
> No, I reviewed the whole series.
> 
> But other patches are an enhancement, so it would not hurt to have more
> reviews.
> 
> I am fine either way.

I prefer not breaking series.
How urgent is it? Should I merge it today?

Re: [PATCH v3 1/4] vhost: fix vq use after free on NUMA reallocation

[Maxime Coquelin]

On 9/14/22 11:45, Thomas Monjalon wrote:
> 14/09/2022 11:34, Maxime Coquelin:
>>
>> On 9/14/22 11:15, Thomas Monjalon wrote:
>>> 14/09/2022 09:14, Maxime Coquelin:
>>>> On 9/14/22 03:05, Xia, Chenbo wrote:
>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>> On 7/26/22 09:55, Maxime Coquelin wrote:
>>>>>> The bug this patch is fixing is being reproduced downstream.
>>>>>> It would be great it gets merged in main branch rapidly so that we can
>>>>>> perform the backport.
>>>>>>
>>>>>> Chenbo, are you planning a pull request for vhost/virtio in the next few
>>>>>> days? If not, should the main branch maintainer pick this single patch
>>>>>> directly and let the rest of the series more time for reviews?
>>>>>
>>>>> Based on the status of all patches in the list, I guess PR will not happen
>>>>> this week. So it will be good if David/Thomas can directly pick up this.
>>>>
>>>> OK, sounds good to me.
>>>>
>>>> Thomas/David, is that good on your side?
>>>
>>> Is there a blocker to merge the whole series?
>>
>> No, I reviewed the whole series.
>>
>> But other patches are an enhancement, so it would not hurt to have more
>> reviews.
>>
>> I am fine either way.
> 
> I prefer not breaking series.
> How urgent is it? Should I merge it today?
> 
> 

It causes failures in some of our QE tests, so it is urgent for us.
If you prefer not breaking the series, I am fine if you take it all.

Thanks,
Maxime