[dpdk-dev] [PATCH] net/bonding: fix socket id check

[dpdk-dev] [PATCH] net/bonding: fix socket id check

[Min Hu (Connor)]

From: Chengchang Tang <tangchengchang@huawei.com>

The socket ID entered by user is cast to an unsigned integer. However,
the value may be an illegal negative value, which may cause some
problems. In this case, an error should be returned.

In addition, the socket ID may be an invalid positive number, which is
also processed in this patch.

Fixes: 2efb58cbab6e ("bond: new link bonding library")
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_args.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
index 8c5f90d..bcc0fe3 100644
--- a/drivers/net/bonding/rte_eth_bond_args.c
+++ b/drivers/net/bonding/rte_eth_bond_args.c
@@ -207,12 +207,12 @@ bond_ethdev_parse_socket_id_kvarg(const char *key __rte_unused,
 		return -1;
 
 	errno = 0;
-	socket_id = (uint8_t)strtol(value, &endptr, 10);
+	socket_id = strtol(value, &endptr, 10);
 	if (*endptr != 0 || errno != 0)
 		return -1;
 
 	/* validate socket id value */
-	if (socket_id >= 0) {
+	if (socket_id >= 0 && socket_id < RTE_MAX_NUMA_NODES) {
 		*(uint8_t *)extra_args = (uint8_t)socket_id;
 		return 0;
 	}
-- 
2.7.4

Re: [dpdk-dev] [PATCH] net/bonding: fix socket id check

[Ferruh Yigit]

On 4/22/2021 8:12 AM, Min Hu (Connor) wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> The socket ID entered by user is cast to an unsigned integer. However,
> the value may be an illegal negative value, which may cause some
> problems. In this case, an error should be returned.
> 

+1 to fix

> In addition, the socket ID may be an invalid positive number, which is
> also processed in this patch.
> 
> Fixes: 2efb58cbab6e ("bond: new link bonding library")
> 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_args.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
> index 8c5f90d..bcc0fe3 100644
> --- a/drivers/net/bonding/rte_eth_bond_args.c
> +++ b/drivers/net/bonding/rte_eth_bond_args.c
> @@ -207,12 +207,12 @@ bond_ethdev_parse_socket_id_kvarg(const char *key __rte_unused,
>  		return -1;
>  
>  	errno = 0;
> -	socket_id = (uint8_t)strtol(value, &endptr, 10);
> +	socket_id = strtol(value, &endptr, 10);

'strtol()' returns 'long int', but implicitly casting it to 'int'. My concern is
if this cause a static analysis tool warning.
What do you think to have 'socket_id' type as 'long int'?

>  	if (*endptr != 0 || errno != 0)
>  		return -1;
>  
>  	/* validate socket id value */
> -	if (socket_id >= 0) {
> +	if (socket_id >= 0 && socket_id < RTE_MAX_NUMA_NODES) {>  		*(uint8_t *)extra_args = (uint8_t)socket_id;

Here there is an assumption that RTE_MAX_NUMA_NODES will be less than
'UCHAR_MAX', perhaps it can be good to add a check to verify this assumption.

>  		return 0;
>  	}
> 

Re: [dpdk-dev] [PATCH] net/bonding: fix socket id check

[Chengchang Tang]

On 2021/4/26 22:54, Ferruh Yigit wrote:
> On 4/22/2021 8:12 AM, Min Hu (Connor) wrote:
>> From: Chengchang Tang <tangchengchang@huawei.com>
>>
>> The socket ID entered by user is cast to an unsigned integer. However,
>> the value may be an illegal negative value, which may cause some
>> problems. In this case, an error should be returned.
>>
> 
> +1 to fix
> 
>> In addition, the socket ID may be an invalid positive number, which is
>> also processed in this patch.
>>
>> Fixes: 2efb58cbab6e ("bond: new link bonding library")
>> 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_args.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
>> index 8c5f90d..bcc0fe3 100644
>> --- a/drivers/net/bonding/rte_eth_bond_args.c
>> +++ b/drivers/net/bonding/rte_eth_bond_args.c
>> @@ -207,12 +207,12 @@ bond_ethdev_parse_socket_id_kvarg(const char *key __rte_unused,
>>  		return -1;
>>  
>>  	errno = 0;
>> -	socket_id = (uint8_t)strtol(value, &endptr, 10);
>> +	socket_id = strtol(value, &endptr, 10);
> 
> 'strtol()' returns 'long int', but implicitly casting it to 'int'. My concern is
> if this cause a static analysis tool warning.
> What do you think to have 'socket_id' type as 'long int'?
> 
I think it would be better to cast to the 'int' here, for reasons below.

>>  	if (*endptr != 0 || errno != 0)
>>  		return -1;
>>  
>>  	/* validate socket id value */
>> -	if (socket_id >= 0) {
>> +	if (socket_id >= 0 && socket_id < RTE_MAX_NUMA_NODES) {>  		*(uint8_t *)extra_args = (uint8_t)socket_id;
> 
> Here there is an assumption that RTE_MAX_NUMA_NODES will be less than
> 'UCHAR_MAX', perhaps it can be good to add a check to verify this assumption.

Currently, it is unlikely that RTE_MAX_NUMA_NODES will be greater than 256. Therefore,
adding such check will not cause any problems. But I don't think it's necessary to put
such restrictions on it (i.e. RTE_MAX_NUMA_NODES should be less than UCHAR_MAX).
I checked all references to RTE_MAX_NUMA_NODES, and usually socket_id is of type 'int'
or 'unsigned int' (Only the efd, node, and bonding specify 'unsigned char' for socket
IDs.). And for that, I think it will be better to change the socket id type to 'int'
in this patch. For the type of socket id in efd and node, I will send new patches to
modify it.
> 
>>  		return 0;
>>  	}
>>
> 
> 
> .
> 

Re: [dpdk-dev] [PATCH] net/bonding: fix socket id check

[Ferruh Yigit]

On 4/27/2021 3:44 AM, Chengchang Tang wrote:
> 
> 
> On 2021/4/26 22:54, Ferruh Yigit wrote:
>> On 4/22/2021 8:12 AM, Min Hu (Connor) wrote:
>>> From: Chengchang Tang <tangchengchang@huawei.com>
>>>
>>> The socket ID entered by user is cast to an unsigned integer. However,
>>> the value may be an illegal negative value, which may cause some
>>> problems. In this case, an error should be returned.
>>>
>>
>> +1 to fix
>>
>>> In addition, the socket ID may be an invalid positive number, which is
>>> also processed in this patch.
>>>
>>> Fixes: 2efb58cbab6e ("bond: new link bonding library")
>>> 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_args.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
>>> index 8c5f90d..bcc0fe3 100644
>>> --- a/drivers/net/bonding/rte_eth_bond_args.c
>>> +++ b/drivers/net/bonding/rte_eth_bond_args.c
>>> @@ -207,12 +207,12 @@ bond_ethdev_parse_socket_id_kvarg(const char *key __rte_unused,
>>>  		return -1;
>>>  
>>>  	errno = 0;
>>> -	socket_id = (uint8_t)strtol(value, &endptr, 10);
>>> +	socket_id = strtol(value, &endptr, 10);
>>
>> 'strtol()' returns 'long int', but implicitly casting it to 'int'. My concern is
>> if this cause a static analysis tool warning.
>> What do you think to have 'socket_id' type as 'long int'?
>>
> I think it would be better to cast to the 'int' here, for reasons below.
> 

Independent from below reasons, converting from user provided "long int" to
'int' will cause losing value and may lead wrong checks,

Like if user provided '-4294967281' (0xffffffff0000000f), when you cast to
'int', it will become '15' (0xf) and will pass from validation checks.

So I think better to verify the value first as "long int", later cast it to 'int'.

>>>  	if (*endptr != 0 || errno != 0)
>>>  		return -1;
>>>  
>>>  	/* validate socket id value */
>>> -	if (socket_id >= 0) {
>>> +	if (socket_id >= 0 && socket_id < RTE_MAX_NUMA_NODES) {>  		*(uint8_t *)extra_args = (uint8_t)socket_id;
>>
>> Here there is an assumption that RTE_MAX_NUMA_NODES will be less than
>> 'UCHAR_MAX', perhaps it can be good to add a check to verify this assumption.
> 
> Currently, it is unlikely that RTE_MAX_NUMA_NODES will be greater than 256. Therefore,
> adding such check will not cause any problems. But I don't think it's necessary to put
> such restrictions on it (i.e. RTE_MAX_NUMA_NODES should be less than UCHAR_MAX).

Restriction comes from provided 'extra_args' being 'uint8_t', I just suggest
checking this.

> I checked all references to RTE_MAX_NUMA_NODES, and usually socket_id is of type 'int'
> or 'unsigned int' (Only the efd, node, and bonding specify 'unsigned char' for socket
> IDs.). And for that, I think it will be better to change the socket id type to 'int'
> in this patch. For the type of socket id in efd and node, I will send new patches to
> modify it.
>>
>>>  		return 0;
>>>  	}
>>>
>>
>>
>> .
>>
> 

[dpdk-dev] [PATCH v2] net/bonding: fix socket id check

[Min Hu (Connor)]

From: Chengchang Tang <tangchengchang@huawei.com>

The socket ID entered by user is cast to an unsigned integer. However,
the value may be an illegal negative value, which may cause some
problems. In this case, an error should be returned.

In addition, the socket ID may be an invalid positive number, which is
also processed in this patch.

Fixes: 2efb58cbab6e ("bond: new link bonding library")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
v2:
* fixed socket id type.
---
 drivers/net/bonding/rte_eth_bond_args.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
index 8c5f90d..977f3fe 100644
--- a/drivers/net/bonding/rte_eth_bond_args.c
+++ b/drivers/net/bonding/rte_eth_bond_args.c
@@ -207,13 +207,13 @@ bond_ethdev_parse_socket_id_kvarg(const char *key __rte_unused,
 		return -1;
 
 	errno = 0;
-	socket_id = (uint8_t)strtol(value, &endptr, 10);
+	socket_id = (int)strtol(value, &endptr, 10);
 	if (*endptr != 0 || errno != 0)
 		return -1;
 
 	/* validate socket id value */
-	if (socket_id >= 0) {
-		*(uint8_t *)extra_args = (uint8_t)socket_id;
+	if (socket_id >= 0 && socket_id < RTE_MAX_NUMA_NODES) {
+		*(int *)extra_args = socket_id;
 		return 0;
 	}
 	return -1;
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] net/bonding: fix socket id check

[Ferruh Yigit]

On 4/27/2021 10:16 AM, Min Hu (Connor) wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> The socket ID entered by user is cast to an unsigned integer. However,
> the value may be an illegal negative value, which may cause some
> problems. In this case, an error should be returned.
> 
> In addition, the socket ID may be an invalid positive number, which is
> also processed in this patch.
> 
> Fixes: 2efb58cbab6e ("bond: new link bonding library")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
> v2:
> * fixed socket id type.
> ---
>  drivers/net/bonding/rte_eth_bond_args.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
> index 8c5f90d..977f3fe 100644
> --- a/drivers/net/bonding/rte_eth_bond_args.c
> +++ b/drivers/net/bonding/rte_eth_bond_args.c
> @@ -207,13 +207,13 @@ bond_ethdev_parse_socket_id_kvarg(const char *key __rte_unused,
>  		return -1;
>  
>  	errno = 0;
> -	socket_id = (uint8_t)strtol(value, &endptr, 10);
> +	socket_id = (int)strtol(value, &endptr, 10);

Already provided some comment to v1, why it is better to do checks first and
cast later.

>  	if (*endptr != 0 || errno != 0)
>  		return -1;
>  
>  	/* validate socket id value */
> -	if (socket_id >= 0) {
> -		*(uint8_t *)extra_args = (uint8_t)socket_id;
> +	if (socket_id >= 0 && socket_id < RTE_MAX_NUMA_NODES) {
> +		*(int *)extra_args = socket_id;

The provided 'extra_args' is 'uint8_t', we can't just cast it to "int *" and
assign to it.

[dpdk-dev] [PATCH v4] net/bonding: fix socket id check

[Min Hu (Connor)]

From: Chengchang Tang <tangchengchang@huawei.com>

The socket ID entered by user is cast to an unsigned integer. However,
the value may be an illegal negative value, which may cause some
problems. In this case, an error should be returned.

In addition, the socket ID may be an invalid positive number, which is
also processed in this patch.

Fixes: 2efb58cbab6e ("bond: new link bonding library")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
v4:
* changed type of socket id getting from 'strtol'.
* delete unused comments.

v3:
* changed type of socket id.

v2:
* fixed socket id type.
---
 drivers/net/bonding/rte_eth_bond_args.c | 8 ++++----
 drivers/net/bonding/rte_eth_bond_pmd.c  | 5 +++--
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
index 8c5f90d..dc68e52 100644
--- a/drivers/net/bonding/rte_eth_bond_args.c
+++ b/drivers/net/bonding/rte_eth_bond_args.c
@@ -200,20 +200,20 @@ int
 bond_ethdev_parse_socket_id_kvarg(const char *key __rte_unused,
 		const char *value, void *extra_args)
 {
-	int socket_id;
+	long socket_id;
 	char *endptr;
 
 	if (value == NULL || extra_args == NULL)
 		return -1;
 
 	errno = 0;
-	socket_id = (uint8_t)strtol(value, &endptr, 10);
+	socket_id = strtol(value, &endptr, 10);
 	if (*endptr != 0 || errno != 0)
 		return -1;
 
 	/* validate socket id value */
-	if (socket_id >= 0) {
-		*(uint8_t *)extra_args = (uint8_t)socket_id;
+	if (socket_id >= 0 && socket_id < RTE_MAX_NUMA_NODES) {
+		*(int *)extra_args = (int)socket_id;
 		return 0;
 	}
 	return -1;
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 2e9cea5..2f7d6ad 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -3333,8 +3333,9 @@ bond_probe(struct rte_vdev_device *dev)
 	const char *name;
 	struct bond_dev_private *internals;
 	struct rte_kvargs *kvlist;
-	uint8_t bonding_mode, socket_id/*, agg_mode*/;
-	int  arg_count, port_id;
+	uint8_t bonding_mode;
+	int arg_count, port_id;
+	int socket_id;
 	uint8_t agg_mode;
 	struct rte_eth_dev *eth_dev;
 
-- 
2.7.4

Re: [dpdk-dev] [PATCH v4] net/bonding: fix socket id check

[Ferruh Yigit]

On 4/27/2021 12:39 PM, Min Hu (Connor) wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> The socket ID entered by user is cast to an unsigned integer. However,
> the value may be an illegal negative value, which may cause some
> problems. In this case, an error should be returned.
> 
> In addition, the socket ID may be an invalid positive number, which is
> also processed in this patch.
> 
> Fixes: 2efb58cbab6e ("bond: new link bonding library")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

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

[dpdk-dev] [PATCH] net/bonding: fix socket id check

[David Marchand]

Caught by code review, rte_eth_dev_socket_id() returns -1 on error.
The code should behave the same, but still, do not use LCORE_ID_ANY for
something that is not a lcore id.

Fixes: c15c5897340d ("net/bonding: avoid allocating mempool on unknown socket")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.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 b77a37ddb3..b7ffa2f2cf 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.c
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
@@ -1043,7 +1043,7 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
 	RTE_ASSERT(port->tx_ring == NULL);
 
 	socket_id = rte_eth_dev_socket_id(slave_id);
-	if (socket_id == (int)LCORE_ID_ANY)
+	if (socket_id == -1)
 		socket_id = rte_socket_id();
 
 	element_size = sizeof(struct slow_protocol_frame) +
-- 
2.23.0

Re: [dpdk-dev] [PATCH] net/bonding: fix socket id check

[Chas Williams]

Acked-by: Chas Williams <chas3@att.com>

On 6/16/20 5:46 AM, David Marchand wrote:
> Caught by code review, rte_eth_dev_socket_id() returns -1 on error.
> The code should behave the same, but still, do not use LCORE_ID_ANY for
> something that is not a lcore id.
> 
> Fixes: c15c5897340d ("net/bonding: avoid allocating mempool on unknown socket")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.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 b77a37ddb3..b7ffa2f2cf 100644
> --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
> @@ -1043,7 +1043,7 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
>   	RTE_ASSERT(port->tx_ring == NULL);
>   
>   	socket_id = rte_eth_dev_socket_id(slave_id);
> -	if (socket_id == (int)LCORE_ID_ANY)
> +	if (socket_id == -1)

Testing against < 0 would probably be more future proof. But if someone 
decides to update rte_eth_dev_socket_id they will hopefully update 
callers as well.

>   		socket_id = rte_socket_id();
>   
>   	element_size = sizeof(struct slow_protocol_frame) +
> 

Re: [dpdk-dev] [PATCH] net/bonding: fix socket id check

[Ferruh Yigit]

On 6/16/2020 11:09 AM, Chas Williams wrote:

> On 6/16/20 5:46 AM, David Marchand wrote:
>> Caught by code review, rte_eth_dev_socket_id() returns -1 on error.
>> The code should behave the same, but still, do not use LCORE_ID_ANY for
>> something that is not a lcore id.
>>
>> Fixes: c15c5897340d ("net/bonding: avoid allocating mempool on unknown socket")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>
> Acked-by: Chas Williams <chas3@att.com>
>

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

>> ---
>>   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 b77a37ddb3..b7ffa2f2cf 100644
>> --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
>> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
>> @@ -1043,7 +1043,7 @@ bond_mode_8023ad_activate_slave(struct rte_eth_dev *bond_dev,
>>   	RTE_ASSERT(port->tx_ring == NULL);
>>   
>>   	socket_id = rte_eth_dev_socket_id(slave_id);
>> -	if (socket_id == (int)LCORE_ID_ANY)
>> +	if (socket_id == -1)
> 
> Testing against < 0 would probably be more future proof. But if someone 
> decides to update rte_eth_dev_socket_id they will hopefully update 
> callers as well.
> 
>>   		socket_id = rte_socket_id();
>>   
>>   	element_size = sizeof(struct slow_protocol_frame) +
>>