DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] ethdev: keep count of allocated and used representor ranges
@ 2021-07-02 14:23 Andrew Rybchenko
  2021-07-03  1:32 ` Xueming(Steven) Li
  2021-07-05 10:02 ` [dpdk-dev] [PATCH v2] " Andrew Rybchenko
  0 siblings, 2 replies; 7+ messages in thread
From: Andrew Rybchenko @ 2021-07-02 14:23 UTC (permalink / raw)
  To: Matan Azrad, Shahaf Shuler, Viacheslav Ovsiienko,
	Thomas Monjalon, Ferruh Yigit, Xueming Li
  Cc: dev, Viacheslav Galaktionov, stable

From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>

In its current state, the API can overflow the user-passed buffer if a new
representor range appears between function calls.

In order to solve this problem, augment the representor info structure with
the numbers of allocated and initialized ranges. This way the users of this
structure can be sure they will not overrun the buffer.

Fixes: 85e1588ca72f ("ethdev: add API to get representor info")
Cc: stable@dpdk.org

Signed-off-by: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 drivers/net/mlx5/mlx5_ethdev.c | 15 +++++++++++++++
 lib/ethdev/rte_ethdev.c        |  6 ++++--
 lib/ethdev/rte_ethdev.h        |  9 ++++++++-
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index 90baee5aa4..61b1c0e75c 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -413,9 +413,15 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
 	int n_type = 4; /* Representor types, VF, HPF@VF, SF and HPF@SF. */
 	int n_pf = 2; /* Number of PFs. */
 	int i = 0, pf;
+	int n_entries;
 
 	if (info == NULL)
 		goto out;
+
+	n_entries = n_type * n_pf;
+	if ((uint32_t)n_entries > info->nb_ranges_alloc)
+		n_entries = info->nb_ranges_alloc;
+
 	info->controller = 0;
 	info->pf = priv->pf_bond >= 0 ? priv->pf_bond : 0;
 	for (pf = 0; pf < n_pf; ++pf) {
@@ -431,6 +437,8 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
 		snprintf(info->ranges[i].name,
 			 sizeof(info->ranges[i].name), "pf%dvf", pf);
 		i++;
+		if (i == n_entries)
+			break;
 		/* HPF range of VF type. */
 		info->ranges[i].type = RTE_ETH_REPRESENTOR_VF;
 		info->ranges[i].controller = 0;
@@ -443,6 +451,8 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
 		snprintf(info->ranges[i].name,
 			 sizeof(info->ranges[i].name), "pf%dvf", pf);
 		i++;
+		if (i == n_entries)
+			break;
 		/* SF range. */
 		info->ranges[i].type = RTE_ETH_REPRESENTOR_SF;
 		info->ranges[i].controller = 0;
@@ -455,6 +465,8 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
 		snprintf(info->ranges[i].name,
 			 sizeof(info->ranges[i].name), "pf%dsf", pf);
 		i++;
+		if (i == n_entries)
+			break;
 		/* HPF range of SF type. */
 		info->ranges[i].type = RTE_ETH_REPRESENTOR_SF;
 		info->ranges[i].controller = 0;
@@ -467,8 +479,11 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
 		snprintf(info->ranges[i].name,
 			 sizeof(info->ranges[i].name), "pf%dsf", pf);
 		i++;
+		if (i == n_entries)
+			break;
 	}
 out:
+	info->nb_ranges = i;
 	return n_type * n_pf;
 }
 
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index c607eabb5b..30abc93b18 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5987,7 +5987,8 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
 			   int controller, int pf, int representor_port,
 			   uint16_t *repr_id)
 {
-	int ret, n, i, count;
+	int ret, n, count;
+	uint32_t i;
 	struct rte_eth_representor_info *info = NULL;
 	size_t size;
 
@@ -6011,6 +6012,7 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
 	info = calloc(1, size);
 	if (info == NULL)
 		return -ENOMEM;
+	info->nb_ranges_alloc = n;
 	ret = rte_eth_representor_info_get(ethdev->data->port_id, info);
 	if (ret < 0)
 		goto out;
@@ -6023,7 +6025,7 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
 
 	/* Locate representor ID. */
 	ret = -ENOENT;
-	for (i = 0; i < n; ++i) {
+	for (i = 0; i < info->nb_ranges; ++i) {
 		if (info->ranges[i].type != type)
 			continue;
 		if (info->ranges[i].controller != controller)
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index faf3bd901d..d2b27c351f 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -4856,6 +4856,8 @@ struct rte_eth_representor_range {
 struct rte_eth_representor_info {
 	uint16_t controller; /**< Controller ID of caller device. */
 	uint16_t pf; /**< Physical function ID of caller device. */
+	uint32_t nb_ranges_alloc; /**< Size of the ranges array. */
+	uint32_t nb_ranges; /**< Number of initialized ranges. */
 	struct rte_eth_representor_range ranges[];/**< Representor ID range. */
 };
 
@@ -4871,11 +4873,16 @@ struct rte_eth_representor_info {
  *   A pointer to a representor info structure.
  *   NULL to return number of range entries and allocate memory
  *   for next call to store detail.
+ *   The number of ranges that were written into this structure
+ *   will be placed into its nb_ranges field. This number cannot be
+ *   larger than the nb_ranges_alloc that by the user before calling
+ *   this function. It can be smaller than the value returned by the
+ *   function, however.
  * @return
  *   - (-ENOTSUP) if operation is not supported.
  *   - (-ENODEV) if *port_id* invalid.
  *   - (-EIO) if device is removed.
- *   - (>=0) number of representor range entries supported by device.
+ *   - (>=0) number of available representor range entries.
  */
 __rte_experimental
 int rte_eth_representor_info_get(uint16_t port_id,
-- 
2.30.2


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

* Re: [dpdk-dev] [PATCH] ethdev: keep count of allocated and used representor ranges
  2021-07-02 14:23 [dpdk-dev] [PATCH] ethdev: keep count of allocated and used representor ranges Andrew Rybchenko
@ 2021-07-03  1:32 ` Xueming(Steven) Li
  2021-07-03 10:01   ` Viacheslav Galaktionov
  2021-07-05 10:02 ` [dpdk-dev] [PATCH v2] " Andrew Rybchenko
  1 sibling, 1 reply; 7+ messages in thread
From: Xueming(Steven) Li @ 2021-07-03  1:32 UTC (permalink / raw)
  To: Andrew Rybchenko, Matan Azrad, Shahaf Shuler, Slava Ovsiienko,
	NBU-Contact-Thomas Monjalon, Ferruh Yigit
  Cc: dev, Viacheslav Galaktionov, stable

Hi

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Friday, July 2, 2021 10:23 PM
> To: Matan Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-
> Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Xueming(Steven) Li
> <xuemingl@nvidia.com>
> Cc: dev@dpdk.org; Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>; stable@dpdk.org
> Subject: [PATCH] ethdev: keep count of allocated and used representor ranges
> 
> From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
> 
> In its current state, the API can overflow the user-passed buffer if a new representor range appears between function calls.
> 
> In order to solve this problem, augment the representor info structure with the numbers of allocated and initialized ranges. This way
> the users of this structure can be sure they will not overrun the buffer.

Thanks for making this api more robust!

> 
> Fixes: 85e1588ca72f ("ethdev: add API to get representor info")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---
>  drivers/net/mlx5/mlx5_ethdev.c | 15 +++++++++++++++
>  lib/ethdev/rte_ethdev.c        |  6 ++++--
>  lib/ethdev/rte_ethdev.h        |  9 ++++++++-
>  3 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c index 90baee5aa4..61b1c0e75c 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -413,9 +413,15 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
>  	int n_type = 4; /* Representor types, VF, HPF@VF, SF and HPF@SF. */
>  	int n_pf = 2; /* Number of PFs. */
>  	int i = 0, pf;
> +	int n_entries;
> 
>  	if (info == NULL)
>  		goto out;
> +
> +	n_entries = n_type * n_pf;
> +	if ((uint32_t)n_entries > info->nb_ranges_alloc)
> +		n_entries = info->nb_ranges_alloc;
> +
>  	info->controller = 0;
>  	info->pf = priv->pf_bond >= 0 ? priv->pf_bond : 0;
>  	for (pf = 0; pf < n_pf; ++pf) {
> @@ -431,6 +437,8 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
>  		snprintf(info->ranges[i].name,
>  			 sizeof(info->ranges[i].name), "pf%dvf", pf);
>  		i++;
> +		if (i == n_entries)
> +			break;
>  		/* HPF range of VF type. */
>  		info->ranges[i].type = RTE_ETH_REPRESENTOR_VF;
>  		info->ranges[i].controller = 0;
> @@ -443,6 +451,8 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
>  		snprintf(info->ranges[i].name,
>  			 sizeof(info->ranges[i].name), "pf%dvf", pf);
>  		i++;
> +		if (i == n_entries)
> +			break;
>  		/* SF range. */
>  		info->ranges[i].type = RTE_ETH_REPRESENTOR_SF;
>  		info->ranges[i].controller = 0;
> @@ -455,6 +465,8 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
>  		snprintf(info->ranges[i].name,
>  			 sizeof(info->ranges[i].name), "pf%dsf", pf);
>  		i++;
> +		if (i == n_entries)
> +			break;
>  		/* HPF range of SF type. */
>  		info->ranges[i].type = RTE_ETH_REPRESENTOR_SF;
>  		info->ranges[i].controller = 0;
> @@ -467,8 +479,11 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
>  		snprintf(info->ranges[i].name,
>  			 sizeof(info->ranges[i].name), "pf%dsf", pf);
>  		i++;
> +		if (i == n_entries)
> +			break;
>  	}
>  out:
> +	info->nb_ranges = i;

Here info maybe NULL.

>  	return n_type * n_pf;
>  }
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c index c607eabb5b..30abc93b18 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -5987,7 +5987,8 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
>  			   int controller, int pf, int representor_port,
>  			   uint16_t *repr_id)
>  {
> -	int ret, n, i, count;
> +	int ret, n, count;
> +	uint32_t i;
>  	struct rte_eth_representor_info *info = NULL;
>  	size_t size;
> 
> @@ -6011,6 +6012,7 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
>  	info = calloc(1, size);
>  	if (info == NULL)
>  		return -ENOMEM;
> +	info->nb_ranges_alloc = n;
>  	ret = rte_eth_representor_info_get(ethdev->data->port_id, info);
>  	if (ret < 0)
>  		goto out;
> @@ -6023,7 +6025,7 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
> 
>  	/* Locate representor ID. */
>  	ret = -ENOENT;
> -	for (i = 0; i < n; ++i) {
> +	for (i = 0; i < info->nb_ranges; ++i) {
>  		if (info->ranges[i].type != type)
>  			continue;
>  		if (info->ranges[i].controller != controller) diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h index
> faf3bd901d..d2b27c351f 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -4856,6 +4856,8 @@ struct rte_eth_representor_range {  struct rte_eth_representor_info {
>  	uint16_t controller; /**< Controller ID of caller device. */
>  	uint16_t pf; /**< Physical function ID of caller device. */
> +	uint32_t nb_ranges_alloc; /**< Size of the ranges array. */
> +	uint32_t nb_ranges; /**< Number of initialized ranges. */

How about rte_eth_representor_info_get(info) return max ranges size if info is NULL, 
return real initialized ranges if info not NULL?

>  	struct rte_eth_representor_range ranges[];/**< Representor ID range. */  };
> 
> @@ -4871,11 +4873,16 @@ struct rte_eth_representor_info {
>   *   A pointer to a representor info structure.
>   *   NULL to return number of range entries and allocate memory
>   *   for next call to store detail.
> + *   The number of ranges that were written into this structure
> + *   will be placed into its nb_ranges field. This number cannot be
> + *   larger than the nb_ranges_alloc that by the user before calling
> + *   this function. It can be smaller than the value returned by the
> + *   function, however.
>   * @return
>   *   - (-ENOTSUP) if operation is not supported.
>   *   - (-ENODEV) if *port_id* invalid.
>   *   - (-EIO) if device is removed.
> - *   - (>=0) number of representor range entries supported by device.
> + *   - (>=0) number of available representor range entries.
>   */
>  __rte_experimental
>  int rte_eth_representor_info_get(uint16_t port_id,
> --
> 2.30.2


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

* Re: [dpdk-dev] [PATCH] ethdev: keep count of allocated and used representor ranges
  2021-07-03  1:32 ` Xueming(Steven) Li
@ 2021-07-03 10:01   ` Viacheslav Galaktionov
  2021-07-05  2:50     ` Xueming(Steven) Li
  0 siblings, 1 reply; 7+ messages in thread
From: Viacheslav Galaktionov @ 2021-07-03 10:01 UTC (permalink / raw)
  To: Xueming(Steven) Li
  Cc: Andrew Rybchenko, Matan Azrad, Shahaf Shuler, Slava Ovsiienko,
	NBU-Contact-Thomas Monjalon, Ferruh Yigit, dev, stable

Hello!

On 2021-07-03 04:32, Xueming(Steven) Li wrote:
>> +		if (i == n_entries)
>> +			break;
>>  	}
>>  out:
>> +	info->nb_ranges = i;
> 
> Here info maybe NULL.

Good catch, thanks for noticing!

>> faf3bd901d..d2b27c351f 100644
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -4856,6 +4856,8 @@ struct rte_eth_representor_range {  struct 
>> rte_eth_representor_info {
>>  	uint16_t controller; /**< Controller ID of caller device. */
>>  	uint16_t pf; /**< Physical function ID of caller device. */
>> +	uint32_t nb_ranges_alloc; /**< Size of the ranges array. */
>> +	uint32_t nb_ranges; /**< Number of initialized ranges. */
> 
> How about rte_eth_representor_info_get(info) return max ranges size if
> info is NULL,
> return real initialized ranges if info not NULL?

I'm not sure how I feel about it. I think it'd be best if the function 
returned
just one thing.

Moreover, there are drivers that don't have a fixed structure for 
representor
IDs, e.g. net/sfc, where every range will contain a single ID. If a new
representor range is created between invocations of this function, there
probably should be a way for the caller to know about this.

Perhaps we should move the total number of representors to an out 
parameter and
use the return value for the number of initialized ranges. What do you 
think
about this?

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

* Re: [dpdk-dev] [PATCH] ethdev: keep count of allocated and used representor ranges
  2021-07-03 10:01   ` Viacheslav Galaktionov
@ 2021-07-05  2:50     ` Xueming(Steven) Li
  0 siblings, 0 replies; 7+ messages in thread
From: Xueming(Steven) Li @ 2021-07-05  2:50 UTC (permalink / raw)
  To: Viacheslav Galaktionov
  Cc: Andrew Rybchenko, Matan Azrad, Shahaf Shuler, Slava Ovsiienko,
	NBU-Contact-Thomas Monjalon, Ferruh Yigit, dev, stable



> -----Original Message-----
> From: Viacheslav Galaktionov <Viacheslav.Galaktionov@oktetlabs.ru>
> Sent: Saturday, July 3, 2021 6:01 PM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>
> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Matan Azrad <matan@nvidia.com>; Shahaf Shuler
> <shahafs@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
> Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH] ethdev: keep count of allocated and used representor ranges
> 
> Hello!
> 
> On 2021-07-03 04:32, Xueming(Steven) Li wrote:
> >> +		if (i == n_entries)
> >> +			break;
> >>  	}
> >>  out:
> >> +	info->nb_ranges = i;
> >
> > Here info maybe NULL.
> 
> Good catch, thanks for noticing!
> 
> >> faf3bd901d..d2b27c351f 100644
> >> --- a/lib/ethdev/rte_ethdev.h
> >> +++ b/lib/ethdev/rte_ethdev.h
> >> @@ -4856,6 +4856,8 @@ struct rte_eth_representor_range {  struct
> >> rte_eth_representor_info {
> >>  	uint16_t controller; /**< Controller ID of caller device. */
> >>  	uint16_t pf; /**< Physical function ID of caller device. */
> >> +	uint32_t nb_ranges_alloc; /**< Size of the ranges array. */
> >> +	uint32_t nb_ranges; /**< Number of initialized ranges. */
> >
> > How about rte_eth_representor_info_get(info) return max ranges size if
> > info is NULL, return real initialized ranges if info not NULL?
> 
> I'm not sure how I feel about it. I think it'd be best if the function returned just one thing.
> 
> Moreover, there are drivers that don't have a fixed structure for representor IDs, e.g. net/sfc, where every range will contain a single
> ID. If a new representor range is created between invocations of this function, there probably should be a way for the caller to know
> about this.
> 
> Perhaps we should move the total number of representors to an out parameter and use the return value for the number of initialized
> ranges. What do you think about this?

No big difference, I'm ok with your original design.

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

* [dpdk-dev] [PATCH v2] ethdev: keep count of allocated and used representor ranges
  2021-07-02 14:23 [dpdk-dev] [PATCH] ethdev: keep count of allocated and used representor ranges Andrew Rybchenko
  2021-07-03  1:32 ` Xueming(Steven) Li
@ 2021-07-05 10:02 ` Andrew Rybchenko
  2021-07-05 11:07   ` Xueming(Steven) Li
  1 sibling, 1 reply; 7+ messages in thread
From: Andrew Rybchenko @ 2021-07-05 10:02 UTC (permalink / raw)
  To: Matan Azrad, Shahaf Shuler, Viacheslav Ovsiienko,
	Thomas Monjalon, Ferruh Yigit, Xueming Li
  Cc: dev, Viacheslav Galaktionov, stable

From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>

In its current state, the API can overflow the user-passed buffer if a new
representor range appears between function calls.

In order to solve this problem, augment the representor info structure with
the numbers of allocated and initialized ranges. This way the users of this
structure can be sure they will not overrun the buffer.

Fixes: 85e1588ca72f ("ethdev: add API to get representor info")
Cc: stable@dpdk.org

Signed-off-by: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
v2:
 - fixed a potential null pointer dereference

 drivers/net/mlx5/mlx5_ethdev.c | 15 +++++++++++++++
 lib/ethdev/rte_ethdev.c        |  6 ++++--
 lib/ethdev/rte_ethdev.h        |  9 ++++++++-
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index 90baee5aa4..c32eeefd88 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -413,9 +413,15 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
 	int n_type = 4; /* Representor types, VF, HPF@VF, SF and HPF@SF. */
 	int n_pf = 2; /* Number of PFs. */
 	int i = 0, pf;
+	int n_entries;
 
 	if (info == NULL)
 		goto out;
+
+	n_entries = n_type * n_pf;
+	if ((uint32_t)n_entries > info->nb_ranges_alloc)
+		n_entries = info->nb_ranges_alloc;
+
 	info->controller = 0;
 	info->pf = priv->pf_bond >= 0 ? priv->pf_bond : 0;
 	for (pf = 0; pf < n_pf; ++pf) {
@@ -431,6 +437,8 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
 		snprintf(info->ranges[i].name,
 			 sizeof(info->ranges[i].name), "pf%dvf", pf);
 		i++;
+		if (i == n_entries)
+			break;
 		/* HPF range of VF type. */
 		info->ranges[i].type = RTE_ETH_REPRESENTOR_VF;
 		info->ranges[i].controller = 0;
@@ -443,6 +451,8 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
 		snprintf(info->ranges[i].name,
 			 sizeof(info->ranges[i].name), "pf%dvf", pf);
 		i++;
+		if (i == n_entries)
+			break;
 		/* SF range. */
 		info->ranges[i].type = RTE_ETH_REPRESENTOR_SF;
 		info->ranges[i].controller = 0;
@@ -455,6 +465,8 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
 		snprintf(info->ranges[i].name,
 			 sizeof(info->ranges[i].name), "pf%dsf", pf);
 		i++;
+		if (i == n_entries)
+			break;
 		/* HPF range of SF type. */
 		info->ranges[i].type = RTE_ETH_REPRESENTOR_SF;
 		info->ranges[i].controller = 0;
@@ -467,7 +479,10 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
 		snprintf(info->ranges[i].name,
 			 sizeof(info->ranges[i].name), "pf%dsf", pf);
 		i++;
+		if (i == n_entries)
+			break;
 	}
+	info->nb_ranges = i;
 out:
 	return n_type * n_pf;
 }
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index c607eabb5b..30abc93b18 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5987,7 +5987,8 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
 			   int controller, int pf, int representor_port,
 			   uint16_t *repr_id)
 {
-	int ret, n, i, count;
+	int ret, n, count;
+	uint32_t i;
 	struct rte_eth_representor_info *info = NULL;
 	size_t size;
 
@@ -6011,6 +6012,7 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
 	info = calloc(1, size);
 	if (info == NULL)
 		return -ENOMEM;
+	info->nb_ranges_alloc = n;
 	ret = rte_eth_representor_info_get(ethdev->data->port_id, info);
 	if (ret < 0)
 		goto out;
@@ -6023,7 +6025,7 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
 
 	/* Locate representor ID. */
 	ret = -ENOENT;
-	for (i = 0; i < n; ++i) {
+	for (i = 0; i < info->nb_ranges; ++i) {
 		if (info->ranges[i].type != type)
 			continue;
 		if (info->ranges[i].controller != controller)
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index faf3bd901d..d2b27c351f 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -4856,6 +4856,8 @@ struct rte_eth_representor_range {
 struct rte_eth_representor_info {
 	uint16_t controller; /**< Controller ID of caller device. */
 	uint16_t pf; /**< Physical function ID of caller device. */
+	uint32_t nb_ranges_alloc; /**< Size of the ranges array. */
+	uint32_t nb_ranges; /**< Number of initialized ranges. */
 	struct rte_eth_representor_range ranges[];/**< Representor ID range. */
 };
 
@@ -4871,11 +4873,16 @@ struct rte_eth_representor_info {
  *   A pointer to a representor info structure.
  *   NULL to return number of range entries and allocate memory
  *   for next call to store detail.
+ *   The number of ranges that were written into this structure
+ *   will be placed into its nb_ranges field. This number cannot be
+ *   larger than the nb_ranges_alloc that by the user before calling
+ *   this function. It can be smaller than the value returned by the
+ *   function, however.
  * @return
  *   - (-ENOTSUP) if operation is not supported.
  *   - (-ENODEV) if *port_id* invalid.
  *   - (-EIO) if device is removed.
- *   - (>=0) number of representor range entries supported by device.
+ *   - (>=0) number of available representor range entries.
  */
 __rte_experimental
 int rte_eth_representor_info_get(uint16_t port_id,
-- 
2.30.2


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

* Re: [dpdk-dev] [PATCH v2] ethdev: keep count of allocated and used representor ranges
  2021-07-05 10:02 ` [dpdk-dev] [PATCH v2] " Andrew Rybchenko
@ 2021-07-05 11:07   ` Xueming(Steven) Li
  2021-07-10  9:46     ` Thomas Monjalon
  0 siblings, 1 reply; 7+ messages in thread
From: Xueming(Steven) Li @ 2021-07-05 11:07 UTC (permalink / raw)
  To: Andrew Rybchenko, Matan Azrad, Shahaf Shuler, Slava Ovsiienko,
	NBU-Contact-Thomas Monjalon, Ferruh Yigit
  Cc: dev, Viacheslav Galaktionov, stable, Xueming(Steven) Li



> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Monday, July 5, 2021 6:03 PM
> To: Matan Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-
> Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Xueming(Steven) Li
> <xuemingl@nvidia.com>
> Cc: dev@dpdk.org; Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>; stable@dpdk.org
> Subject: [PATCH v2] ethdev: keep count of allocated and used representor ranges
> 
> From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
> 
> In its current state, the API can overflow the user-passed buffer if a new representor range appears between function calls.
> 
> In order to solve this problem, augment the representor info structure with the numbers of allocated and initialized ranges. This way
> the users of this structure can be sure they will not overrun the buffer.
> 
> Fixes: 85e1588ca72f ("ethdev: add API to get representor info")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---
> v2:
>  - fixed a potential null pointer dereference
> 
>  drivers/net/mlx5/mlx5_ethdev.c | 15 +++++++++++++++
>  lib/ethdev/rte_ethdev.c        |  6 ++++--
>  lib/ethdev/rte_ethdev.h        |  9 ++++++++-
>  3 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c index 90baee5aa4..c32eeefd88 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -413,9 +413,15 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
>  	int n_type = 4; /* Representor types, VF, HPF@VF, SF and HPF@SF. */
>  	int n_pf = 2; /* Number of PFs. */
>  	int i = 0, pf;
> +	int n_entries;
> 
>  	if (info == NULL)
>  		goto out;
> +
> +	n_entries = n_type * n_pf;
> +	if ((uint32_t)n_entries > info->nb_ranges_alloc)
> +		n_entries = info->nb_ranges_alloc;
> +
>  	info->controller = 0;
>  	info->pf = priv->pf_bond >= 0 ? priv->pf_bond : 0;
>  	for (pf = 0; pf < n_pf; ++pf) {
> @@ -431,6 +437,8 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
>  		snprintf(info->ranges[i].name,
>  			 sizeof(info->ranges[i].name), "pf%dvf", pf);
>  		i++;
> +		if (i == n_entries)
> +			break;
>  		/* HPF range of VF type. */
>  		info->ranges[i].type = RTE_ETH_REPRESENTOR_VF;
>  		info->ranges[i].controller = 0;
> @@ -443,6 +451,8 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
>  		snprintf(info->ranges[i].name,
>  			 sizeof(info->ranges[i].name), "pf%dvf", pf);
>  		i++;
> +		if (i == n_entries)
> +			break;
>  		/* SF range. */
>  		info->ranges[i].type = RTE_ETH_REPRESENTOR_SF;
>  		info->ranges[i].controller = 0;
> @@ -455,6 +465,8 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
>  		snprintf(info->ranges[i].name,
>  			 sizeof(info->ranges[i].name), "pf%dsf", pf);
>  		i++;
> +		if (i == n_entries)
> +			break;
>  		/* HPF range of SF type. */
>  		info->ranges[i].type = RTE_ETH_REPRESENTOR_SF;
>  		info->ranges[i].controller = 0;
> @@ -467,7 +479,10 @@ mlx5_representor_info_get(struct rte_eth_dev *dev,
>  		snprintf(info->ranges[i].name,
>  			 sizeof(info->ranges[i].name), "pf%dsf", pf);
>  		i++;
> +		if (i == n_entries)
> +			break;
>  	}
> +	info->nb_ranges = i;
>  out:
>  	return n_type * n_pf;
>  }
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c index c607eabb5b..30abc93b18 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -5987,7 +5987,8 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
>  			   int controller, int pf, int representor_port,
>  			   uint16_t *repr_id)
>  {
> -	int ret, n, i, count;
> +	int ret, n, count;
> +	uint32_t i;
>  	struct rte_eth_representor_info *info = NULL;
>  	size_t size;
> 
> @@ -6011,6 +6012,7 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
>  	info = calloc(1, size);
>  	if (info == NULL)
>  		return -ENOMEM;
> +	info->nb_ranges_alloc = n;
>  	ret = rte_eth_representor_info_get(ethdev->data->port_id, info);
>  	if (ret < 0)
>  		goto out;
> @@ -6023,7 +6025,7 @@ rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
> 
>  	/* Locate representor ID. */
>  	ret = -ENOENT;
> -	for (i = 0; i < n; ++i) {
> +	for (i = 0; i < info->nb_ranges; ++i) {
>  		if (info->ranges[i].type != type)
>  			continue;
>  		if (info->ranges[i].controller != controller) diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h index
> faf3bd901d..d2b27c351f 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -4856,6 +4856,8 @@ struct rte_eth_representor_range {  struct rte_eth_representor_info {
>  	uint16_t controller; /**< Controller ID of caller device. */
>  	uint16_t pf; /**< Physical function ID of caller device. */
> +	uint32_t nb_ranges_alloc; /**< Size of the ranges array. */
> +	uint32_t nb_ranges; /**< Number of initialized ranges. */
>  	struct rte_eth_representor_range ranges[];/**< Representor ID range. */  };
> 
> @@ -4871,11 +4873,16 @@ struct rte_eth_representor_info {
>   *   A pointer to a representor info structure.
>   *   NULL to return number of range entries and allocate memory
>   *   for next call to store detail.
> + *   The number of ranges that were written into this structure
> + *   will be placed into its nb_ranges field. This number cannot be
> + *   larger than the nb_ranges_alloc that by the user before calling
> + *   this function. It can be smaller than the value returned by the
> + *   function, however.
>   * @return
>   *   - (-ENOTSUP) if operation is not supported.
>   *   - (-ENODEV) if *port_id* invalid.
>   *   - (-EIO) if device is removed.
> - *   - (>=0) number of representor range entries supported by device.
> + *   - (>=0) number of available representor range entries.
>   */
>  __rte_experimental
>  int rte_eth_representor_info_get(uint16_t port_id,
> --
> 2.30.2

Reviewed-by: Xueming Li <xuemingl@nvidia.com>

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

* Re: [dpdk-dev] [PATCH v2] ethdev: keep count of allocated and used representor ranges
  2021-07-05 11:07   ` Xueming(Steven) Li
@ 2021-07-10  9:46     ` Thomas Monjalon
  0 siblings, 0 replies; 7+ messages in thread
From: Thomas Monjalon @ 2021-07-10  9:46 UTC (permalink / raw)
  To: Andrew Rybchenko, Viacheslav Galaktionov, Xueming(Steven) Li
  Cc: Matan Azrad, Slava Ovsiienko, Ferruh Yigit, dev, stable

05/07/2021 13:07, Xueming(Steven) Li:
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
> > 
> > In its current state, the API can overflow the user-passed buffer if a new representor range appears between function calls.
> > 
> > In order to solve this problem, augment the representor info structure with the numbers of allocated and initialized ranges. This way
> > the users of this structure can be sure they will not overrun the buffer.
> > 
> > Fixes: 85e1588ca72f ("ethdev: add API to get representor info")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
> > Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> 
> Reviewed-by: Xueming Li <xuemingl@nvidia.com>

Applied, thanks.




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

end of thread, other threads:[~2021-07-10  9:46 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-02 14:23 [dpdk-dev] [PATCH] ethdev: keep count of allocated and used representor ranges Andrew Rybchenko
2021-07-03  1:32 ` Xueming(Steven) Li
2021-07-03 10:01   ` Viacheslav Galaktionov
2021-07-05  2:50     ` Xueming(Steven) Li
2021-07-05 10:02 ` [dpdk-dev] [PATCH v2] " Andrew Rybchenko
2021-07-05 11:07   ` Xueming(Steven) Li
2021-07-10  9:46     ` Thomas Monjalon

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