[PATCH] app/pdump: check lcore is not the maximum core

[PATCH] app/pdump: check lcore is not the maximum core

[Reshma Pattan]

Check lcore id value is not the maximum core supported.
Using lcore id without this check might cause
out of bound access inside the rte_eal_wait_lcore.

Coverity issue: 375841
Fixes: b2854d5317e8 ("app/pdump: support multi-core capture")
Cc: vipin.varghese@intel.com
Cc: stable@dpdk.org

Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
---
 app/pdump/main.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index 04a38e8911..686c27d965 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -931,11 +931,19 @@ dump_packets(void)
 	}
 
 	lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
+	if (lcore_id == RTE_MAX_LCORE) {
+		printf("Invalid core %u for the packet capture!\n", lcore_id);
+		return;
+	}
 
 	for (i = 0; i < num_tuples; i++) {
 		rte_eal_remote_launch(dump_packets_core,
 				&pdump_t[i], lcore_id);
 		lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
+		if (lcore_id == RTE_MAX_LCORE) {
+			printf("Invalid core %u for the capture for the tuple=%d!\n", lcore_id, i);
+			return;
+		}
 
 		if (rte_eal_wait_lcore(lcore_id) < 0)
 			rte_exit(EXIT_FAILURE, "failed to wait\n");
-- 
2.25.1

Re: [PATCH] app/pdump: check lcore is not the maximum core

[Stephen Hemminger]

On Fri, 18 Feb 2022 15:18:41 +0000
Reshma Pattan <reshma.pattan@intel.com> wrote:

>  	lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
> +	if (lcore_id == RTE_MAX_LCORE) {
> +		printf("Invalid core %u for the packet capture!\n", lcore_id);
> +		return;
> +	}

Since nothing useful can be done, maybe rte_exit()?
Or at least print to stderr and return error status.

Also, if you write same code in two places, it should
be a function.

[PATCH v2] app/pdump: check lcore is not the maximum core

[Reshma Pattan]

Check lcore id value is not the maximum core supported.
Using lcore id without this check might cause
out of bound access inside the rte_eal_wait_lcore.

Coverity issue: 375841
Fixes: b2854d5317e8 ("app/pdump: support multi-core capture")
Cc: vipin.varghese@intel.com
Cc: stable@dpdk.org

Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
---
 app/pdump/main.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index 04a38e8911..7677a5f8f5 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -930,12 +930,15 @@ dump_packets(void)
 		return;
 	}
 
-	lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
 
 	for (i = 0; i < num_tuples; i++) {
+		lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
+		if (lcore_id == RTE_MAX_LCORE)
+			rte_exit(EXIT_FAILURE,
+				"Max core limit %u reached for packet capture", lcore_id);
+
 		rte_eal_remote_launch(dump_packets_core,
 				&pdump_t[i], lcore_id);
-		lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
 
 		if (rte_eal_wait_lcore(lcore_id) < 0)
 			rte_exit(EXIT_FAILURE, "failed to wait\n");
-- 
2.27.0

[PATCH v3] app/pdump: check lcore is not the maximum core

[Reshma Pattan]

Check lcore id value is not the maximum core supported.
Using lcore id without this check might cause
out of bound access inside the rte_eal_wait_lcore.

Coverity issue: 375841
Fixes: b2854d5317e8 ("app/pdump: support multi-core capture")
Cc: vipin.varghese@intel.com
Cc: stable@dpdk.org

Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
---
v3: add new function to get next core id and validate it.
---
 app/pdump/main.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index 04a38e8911..e4e62811c9 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -900,6 +900,15 @@ dump_packets_core(void *arg)
 	return 0;
 }
 
+static inline void
+get_next_core(uint32_t *lcore_id)
+{
+	*lcore_id = rte_get_next_lcore(*lcore_id, 1, 0);
+	if (*lcore_id == RTE_MAX_LCORE)
+		rte_exit(EXIT_FAILURE,
+				"Max core limit %u reached for packet capture", *lcore_id);
+}
+
 static inline void
 dump_packets(void)
 {
@@ -930,12 +939,12 @@ dump_packets(void)
 		return;
 	}
 
-	lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
+	get_next_core(&lcore_id);
 
 	for (i = 0; i < num_tuples; i++) {
 		rte_eal_remote_launch(dump_packets_core,
 				&pdump_t[i], lcore_id);
-		lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
+		get_next_core(&lcore_id);
 
 		if (rte_eal_wait_lcore(lcore_id) < 0)
 			rte_exit(EXIT_FAILURE, "failed to wait\n");
-- 
2.25.1

RE: [PATCH v3] app/pdump: check lcore is not the maximum core

[Pattan, Reshma]

> -----Original Message-----
> From: Pattan, Reshma <reshma.pattan@intel.com>

<snip>

Hi Stephen,

Can you take a look and ack the patch.

Thanks,
Reshma

Re: [PATCH v3] app/pdump: check lcore is not the maximum core

[Stephen Hemminger]

On Tue, 22 Feb 2022 11:02:24 +0000
Reshma Pattan <reshma.pattan@intel.com> wrote:

> Check lcore id value is not the maximum core supported.
> Using lcore id without this check might cause
> out of bound access inside the rte_eal_wait_lcore.
> 
> Coverity issue: 375841
> Fixes: b2854d5317e8 ("app/pdump: support multi-core capture")
> Cc: vipin.varghese@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
> ---
> v3: add new function to get next core id and validate it.
> ---
>  app/pdump/main.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/app/pdump/main.c b/app/pdump/main.c
> index 04a38e8911..e4e62811c9 100644
> --- a/app/pdump/main.c
> +++ b/app/pdump/main.c
> @@ -900,6 +900,15 @@ dump_packets_core(void *arg)
>  	return 0;
>  }
>  
> +static inline void
> +get_next_core(uint32_t *lcore_id)
> +{
> +	*lcore_id = rte_get_next_lcore(*lcore_id, 1, 0);
> +	if (*lcore_id == RTE_MAX_LCORE)
> +		rte_exit(EXIT_FAILURE,
> +				"Max core limit %u reached for packet capture", *lcore_id);
> +}
> +

This looks good, can I make a some suggestions.

Since function either exits or returns a good value, I would prefer
that it returned the lcore id. Avoiding call by reference if possible.

Also, the lcore is currently typed unsigned int in rte_lcore.h
therefore use that type?

Inline is certainly unnecessary here, not critical path and compiler
is likely to do it anyway.

Also, DPDK uses lcore for most places (rather than core) so use that name.

Result is:

static unsigned int
get_next_lcore(unsigned int lcore)
{
	lcore = rte_get_next_lcore(lcore, 1, 0);
	if (lcore >= RTE_MAX_LCORE)
			"Max core limit %u reached for packet capture", lcore);
	return lcore;
}

>  static inline void
>  dump_packets(void)
>  {
> @@ -930,12 +939,12 @@ dump_packets(void)
>  		return;
>  	}
>  
> -	lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
> +	get_next_core(&lcore_id);
>  
>  	for (i = 0; i < num_tuples; i++) {
>  		rte_eal_remote_launch(dump_packets_core,
>  				&pdump_t[i], lcore_id);
> -		lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
> +		get_next_core(&lcore_id);
>  
>  		if (rte_eal_wait_lcore(lcore_id) < 0)
>  			rte_exit(EXIT_FAILURE, "failed to wait\n");

[PATCH v4] app/pdump: check lcore is not the maximum core

[Reshma Pattan]

Check lcore id value is not the maximum core supported.
Using lcore id without this check might cause
out of bound access inside the rte_eal_wait_lcore.

Coverity issue: 375841
Fixes: b2854d5317e8 ("app/pdump: support multi-core capture")
Cc: vipin.varghese@intel.com
Cc: stable@dpdk.org

Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
---
v4: Remove inline of new function
    Change lcore type as unsigned int
    return lcore from the function
---
 app/pdump/main.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index 04a38e8911..96fa76f8da 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -900,11 +900,21 @@ dump_packets_core(void *arg)
 	return 0;
 }
 
+static unsigned int
+get_next_core(unsigned int lcore)
+{
+	lcore = rte_get_next_lcore(lcore, 1, 0);
+	if (lcore == RTE_MAX_LCORE)
+		rte_exit(EXIT_FAILURE,
+				"Max core limit %u reached for packet capture", lcore);
+	return lcore;
+}
+
 static inline void
 dump_packets(void)
 {
 	int i;
-	uint32_t lcore_id = 0;
+	unsigned int lcore_id = 0;
 
 	if (!multiple_core_capture) {
 		printf(" core (%u), capture for (%d) tuples\n",
@@ -930,12 +940,12 @@ dump_packets(void)
 		return;
 	}
 
-	lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
+	lcore_id = get_next_core(lcore_id);
 
 	for (i = 0; i < num_tuples; i++) {
 		rte_eal_remote_launch(dump_packets_core,
 				&pdump_t[i], lcore_id);
-		lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
+		lcore_id = get_next_core(lcore_id);
 
 		if (rte_eal_wait_lcore(lcore_id) < 0)
 			rte_exit(EXIT_FAILURE, "failed to wait\n");
-- 
2.25.1

Re: [PATCH v4] app/pdump: check lcore is not the maximum core

[Stephen Hemminger]

On Mon, 28 Feb 2022 09:58:56 +0000
Reshma Pattan <reshma.pattan@intel.com> wrote:

> Check lcore id value is not the maximum core supported.
> Using lcore id without this check might cause
> out of bound access inside the rte_eal_wait_lcore.
> 
> Coverity issue: 375841
> Fixes: b2854d5317e8 ("app/pdump: support multi-core capture")
> Cc: vipin.varghese@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>

Great thanks.

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

RE: [PATCH v4] app/pdump: check lcore is not the maximum core

[Mcnamara, John]

Thomas, could you include this in RC3 if possible.

> -----Original Message-----
> From: Reshma Pattan <reshma.pattan@intel.com>
> Sent: Monday, February 28, 2022 9:59 AM
> To: dev@dpdk.org
> Cc: stephen@networkplumber.org; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Pattan, Reshma <reshma.pattan@intel.com>; vipin.varghese@intel.com;
> stable@dpdk.org
> Subject: [PATCH v4] app/pdump: check lcore is not the maximum core
> 
> Check lcore id value is not the maximum core supported.
> Using lcore id without this check might cause out of bound access inside
> the rte_eal_wait_lcore.
> 
> Coverity issue: 375841
> Fixes: b2854d5317e8 ("app/pdump: support multi-core capture")
> Cc: vipin.varghese@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
> ---
> v4: Remove inline of new function
>     Change lcore type as unsigned int
>     return lcore from the function
> ---
>  app/pdump/main.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/app/pdump/main.c b/app/pdump/main.c index
> 04a38e8911..96fa76f8da 100644
> --- a/app/pdump/main.c
> +++ b/app/pdump/main.c
> @@ -900,11 +900,21 @@ dump_packets_core(void *arg)
>  	return 0;
>  }
> 
> +static unsigned int
> +get_next_core(unsigned int lcore)
> +{
> +	lcore = rte_get_next_lcore(lcore, 1, 0);
> +	if (lcore == RTE_MAX_LCORE)
> +		rte_exit(EXIT_FAILURE,
> +				"Max core limit %u reached for packet capture",
> lcore);
> +	return lcore;
> +}
> +
>  static inline void
>  dump_packets(void)
>  {
>  	int i;
> -	uint32_t lcore_id = 0;
> +	unsigned int lcore_id = 0;
> 
>  	if (!multiple_core_capture) {
>  		printf(" core (%u), capture for (%d) tuples\n", @@ -930,12
> +940,12 @@ dump_packets(void)
>  		return;
>  	}
> 
> -	lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
> +	lcore_id = get_next_core(lcore_id);
> 
>  	for (i = 0; i < num_tuples; i++) {
>  		rte_eal_remote_launch(dump_packets_core,
>  				&pdump_t[i], lcore_id);
> -		lcore_id = rte_get_next_lcore(lcore_id, 1, 0);
> +		lcore_id = get_next_core(lcore_id);
> 
>  		if (rte_eal_wait_lcore(lcore_id) < 0)
>  			rte_exit(EXIT_FAILURE, "failed to wait\n");
> --
> 2.25.1

Re: [PATCH v4] app/pdump: check lcore is not the maximum core

[Thomas Monjalon]

08/03/2022 14:47, Mcnamara, John:
> Thomas, could you include this in RC3 if possible.

I did already yesterday:
	https://git.dpdk.org/dpdk/commit/?id=3ee04ebc91

Looks like I forgot to send an email.

Applied, thanks :)

RE: [PATCH v4] app/pdump: check lcore is not the maximum core

[Mcnamara, John]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Subject: Re: [PATCH v4] app/pdump: check lcore is not the maximum core
> 
> 08/03/2022 14:47, Mcnamara, John:
> > Thomas, could you include this in RC3 if possible.
> 
> I did already yesterday:
> 	https://git.dpdk.org/dpdk/commit/?id=3ee04ebc91
> 
> Looks like I forgot to send an email.
> 
> Applied, thanks :)

Thank you. :-)