[dpdk-dev] [PATCH 00/10] fixes for clean code

[dpdk-dev] [PATCH 00/10] fixes for clean code

[Min Hu (Connor)]

This patchset fix some bugs, to make the code more clean.

Chengchang Tang (1):
  net/bonding: fix configuration assignment overflow

HongBo Zheng (5):
  app/test: add NULL pointer check of memory allocation
  lib/librte_pipeline: fix the use of unsafe strcpy
  examples/l3fwd: add function return value check
  crypto/virtio: fix return values check error
  net/e1000: add function return value check

Min Hu (Connor) (4):
  net/pfe: check return value
  common/sfc_efx/base: delete redundant handling
  bus/dpaa: fix management command init calling
  app/regex: fix division by zero

 app/test-regex/main.c                     | 8 +++++---
 app/test/test_crc.c                       | 2 ++
 drivers/bus/dpaa/base/qbman/bman.c        | 6 ++----
 drivers/common/sfc_efx/base/rhead_nic.c   | 3 +--
 drivers/crypto/virtio/virtio_rxtx.c       | 4 ++--
 drivers/net/bonding/rte_eth_bond_8023ad.c | 2 +-
 drivers/net/e1000/base/e1000_ich8lan.c    | 2 ++
 drivers/net/pfe/pfe_ethdev.c              | 6 ++++++
 examples/l3fwd/l3fwd_event.c              | 6 +++++-
 lib/librte_pipeline/rte_swx_pipeline.c    | 4 ++--
 10 files changed, 28 insertions(+), 15 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH 01/10] net/pfe: check return value

[Min Hu (Connor)]

Variable 'fd', which may receive negative value when open
"/dev/mem" file.

This patch added checking return value process.

Fixes: 67fc3ff97c39 ("net/pfe: introduce basic functions")
Cc: stable@dpdk.org

Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 drivers/net/pfe/pfe_ethdev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/pfe/pfe_ethdev.c b/drivers/net/pfe/pfe_ethdev.c
index 8cf59e2..b4f5591 100644
--- a/drivers/net/pfe/pfe_ethdev.c
+++ b/drivers/net/pfe/pfe_ethdev.c
@@ -1049,6 +1049,12 @@ pmd_pfe_probe(struct rte_vdev_device *vdev)
 	g_pfe->cbus_size = cbus_size;
 
 	fd = open("/dev/mem", O_RDWR);
+	if (fd < 0) {
+		PFE_PMD_ERR("Can not open /dev/mem");
+		rc = -EIO;
+		goto err;
+	}
+
 	g_pfe->cbus_baseaddr = mmap(NULL, cbus_size, PROT_READ | PROT_WRITE,
 					MAP_SHARED, fd, cbus_addr);
 	close(fd);
-- 
2.7.4

Re: [dpdk-dev] [PATCH 01/10] net/pfe: check return value

[Stephen Hemminger]

On Mon, 19 Apr 2021 21:34:40 +0800
"Min Hu (Connor)" <humin29@huawei.com> wrote:

>  
>  	fd = open("/dev/mem", O_RDWR);
> +	if (fd < 0) {
> +		PFE_PMD_ERR("Can not open /dev/mem");
> +		rc = -EIO;
> +		goto err;
> +	}
> +

This patch makes sense and should be applied.
But the errno is most like EPERM so maybe:
	rc = -errno;

PS: /dev/mem is a bad idea, Linux kernel config often disables
it in distributions. Not sure why this driver can't use UIO or VFIO
like normal devices. This should have been caught during code review.

[dpdk-dev] [PATCH 02/10] common/sfc_efx/base: delete redundant handling

[Min Hu (Connor)]

the default case in 'rhead_nic_get_bar_region' is unreachable.

This patch fixed that.

Fixes: 3c1c5cc4a786 ("common/sfc_efx/base: add Riverhead support to NIC module")
Cc: stable@dpdk.org

Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 drivers/common/sfc_efx/base/rhead_nic.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/common/sfc_efx/base/rhead_nic.c b/drivers/common/sfc_efx/base/rhead_nic.c
index f2c18c1..b9af348 100644
--- a/drivers/common/sfc_efx/base/rhead_nic.c
+++ b/drivers/common/sfc_efx/base/rhead_nic.c
@@ -483,8 +483,7 @@ rhead_nic_get_bar_region(
 		break;
 
 	default:
-		rc = EINVAL;
-		goto fail1;
+		break;
 	}
 
 	return (0);
-- 
2.7.4

Re: [dpdk-dev] [PATCH 02/10] common/sfc_efx/base: delete redundant handling

[Andrew Rybchenko]

On 4/19/21 4:34 PM, Min Hu (Connor) wrote:
> the default case in 'rhead_nic_get_bar_region' is unreachable.

Why? May be it is true right now, but default case is required
to handle future changes in enum and missing update here.

> 
> This patch fixed that.
> 
> Fixes: 3c1c5cc4a786 ("common/sfc_efx/base: add Riverhead support to NIC module")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>

Nack

> ---
>  drivers/common/sfc_efx/base/rhead_nic.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/common/sfc_efx/base/rhead_nic.c b/drivers/common/sfc_efx/base/rhead_nic.c
> index f2c18c1..b9af348 100644
> --- a/drivers/common/sfc_efx/base/rhead_nic.c
> +++ b/drivers/common/sfc_efx/base/rhead_nic.c
> @@ -483,8 +483,7 @@ rhead_nic_get_bar_region(
>  		break;
>  
>  	default:
> -		rc = EINVAL;
> -		goto fail1;
> +		break;
>  	}
>  
>  	return (0);
> 

Re: [dpdk-dev] [PATCH 02/10] common/sfc_efx/base: delete redundant handling

[Min Hu (Connor)]

在 2021/4/20 17:33, Andrew Rybchenko 写道:
> On 4/19/21 4:34 PM, Min Hu (Connor) wrote:
>> the default case in 'rhead_nic_get_bar_region' is unreachable.
> 
> Why? May be it is true right now, but default case is required
> to handle future changes in enum and missing update here.
> 
Well, agreed, this patch can be abandoned.
>>
>> This patch fixed that.
>>
>> Fixes: 3c1c5cc4a786 ("common/sfc_efx/base: add Riverhead support to NIC module")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> 
> Nack
> 
>> ---
>>   drivers/common/sfc_efx/base/rhead_nic.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/common/sfc_efx/base/rhead_nic.c b/drivers/common/sfc_efx/base/rhead_nic.c
>> index f2c18c1..b9af348 100644
>> --- a/drivers/common/sfc_efx/base/rhead_nic.c
>> +++ b/drivers/common/sfc_efx/base/rhead_nic.c
>> @@ -483,8 +483,7 @@ rhead_nic_get_bar_region(
>>   		break;
>>   
>>   	default:
>> -		rc = EINVAL;
>> -		goto fail1;
>> +		break;
>>   	}
>>   
>>   	return (0);
>>
> 
> .
> 

[dpdk-dev] [PATCH 03/10] bus/dpaa: fix management command init calling

[Min Hu (Connor)]

'bm_mc_init' only return 0, but the function whicl calls int
check the negative ret, and this is redundant.

This patch fixed it by not checking the return value.

Fixes: f38f61e982f8 ("bus/dpaa: add BMAN hardware interfaces")
Cc: stable@dpdk.org

Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 drivers/bus/dpaa/base/qbman/bman.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/bus/dpaa/base/qbman/bman.c b/drivers/bus/dpaa/base/qbman/bman.c
index 8a62907..e1ba2a8 100644
--- a/drivers/bus/dpaa/base/qbman/bman.c
+++ b/drivers/bus/dpaa/base/qbman/bman.c
@@ -70,10 +70,8 @@ struct bman_portal *bman_create_portal(struct bman_portal *portal,
 		pr_err("Bman RCR initialisation failed\n");
 		return NULL;
 	}
-	if (bm_mc_init(p)) {
-		pr_err("Bman MC initialisation failed\n");
-		goto fail_mc;
-	}
+	(void)bm_mc_init(p);
+
 	portal->pools = kmalloc(2 * sizeof(*pools), GFP_KERNEL);
 	if (!portal->pools)
 		goto fail_pools;
-- 
2.7.4

Re: [dpdk-dev] [PATCH 03/10] bus/dpaa: fix management command init calling

[Andrew Rybchenko]

On 4/19/21 4:34 PM, Min Hu (Connor) wrote:
> 'bm_mc_init' only return 0, but the function whicl calls int
> check the negative ret, and this is redundant.
> 
> This patch fixed it by not checking the return value.
> 
> Fixes: f38f61e982f8 ("bus/dpaa: add BMAN hardware interfaces")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>  drivers/bus/dpaa/base/qbman/bman.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bus/dpaa/base/qbman/bman.c b/drivers/bus/dpaa/base/qbman/bman.c
> index 8a62907..e1ba2a8 100644
> --- a/drivers/bus/dpaa/base/qbman/bman.c
> +++ b/drivers/bus/dpaa/base/qbman/bman.c
> @@ -70,10 +70,8 @@ struct bman_portal *bman_create_portal(struct bman_portal *portal,
>  		pr_err("Bman RCR initialisation failed\n");
>  		return NULL;
>  	}
> -	if (bm_mc_init(p)) {
> -		pr_err("Bman MC initialisation failed\n");
> -		goto fail_mc;
> -	}
> +	(void)bm_mc_init(p);
> +

As I understand compiler can do it for you and there is no
point to break the code in the case of future changes if
bm_mc_init() starts to return errors.

>  	portal->pools = kmalloc(2 * sizeof(*pools), GFP_KERNEL);
>  	if (!portal->pools)
>  		goto fail_pools;
> 

Re: [dpdk-dev] [PATCH 03/10] bus/dpaa: fix management command init calling

[Min Hu (Connor)]

在 2021/4/20 17:35, Andrew Rybchenko 写道:
> On 4/19/21 4:34 PM, Min Hu (Connor) wrote:
>> 'bm_mc_init' only return 0, but the function whicl calls int
>> check the negative ret, and this is redundant.
>>
>> This patch fixed it by not checking the return value.
>>
>> Fixes: f38f61e982f8 ("bus/dpaa: add BMAN hardware interfaces")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>>   drivers/bus/dpaa/base/qbman/bman.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/bus/dpaa/base/qbman/bman.c b/drivers/bus/dpaa/base/qbman/bman.c
>> index 8a62907..e1ba2a8 100644
>> --- a/drivers/bus/dpaa/base/qbman/bman.c
>> +++ b/drivers/bus/dpaa/base/qbman/bman.c
>> @@ -70,10 +70,8 @@ struct bman_portal *bman_create_portal(struct bman_portal *portal,
>>   		pr_err("Bman RCR initialisation failed\n");
>>   		return NULL;
>>   	}
>> -	if (bm_mc_init(p)) {
>> -		pr_err("Bman MC initialisation failed\n");
>> -		goto fail_mc;
>> -	}
>> +	(void)bm_mc_init(p);
>> +
> 
> As I understand compiler can do it for you and there is no
> point to break the code in the case of future changes if
> bm_mc_init() starts to return errors.
> 
Agreed, this patch can be abandoned.
>>   	portal->pools = kmalloc(2 * sizeof(*pools), GFP_KERNEL);
>>   	if (!portal->pools)
>>   		goto fail_pools;
>>
> 
> .
> 

[dpdk-dev] [PATCH 04/10] app/regex: fix division by zero

[Min Hu (Connor)]

Variable nb_jobs, which may be zero, is used as a denominator.

This patch fixed it.

Fixes: f5cffb7eb7fb ("app/regex: read data file once at startup")
Cc: stable@dpdk.org

Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 app/test-regex/main.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/app/test-regex/main.c b/app/test-regex/main.c
index 8e665df..b49fa88 100644
--- a/app/test-regex/main.c
+++ b/app/test-regex/main.c
@@ -725,9 +725,11 @@ main(int argc, char **argv)
 	if (data_len <= 0)
 		rte_exit(EXIT_FAILURE, "Error, can't read file, or file is empty.\n");
 
-	job_len = data_len / nb_jobs;
-	if (job_len == 0)
-		rte_exit(EXIT_FAILURE, "Error, To many jobs, for the given input.\n");
+	if (!nb_jobs) {
+		job_len = data_len / nb_jobs;
+		if (job_len == 0)
+			rte_exit(EXIT_FAILURE, "Error, To many jobs, for the given input.\n");
+	}
 
 	if (job_len > nb_max_payload)
 		rte_exit(EXIT_FAILURE, "Error, not enough jobs to cover input.\n");
-- 
2.7.4

Re: [dpdk-dev] [PATCH 04/10] app/regex: fix division by zero

[Ori Kam]

Hi Min,

> -----Original Message-----
> From: Min Hu (Connor) <humin29@huawei.com>
> Sent: Monday, April 19, 2021 4:35 PM
> Subject: [PATCH 04/10] app/regex: fix division by zero
> 
> Variable nb_jobs, which may be zero, is used as a denominator.
> 
> This patch fixed it.
> 
> Fixes: f5cffb7eb7fb ("app/regex: read data file once at startup")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>  app/test-regex/main.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/app/test-regex/main.c b/app/test-regex/main.c
> index 8e665df..b49fa88 100644
> --- a/app/test-regex/main.c
> +++ b/app/test-regex/main.c
> @@ -725,9 +725,11 @@ main(int argc, char **argv)
>  	if (data_len <= 0)
>  		rte_exit(EXIT_FAILURE, "Error, can't read file, or file is
> empty.\n");
> 
> -	job_len = data_len / nb_jobs;
> -	if (job_len == 0)
> -		rte_exit(EXIT_FAILURE, "Error, To many jobs, for the given
> input.\n");
> +	if (!nb_jobs) {
> +		job_len = data_len / nb_jobs;
> +		if (job_len == 0)
> +			rte_exit(EXIT_FAILURE, "Error, To many jobs, for the
> given input.\n");
> +	}
> 
>  	if (job_len > nb_max_payload)
>  		rte_exit(EXIT_FAILURE, "Error, not enough jobs to cover
> input.\n");
> --
> 2.7.4

Acked-by: Ori Kam <orika@nvidia.com>
Thanks,
Ori

[dpdk-dev] [PATCH 05/10] app/test: add null pointer check of memory allocation

[Min Hu (Connor)]

From: HongBo Zheng <zhenghongbo3@huawei.com>

The rte_zmalloc is called in test_crc_calc without null pointer
check. This patch adds null pointer checks on return value of
rte_zmalloc.

Fixes: 9c77b848b1c1 ("test: add CRC computation")
Cc: stable@dpdk.org

Signed-off-by: HongBo Zheng <zhenghongbo3@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 app/test/test_crc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/test/test_crc.c b/app/test/test_crc.c
index bf1d344..8231f81 100644
--- a/app/test/test_crc.c
+++ b/app/test/test_crc.c
@@ -80,6 +80,8 @@ test_crc_calc(void)
 
 	/* 32-bit ethernet CRC: Test 2 */
 	test_data = rte_zmalloc(NULL, CRC32_VEC_LEN1, 0);
+	if (test_data == NULL)
+		return -7;
 
 	for (i = 0; i < CRC32_VEC_LEN1; i += 12)
 		rte_memcpy(&test_data[i], crc32_vec1, 12);
-- 
2.7.4

Re: [dpdk-dev] [PATCH 05/10] app/test: add null pointer check of memory allocation

[Thomas Monjalon]

19/04/2021 15:34, Min Hu (Connor):
> From: HongBo Zheng <zhenghongbo3@huawei.com>
> 
> The rte_zmalloc is called in test_crc_calc without null pointer
> check. This patch adds null pointer checks on return value of
> rte_zmalloc.
> 
> Fixes: 9c77b848b1c1 ("test: add CRC computation")
> Cc: stable@dpdk.org
> 
> Signed-off-by: HongBo Zheng <zhenghongbo3@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>

Applied only this patch, thanks.

[dpdk-dev] [PATCH 06/10] lib/librte_pipeline: fix the use of unsafe strcpy

[Min Hu (Connor)]

From: HongBo Zheng <zhenghongbo3@huawei.com>

'strcpy' is called in rte_swx_ctl_table_info_get, this function
is unsafe, use 'strncpy' instead.

Fixes: 393b96e2aa2a ("pipeline: add SWX pipeline query API")
Cc: stable@dpdk.org

Signed-off-by: HongBo Zheng <zhenghongbo3@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 lib/librte_pipeline/rte_swx_pipeline.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_pipeline/rte_swx_pipeline.c b/lib/librte_pipeline/rte_swx_pipeline.c
index 4455d91..d4db4dd 100644
--- a/lib/librte_pipeline/rte_swx_pipeline.c
+++ b/lib/librte_pipeline/rte_swx_pipeline.c
@@ -9447,8 +9447,8 @@ rte_swx_ctl_table_info_get(struct rte_swx_pipeline *p,
 	if (!t)
 		return -EINVAL;
 
-	strcpy(table->name, t->name);
-	strcpy(table->args, t->args);
+	strncpy(table->name, t->name, RTE_SWX_CTL_NAME_SIZE);
+	strncpy(table->args, t->args, RTE_SWX_CTL_NAME_SIZE);
 	table->n_match_fields = t->n_fields;
 	table->n_actions = t->n_actions;
 	table->default_action_is_const = t->default_action_is_const;
-- 
2.7.4

Re: [dpdk-dev] [PATCH 06/10] lib/librte_pipeline: fix the use of unsafe strcpy

[Andrew Rybchenko]

On 4/19/21 4:34 PM, Min Hu (Connor) wrote:
> From: HongBo Zheng <zhenghongbo3@huawei.com>
> 
> 'strcpy' is called in rte_swx_ctl_table_info_get, this function
> is unsafe, use 'strncpy' instead.
> 
> Fixes: 393b96e2aa2a ("pipeline: add SWX pipeline query API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: HongBo Zheng <zhenghongbo3@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>  lib/librte_pipeline/rte_swx_pipeline.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_pipeline/rte_swx_pipeline.c b/lib/librte_pipeline/rte_swx_pipeline.c
> index 4455d91..d4db4dd 100644
> --- a/lib/librte_pipeline/rte_swx_pipeline.c
> +++ b/lib/librte_pipeline/rte_swx_pipeline.c
> @@ -9447,8 +9447,8 @@ rte_swx_ctl_table_info_get(struct rte_swx_pipeline *p,
>  	if (!t)
>  		return -EINVAL;
>  
> -	strcpy(table->name, t->name);
> -	strcpy(table->args, t->args);
> +	strncpy(table->name, t->name, RTE_SWX_CTL_NAME_SIZE);
> +	strncpy(table->args, t->args, RTE_SWX_CTL_NAME_SIZE);

strlcpy() should be used in fact, since strncpy() has problems
as well.

>  	table->n_match_fields = t->n_fields;
>  	table->n_actions = t->n_actions;
>  	table->default_action_is_const = t->default_action_is_const;
> 

Re: [dpdk-dev] [PATCH 06/10] lib/librte_pipeline: fix the use of unsafe strcpy

[Stephen Hemminger]

On Mon, 19 Apr 2021 21:34:45 +0800
"Min Hu (Connor)" <humin29@huawei.com> wrote:

> From: HongBo Zheng <zhenghongbo3@huawei.com>
> 
> 'strcpy' is called in rte_swx_ctl_table_info_get, this function
> is unsafe, use 'strncpy' instead.
> 
> Fixes: 393b96e2aa2a ("pipeline: add SWX pipeline query API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: HongBo Zheng <zhenghongbo3@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>  lib/librte_pipeline/rte_swx_pipeline.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_pipeline/rte_swx_pipeline.c b/lib/librte_pipeline/rte_swx_pipeline.c
> index 4455d91..d4db4dd 100644
> --- a/lib/librte_pipeline/rte_swx_pipeline.c
> +++ b/lib/librte_pipeline/rte_swx_pipeline.c
> @@ -9447,8 +9447,8 @@ rte_swx_ctl_table_info_get(struct rte_swx_pipeline *p,
>  	if (!t)
>  		return -EINVAL;
>  
> -	strcpy(table->name, t->name);
> -	strcpy(table->args, t->args);
> +	strncpy(table->name, t->name, RTE_SWX_CTL_NAME_SIZE);
> +	strncpy(table->args, t->args, RTE_SWX_CTL_NAME_SIZE);
>  	table->n_match_fields = t->n_fields;
>  	table->n_actions = t->n_actions;
>  	table->default_action_is_const = t->default_action_is_const;

This patch is unnecessary.
Both structures declare the same size for the name and args.
Therefore the strcpy is always safe as long as the table structure
is correctly setup with null terminated string. If not there are worse bugs.

RE: [dpdk-dev] [PATCH 06/10] lib/librte_pipeline: fix the use of unsafe strcpy

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, June 30, 2023 7:09 PM
> To: Min Hu (Connor) <humin29@huawei.com>
> Cc: dev@dpdk.org; ferruh.yigit@intel.com; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; jerinj@marvell.com;
> jianjay.zhou@huawei.com; jia.guo@intel.com; g.singh@nxp.com;
> andrew.rybchenko@oktetlabs.ru; hemant.agrawal@nxp.com; orika@nvidia.com
> Subject: Re: [dpdk-dev] [PATCH 06/10] lib/librte_pipeline: fix the use of unsafe
> strcpy
> 
> On Mon, 19 Apr 2021 21:34:45 +0800
> "Min Hu (Connor)" <humin29@huawei.com> wrote:
> 
> > From: HongBo Zheng <zhenghongbo3@huawei.com>
> >
> > 'strcpy' is called in rte_swx_ctl_table_info_get, this function
> > is unsafe, use 'strncpy' instead.
> >
> > Fixes: 393b96e2aa2a ("pipeline: add SWX pipeline query API")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: HongBo Zheng <zhenghongbo3@huawei.com>
> > Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> > ---
> >  lib/librte_pipeline/rte_swx_pipeline.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_pipeline/rte_swx_pipeline.c
> b/lib/librte_pipeline/rte_swx_pipeline.c
> > index 4455d91..d4db4dd 100644
> > --- a/lib/librte_pipeline/rte_swx_pipeline.c
> > +++ b/lib/librte_pipeline/rte_swx_pipeline.c
> > @@ -9447,8 +9447,8 @@ rte_swx_ctl_table_info_get(struct rte_swx_pipeline
> *p,
> >  	if (!t)
> >  		return -EINVAL;
> >
> > -	strcpy(table->name, t->name);
> > -	strcpy(table->args, t->args);
> > +	strncpy(table->name, t->name, RTE_SWX_CTL_NAME_SIZE);
> > +	strncpy(table->args, t->args, RTE_SWX_CTL_NAME_SIZE);
> >  	table->n_match_fields = t->n_fields;
> >  	table->n_actions = t->n_actions;
> >  	table->default_action_is_const = t->default_action_is_const;
> 
> This patch is unnecessary.
> Both structures declare the same size for the name and args.
> Therefore the strcpy is always safe as long as the table structure
> is correctly setup with null terminated string. If not there are worse bugs.


+1
Agree with Steve, this is not necessary here.

[dpdk-dev] [PATCH 07/10] examples/l3fwd: add function return value check

[Min Hu (Connor)]

From: HongBo Zheng <zhenghongbo3@huawei.com>

Return value of a function 'rte_eth_macaddr_get' called at
l3fwd_eth_dev_port_setup is not checked, but it is usually
checked for this function.

This patch fix this problem.

Fixes: a65bf3d724df ("examples/l3fwd: add ethdev setup based on eventdev")
Cc: stable@dpdk.org

Signed-off-by: HongBo Zheng <zhenghongbo3@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 examples/l3fwd/l3fwd_event.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/examples/l3fwd/l3fwd_event.c b/examples/l3fwd/l3fwd_event.c
index 4d31593..7f704f9 100644
--- a/examples/l3fwd/l3fwd_event.c
+++ b/examples/l3fwd/l3fwd_event.c
@@ -105,7 +105,11 @@ l3fwd_eth_dev_port_setup(struct rte_eth_conf *port_conf)
 				 "Cannot adjust number of descriptors: err=%d, "
 				 "port=%d\n", ret, port_id);
 
-		rte_eth_macaddr_get(port_id, &ports_eth_addr[port_id]);
+		ret = rte_eth_macaddr_get(port_id, &ports_eth_addr[port_id]);
+		if (ret < 0)
+			rte_exit(EXIT_FAILURE,
+				 "Cannot get MAC address: err=%d, port=%d\n",
+				 ret, port_id);
 		print_ethaddr(" Address:", &ports_eth_addr[port_id]);
 		printf(", ");
 		print_ethaddr("Destination:",
-- 
2.7.4

Re: [dpdk-dev] [PATCH 07/10] examples/l3fwd: add function return value check

[Stephen Hemminger]

On Mon, 19 Apr 2021 21:34:46 +0800
"Min Hu (Connor)" <humin29@huawei.com> wrote:

> From: HongBo Zheng <zhenghongbo3@huawei.com>
> 
> Return value of a function 'rte_eth_macaddr_get' called at
> l3fwd_eth_dev_port_setup is not checked, but it is usually
> checked for this function.
> 
> This patch fix this problem.
> 
> Fixes: a65bf3d724df ("examples/l3fwd: add ethdev setup based on eventdev")
> Cc: stable@dpdk.org
> 
> Signed-off-by: HongBo Zheng <zhenghongbo3@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>

This looks correct, but only a buggy driver would never set macaddr.

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

[dpdk-dev] [PATCH 08/10] crypto/virtio: fix return values check error

[Min Hu (Connor)]

From: HongBo Zheng <zhenghongbo3@huawei.com>

In virtio_crypto_pkt_tx_burst, we check the return values of
virtqueue_crypto_enqueue_xmit, which may returns -ENOSPC/-EMSGSIZE,
but we only check ENOSPC/EMSGSIZE, and cause the result of checks
is always false.

This patch fix this problem.

Fixes: 82adb12a1fce ("crypto/virtio: support burst enqueue/dequeue")
Cc: stable@dpdk.org

Signed-off-by: HongBo Zheng <zhenghongbo3@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 drivers/crypto/virtio/virtio_rxtx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/virtio/virtio_rxtx.c b/drivers/crypto/virtio/virtio_rxtx.c
index e1cb4ad..a35a5b0 100644
--- a/drivers/crypto/virtio/virtio_rxtx.c
+++ b/drivers/crypto/virtio/virtio_rxtx.c
@@ -500,10 +500,10 @@ virtio_crypto_pkt_tx_burst(void *tx_queue, struct rte_crypto_op **tx_pkts,
 		/* Enqueue Packet buffers */
 		error = virtqueue_crypto_enqueue_xmit(txvq, tx_pkts[nb_tx]);
 		if (unlikely(error)) {
-			if (error == ENOSPC)
+			if (error == -ENOSPC)
 				VIRTIO_CRYPTO_TX_LOG_ERR(
 					"virtqueue_enqueue Free count = 0");
-			else if (error == EMSGSIZE)
+			else if (error == -EMSGSIZE)
 				VIRTIO_CRYPTO_TX_LOG_ERR(
 					"virtqueue_enqueue Free count < 1");
 			else
-- 
2.7.4

Re: [dpdk-dev] [PATCH 08/10] crypto/virtio: fix return values check error

[Stephen Hemminger]

On Mon, 19 Apr 2021 21:34:47 +0800
"Min Hu (Connor)" <humin29@huawei.com> wrote:

> From: HongBo Zheng <zhenghongbo3@huawei.com>
> 
> In virtio_crypto_pkt_tx_burst, we check the return values of
> virtqueue_crypto_enqueue_xmit, which may returns -ENOSPC/-EMSGSIZE,
> but we only check ENOSPC/EMSGSIZE, and cause the result of checks
> is always false.
> 
> This patch fix this problem.
> 
> Fixes: 82adb12a1fce ("crypto/virtio: support burst enqueue/dequeue")
> Cc: stable@dpdk.org
> 
> Signed-off-by: HongBo Zheng <zhenghongbo3@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>

This patch looks correct.

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

[dpdk-dev] [PATCH 09/10] net/e1000: add function return value check

[Min Hu (Connor)]

From: HongBo Zheng <zhenghongbo3@huawei.com>

Return value of a function 'e1000_phy_has_link_generic'
called at e1000_kmrn_lock_loss_workaround_ich8lan is not
checked, but it is usually checked for this function.

This patch fix this problem.

Fixes: 5a32a257f957 ("e1000: more NICs in base driver")
Cc: stable@dpdk.org

Signed-off-by: HongBo Zheng <zhenghongbo3@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 drivers/net/e1000/base/e1000_ich8lan.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/e1000/base/e1000_ich8lan.c b/drivers/net/e1000/base/e1000_ich8lan.c
index 14f86b7..67adc46 100644
--- a/drivers/net/e1000/base/e1000_ich8lan.c
+++ b/drivers/net/e1000/base/e1000_ich8lan.c
@@ -5406,6 +5406,8 @@ STATIC s32 e1000_kmrn_lock_loss_workaround_ich8lan(struct e1000_hw *hw)
 	 * stability
 	 */
 	ret_val = e1000_phy_has_link_generic(hw, 1, 0, &link);
+	if (ret_val)
+		return ret_val;
 	if (!link)
 		return E1000_SUCCESS;
 
-- 
2.7.4

[dpdk-dev] [PATCH 10/10] net/bonding: fix configuration assignment overflow

[Min Hu (Connor)]

From: Chengchang Tang <tangchengchang@huawei.com>

The expression may cause an overflow.

This patch fix the codeDEX static check warning "INTEGER_OVERFLOW".

Fixes: 46fb43683679 ("bond: add mode 4")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 drivers/net/bonding/rte_eth_bond_8023ad.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
index 128754f..483f082 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.c
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
@@ -1237,7 +1237,7 @@ bond_mode_8023ad_conf_assign(struct mode8023ad_private *mode4,
 	mode4->aggregate_wait_timeout = conf->aggregate_wait_timeout_ms * ms_ticks;
 	mode4->tx_period_timeout = conf->tx_period_ms * ms_ticks;
 	mode4->rx_marker_timeout = conf->rx_marker_period_ms * ms_ticks;
-	mode4->update_timeout_us = conf->update_timeout_ms * 1000;
+	mode4->update_timeout_us = (uint64_t)conf->update_timeout_ms * 1000;
 
 	mode4->dedicated_queues.enabled = 0;
 	mode4->dedicated_queues.rx_qid = UINT16_MAX;
-- 
2.7.4

Re: [dpdk-dev] [PATCH 10/10] net/bonding: fix configuration assignment overflow

[Stephen Hemminger]

On Mon, 19 Apr 2021 21:34:49 +0800
"Min Hu (Connor)" <humin29@huawei.com> wrote:

> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> The expression may cause an overflow.
> 
> This patch fix the codeDEX static check warning "INTEGER_OVERFLOW".
> 
> Fixes: 46fb43683679 ("bond: add mode 4")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
Is the codeDEX static checker publicly available?
Would be good to add to CI infrastructure.


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

Re: [dpdk-dev] [PATCH 10/10] net/bonding: fix configuration assignment overflow

[Stephen Hemminger]

On Mon, 19 Apr 2021 21:34:49 +0800
"Min Hu (Connor)" <humin29@huawei.com> wrote:

> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> The expression may cause an overflow.
> 
> This patch fix the codeDEX static check warning "INTEGER_OVERFLOW".
> 
> Fixes: 46fb43683679 ("bond: add mode 4")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>

Several of these patches in this set have been applied.
But the rest need to be resubmitted and the CI test failures need to be fixed.

Re: [dpdk-dev] [PATCH 10/10] net/bonding: fix configuration assignment overflow

[lihuisong (C)]

Hi Stephen,

Thanks for your review this series again.


在 2024/10/4 0:03, Stephen Hemminger 写道:
> On Mon, 19 Apr 2021 21:34:49 +0800
> "Min Hu (Connor)" <humin29@huawei.com> wrote:
>
>> From: Chengchang Tang <tangchengchang@huawei.com>
>>
>> The expression may cause an overflow.
>>
>> This patch fix the codeDEX static check warning "INTEGER_OVERFLOW".
>>
>> Fixes: 46fb43683679 ("bond: add mode 4")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> Several of these patches in this set have been applied.
> But the rest need to be resubmitted and the CI test failures need to be fixed.

Can you past a link or log about the CI test failures so as to fix it?

> .

Re: [dpdk-dev] [PATCH 10/10] net/bonding: fix configuration assignment overflow

[Stephen Hemminger]

On Mon, 19 Apr 2021 21:34:49 +0800
"Min Hu (Connor)" <humin29@huawei.com> wrote:

> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> The expression may cause an overflow.
> 
> This patch fix the codeDEX static check warning "INTEGER_OVERFLOW".
> 
> Fixes: 46fb43683679 ("bond: add mode 4")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>  drivers/net/bonding/rte_eth_bond_8023ad.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c b/drivers/net/bonding/rte_eth_bond_8023ad.c
> index 128754f..483f082 100644
> --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
> @@ -1237,7 +1237,7 @@ bond_mode_8023ad_conf_assign(struct mode8023ad_private *mode4,
>  	mode4->aggregate_wait_timeout = conf->aggregate_wait_timeout_ms * ms_ticks;
>  	mode4->tx_period_timeout = conf->tx_period_ms * ms_ticks;
>  	mode4->rx_marker_timeout = conf->rx_marker_period_ms * ms_ticks;
> -	mode4->update_timeout_us = conf->update_timeout_ms * 1000;
> +	mode4->update_timeout_us = (uint64_t)conf->update_timeout_ms * 1000;

It could overflow, but that would only happen if the timeout_ms was greater than 2^32 / 1000
which is 4295 seconds!  The default is 100 ms. 
	
The driver should do some more validation in bond_8023ad_setup_validate().
It does check that update_timeout_ms is non zero, but it has no upper bound.