[PATCH RESEND] smp: Fix a potential usage of stale nr_cpus

[PATCH RESEND] smp: Fix a potential usage of stale nr_cpus

[Muchun Song]

The get_option() maybe return 0, it means that the nr_cpus is
not initialized. Then we will use the stale nr_cpus to initialize
the nr_cpu_ids. So fix it.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 kernel/smp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/smp.c b/kernel/smp.c
index 472c2b274c65..2a9a04acf123 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -634,8 +634,7 @@ static int __init nrcpus(char *str)
 {
 	int nr_cpus;
 
-	get_option(&str, &nr_cpus);
-	if (nr_cpus > 0 && nr_cpus < nr_cpu_ids)
+	if (get_option(&str, &nr_cpus) && nr_cpus > 0 && nr_cpus < nr_cpu_ids)
 		nr_cpu_ids = nr_cpus;
 
 	return 0;
-- 
2.11.0

Re: [PATCH RESEND] smp: Fix a potential usage of stale nr_cpus

[Thomas Gleixner]

Muchun,

Muchun Song <songmuchun@bytedance.com> writes:

> The get_option() maybe return 0, it means that the nr_cpus is
> not initialized.

Good catch, but see below.

> Then we will use the stale nr_cpus to initialize

We use nothing. Please describe your changes in technical neutral
language.

> the nr_cpu_ids. So fix it.

'So fix it.' is not much valuable information. What about:

    Check the return value to prevent this.

Hmm?

> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  kernel/smp.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/kernel/smp.c b/kernel/smp.c
> index 472c2b274c65..2a9a04acf123 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -634,8 +634,7 @@ static int __init nrcpus(char *str)
>  {
>  	int nr_cpus;
>  
> -	get_option(&str, &nr_cpus);
> -	if (nr_cpus > 0 && nr_cpus < nr_cpu_ids)
> +	if (get_option(&str, &nr_cpus) && nr_cpus > 0 && nr_cpus < nr_cpu_ids)
>  		nr_cpu_ids = nr_cpus;
>  
>  	return 0;

get_option() can return 0 - 3:

 *      0 - no int in string
 *      1 - int found, no subsequent comma
 *      2 - int found including a subsequent comma
 *      3 - hyphen found to denote a range

For this parameter exists only one valid format: '1 - int found, no comma',
right?

So why fixing it just half and why returning '0' aka success if the
parameter is bogus?

Thanks,

        tglx

Re: [External] Re: [PATCH RESEND] smp: Fix a potential usage of stale nr_cpus

[Muchun Song]

On Sat, Jul 18, 2020 at 4:15 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Muchun,
>
> Muchun Song <songmuchun@bytedance.com> writes:
>
> > The get_option() maybe return 0, it means that the nr_cpus is
> > not initialized.
>
> Good catch, but see below.
>
> > Then we will use the stale nr_cpus to initialize
>
> We use nothing. Please describe your changes in technical neutral
> language.
>
> > the nr_cpu_ids. So fix it.
>
> 'So fix it.' is not much valuable information. What about:
>
>     Check the return value to prevent this.
>
> Hmm?

Looks fine to me. Thanks.

>
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > ---
> >  kernel/smp.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/kernel/smp.c b/kernel/smp.c
> > index 472c2b274c65..2a9a04acf123 100644
> > --- a/kernel/smp.c
> > +++ b/kernel/smp.c
> > @@ -634,8 +634,7 @@ static int __init nrcpus(char *str)
> >  {
> >       int nr_cpus;
> >
> > -     get_option(&str, &nr_cpus);
> > -     if (nr_cpus > 0 && nr_cpus < nr_cpu_ids)
> > +     if (get_option(&str, &nr_cpus) && nr_cpus > 0 && nr_cpus < nr_cpu_ids)
> >               nr_cpu_ids = nr_cpus;
> >
> >       return 0;
>
> get_option() can return 0 - 3:
>
>  *      0 - no int in string
>  *      1 - int found, no subsequent comma
>  *      2 - int found including a subsequent comma
>  *      3 - hyphen found to denote a range
>
> For this parameter exists only one valid format: '1 - int found, no comma',
> right?

Yeah.

>
> So why fixing it just half and why returning '0' aka success if the
> parameter is bogus?

Thanks. Will fix it.

>
> Thanks,
>
>         tglx



-- 
Yours,
Muchun

[tip: sched/core] smp: Fix a potential usage of stale nr_cpus

[tip-bot2 for Muchun Song]

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     589343569d7b58a64ec2446e6686c8e79aea7fcf
Gitweb:        https://git.kernel.org/tip/589343569d7b58a64ec2446e6686c8e79aea7fcf
Author:        Muchun Song <songmuchun@bytedance.com>
AuthorDate:    Thu, 16 Jul 2020 15:04:57 +08:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 22 Jul 2020 10:22:04 +02:00

smp: Fix a potential usage of stale nr_cpus

The get_option() maybe return 0, it means that the nr_cpus is
not initialized. Then we will use the stale nr_cpus to initialize
the nr_cpu_ids. So fix it.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20200716070457.53255-1-songmuchun@bytedance.com
---
 kernel/smp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/smp.c b/kernel/smp.c
index aa17eed..d0ae8eb 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -634,8 +634,7 @@ static int __init nrcpus(char *str)
 {
 	int nr_cpus;
 
-	get_option(&str, &nr_cpus);
-	if (nr_cpus > 0 && nr_cpus < nr_cpu_ids)
+	if (get_option(&str, &nr_cpus) && nr_cpus > 0 && nr_cpus < nr_cpu_ids)
 		nr_cpu_ids = nr_cpus;
 
 	return 0;