Re: [PATCH v3] smp: Fix a potential usage of stale nr_cpus
* Thomas Gleixner wrote: > Ingo Molnar writes: > >> - get_option(, _cpus); > >> + if (get_option(, _cpus) != 1) > >> + return -EINVAL; > >> + > >>if (nr_cpus > 0 && nr_cpus < nr_cpu_ids) > >>nr_cpu_ids = nr_cpus; > >> + else > >> + return -EINVAL; > > > > Exactly what does 'not valid' mean, and why doesn't get_option() > > return -EINVAL in that case? > > What's unclear about invalid? If you specify nr_cpus=-1 or > nr_cpus=200 the its obviously invalid. So this was the old (buggy) code: > { > int nr_cpus; > > get_option(, _cpus); > if (nr_cpus > 0 && nr_cpus < nr_cpu_ids) > nr_cpu_ids = nr_cpus; And this was the explanation given in the changelog: >> When the cmdline of "nr_cpus" is not valid, the @nr_cpu_ids is >> assigned a stale value. The nr_cpus is only valid when get_option() >> return 1. So check the return value to prevent this. The answer to my question is that the bug is that the return value of get_option() wasn't checked properly, and if get_option() returns an error then the nr_cpus local variable is not set - but we used it in the old code, which can result in essentially a random value for nr_cpu_ids. > How should get_option() know that this is invalid? get_option() is a > number parser and does not know about any restrictions on the parsed > value obviously. But that's apparently not the bug here, 'invalid' here was meant as per the parser's syntax. If nr_cpus is out of range (like the 200 example you gave), then nr_cpu_ids might not be set at all, and remains at the 0 initialized value. Which isn't good but not 'stale' either. This is why I was puzzled where a 'stale' value might come from, at first sight I was assuming that some large value was written, like your 20 example. The "stale value" happens if it's invalid syntax and get_option() returns an error, in which case 'nr_cpus' remains uninitialized. And this is the explanation I didn't find at first reading, and which explanation future changelogs should perhaps include. The new code does this: int nr_cpus; if (get_option(, _cpus) != 1) return -EINVAL; if (nr_cpus > 0 && nr_cpus < nr_cpu_ids) nr_cpu_ids = nr_cpus; else return -EINVAL; Which does all the proper error handling and fixes the uninitialized 'nr_cpus' local variable usage. So I agree with the fix: Reviewed-by: Ingo Molnar Thanks, Ingo
Re: [PATCH v3] smp: Fix a potential usage of stale nr_cpus
Ingo Molnar writes: >> -get_option(, _cpus); >> +if (get_option(, _cpus) != 1) >> +return -EINVAL; >> + >> if (nr_cpus > 0 && nr_cpus < nr_cpu_ids) >> nr_cpu_ids = nr_cpus; >> +else >> +return -EINVAL; > > Exactly what does 'not valid' mean, and why doesn't get_option() > return -EINVAL in that case? What's unclear about invalid? If you specify nr_cpus=-1 or nr_cpus=200 the its obviously invalid. How should get_option() know that this is invalid? get_option() is a number parser and does not know about any restrictions on the parsed value obviously. get_option() returns string parsing information: 0 -> not integer found 1 -> integer found, no trailing comma or hyphen 2 -> integer found and trailing comma 3 -> integer found and traling hyphen (range parsing) And that's what is checked in if (get_option() != 1), i.e. anything else than a plain integer is invalid for this command line option. Thanks, tglx
Re: [PATCH v3] smp: Fix a potential usage of stale nr_cpus
* Muchun Song wrote: > When the cmdline of "nr_cpus" is not valid, the @nr_cpu_ids is assigned > a stale value. The nr_cpus is only valid when get_option() return 1. So > check the return value to prevent this. > > Signed-off-by: Muchun Song > --- > changelog in v3: > 1) Return -EINVAL when the parameter is bogus. > > changelog in v2: > 1) Rework the commit log. > 2) Rework the return value check. > > kernel/smp.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/kernel/smp.c b/kernel/smp.c > index a5a66fc28f4e..0dacfcfcf00b 100644 > --- a/kernel/smp.c > +++ b/kernel/smp.c > @@ -772,9 +772,13 @@ static int __init nrcpus(char *str) > { > int nr_cpus; > > - get_option(, _cpus); > + if (get_option(, _cpus) != 1) > + return -EINVAL; > + > if (nr_cpus > 0 && nr_cpus < nr_cpu_ids) > nr_cpu_ids = nr_cpus; > + else > + return -EINVAL; Exactly what does 'not valid' mean, and why doesn't get_option() return -EINVAL in that case? Thanks, Ingo
[PATCH v3] smp: Fix a potential usage of stale nr_cpus
When the cmdline of "nr_cpus" is not valid, the @nr_cpu_ids is assigned a stale value. The nr_cpus is only valid when get_option() return 1. So check the return value to prevent this. Signed-off-by: Muchun Song --- changelog in v3: 1) Return -EINVAL when the parameter is bogus. changelog in v2: 1) Rework the commit log. 2) Rework the return value check. kernel/smp.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/kernel/smp.c b/kernel/smp.c index a5a66fc28f4e..0dacfcfcf00b 100644 --- a/kernel/smp.c +++ b/kernel/smp.c @@ -772,9 +772,13 @@ static int __init nrcpus(char *str) { int nr_cpus; - get_option(, _cpus); + if (get_option(, _cpus) != 1) + return -EINVAL; + if (nr_cpus > 0 && nr_cpus < nr_cpu_ids) nr_cpu_ids = nr_cpus; + else + return -EINVAL; return 0; } -- 2.11.0