Re: [dpdk-dev] [PATCH V1 1/2] examples/ethtool: fix data type of MTU

[David Marchand <david.marchand@redhat.com>]

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
>


-- 
David Marchand