DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/bonding: fix socket id check
@ 2020-06-16  9:46 David Marchand
  2020-06-16 10:09 ` Chas Williams
  0 siblings, 1 reply; 7+ messages in thread
From: David Marchand @ 2020-06-16  9:46 UTC (permalink / raw)
  To: dev; +Cc: stable, Chas Williams, Wei Hu (Xavier), Eric Kinzie, Declan Doherty

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


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

* Re: [dpdk-dev] [PATCH] net/bonding: fix socket id check
  2020-06-16  9:46 [dpdk-dev] [PATCH] net/bonding: fix socket id check David Marchand
@ 2020-06-16 10:09 ` Chas Williams
  2020-07-09 10:36   ` Ferruh Yigit
  0 siblings, 1 reply; 7+ messages in thread
From: Chas Williams @ 2020-06-16 10:09 UTC (permalink / raw)
  To: dev

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

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

* Re: [dpdk-dev] [PATCH] net/bonding: fix socket id check
  2020-06-16 10:09 ` Chas Williams
@ 2020-07-09 10:36   ` Ferruh Yigit
  0 siblings, 0 replies; 7+ messages in thread
From: Ferruh Yigit @ 2020-07-09 10:36 UTC (permalink / raw)
  To: Chas Williams, dev; +Cc: David Marchand

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


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

* Re: [dpdk-dev] [PATCH] net/bonding: fix socket id check
  2021-04-27  2:44   ` Chengchang Tang
@ 2021-04-27 10:45     ` Ferruh Yigit
  0 siblings, 0 replies; 7+ messages in thread
From: Ferruh Yigit @ 2021-04-27 10:45 UTC (permalink / raw)
  To: Chengchang Tang, Min Hu (Connor), dev

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


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

* Re: [dpdk-dev] [PATCH] net/bonding: fix socket id check
  2021-04-26 14:54 ` Ferruh Yigit
@ 2021-04-27  2:44   ` Chengchang Tang
  2021-04-27 10:45     ` Ferruh Yigit
  0 siblings, 1 reply; 7+ messages in thread
From: Chengchang Tang @ 2021-04-27  2:44 UTC (permalink / raw)
  To: Ferruh Yigit, Min Hu (Connor), dev



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


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

* Re: [dpdk-dev] [PATCH] net/bonding: fix socket id check
  2021-04-22  7:12 Min Hu (Connor)
@ 2021-04-26 14:54 ` Ferruh Yigit
  2021-04-27  2:44   ` Chengchang Tang
  0 siblings, 1 reply; 7+ messages in thread
From: Ferruh Yigit @ 2021-04-26 14:54 UTC (permalink / raw)
  To: Min Hu (Connor), dev

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


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

* [dpdk-dev] [PATCH] net/bonding: fix socket id check
@ 2021-04-22  7:12 Min Hu (Connor)
  2021-04-26 14:54 ` Ferruh Yigit
  0 siblings, 1 reply; 7+ messages in thread
From: Min Hu (Connor) @ 2021-04-22  7:12 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, humin29

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


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

end of thread, other threads:[~2021-04-27 10:45 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-16  9:46 [dpdk-dev] [PATCH] net/bonding: fix socket id check David Marchand
2020-06-16 10:09 ` Chas Williams
2020-07-09 10:36   ` Ferruh Yigit
2021-04-22  7:12 Min Hu (Connor)
2021-04-26 14:54 ` Ferruh Yigit
2021-04-27  2:44   ` Chengchang Tang
2021-04-27 10:45     ` Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).