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

[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) +
>>

[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;
>>>  	}
>>>
>>
>>
>> .
>>
>