Messages in this thread | | | From | Yury Norov <> | Date | Tue, 2 Aug 2022 18:22:15 -0700 | Subject | Re: [PATCH] sched, cpumask: don't leak impossible cpus via for_each_cpu_wrap(). |
| |
On Tue, Aug 2, 2022 at 2:41 PM Neel Natu <neelnatu@google.com> wrote: > > The value of 'nr_cpumask_bits' is dependent on CONFIG_CPUMASK_OFFSTACK. > This in turn can change the set of cpus visited by for_each_cpu_wrap() > with a mask that has bits set in the range [nr_cpu_ids, NR_CPUS). > > Specifically on !CONFIG_CPUMASK_OFFSTACK kernels the API can iterate > over cpus outside the 'cpu_possible_mask'. > > Fix this to make its behavior match for_each_cpu() which always limits > the iteration to the range [0, nr_cpu_ids). > > Signed-off-by: Neel Natu <neelnatu@google.com>
The patch itself doesn't look correct because it randomly switches a piece of cpumask API from nr_cpumask_bits to nr_cpu_ids, and doesn't touch others.
However...
I don't know the story behind having 2 variables holding the max possible number of cpus, and it looks like it dates back to prehistoric times. In modern kernel, there are 2 cases where nr_cpumask_bits == nr_cpu_ids for sure: it's CONFIG_CPUMASK_OFFSTACK=y and CONFIG_HOTPLUG_CPU=y. At least one of those is enabled in defconfig of every popular architecture.
In case of HOTPLUG is off, I don't understand why we should have nr_cpu_ids and nr_cpumask_bits different - what case should it cover?... Interestingly, in comments to cpumask functions and in the code those two are referred interchangeably.
Even more interestingly, we have a function bitmap_setall() that sets all bits up to nr_cpumask_bits, and it could trigger the problem that you described, so that someone would complain. (Are there any other valid reasons to set bits behind nr_cpu_ids intentionally?)
Can you share more details about how you triggered that? If you observe those bits set, something else is probably already wrong...
So, if there is no real case in modern kernel to have nr_cpumask_bits and nr_cpu_ids different, the proper fix would be to just drop the first.
If there is such a case, this is probably your case, and we'd know more details to understand how to deal with that.
Thanks, Yury
| |