Messages in this thread | | | From | Neel Natu <> | Date | Wed, 3 Aug 2022 10:49:57 -0700 | Subject | Re: [PATCH] sched, cpumask: don't leak impossible cpus via for_each_cpu_wrap(). |
| |
On Tue, Aug 2, 2022 at 6:22 PM Yury Norov <yury.norov@gmail.com> wrote: > > 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. >
Hmm, in a kernel with CONFIG_HOTPLUG_CPU=y but not CONFIG_CPUMASK_OFFSTACK I see "nr_cpu_ids = 20, nr_cpumask_bits = 512". FYI since it doesn't match the observation above that nr_cpumask_bits == nr_cpu_ids when CONFIG_HOTPLUG_CPU=y.
> 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,
I think you mean cpumask_setall() that in turn calls bitmap_fill(nr_cpumask_bits)?
> so that someone would complain. (Are there any other valid reasons to set > bits behind nr_cpu_ids intentionally?) >
I don't know of any although this wasn't the case that trigger in my case.
> Can you share more details about how you triggered that? If you observe > those bits set, something else is probably already wrong...
The non-intuitive behavior of for_each_cpu_wrap() was triggered when iterating over a cpumask passed by userspace that set a bit in the [nr_cpu_ids, nr_cpumask_bits) range.
The kernel code is out of tree but open source so happy to provide a pointer if needed.
I considered ANDing the user supplied mask with 'cpu_possible_mask' but that felt like working around an inconsistency in the kernel API (hence the proposed fix).
> 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. >
I'll let other people more knowledgable than me in this area chime in. I'll be happy either way if that fixes the problem at hand.
best Neel
> 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
| |