lkml.org 
[lkml]   [2022]   [Aug]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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

\
 
 \ /
  Last update: 2022-08-03 19:51    [W:0.106 / U:0.232 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site