diff mbox series

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

Message ID 20220725203206.427083-2-david.marchand@redhat.com (mailing list archive)
State Accepted, archived
Delegated to: Maxime Coquelin
Headers
Series vHost IOTLB cache rework |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

David Marchand July 25, 2022, 8:32 p.m. UTC 
  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(+)

Comments

Maxime Coquelin July 26, 2022, 7:55 a.m. UTC | #1 
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
Maxime Coquelin Sept. 13, 2022, 3:02 p.m. UTC | #2 
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
Chenbo Xia Sept. 14, 2022, 1:05 a.m. UTC | #3 
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
Maxime Coquelin Sept. 14, 2022, 7:14 a.m. UTC | #4 
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
>
Thomas Monjalon Sept. 14, 2022, 9:15 a.m. UTC | #5 
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?
Maxime Coquelin Sept. 14, 2022, 9:34 a.m. UTC | #6 
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
Thomas Monjalon Sept. 14, 2022, 9:45 a.m. UTC | #7 
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?
Maxime Coquelin Sept. 14, 2022, 11:48 a.m. UTC | #8 
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
diff mbox series

Patch

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);
 			}
 		}