DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] ethdev: verify reserved HW ring
@ 2020-06-23 16:41 Ferruh Yigit
  2020-06-23 16:58 ` Andrew Rybchenko
  2020-06-24  9:35 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
  0 siblings, 2 replies; 5+ messages in thread
From: Ferruh Yigit @ 2020-06-23 16:41 UTC (permalink / raw)
  To: Thomas Monjalon, Andrew Rybchenko
  Cc: dev, Ferruh Yigit, Renata Saiakhova, Anatoly Burakov

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


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

* Re: [dpdk-dev] [PATCH] ethdev: verify reserved HW ring
  2020-06-23 16:41 [dpdk-dev] [PATCH] ethdev: verify reserved HW ring Ferruh Yigit
@ 2020-06-23 16:58 ` Andrew Rybchenko
  2020-06-24  7:24   ` Ferruh Yigit
  2020-06-24  9:35 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
  1 sibling, 1 reply; 5+ messages in thread
From: Andrew Rybchenko @ 2020-06-23 16:58 UTC (permalink / raw)
  To: Ferruh Yigit, Thomas Monjalon; +Cc: dev, Renata Saiakhova, Anatoly Burakov

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


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

* Re: [dpdk-dev] [PATCH] ethdev: verify reserved HW ring
  2020-06-23 16:58 ` Andrew Rybchenko
@ 2020-06-24  7:24   ` Ferruh Yigit
  0 siblings, 0 replies; 5+ messages in thread
From: Ferruh Yigit @ 2020-06-24  7:24 UTC (permalink / raw)
  To: Andrew Rybchenko, Thomas Monjalon; +Cc: dev, Renata Saiakhova, Anatoly Burakov

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


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

* [dpdk-dev] [PATCH v2] ethdev: verify reserved HW ring
  2020-06-23 16:41 [dpdk-dev] [PATCH] ethdev: verify reserved HW ring Ferruh Yigit
  2020-06-23 16:58 ` Andrew Rybchenko
@ 2020-06-24  9:35 ` Ferruh Yigit
  2020-07-08 18:14   ` Ferruh Yigit
  1 sibling, 1 reply; 5+ messages in thread
From: Ferruh Yigit @ 2020-06-24  9:35 UTC (permalink / raw)
  To: Thomas Monjalon, Andrew Rybchenko
  Cc: dev, Ferruh Yigit, Renata Saiakhova, Anatoly Burakov

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


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

* Re: [dpdk-dev] [PATCH v2] ethdev: verify reserved HW ring
  2020-06-24  9:35 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
@ 2020-07-08 18:14   ` Ferruh Yigit
  0 siblings, 0 replies; 5+ messages in thread
From: Ferruh Yigit @ 2020-07-08 18:14 UTC (permalink / raw)
  To: Thomas Monjalon, Andrew Rybchenko; +Cc: dev, Renata Saiakhova, Anatoly Burakov

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.

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

end of thread, other threads:[~2020-07-08 18:14 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-23 16:41 [dpdk-dev] [PATCH] ethdev: verify reserved HW ring Ferruh Yigit
2020-06-23 16:58 ` Andrew Rybchenko
2020-06-24  7:24   ` Ferruh Yigit
2020-06-24  9:35 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
2020-07-08 18:14   ` Ferruh Yigit

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git