sched: fix integer handling issue

  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(-)
  

> 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
>
  

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.
  

> 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.
  

> 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
  

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.
  

+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.
  

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.
>
  

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?
  

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.