[V1,1/2] examples/ethtool: fix data type of MTU
Checks
Commit Message
From: Huisong Li <lihuisong@huawei.com>
This patch changes the data type of 'mtu' in rte_ethtool_net_change_mtu()
from 'int' to 'uint16_t'.
Fixes: bda68ab9d1e7 ("examples/ethtool: add user-space ethtool sample application")
Cc: stable@dpdk.org
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
examples/ethtool/ethtool-app/ethapp.c | 6 +-----
examples/ethtool/lib/rte_ethtool.c | 9 +++++----
examples/ethtool/lib/rte_ethtool.h | 2 +-
3 files changed, 7 insertions(+), 10 deletions(-)
Comments
On Thu, Apr 29, 2021 at 12:53 PM Min Hu (Connor) <humin29@huawei.com> wrote:
>
> From: Huisong Li <lihuisong@huawei.com>
>
> This patch changes the data type of 'mtu' in rte_ethtool_net_change_mtu()
> from 'int' to 'uint16_t'.
You did not describe why this change is needed.
>
> Fixes: bda68ab9d1e7 ("examples/ethtool: add user-space ethtool sample application")
> Cc: stable@dpdk.org
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
> examples/ethtool/ethtool-app/ethapp.c | 6 +-----
> examples/ethtool/lib/rte_ethtool.c | 9 +++++----
> examples/ethtool/lib/rte_ethtool.h | 2 +-
> 3 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/examples/ethtool/ethtool-app/ethapp.c b/examples/ethtool/ethtool-app/ethapp.c
> index 36a1c37..fc743ce 100644
> --- a/examples/ethtool/ethtool-app/ethapp.c
> +++ b/examples/ethtool/ethtool-app/ethapp.c
> @@ -521,13 +521,9 @@ pcmd_mtu_callback(void *ptr_params,
> {
> struct pcmd_intstr_params *params = ptr_params;
> int stat;
> - int new_mtu;
> + uint16_t new_mtu;
strtoul can overflow the stack, when storing to new_mtu some lines below.
You should either change new_mtu to unsigned long int or switch
strtoul to some other integer format helper.
> char *ptr_parse_end;
>
> - if (!rte_eth_dev_is_valid_port(params->port)) {
> - printf("Error: Invalid port number %i\n", params->port);
> - return;
> - }
Adding the check in lib/rte_ethtool.c is ok, but why not keep the
check in the application?
> new_mtu = strtoul(params->opt, &ptr_parse_end, 10);
> if (*ptr_parse_end != '\0' ||
> new_mtu < RTE_ETHER_MIN_MTU ||
With this patch, rte_ethtool_net_change_mtu() now takes a uint16_t.
You should add a check on new_mtu < UINT16_MAX.
> diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c
> index 4132516..73193ed 100644
> --- a/examples/ethtool/lib/rte_ethtool.c
> +++ b/examples/ethtool/lib/rte_ethtool.c
> @@ -345,12 +345,13 @@ rte_ethtool_net_validate_addr(uint16_t port_id __rte_unused,
> return rte_is_valid_assigned_ether_addr(addr);
> }
>
> +
> int
> -rte_ethtool_net_change_mtu(uint16_t port_id, int mtu)
> +rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu)
> {
> - if (mtu < 0 || mtu > UINT16_MAX)
> - return -EINVAL;
> - return rte_eth_dev_set_mtu(port_id, (uint16_t)mtu);
> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> + return rte_eth_dev_set_mtu(port_id, mtu);
> }
>
> int
> diff --git a/examples/ethtool/lib/rte_ethtool.h b/examples/ethtool/lib/rte_ethtool.h
> index f177096..fe3250e 100644
> --- a/examples/ethtool/lib/rte_ethtool.h
> +++ b/examples/ethtool/lib/rte_ethtool.h
> @@ -309,7 +309,7 @@ int rte_ethtool_net_validate_addr(uint16_t port_id,
> * - (-EINVAL) if parameters invalid.
> * - others depends on the specific operations implementation.
> */
> -int rte_ethtool_net_change_mtu(uint16_t port_id, int mtu);
> +int rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu);
>
> /**
> * Retrieve the Ethernet device traffic statistics
> --
> 2.8.1
>
在 2021/11/18 1:49, David Marchand 写道:
> On Thu, Apr 29, 2021 at 12:53 PM Min Hu (Connor) <humin29@huawei.com> wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> This patch changes the data type of 'mtu' in rte_ethtool_net_change_mtu()
>> from 'int' to 'uint16_t'.
> You did not describe why this change is needed.
Ensure that the input parameter of rte_ethtool_net_change_mtu() API in lib/rte_ethtool.c
re consistent with rte_eth_dev_set_mtu() in ethdev layer.
I will fix commit log.
>
>
>> Fixes: bda68ab9d1e7 ("examples/ethtool: add user-space ethtool sample application")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>> examples/ethtool/ethtool-app/ethapp.c | 6 +-----
>> examples/ethtool/lib/rte_ethtool.c | 9 +++++----
>> examples/ethtool/lib/rte_ethtool.h | 2 +-
>> 3 files changed, 7 insertions(+), 10 deletions(-)
>>
>> diff --git a/examples/ethtool/ethtool-app/ethapp.c b/examples/ethtool/ethtool-app/ethapp.c
>> index 36a1c37..fc743ce 100644
>> --- a/examples/ethtool/ethtool-app/ethapp.c
>> +++ b/examples/ethtool/ethtool-app/ethapp.c
>> @@ -521,13 +521,9 @@ pcmd_mtu_callback(void *ptr_params,
>> {
>> struct pcmd_intstr_params *params = ptr_params;
>> int stat;
>> - int new_mtu;
>> + uint16_t new_mtu;
> strtoul can overflow the stack, when storing to new_mtu some lines below.
> You should either change new_mtu to unsigned long int or switch
> strtoul to some other integer format helper.
You are right. I want add an mtu specific cmd parameter structure:
struct pcmd_mtu_params {
cmdline_fixed_string_t cmd;
uint16_t port;
uint16_t mtu_value;
};
MTU size is limited when users enter the value.
Like:
cmdline_parse_token_num_t pcmd_mtu_token_port =
TOKEN_NUM_INITIALIZER(struct pcmd_mtu_params, mtu_value, RTE_UINT16);
What do you think, David?
>
>> char *ptr_parse_end;
>>
>> - if (!rte_eth_dev_is_valid_port(params->port)) {
>> - printf("Error: Invalid port number %i\n", params->port);
>> - return;
>> - }
> Adding the check in lib/rte_ethtool.c is ok, but why not keep the
> check in the application?
All right, let it stay here.
>
>
>> new_mtu = strtoul(params->opt, &ptr_parse_end, 10);
>> if (*ptr_parse_end != '\0' ||
>> new_mtu < RTE_ETHER_MIN_MTU ||
> With this patch, rte_ethtool_net_change_mtu() now takes a uint16_t.
> You should add a check on new_mtu < UINT16_MAX.
>
If we use above new method, this check can be removed.
>> diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c
>> index 4132516..73193ed 100644
>> --- a/examples/ethtool/lib/rte_ethtool.c
>> +++ b/examples/ethtool/lib/rte_ethtool.c
>> @@ -345,12 +345,13 @@ rte_ethtool_net_validate_addr(uint16_t port_id __rte_unused,
>> return rte_is_valid_assigned_ether_addr(addr);
>> }
>>
>> +
>> int
>> -rte_ethtool_net_change_mtu(uint16_t port_id, int mtu)
>> +rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu)
>> {
>> - if (mtu < 0 || mtu > UINT16_MAX)
>> - return -EINVAL;
>> - return rte_eth_dev_set_mtu(port_id, (uint16_t)mtu);
>> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + return rte_eth_dev_set_mtu(port_id, mtu);
>> }
>>
>> int
>> diff --git a/examples/ethtool/lib/rte_ethtool.h b/examples/ethtool/lib/rte_ethtool.h
>> index f177096..fe3250e 100644
>> --- a/examples/ethtool/lib/rte_ethtool.h
>> +++ b/examples/ethtool/lib/rte_ethtool.h
>> @@ -309,7 +309,7 @@ int rte_ethtool_net_validate_addr(uint16_t port_id,
>> * - (-EINVAL) if parameters invalid.
>> * - others depends on the specific operations implementation.
>> */
>> -int rte_ethtool_net_change_mtu(uint16_t port_id, int mtu);
>> +int rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu);
>>
>> /**
>> * Retrieve the Ethernet device traffic statistics
>> --
>> 2.8.1
>>
>
@@ -521,13 +521,9 @@ pcmd_mtu_callback(void *ptr_params,
{
struct pcmd_intstr_params *params = ptr_params;
int stat;
- int new_mtu;
+ uint16_t new_mtu;
char *ptr_parse_end;
- if (!rte_eth_dev_is_valid_port(params->port)) {
- printf("Error: Invalid port number %i\n", params->port);
- return;
- }
new_mtu = strtoul(params->opt, &ptr_parse_end, 10);
if (*ptr_parse_end != '\0' ||
new_mtu < RTE_ETHER_MIN_MTU ||
@@ -345,12 +345,13 @@ rte_ethtool_net_validate_addr(uint16_t port_id __rte_unused,
return rte_is_valid_assigned_ether_addr(addr);
}
+
int
-rte_ethtool_net_change_mtu(uint16_t port_id, int mtu)
+rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu)
{
- if (mtu < 0 || mtu > UINT16_MAX)
- return -EINVAL;
- return rte_eth_dev_set_mtu(port_id, (uint16_t)mtu);
+ RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+ return rte_eth_dev_set_mtu(port_id, mtu);
}
int
@@ -309,7 +309,7 @@ int rte_ethtool_net_validate_addr(uint16_t port_id,
* - (-EINVAL) if parameters invalid.
* - others depends on the specific operations implementation.
*/
-int rte_ethtool_net_change_mtu(uint16_t port_id, int mtu);
+int rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu);
/**
* Retrieve the Ethernet device traffic statistics