* [PATCH v1] ethdev: fix int overflow in descriptor count logic @ 2024-09-23 9:26 Niall Meade 2024-09-23 9:52 ` Andrew Rybchenko 2024-09-25 23:16 ` Ferruh Yigit 0 siblings, 2 replies; 7+ messages in thread From: Niall Meade @ 2024-09-23 9:26 UTC (permalink / raw) To: Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Roman Zhukov Cc: dev, Niall Meade 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. 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 -- 2.34.1 -------------------------------------------------------------- Intel Research and Development Ireland Limited Registered in Ireland Registered Office: Collinstown Industrial Park, Leixlip, County Kildare Registered Number: 308263 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] ethdev: fix int overflow in descriptor count logic 2024-09-23 9:26 [PATCH v1] ethdev: fix int overflow in descriptor count logic Niall Meade @ 2024-09-23 9:52 ` Andrew Rybchenko 2024-09-25 23:16 ` Ferruh Yigit 1 sibling, 0 replies; 7+ messages in thread From: Andrew Rybchenko @ 2024-09-23 9:52 UTC (permalink / raw) To: Niall Meade, Thomas Monjalon, Ferruh Yigit, Roman Zhukov; +Cc: dev On 9/23/24 12:26, 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. > > Fixes: 0f67fc3baeb9 ("ethdev: add function to adjust number of descriptors") > > Signed-off-by: Niall Meade <niall.meade@intel.com> Thanks a lot for the fix. Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] ethdev: fix int overflow in descriptor count logic 2024-09-23 9:26 [PATCH v1] ethdev: fix int overflow in descriptor count logic Niall Meade 2024-09-23 9:52 ` Andrew Rybchenko @ 2024-09-25 23:16 ` Ferruh Yigit 2024-09-26 14:03 ` Meade, Niall 1 sibling, 1 reply; 7+ messages in thread From: Ferruh Yigit @ 2024-09-25 23:16 UTC (permalink / raw) To: Niall Meade, Thomas Monjalon, Andrew Rybchenko, Roman Zhukov; +Cc: dev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] ethdev: fix int overflow in descriptor count logic 2024-09-25 23:16 ` Ferruh Yigit @ 2024-09-26 14:03 ` Meade, Niall 2024-09-26 23:51 ` Ferruh Yigit 0 siblings, 1 reply; 7+ messages in thread From: Meade, Niall @ 2024-09-26 14:03 UTC (permalink / raw) To: Ferruh Yigit, andrew.rybchenko; +Cc: dev, thomas, roman.zhukov > From: Ferruh Yigit <ferruh.yigit@amd.com> > Sent: Thursday, September 26, 2024 12:16 AM > To: Meade, Niall <niall.meade@intel.com>; Thomas Monjalon <thomas@monjalon.net>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Roman Zhukov <roman.zhukov@arknetworks.am> > Cc: dev@dpdk.org <dev@dpdk.org> > Subject: Re: [PATCH v1] ethdev: fix int overflow in descriptor count logic <snip> > > 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, Hi Ferruh, > 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). > I tested my code with these values and the behaviour is as expected from what I can see. At a high level I ran into this issue when passing uint16_tMAX into rte_eth_dev_adjust_nb_rx_tx_desc() with the intent of selecting the maximum ring descriptor size but the minimum was selected. > Or maybe I am missing a case, can you please give some actual numbers to > show the problem and the fix? Yes sure! If we take an example of val= (2^16)-1 and align= 32. RTE_ALIGN_CEIL(val, align) calls RTE_ALIGN_FLOOR(val + align - 1, align). With val as a uint16_t this subsequent macro call results in a wrap around for val (originally was the max uint16_t and now we are attempting to add align to it). The returned value of RTE_ALIGN_CEIL() in this case is 0. This results in nb_desc being set to 0, and later set to the minimum ring descriptor size for that NIC with *nb_desc = RTE_MAX(*nb_desc, desc_lim->nb_min). While this example is an unreasonably large request for a descriptor ring size, the expected behaviour would be that the descriptor ring size defaults back to the maximum possible for that particular NIC, not to the minimum which it currently does. By introducing a uint32_t, the wrap around in RTE_ALIGN_FLOOR() is avoided, keeping the large value of nb_desc_32 which is later set to an appropriate size in RTE_MIN(*nb_desc_32, desc_lim->nb_max) <snip> > > { > > + /* 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); <snip> -------------------------------------------------------------- Intel Research and Development Ireland Limited Registered in Ireland Registered Office: Collinstown Industrial Park, Leixlip, County Kildare Registered Number: 308263 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] ethdev: fix int overflow in descriptor count logic 2024-09-26 14:03 ` Meade, Niall @ 2024-09-26 23:51 ` Ferruh Yigit 2024-09-27 10:46 ` Meade, Niall 0 siblings, 1 reply; 7+ messages in thread From: Ferruh Yigit @ 2024-09-26 23:51 UTC (permalink / raw) To: Meade, Niall, andrew.rybchenko; +Cc: dev, thomas, roman.zhukov On 9/26/2024 3:03 PM, Meade, Niall wrote: >> From: Ferruh Yigit <ferruh.yigit@amd.com> >> Sent: Thursday, September 26, 2024 12:16 AM >> To: Meade, Niall <niall.meade@intel.com>; Thomas Monjalon <thomas@monjalon.net>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Roman Zhukov <roman.zhukov@arknetworks.am> >> Cc: dev@dpdk.org <dev@dpdk.org> >> Subject: Re: [PATCH v1] ethdev: fix int overflow in descriptor count logic > <snip> >>> 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, > > Hi Ferruh, > >> 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). >> > > I tested my code with these values and the behaviour is as expected from > what I can see. > At a high level I ran into this issue when passing uint16_tMAX into > rte_eth_dev_adjust_nb_rx_tx_desc() with the intent of selecting the maximum > ring descriptor size but the minimum was selected. > >> Or maybe I am missing a case, can you please give some actual numbers to >> show the problem and the fix? > > Yes sure! If we take an example of val= (2^16)-1 and align= 32. > RTE_ALIGN_CEIL(val, align) calls RTE_ALIGN_FLOOR(val + align - 1, align). With > val as a uint16_t this subsequent macro call results in a wrap around for val > (originally was the max uint16_t and now we are attempting to add align to > it). The returned value of RTE_ALIGN_CEIL() in this case is 0. This results in > nb_desc being set to 0, and later set to the minimum ring descriptor size for > that NIC with *nb_desc = RTE_MAX(*nb_desc, desc_lim->nb_min). > > While this example is an unreasonably large request for a descriptor ring size, > the expected behaviour would be that the descriptor ring size defaults back to > the maximum possible for that particular NIC, not to the minimum which it > currently does. > By introducing a uint32_t, the wrap around in RTE_ALIGN_FLOOR() is avoided, > keeping the large value of nb_desc_32 which is later set to an appropriate size > in RTE_MIN(*nb_desc_32, desc_lim->nb_max) > I see the problem now, thanks. When value > (2^16 - align), next aligned value is 2^16, which is UINT16_MAX + 1, hence wraps to 0, this is kind of expected. For the relevant code, assuming 'desc_lim->nb_max' & 'desc_lim->nb_min' are already aligned to 'desc_lim->nb_align', following should fix the issue, that seems simpler to me, what do you think: ``` if (desc_lim->nb_max != 0) *nb_desc = RTE_MIN(*nb_desc, desc_lim->nb_max); nb_desc_32 = RTE_MAX(nb_desc_32, desc_lim->nb_min); if (desc_lim->nb_align != 0) *nb_desc = RTE_ALIGN_CEIL(*nb_desc, desc_lim->nb_align); ``` Basically just changing the order of the operations... It is not easy to see the problem, can you please give sample values in the commit log (for '*nb_desc', 'nb_align', 'nb_max' & 'nb_min'), that makes much easier to see why above works. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] ethdev: fix int overflow in descriptor count logic 2024-09-26 23:51 ` Ferruh Yigit @ 2024-09-27 10:46 ` Meade, Niall 2024-09-27 21:39 ` Ferruh Yigit 0 siblings, 1 reply; 7+ messages in thread From: Meade, Niall @ 2024-09-27 10:46 UTC (permalink / raw) To: Ferruh Yigit, andrew.rybchenko; +Cc: dev, thomas, roman.zhukov >On 9/26/2024 3:03 PM, Meade, Niall wrote: >>> From: Ferruh Yigit <ferruh.yigit@amd.com> >>> Sent: Thursday, September 26, 2024 12:16 AM >>> To: Meade, Niall <niall.meade@intel.com>; Thomas Monjalon <thomas@monjalon.net>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Roman Zhukov <roman.zhukov@arknetworks.am> >>> Cc: dev@dpdk.org <dev@dpdk.org> >>> Subject: Re: [PATCH v1] ethdev: fix int overflow in descriptor count logic >> <snip> >>>> 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, >> >> Hi Ferruh, >> >>> 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). >>> >> >> I tested my code with these values and the behaviour is as expected from >> what I can see. >> At a high level I ran into this issue when passing uint16_tMAX into >> rte_eth_dev_adjust_nb_rx_tx_desc() with the intent of selecting the maximum >> ring descriptor size but the minimum was selected. >> >>> Or maybe I am missing a case, can you please give some actual numbers to >>> show the problem and the fix? >> >> Yes sure! If we take an example of val= (2^16)-1 and align= 32. >> RTE_ALIGN_CEIL(val, align) calls RTE_ALIGN_FLOOR(val + align - 1, align). With >> val as a uint16_t this subsequent macro call results in a wrap around for val >> (originally was the max uint16_t and now we are attempting to add align to >> it). The returned value of RTE_ALIGN_CEIL() in this case is 0. This results in >> nb_desc being set to 0, and later set to the minimum ring descriptor size for >> that NIC with *nb_desc = RTE_MAX(*nb_desc, desc_lim->nb_min). >> >> While this example is an unreasonably large request for a descriptor ring size, >> the expected behaviour would be that the descriptor ring size defaults back to >> the maximum possible for that particular NIC, not to the minimum which it >> currently does. >> By introducing a uint32_t, the wrap around in RTE_ALIGN_FLOOR() is avoided, >> keeping the large value of nb_desc_32 which is later set to an appropriate size >> in RTE_MIN(*nb_desc_32, desc_lim->nb_max) >> > >I see the problem now, thanks. > >When value > (2^16 - align), next aligned value is 2^16, which is >UINT16_MAX + 1, hence wraps to 0, this is kind of expected. > >For the relevant code, assuming 'desc_lim->nb_max' & 'desc_lim->nb_min' >are already aligned to 'desc_lim->nb_align', following should fix the >issue, that seems simpler to me, what do you think: Yes, while it is a simpler solution there is still potential for an overflow if nb_max is equal to 0. If nb_max is 0 while nb_desc is UINT16_MAX, UINT16_MAX will be passed to the align macro resulting in an overflow again. > >``` >if (desc_lim->nb_max != 0) > *nb_desc = RTE_MIN(*nb_desc, desc_lim->nb_max); > >nb_desc_32 = RTE_MAX(nb_desc_32, desc_lim->nb_min); > >if (desc_lim->nb_align != 0) > *nb_desc = RTE_ALIGN_CEIL(*nb_desc, desc_lim->nb_align); >``` > >Basically just changing the order of the operations... > >It is not easy to see the problem, can you please give sample values in >the commit log (for '*nb_desc', 'nb_align', 'nb_max' & 'nb_min'), that >makes much easier to see why above works. Yes, good idea! I'll add an example to the commit log for clarity. -------------------------------------------------------------- Intel Research and Development Ireland Limited Registered in Ireland Registered Office: Collinstown Industrial Park, Leixlip, County Kildare Registered Number: 308263 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] ethdev: fix int overflow in descriptor count logic 2024-09-27 10:46 ` Meade, Niall @ 2024-09-27 21:39 ` Ferruh Yigit 0 siblings, 0 replies; 7+ messages in thread From: Ferruh Yigit @ 2024-09-27 21:39 UTC (permalink / raw) To: Meade, Niall, andrew.rybchenko; +Cc: dev, thomas, roman.zhukov On 9/27/2024 11:46 AM, Meade, Niall wrote: >> On 9/26/2024 3:03 PM, Meade, Niall wrote: >>>> From: Ferruh Yigit <ferruh.yigit@amd.com> >>>> Sent: Thursday, September 26, 2024 12:16 AM >>>> To: Meade, Niall <niall.meade@intel.com>; Thomas Monjalon <thomas@monjalon.net>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Roman Zhukov <roman.zhukov@arknetworks.am> >>>> Cc: dev@dpdk.org <dev@dpdk.org> >>>> Subject: Re: [PATCH v1] ethdev: fix int overflow in descriptor count logic >>> <snip> >>>>> 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, >>> >>> Hi Ferruh, >>> >>>> 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). >>>> >>> >>> I tested my code with these values and the behaviour is as expected from >>> what I can see. >>> At a high level I ran into this issue when passing uint16_tMAX into >>> rte_eth_dev_adjust_nb_rx_tx_desc() with the intent of selecting the maximum >>> ring descriptor size but the minimum was selected. >>> >>>> Or maybe I am missing a case, can you please give some actual numbers to >>>> show the problem and the fix? >>> >>> Yes sure! If we take an example of val= (2^16)-1 and align= 32. >>> RTE_ALIGN_CEIL(val, align) calls RTE_ALIGN_FLOOR(val + align - 1, align). With >>> val as a uint16_t this subsequent macro call results in a wrap around for val >>> (originally was the max uint16_t and now we are attempting to add align to >>> it). The returned value of RTE_ALIGN_CEIL() in this case is 0. This results in >>> nb_desc being set to 0, and later set to the minimum ring descriptor size for >>> that NIC with *nb_desc = RTE_MAX(*nb_desc, desc_lim->nb_min). >>> >>> While this example is an unreasonably large request for a descriptor ring size, >>> the expected behaviour would be that the descriptor ring size defaults back to >>> the maximum possible for that particular NIC, not to the minimum which it >>> currently does. >>> By introducing a uint32_t, the wrap around in RTE_ALIGN_FLOOR() is avoided, >>> keeping the large value of nb_desc_32 which is later set to an appropriate size >>> in RTE_MIN(*nb_desc_32, desc_lim->nb_max) >>> >> >> I see the problem now, thanks. >> >> When value > (2^16 - align), next aligned value is 2^16, which is >> UINT16_MAX + 1, hence wraps to 0, this is kind of expected. >> >> For the relevant code, assuming 'desc_lim->nb_max' & 'desc_lim->nb_min' >> are already aligned to 'desc_lim->nb_align', following should fix the >> issue, that seems simpler to me, what do you think: > > Yes, while it is a simpler solution there is still potential for an overflow if nb_max > is equal to 0. If nb_max is 0 while nb_desc is UINT16_MAX, UINT16_MAX will be > passed to the align macro resulting in an overflow again. > ack >> >> ``` >> if (desc_lim->nb_max != 0) >> *nb_desc = RTE_MIN(*nb_desc, desc_lim->nb_max); >> >> nb_desc_32 = RTE_MAX(nb_desc_32, desc_lim->nb_min); >> >> if (desc_lim->nb_align != 0) >> *nb_desc = RTE_ALIGN_CEIL(*nb_desc, desc_lim->nb_align); >> ``` >> >> Basically just changing the order of the operations... >> >> It is not easy to see the problem, can you please give sample values in >> the commit log (for '*nb_desc', 'nb_align', 'nb_max' & 'nb_min'), that >> makes much easier to see why above works. > > Yes, good idea! I'll add an example to the commit log for clarity. > -------------------------------------------------------------- > Intel Research and Development Ireland Limited > Registered in Ireland > Registered Office: Collinstown Industrial Park, Leixlip, County Kildare > Registered Number: 308263 > > > This e-mail and any attachments may contain confidential material for the sole > use of the intended recipient(s). Any review or distribution by others is > strictly prohibited. If you are not the intended recipient, please contact the > sender and delete all copies. > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-09-27 22:14 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-09-23 9:26 [PATCH v1] ethdev: fix int overflow in descriptor count logic Niall Meade 2024-09-23 9:52 ` Andrew Rybchenko 2024-09-25 23:16 ` Ferruh Yigit 2024-09-26 14:03 ` Meade, Niall 2024-09-26 23:51 ` Ferruh Yigit 2024-09-27 10:46 ` Meade, Niall 2024-09-27 21:39 ` 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).