diff mbox series

[1/7] vhost: fix virtqueues metadata allocation

Message ID 20201019173415.582407-2-maxime.coquelin@redhat.com (mailing list archive)
State Accepted, archived
Delegated to: Maxime Coquelin
Headers
Series vhost: make VQ metadata dereferencing robust |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Maxime Coquelin Oct. 19, 2020, 5:34 p.m. UTC 
  The Vhost-user backend implementation assumes there will be
no holes in the device's array of virtqueues metadata
pointers.

It can happen though, and would cause segmentation faults,
memory leaks or undefined behaviour.

This patch keep the assumption that there is no holes in this
array, and allocate all uninitialized virtqueues metadata up
to requested index.

Fixes: 160cbc815b41 ("vhost: remove a hack on queue allocation")
Cc: stable@dpdk.org

Suggested-by: Adrian Moreno <amorenoz@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

Comments

Chenbo Xia Oct. 21, 2020, 11:10 a.m. UTC | #1 
Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, October 20, 2020 1:34 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [PATCH 1/7] vhost: fix virtqueues metadata allocation
> 
> The Vhost-user backend implementation assumes there will be
> no holes in the device's array of virtqueues metadata
> pointers.
> 
> It can happen though, and would cause segmentation faults,
> memory leaks or undefined behaviour.

Could I ask when will this happen? 

When QEMU does not configure all virtqueues? I'm not very sure.
Could you point that out for me?

Thanks!
Chenbo

> 
> This patch keep the assumption that there is no holes in this
> array, and allocate all uninitialized virtqueues metadata up
> to requested index.
> 
> Fixes: 160cbc815b41 ("vhost: remove a hack on queue allocation")
> Cc: stable@dpdk.org
> 
> Suggested-by: Adrian Moreno <amorenoz@redhat.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/librte_vhost/vhost.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index 6068c38ec6..0c9ba3b3af 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -579,22 +579,29 @@ int
>  alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx)
>  {
>  	struct vhost_virtqueue *vq;
> +	uint32_t i;
> 
> -	vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
> -	if (vq == NULL) {
> -		VHOST_LOG_CONFIG(ERR,
> -			"Failed to allocate memory for vring:%u.\n", vring_idx);
> -		return -1;
> -	}
> +	/* Also allocate holes, if any, up to requested vring index. */
> +	for (i = 0; i <= vring_idx; i++) {
> +		if (dev->virtqueue[i])
> +			continue;
> 
> -	dev->virtqueue[vring_idx] = vq;
> -	init_vring_queue(dev, vring_idx);
> -	rte_spinlock_init(&vq->access_lock);
> -	vq->avail_wrap_counter = 1;
> -	vq->used_wrap_counter = 1;
> -	vq->signalled_used_valid = false;
> +		vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
> +		if (vq == NULL) {
> +			VHOST_LOG_CONFIG(ERR,
> +				"Failed to allocate memory for vring:%u.\n", i);
> +			return -1;
> +		}
> +
> +		dev->virtqueue[i] = vq;
> +		init_vring_queue(dev, vring_idx);
> +		rte_spinlock_init(&vq->access_lock);
> +		vq->avail_wrap_counter = 1;
> +		vq->used_wrap_counter = 1;
> +		vq->signalled_used_valid = false;
> +	}
> 
> -	dev->nr_vring += 1;
> +	dev->nr_vring = RTE_MAX(dev->nr_vring, vring_idx + 1);
> 
>  	return 0;
>  }
> --
> 2.26.2
Maxime Coquelin Oct. 21, 2020, 12:06 p.m. UTC | #2 
Hi Chenbon

On 10/21/20 1:10 PM, Xia, Chenbo wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Tuesday, October 20, 2020 1:34 AM
>> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
>> Subject: [PATCH 1/7] vhost: fix virtqueues metadata allocation
>>
>> The Vhost-user backend implementation assumes there will be
>> no holes in the device's array of virtqueues metadata
>> pointers.
>>
>> It can happen though, and would cause segmentation faults,
>> memory leaks or undefined behaviour.
> 
> Could I ask when will this happen? 
> 
> When QEMU does not configure all virtqueues? I'm not very sure.
> Could you point that out for me?

It has been reported by our QE when doing reconnect with multiqueue with
vIOMMU enabled:
https://bugzilla.redhat.com/show_bug.cgi?id=1880299

Regards,
Maxime

> Thanks!
> Chenbo
> 
>>
>> This patch keep the assumption that there is no holes in this
>> array, and allocate all uninitialized virtqueues metadata up
>> to requested index.
>>
>> Fixes: 160cbc815b41 ("vhost: remove a hack on queue allocation")
>> Cc: stable@dpdk.org
>>
>> Suggested-by: Adrian Moreno <amorenoz@redhat.com>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  lib/librte_vhost/vhost.c | 33 ++++++++++++++++++++-------------
>>  1 file changed, 20 insertions(+), 13 deletions(-)
>>
>> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
>> index 6068c38ec6..0c9ba3b3af 100644
>> --- a/lib/librte_vhost/vhost.c
>> +++ b/lib/librte_vhost/vhost.c
>> @@ -579,22 +579,29 @@ int
>>  alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx)
>>  {
>>  	struct vhost_virtqueue *vq;
>> +	uint32_t i;
>>
>> -	vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
>> -	if (vq == NULL) {
>> -		VHOST_LOG_CONFIG(ERR,
>> -			"Failed to allocate memory for vring:%u.\n", vring_idx);
>> -		return -1;
>> -	}
>> +	/* Also allocate holes, if any, up to requested vring index. */
>> +	for (i = 0; i <= vring_idx; i++) {
>> +		if (dev->virtqueue[i])
>> +			continue;
>>
>> -	dev->virtqueue[vring_idx] = vq;
>> -	init_vring_queue(dev, vring_idx);
>> -	rte_spinlock_init(&vq->access_lock);
>> -	vq->avail_wrap_counter = 1;
>> -	vq->used_wrap_counter = 1;
>> -	vq->signalled_used_valid = false;
>> +		vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
>> +		if (vq == NULL) {
>> +			VHOST_LOG_CONFIG(ERR,
>> +				"Failed to allocate memory for vring:%u.\n", i);
>> +			return -1;
>> +		}
>> +
>> +		dev->virtqueue[i] = vq;
>> +		init_vring_queue(dev, vring_idx);
>> +		rte_spinlock_init(&vq->access_lock);
>> +		vq->avail_wrap_counter = 1;
>> +		vq->used_wrap_counter = 1;
>> +		vq->signalled_used_valid = false;
>> +	}
>>
>> -	dev->nr_vring += 1;
>> +	dev->nr_vring = RTE_MAX(dev->nr_vring, vring_idx + 1);
>>
>>  	return 0;
>>  }
>> --
>> 2.26.2
>
Chenbo Xia Oct. 22, 2020, 11 a.m. UTC | #3 
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Wednesday, October 21, 2020 8:07 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org; amorenoz@redhat.com
> Cc: stable@dpdk.org
> Subject: Re: [PATCH 1/7] vhost: fix virtqueues metadata allocation
> 
> Hi Chenbon
> 
> On 10/21/20 1:10 PM, Xia, Chenbo wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Tuesday, October 20, 2020 1:34 AM
> >> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>;
> amorenoz@redhat.com
> >> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> >> Subject: [PATCH 1/7] vhost: fix virtqueues metadata allocation
> >>
> >> The Vhost-user backend implementation assumes there will be
> >> no holes in the device's array of virtqueues metadata
> >> pointers.
> >>
> >> It can happen though, and would cause segmentation faults,
> >> memory leaks or undefined behaviour.
> >
> > Could I ask when will this happen?
> >
> > When QEMU does not configure all virtqueues? I'm not very sure.
> > Could you point that out for me?
> 
> It has been reported by our QE when doing reconnect with multiqueue with
> vIOMMU enabled:
> https://bugzilla.redhat.com/show_bug.cgi?id=1880299
> 
> Regards,
> Maxime
> 
> > Thanks!
> > Chenbo
> >
> >>
> >> This patch keep the assumption that there is no holes in this
> >> array, and allocate all uninitialized virtqueues metadata up
> >> to requested index.
> >>
> >> Fixes: 160cbc815b41 ("vhost: remove a hack on queue allocation")
> >> Cc: stable@dpdk.org
> >>
> >> Suggested-by: Adrian Moreno <amorenoz@redhat.com>
> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> ---
> >>  lib/librte_vhost/vhost.c | 33 ++++++++++++++++++++-------------
> >>  1 file changed, 20 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> >> index 6068c38ec6..0c9ba3b3af 100644
> >> --- a/lib/librte_vhost/vhost.c
> >> +++ b/lib/librte_vhost/vhost.c
> >> @@ -579,22 +579,29 @@ int
> >>  alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx)
> >>  {
> >>  	struct vhost_virtqueue *vq;
> >> +	uint32_t i;
> >>
> >> -	vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
> >> -	if (vq == NULL) {
> >> -		VHOST_LOG_CONFIG(ERR,
> >> -			"Failed to allocate memory for vring:%u.\n", vring_idx);
> >> -		return -1;
> >> -	}
> >> +	/* Also allocate holes, if any, up to requested vring index. */
> >> +	for (i = 0; i <= vring_idx; i++) {
> >> +		if (dev->virtqueue[i])
> >> +			continue;
> >>
> >> -	dev->virtqueue[vring_idx] = vq;
> >> -	init_vring_queue(dev, vring_idx);
> >> -	rte_spinlock_init(&vq->access_lock);
> >> -	vq->avail_wrap_counter = 1;
> >> -	vq->used_wrap_counter = 1;
> >> -	vq->signalled_used_valid = false;
> >> +		vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
> >> +		if (vq == NULL) {
> >> +			VHOST_LOG_CONFIG(ERR,
> >> +				"Failed to allocate memory for vring:%u.\n", i);
> >> +			return -1;
> >> +		}
> >> +
> >> +		dev->virtqueue[i] = vq;
> >> +		init_vring_queue(dev, vring_idx);
> >> +		rte_spinlock_init(&vq->access_lock);
> >> +		vq->avail_wrap_counter = 1;
> >> +		vq->used_wrap_counter = 1;
> >> +		vq->signalled_used_valid = false;
> >> +	}
> >>
> >> -	dev->nr_vring += 1;
> >> +	dev->nr_vring = RTE_MAX(dev->nr_vring, vring_idx + 1);
> >>
> >>  	return 0;
> >>  }
> >> --
> >> 2.26.2
> >

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
diff mbox series

Patch

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 6068c38ec6..0c9ba3b3af 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -579,22 +579,29 @@  int
 alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx)
 {
 	struct vhost_virtqueue *vq;
+	uint32_t i;
 
-	vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
-	if (vq == NULL) {
-		VHOST_LOG_CONFIG(ERR,
-			"Failed to allocate memory for vring:%u.\n", vring_idx);
-		return -1;
-	}
+	/* Also allocate holes, if any, up to requested vring index. */
+	for (i = 0; i <= vring_idx; i++) {
+		if (dev->virtqueue[i])
+			continue;
 
-	dev->virtqueue[vring_idx] = vq;
-	init_vring_queue(dev, vring_idx);
-	rte_spinlock_init(&vq->access_lock);
-	vq->avail_wrap_counter = 1;
-	vq->used_wrap_counter = 1;
-	vq->signalled_used_valid = false;
+		vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
+		if (vq == NULL) {
+			VHOST_LOG_CONFIG(ERR,
+				"Failed to allocate memory for vring:%u.\n", i);
+			return -1;
+		}
+
+		dev->virtqueue[i] = vq;
+		init_vring_queue(dev, vring_idx);
+		rte_spinlock_init(&vq->access_lock);
+		vq->avail_wrap_counter = 1;
+		vq->used_wrap_counter = 1;
+		vq->signalled_used_valid = false;
+	}
 
-	dev->nr_vring += 1;
+	dev->nr_vring = RTE_MAX(dev->nr_vring, vring_idx + 1);
 
 	return 0;
 }