[PATCH] sched: fix integer handling issue

[PATCH] sched: fix integer handling issue

[Megha Ajmera]

Masking of core mask was incorrect. Instead of using 1U for shifting, it
should be using 1LU as the result is assigned to uint64.

CID 375859: Potentially overflowing expression "1U << app_main_core" with
type "unsigned int" (32 bits, unsigned) is evaluated using 32-bit arithmetic,
and then used in a context that expects an expression of type "uint64_t"
(64 bits, unsigned).

Coverity issue: 375859

Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
---
 examples/qos_sched/args.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
index 10ca7bea61..44f2f5640e 100644
--- a/examples/qos_sched/args.c
+++ b/examples/qos_sched/args.c
@@ -433,7 +433,7 @@ app_parse_args(int argc, char **argv)
 			return -1;
 		}
 	}
-	app_used_core_mask |= 1u << app_main_core;
+	app_used_core_mask |= 1lu << app_main_core;
 
 	if ((app_used_core_mask != app_eal_core_mask()) ||
 			(app_main_core != rte_get_main_lcore())) {
-- 
2.25.1

RE: [PATCH] sched: fix integer handling issue

[Morten Brørup]

> From: Megha Ajmera [mailto:megha.ajmera@intel.com]
> Sent: Tuesday, 22 February 2022 14.19
> 
> Masking of core mask was incorrect. Instead of using 1U for shifting,
> it
> should be using 1LU as the result is assigned to uint64.
> 
> CID 375859: Potentially overflowing expression "1U << app_main_core"
> with
> type "unsigned int" (32 bits, unsigned) is evaluated using 32-bit
> arithmetic,
> and then used in a context that expects an expression of type
> "uint64_t"
> (64 bits, unsigned).
> 
> Coverity issue: 375859
> 
> Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> ---
>  examples/qos_sched/args.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
> index 10ca7bea61..44f2f5640e 100644
> --- a/examples/qos_sched/args.c
> +++ b/examples/qos_sched/args.c
> @@ -433,7 +433,7 @@ app_parse_args(int argc, char **argv)
>  			return -1;
>  		}
>  	}
> -	app_used_core_mask |= 1u << app_main_core;
> +	app_used_core_mask |= 1lu << app_main_core;

Still wrong on 32 bit platforms, where long unsigned int is still 32 bits.

Use this instead:
app_used_core_mask |= RTE_BIT64(app_main_core);

Ref:
https://elixir.bootlin.com/dpdk/latest/source/lib/eal/include/rte_bitops.h#L26

> 
>  	if ((app_used_core_mask != app_eal_core_mask()) ||
>  			(app_main_core != rte_get_main_lcore())) {
> --
> 2.25.1
> 

Re: [PATCH] sched: fix integer handling issue

[Stephen Hemminger]

On Tue, 22 Feb 2022 15:13:53 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:

> > From: Megha Ajmera [mailto:megha.ajmera@intel.com]
> > Sent: Tuesday, 22 February 2022 14.19
> > 
> > Masking of core mask was incorrect. Instead of using 1U for shifting,
> > it
> > should be using 1LU as the result is assigned to uint64.
> > 
> > CID 375859: Potentially overflowing expression "1U << app_main_core"
> > with
> > type "unsigned int" (32 bits, unsigned) is evaluated using 32-bit
> > arithmetic,
> > and then used in a context that expects an expression of type
> > "uint64_t"
> > (64 bits, unsigned).
> > 
> > Coverity issue: 375859
> > 
> > Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> > ---
> >  examples/qos_sched/args.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
> > index 10ca7bea61..44f2f5640e 100644
> > --- a/examples/qos_sched/args.c
> > +++ b/examples/qos_sched/args.c
> > @@ -433,7 +433,7 @@ app_parse_args(int argc, char **argv)
> >  			return -1;
> >  		}
> >  	}
> > -	app_used_core_mask |= 1u << app_main_core;
> > +	app_used_core_mask |= 1lu << app_main_core;  
> 
> Still wrong on 32 bit platforms, where long unsigned int is still 32 bits.
> 
> Use this instead:
> app_used_core_mask |= RTE_BIT64(app_main_core);

DPDK now supports > 64 lcores. So all code using/assuming a 64 bit mask is broken.

RE: [PATCH] sched: fix integer handling issue

[Ajmera, Megha]

> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, February 22, 2022 9:33 PM
> 
> On Tue, 22 Feb 2022 15:13:53 +0100
> Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> > > From: Megha Ajmera [mailto:megha.ajmera@intel.com]
> > > Sent: Tuesday, 22 February 2022 14.19
> > >
> > > Masking of core mask was incorrect. Instead of using 1U for
> > > shifting, it should be using 1LU as the result is assigned to
> > > uint64.
> > >
> > > CID 375859: Potentially overflowing expression "1U << app_main_core"
> > > with
> > > type "unsigned int" (32 bits, unsigned) is evaluated using 32-bit
> > > arithmetic, and then used in a context that expects an expression of
> > > type "uint64_t"
> > > (64 bits, unsigned).
> > >
> > > Coverity issue: 375859
> > >
> > > Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> > > ---
> > >  examples/qos_sched/args.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
> > > index 10ca7bea61..44f2f5640e 100644
> > > --- a/examples/qos_sched/args.c
> > > +++ b/examples/qos_sched/args.c
> > > @@ -433,7 +433,7 @@ app_parse_args(int argc, char **argv)
> > >  			return -1;
> > >  		}
> > >  	}
> > > -	app_used_core_mask |= 1u << app_main_core;
> > > +	app_used_core_mask |= 1lu << app_main_core;
> >
> > Still wrong on 32 bit platforms, where long unsigned int is still 32 bits.
> >
> > Use this instead:
> > app_used_core_mask |= RTE_BIT64(app_main_core);
> 
> DPDK now supports > 64 lcores. So all code using/assuming a 64 bit mask is
> broken.
I checked in dpdk-testpmd and looks like it still does not support more than 64 cores.
Here is the snippet:

    else if (set_fwd_lcores_mask((uint64_t) cm) < 0)
        rte_exit(EXIT_FAILURE, "coremask is not valid\n");

It is type casting the core mask to uint64_t.

RE: [PATCH] sched: fix integer handling issue

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, 22 February 2022 17.03
> 
> On Tue, 22 Feb 2022 15:13:53 +0100
> Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> > > From: Megha Ajmera [mailto:megha.ajmera@intel.com]
> > > Sent: Tuesday, 22 February 2022 14.19
> > >
> > > Masking of core mask was incorrect. Instead of using 1U for
> shifting,
> > > it
> > > should be using 1LU as the result is assigned to uint64.
> > >
> > > CID 375859: Potentially overflowing expression "1U <<
> app_main_core"
> > > with
> > > type "unsigned int" (32 bits, unsigned) is evaluated using 32-bit
> > > arithmetic,
> > > and then used in a context that expects an expression of type
> > > "uint64_t"
> > > (64 bits, unsigned).
> > >
> > > Coverity issue: 375859
> > >
> > > Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> > > ---
> > >  examples/qos_sched/args.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
> > > index 10ca7bea61..44f2f5640e 100644
> > > --- a/examples/qos_sched/args.c
> > > +++ b/examples/qos_sched/args.c
> > > @@ -433,7 +433,7 @@ app_parse_args(int argc, char **argv)
> > >  			return -1;
> > >  		}
> > >  	}
> > > -	app_used_core_mask |= 1u << app_main_core;
> > > +	app_used_core_mask |= 1lu << app_main_core;
> >
> > Still wrong on 32 bit platforms, where long unsigned int is still 32
> bits.
> >
> > Use this instead:
> > app_used_core_mask |= RTE_BIT64(app_main_core);
> 
> DPDK now supports > 64 lcores. So all code using/assuming a 64 bit mask
> is broken.
> 

Good point. Is there a TODO-list where such a general review request can be filed, or should we just file it as a system-wide bug in Bugzilla?

Nonetheless, I think this one-line fix should be accepted as a short term solution.

-Morten

Re: [PATCH] sched: fix integer handling issue

[Ferruh Yigit]

On 2/23/2022 7:17 AM, Morten Brørup wrote:
>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
>> Sent: Tuesday, 22 February 2022 17.03
>>
>> On Tue, 22 Feb 2022 15:13:53 +0100
>> Morten Brørup <mb@smartsharesystems.com> wrote:
>>
>>>> From: Megha Ajmera [mailto:megha.ajmera@intel.com]
>>>> Sent: Tuesday, 22 February 2022 14.19
>>>>
>>>> Masking of core mask was incorrect. Instead of using 1U for
>> shifting,
>>>> it
>>>> should be using 1LU as the result is assigned to uint64.
>>>>
>>>> CID 375859: Potentially overflowing expression "1U <<
>> app_main_core"
>>>> with
>>>> type "unsigned int" (32 bits, unsigned) is evaluated using 32-bit
>>>> arithmetic,
>>>> and then used in a context that expects an expression of type
>>>> "uint64_t"
>>>> (64 bits, unsigned).
>>>>
>>>> Coverity issue: 375859
>>>>
>>>> Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
>>>> ---
>>>>   examples/qos_sched/args.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
>>>> index 10ca7bea61..44f2f5640e 100644
>>>> --- a/examples/qos_sched/args.c
>>>> +++ b/examples/qos_sched/args.c
>>>> @@ -433,7 +433,7 @@ app_parse_args(int argc, char **argv)
>>>>   			return -1;
>>>>   		}
>>>>   	}
>>>> -	app_used_core_mask |= 1u << app_main_core;
>>>> +	app_used_core_mask |= 1lu << app_main_core;
>>>
>>> Still wrong on 32 bit platforms, where long unsigned int is still 32
>> bits.
>>>
>>> Use this instead:
>>> app_used_core_mask |= RTE_BIT64(app_main_core);
>>
>> DPDK now supports > 64 lcores. So all code using/assuming a 64 bit mask
>> is broken.
>>
> 
> Good point. Is there a TODO-list where such a general review request can be filed, or should we just file it as a system-wide bug in Bugzilla?
> 
> Nonetheless, I think this one-line fix should be accepted as a short term solution.
> 

Hi Morten,

I suspect there can be more places that testpmd assumes
max core number is 64, someone needs to spend time to
analyze and fix it.

RE: more than 64 lcores not properly supported

[Morten Brørup]

+Thomas, you may be interested in this discussion about applications using an uint64_t bit mask to identify active lcores.

> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Wednesday, 23 February 2022 11.03
> 
> On 2/23/2022 7:17 AM, Morten Brørup wrote:
> >> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> >> Sent: Tuesday, 22 February 2022 17.03

[...]

> >>
> >> DPDK now supports > 64 lcores. So all code using/assuming a 64 bit
> mask
> >> is broken.
> >>
> >
> > Good point. Is there a TODO-list where such a general review request
> can be filed, or should we just file it as a system-wide bug in
> Bugzilla?
> >
> > Nonetheless, I think this one-line fix should be accepted as a short
> term solution.
> >
> 
> Hi Morten,
> 
> I suspect there can be more places that testpmd assumes
> max core number is 64, someone needs to spend time to
> analyze and fix it.

My point exactly. Someone needs to spend time to analyze all DPDK libraries and applications, and fix it where appropriate. Where do we register this required effort, so it can be picked up by someone?

Also, it should probably be mentioned as a known bug in the 22.03 release notes.

Re: more than 64 lcores not properly supported

[Ferruh Yigit]

On 2/23/2022 10:42 AM, Morten Brørup wrote:
> +Thomas, you may be interested in this discussion about applications using an uint64_t bit mask to identify active lcores.
> 
>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>> Sent: Wednesday, 23 February 2022 11.03
>>
>> On 2/23/2022 7:17 AM, Morten Brørup wrote:
>>>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
>>>> Sent: Tuesday, 22 February 2022 17.03
> 
> [...]
> 
>>>>
>>>> DPDK now supports > 64 lcores. So all code using/assuming a 64 bit
>> mask
>>>> is broken.
>>>>
>>>
>>> Good point. Is there a TODO-list where such a general review request
>> can be filed, or should we just file it as a system-wide bug in
>> Bugzilla?
>>>
>>> Nonetheless, I think this one-line fix should be accepted as a short
>> term solution.
>>>
>>
>> Hi Morten,
>>
>> I suspect there can be more places that testpmd assumes
>> max core number is 64, someone needs to spend time to
>> analyze and fix it.
> 
> My point exactly. Someone needs to spend time to analyze all DPDK libraries and applications, and fix it where appropriate. Where do we register this required effort, so it can be picked up by someone?
> 

testpmd is an application and it has its own restrictions,
I *assumed* libraries are safe and restriction is only in
testpmd, but may be better to verify this as well.

> Also, it should probably be mentioned as a known bug in the 22.03 release notes.
> 

Re: more than 64 lcores not properly supported

[Thomas Monjalon]

23/02/2022 12:20, Ferruh Yigit:
> On 2/23/2022 10:42 AM, Morten Brørup wrote:
> > +Thomas, you may be interested in this discussion about applications using an uint64_t bit mask to identify active lcores.
> > 
> >> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> >> Sent: Wednesday, 23 February 2022 11.03
> >>
> >> On 2/23/2022 7:17 AM, Morten Brørup wrote:
> >>>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> >>>> Sent: Tuesday, 22 February 2022 17.03
> > 
> > [...]
> > 
> >>>>
> >>>> DPDK now supports > 64 lcores. So all code using/assuming a 64 bit
> >> mask
> >>>> is broken.
> >>>>
> >>>
> >>> Good point. Is there a TODO-list where such a general review request
> >> can be filed, or should we just file it as a system-wide bug in
> >> Bugzilla?
> >>>
> >>> Nonetheless, I think this one-line fix should be accepted as a short
> >> term solution.
> >>>
> >>
> >> Hi Morten,
> >>
> >> I suspect there can be more places that testpmd assumes
> >> max core number is 64, someone needs to spend time to
> >> analyze and fix it.
> > 
> > My point exactly. Someone needs to spend time to analyze all DPDK libraries and applications, and fix it where appropriate. Where do we register this required effort, so it can be picked up by someone?
> > 
> 
> testpmd is an application and it has its own restrictions,
> I *assumed* libraries are safe and restriction is only in
> testpmd, but may be better to verify this as well.
> 
> > Also, it should probably be mentioned as a known bug in the 22.03 release notes.

There are known bugs and things to verify.
Known bugs should be in bugzilla + release notes.
Verification tasks are difficult to track because there is no point
where we can be sure that things are fully verified.
Instead I think such kind of verification should be managed
as permanent tasks. Do you have a tool or process in mind?

Re: more than 64 lcores not properly supported

[Stephen Hemminger]

On Wed, 23 Feb 2022 14:49:12 +0100
Thomas Monjalon <thomas@monjalon.net> wrote:

> 23/02/2022 12:20, Ferruh Yigit:
> > On 2/23/2022 10:42 AM, Morten Brørup wrote:  
> > > +Thomas, you may be interested in this discussion about applications using an uint64_t bit mask to identify active lcores.
> > >   
> > >> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> > >> Sent: Wednesday, 23 February 2022 11.03
> > >>
> > >> On 2/23/2022 7:17 AM, Morten Brørup wrote:  
> > >>>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > >>>> Sent: Tuesday, 22 February 2022 17.03  
> > > 
> > > [...]
> > >   
> > >>>>
> > >>>> DPDK now supports > 64 lcores. So all code using/assuming a 64 bit  
> > >> mask  
> > >>>> is broken.
> > >>>>  
> > >>>
> > >>> Good point. Is there a TODO-list where such a general review request  
> > >> can be filed, or should we just file it as a system-wide bug in
> > >> Bugzilla?  
> > >>>
> > >>> Nonetheless, I think this one-line fix should be accepted as a short  
> > >> term solution.  
> > >>>  
> > >>
> > >> Hi Morten,
> > >>
> > >> I suspect there can be more places that testpmd assumes
> > >> max core number is 64, someone needs to spend time to
> > >> analyze and fix it.  
> > > 
> > > My point exactly. Someone needs to spend time to analyze all DPDK libraries and applications, and fix it where appropriate. Where do we register this required effort, so it can be picked up by someone?
> > >   
> > 
> > testpmd is an application and it has its own restrictions,
> > I *assumed* libraries are safe and restriction is only in
> > testpmd, but may be better to verify this as well.
> >   
> > > Also, it should probably be mentioned as a known bug in the 22.03 release notes.  
> 
> There are known bugs and things to verify.
> Known bugs should be in bugzilla + release notes.
> Verification tasks are difficult to track because there is no point
> where we can be sure that things are fully verified.
> Instead I think such kind of verification should be managed
> as permanent tasks. Do you have a tool or process in mind?
> 
> 

Agree take the fix for now.
Since many places use a mask of cpus and/or ports. It would be good
to have common code for handling this.

[PATCH v2] sched: fix integer handling issue

[Megha Ajmera]

Masking of core mask was incorrect. Instead of using 1U for shifting, it
should be using 1LU as the result is assigned to uint64.

CID 375859: Potentially overflowing expression "1U << app_main_core" with
type "unsigned int" (32 bits, unsigned) is evaluated using 32-bit
arithmetic, and then used in a context that expects an expression of
type "uint64_t" (64 bits, unsigned).

Coverity issue: 375859

Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
---
 examples/qos_sched/args.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
index 10ca7bea61..562d9ca150 100644
--- a/examples/qos_sched/args.c
+++ b/examples/qos_sched/args.c
@@ -427,13 +427,13 @@ app_parse_args(int argc, char **argv)
 
 	/* check main core index validity */
 	for (i = 0; i <= app_main_core; i++) {
-		if (app_used_core_mask & (1u << app_main_core)) {
+		if (app_used_core_mask & (RTE_BIT64(app_main_core))) {
 			RTE_LOG(ERR, APP, "Main core index is not configured properly\n");
 			app_usage(prgname);
 			return -1;
 		}
 	}
-	app_used_core_mask |= 1u << app_main_core;
+	app_used_core_mask |= RTE_BIT64(app_main_core);
 
 	if ((app_used_core_mask != app_eal_core_mask()) ||
 			(app_main_core != rte_get_main_lcore())) {
-- 
2.25.1

RE: [PATCH v2] sched: fix integer handling issue

[Morten Brørup]

> From: Megha Ajmera [mailto:megha.ajmera@intel.com]
> Sent: Wednesday, 23 February 2022 18.37
> 
> Masking of core mask was incorrect. Instead of using 1U for shifting,
> it
> should be using 1LU as the result is assigned to uint64.
> 
> CID 375859: Potentially overflowing expression "1U << app_main_core"
> with
> type "unsigned int" (32 bits, unsigned) is evaluated using 32-bit
> arithmetic, and then used in a context that expects an expression of
> type "uint64_t" (64 bits, unsigned).
> 
> Coverity issue: 375859
> 
> Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> ---
>  examples/qos_sched/args.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
> index 10ca7bea61..562d9ca150 100644
> --- a/examples/qos_sched/args.c
> +++ b/examples/qos_sched/args.c
> @@ -427,13 +427,13 @@ app_parse_args(int argc, char **argv)
> 
>  	/* check main core index validity */
>  	for (i = 0; i <= app_main_core; i++) {
> -		if (app_used_core_mask & (1u << app_main_core)) {
> +		if (app_used_core_mask & (RTE_BIT64(app_main_core))) {

No need for parenthesis around RTE_BIT64(app_main_core).

>  			RTE_LOG(ERR, APP, "Main core index is not configured
> properly\n");
>  			app_usage(prgname);
>  			return -1;
>  		}
>  	}
> -	app_used_core_mask |= 1u << app_main_core;
> +	app_used_core_mask |= RTE_BIT64(app_main_core);
> 
>  	if ((app_used_core_mask != app_eal_core_mask()) ||
>  			(app_main_core != rte_get_main_lcore())) {
> --
> 2.25.1
> 

Acked-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH v2] sched: fix integer handling issue

[Thomas Monjalon]

23/02/2022 19:13, Morten Brørup:
> > From: Megha Ajmera [mailto:megha.ajmera@intel.com]
> > Sent: Wednesday, 23 February 2022 18.37
> > 
> > Masking of core mask was incorrect. Instead of using 1U for shifting,
> > it
> > should be using 1LU as the result is assigned to uint64.
> > 
> > CID 375859: Potentially overflowing expression "1U << app_main_core"
> > with
> > type "unsigned int" (32 bits, unsigned) is evaluated using 32-bit
> > arithmetic, and then used in a context that expects an expression of
> > type "uint64_t" (64 bits, unsigned).
> > 
> > Coverity issue: 375859
> > 
> > Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> > ---
> > -		if (app_used_core_mask & (1u << app_main_core)) {
> > +		if (app_used_core_mask & (RTE_BIT64(app_main_core))) {
> 
> No need for parenthesis around RTE_BIT64(app_main_core).

Fixed while merging.

> Acked-by: Morten Brørup <mb@smartsharesystems.com>

Applied, thanks.