[dpdk-dev] [PATCH] ethdev: verify reserved HW ring

[dpdk-dev] [PATCH] ethdev: verify reserved HW ring

[Ferruh Yigit]

Function 'rte_eth_dma_zone_reserve()' returns an existing memzone based
on name match, but other requested attributes are discarded.
This may cause driver using a memzone with wrong size or alignment.

Verify size, alignment and socket_id for matched memzone, and do not use
memzone if any one of the attributes are not justified.

Reported-by: Renata Saiakhova <renata.saiakhova@ekinops.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_ethdev/rte_ethdev.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 8e10a6fc3..0fe84aadc 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -4202,8 +4202,18 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name,
 	}
 
 	mz = rte_memzone_lookup(z_name);
-	if (mz)
+	if (mz) {
+		if ((socket_id != SOCKET_ID_ANY && socket_id != mz->socket_id) ||
+				size > mz->len ||
+				(uintptr_t)mz->addr & (align - 1)) {
+			RTE_ETHDEV_LOG(ERR,
+				"memzone %s does not justify the requested attributes\n",
+				mz->name);
+			return NULL;
+		}
+
 		return mz;
+	}
 
 	return rte_memzone_reserve_aligned(z_name, size, socket_id,
 			RTE_MEMZONE_IOVA_CONTIG, align);
-- 
2.25.4

Re: [dpdk-dev] [PATCH] ethdev: verify reserved HW ring

[Andrew Rybchenko]

On 6/23/20 7:41 PM, Ferruh Yigit wrote:
> Function 'rte_eth_dma_zone_reserve()' returns an existing memzone based
> on name match, but other requested attributes are discarded.
> This may cause driver using a memzone with wrong size or alignment.
> 
> Verify size, alignment and socket_id for matched memzone, and do not use
> memzone if any one of the attributes are not justified.
> 
> Reported-by: Renata Saiakhova <renata.saiakhova@ekinops.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Few minor notes below, other than that:
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

> ---
> Cc: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/librte_ethdev/rte_ethdev.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 8e10a6fc3..0fe84aadc 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -4202,8 +4202,18 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name,
>  	}
>  
>  	mz = rte_memzone_lookup(z_name);
> -	if (mz)
> +	if (mz) {
> +		if ((socket_id != SOCKET_ID_ANY && socket_id != mz->socket_id) ||
> +				size > mz->len ||
> +				(uintptr_t)mz->addr & (align - 1)) {

IMHO, it would be a bit more readable if the result compared
with 0. Up to you.

> +			RTE_ETHDEV_LOG(ERR,
> +				"memzone %s does not justify the requested attributes\n",
> +				mz->name);
> +			return NULL;

Initially I thought it is a bad behaviour and memzone should
be freed and reallocated, but it is even worse (implicit free).
May be it is better to highlight it in the description.

> +		}
> +
>  		return mz;
> +	}
>  
>  	return rte_memzone_reserve_aligned(z_name, size, socket_id,
>  			RTE_MEMZONE_IOVA_CONTIG, align);
> 

Re: [dpdk-dev] [PATCH] ethdev: verify reserved HW ring

[Ferruh Yigit]

On 6/23/2020 5:58 PM, Andrew Rybchenko wrote:
> On 6/23/20 7:41 PM, Ferruh Yigit wrote:
>> Function 'rte_eth_dma_zone_reserve()' returns an existing memzone based
>> on name match, but other requested attributes are discarded.
>> This may cause driver using a memzone with wrong size or alignment.
>>
>> Verify size, alignment and socket_id for matched memzone, and do not use
>> memzone if any one of the attributes are not justified.
>>
>> Reported-by: Renata Saiakhova <renata.saiakhova@ekinops.com>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> Few minor notes below, other than that:
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> 
>> ---
>> Cc: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>  lib/librte_ethdev/rte_ethdev.c | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index 8e10a6fc3..0fe84aadc 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -4202,8 +4202,18 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name,
>>  	}
>>  
>>  	mz = rte_memzone_lookup(z_name);
>> -	if (mz)
>> +	if (mz) {
>> +		if ((socket_id != SOCKET_ID_ANY && socket_id != mz->socket_id) ||
>> +				size > mz->len ||
>> +				(uintptr_t)mz->addr & (align - 1)) {
> 
> IMHO, it would be a bit more readable if the result compared
> with 0. Up to you.

Both OK, I can update.

> 
>> +			RTE_ETHDEV_LOG(ERR,
>> +				"memzone %s does not justify the requested attributes\n",
>> +				mz->name);
>> +			return NULL;
> 
> Initially I thought it is a bad behaviour and memzone should
> be freed and reallocated, but it is even worse (implicit free).

+1

> May be it is better to highlight it in the description.

ok

> 
>> +		}
>> +
>>  		return mz;
>> +	}
>>  
>>  	return rte_memzone_reserve_aligned(z_name, size, socket_id,
>>  			RTE_MEMZONE_IOVA_CONTIG, align);
>>
> 

[dpdk-dev] [PATCH v2] ethdev: verify reserved HW ring

[Ferruh Yigit]

Function 'rte_eth_dma_zone_reserve()' returns an existing memzone based
on name match, but other requested attributes are discarded.
This may cause driver using a memzone with wrong size or alignment.

Verify size, alignment and socket_id for matched memzone, and do not use
memzone if any one of the attributes are not justified.

It is possible to free the existing memzone and allocate again with the
requested attributes but it is better caller do the explicit free.

Reported-by: Renata Saiakhova <renata.saiakhova@ekinops.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
Cc: Anatoly Burakov <anatoly.burakov@intel.com>

v2:
* Updated commit log related decision on not doing implicit free
* Updated alignment check to make it more explicit
---
 lib/librte_ethdev/rte_ethdev.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 8e10a6fc3..d24db66fb 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -4202,8 +4202,18 @@ rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char *ring_name,
 	}
 
 	mz = rte_memzone_lookup(z_name);
-	if (mz)
+	if (mz) {
+		if ((socket_id != SOCKET_ID_ANY && socket_id != mz->socket_id) ||
+				size > mz->len ||
+				((uintptr_t)mz->addr & (align - 1)) != 0) {
+			RTE_ETHDEV_LOG(ERR,
+				"memzone %s does not justify the requested attributes\n",
+				mz->name);
+			return NULL;
+		}
+
 		return mz;
+	}
 
 	return rte_memzone_reserve_aligned(z_name, size, socket_id,
 			RTE_MEMZONE_IOVA_CONTIG, align);
-- 
2.25.4

Re: [dpdk-dev] [PATCH v2] ethdev: verify reserved HW ring

[Ferruh Yigit]

On 6/24/2020 10:35 AM, Ferruh Yigit wrote:
> Function 'rte_eth_dma_zone_reserve()' returns an existing memzone based
> on name match, but other requested attributes are discarded.
> This may cause driver using a memzone with wrong size or alignment.
> 
> Verify size, alignment and socket_id for matched memzone, and do not use
> memzone if any one of the attributes are not justified.
> 
> It is possible to free the existing memzone and allocate again with the
> requested attributes but it is better caller do the explicit free.
> 
> Reported-by: Renata Saiakhova <renata.saiakhova@ekinops.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

Applied to dpdk-next-net/master, thanks.