Re: [PATCH v1] ethdev: fix int overflow in descriptor count logic

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 9/23/2024 10:26 AM, Niall Meade wrote:
> Addressed a specific overflow issue in the eth_dev_adjust_nb_desc()
> function where the uint16_t variable nb_desc would overflow when its
> value was greater than (2^16 - nb_align). This overflow caused nb_desc
> to incorrectly wrap around between 0 and nb_align-1, leading to the
> function setting nb_desc to nb_min instead of the expected nb_max.
> 
> The resolution involves upcasting nb_desc to a uint32_t before the
> RTE_ALIGN_CEIL macro is applied. This change ensures that the subsequent
> call to RTE_ALIGN_FLOOR(nb_desc + (nb_align - 1), nb_align) does not
> result in an overflow, as it would when nb_desc is a uint16_t. By using
> a uint32_t for these operations, the correct behavior is maintained
> without the risk of overflow.
> 

Hi Niall,


Thanks for the patch.

For the 'RTE_ALIGN_CEIL(val, align)' macro, 'align' should be power of
two, as 'desc_lim->nb_align' is uint16_t, max value it can get is 2^15.
'val' should be smaller than or equal to 'align', so '*nb_desc' can be
maximum 2^15.

So RTE_ALIGN_CEIL(2^15-1, 2^15) = 2^15, I think this should work fine
(although I didn't test).

And even with your uint32_t cast, I think following will fail:
RTE_ALIGN_CEIL(2^16-1, 2^15)
(again, not tested).

Or maybe I am missing a case, can you please give some actual numbers to
show the problem and the fix?


Perhaps what we need is to verify mentioned requirements of the macro in
the function:
- 'align' should be power of two
- val <= align
But as this is a static function, these checks can be done in caller
function and preconditions can be enforced.


> Fixes: 0f67fc3baeb9 ("ethdev: add function to adjust number of descriptors")
> 
> Signed-off-by: Niall Meade <niall.meade@intel.com>
> ---
>  .mailmap                |  1 +
>  lib/ethdev/rte_ethdev.c | 12 +++++++++---
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/.mailmap b/.mailmap
> index 4a508bafad..c1941e78bb 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -1053,6 +1053,7 @@ Nelson Escobar <neescoba@cisco.com>
>  Nemanja Marjanovic <nemanja.marjanovic@intel.com>
>  Netanel Belgazal <netanel@amazon.com>
>  Netanel Gonen <netanelg@mellanox.com>
> +Niall Meade <niall.meade@intel.com>
>  Niall Power <niall.power@intel.com>
>  Nicholas Pratte <npratte@iol.unh.edu>
>  Nick Connolly <nick.connolly@arm.com> <nick.connolly@mayadata.io>
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index f1c658f49e..f978283edf 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -6577,13 +6577,19 @@ static void
>  eth_dev_adjust_nb_desc(uint16_t *nb_desc,
>  		const struct rte_eth_desc_lim *desc_lim)
>  {
> +	/* Upcast to uint32 to avoid potential overflow with RTE_ALIGN_CEIL(). */
> +	uint32_t nb_desc_32 = *nb_desc;
> +
>  	if (desc_lim->nb_align != 0)
> -		*nb_desc = RTE_ALIGN_CEIL(*nb_desc, desc_lim->nb_align);
> +		nb_desc_32 = RTE_ALIGN_CEIL(nb_desc_32, desc_lim->nb_align);
>  
>  	if (desc_lim->nb_max != 0)
> -		*nb_desc = RTE_MIN(*nb_desc, desc_lim->nb_max);
> +		nb_desc_32 = RTE_MIN(nb_desc_32, desc_lim->nb_max);
> +
> +	nb_desc_32 = RTE_MAX(nb_desc_32, desc_lim->nb_min);
>  
> -	*nb_desc = RTE_MAX(*nb_desc, desc_lim->nb_min);
> +	/* Assign clipped u32 back to u16. */
> +	*nb_desc = nb_desc_32;
>  }
>  
>  int