Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220802214041.2656586-1-neelnatu@google.com>
In-Reply-To: <20220802214041.2656586-1-neelnatu@google.com>
From:   Yury Norov <yury.norov@gmail.com>
Date:   Tue, 2 Aug 2022 18:22:15 -0700
Message-ID: <CAAH8bW_z3TjjafPRJPZmzDdOFjmU7xTLxOinw8D4nsZ2B6WN4w@mail.gmail.com>
Subject: Re: [PATCH] sched, cpumask: don't leak impossible cpus via for_each_cpu_wrap().
To:     Neel Natu <neelnatu@google.com>
Cc:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        Peter Zijlstra <peterz@infradead.org>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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