Commit 78e5a3399421 ("cpumask: fix checking valid cpu range") has
started issuing warnings[*] when cpu indices equal to nr_cpu_ids - 1
are passed to cpumask_next* functions. seq_read_iter() and cpuinfo's
start and next seq operations implement a pattern like
n = cpumask_next(n - 1, mask);
show(n);
while (1) {
++n;
n = cpumask_next(n - 1, mask);
if (n >= nr_cpu_ids)
break;
show(n);
}
which will issue the warning when reading /proc/cpuinfo.
[*] Warnings will only appear with DEBUG_PER_CPU_MAPS enabled.
This series address the issue for x86 and riscv, but from a quick
grep of cpuinfo seq operations, I think at least openrisc, powerpc,
and s390 also need an equivalent patch. While the test is simple (see
next paragraph) I'm not equipped to test on each architecture.
To test, just build a kernel with DEBUG_PER_CPU_MAPS enabled, boot to
a shell, do 'cat /proc/cpuinfo', and look for a kernel warning.
While the patches are being posted together in a series since they're
for two different architectures they don't necessarily need to go
through the same tree.
v3:
- Change condition from >= to == in order to still get a warning
for > as that's unexpected. [Yury]
- Picked up tags on the riscv patch
v2:
- Added all the information I should have in the first place
to the commit message [Boris]
- Changed style of fix [Boris]
Andrew Jones (2):
RISC-V: Fix /proc/cpuinfo cpumask warning
x86: Fix /proc/cpuinfo cpumask warning
arch/riscv/kernel/cpu.c | 3 +++
arch/x86/kernel/cpu/proc.c | 3 +++
2 files changed, 6 insertions(+)
--
2.37.3
Commit 78e5a3399421 ("cpumask: fix checking valid cpu range") has
started issuing warnings[*] when cpu indices equal to nr_cpu_ids - 1
are passed to cpumask_next* functions. seq_read_iter() and cpuinfo's
start and next seq operations implement a pattern like
n = cpumask_next(n - 1, mask);
show(n);
while (1) {
++n;
n = cpumask_next(n - 1, mask);
if (n >= nr_cpu_ids)
break;
show(n);
}
which will issue the warning when reading /proc/cpuinfo. Ensure no
warning is generated by validating the cpu index before calling
cpumask_next().
[*] Warnings will only appear with DEBUG_PER_CPU_MAPS enabled.
Signed-off-by: Andrew Jones <[email protected]>
Cc: Yury Norov <[email protected]>
---
arch/x86/kernel/cpu/proc.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
index 099b6f0d96bd..de3f93ac6e49 100644
--- a/arch/x86/kernel/cpu/proc.c
+++ b/arch/x86/kernel/cpu/proc.c
@@ -153,6 +153,9 @@ static int show_cpuinfo(struct seq_file *m, void *v)
static void *c_start(struct seq_file *m, loff_t *pos)
{
+ if (*pos == nr_cpu_ids)
+ return NULL;
+
*pos = cpumask_next(*pos - 1, cpu_online_mask);
if ((*pos) < nr_cpu_ids)
return &cpu_data(*pos);
--
2.37.3
On Fri, Oct 14, 2022 at 05:58:43PM +0200, Andrew Jones wrote:
> Commit 78e5a3399421 ("cpumask: fix checking valid cpu range") has
> started issuing warnings[*] when cpu indices equal to nr_cpu_ids - 1
> are passed to cpumask_next* functions. seq_read_iter() and cpuinfo's
> start and next seq operations implement a pattern like
>
> n = cpumask_next(n - 1, mask);
> show(n);
> while (1) {
> ++n;
> n = cpumask_next(n - 1, mask);
> if (n >= nr_cpu_ids)
> break;
> show(n);
> }
>
> which will issue the warning when reading /proc/cpuinfo.
>
> [*] Warnings will only appear with DEBUG_PER_CPU_MAPS enabled.
>
> This series address the issue for x86 and riscv, but from a quick
> grep of cpuinfo seq operations, I think at least openrisc, powerpc,
> and s390 also need an equivalent patch. While the test is simple (see
> next paragraph) I'm not equipped to test on each architecture.
>
> To test, just build a kernel with DEBUG_PER_CPU_MAPS enabled, boot to
> a shell, do 'cat /proc/cpuinfo', and look for a kernel warning.
>
> While the patches are being posted together in a series since they're
> for two different architectures they don't necessarily need to go
> through the same tree.
Acked-by: Yury Norov <[email protected]
On Fri, 14 Oct 2022 08:58:43 PDT (-0700), [email protected] wrote:
> Commit 78e5a3399421 ("cpumask: fix checking valid cpu range") has
> started issuing warnings[*] when cpu indices equal to nr_cpu_ids - 1
> are passed to cpumask_next* functions. seq_read_iter() and cpuinfo's
> start and next seq operations implement a pattern like
>
> n = cpumask_next(n - 1, mask);
> show(n);
> while (1) {
> ++n;
> n = cpumask_next(n - 1, mask);
> if (n >= nr_cpu_ids)
> break;
> show(n);
> }
>
> which will issue the warning when reading /proc/cpuinfo.
>
> [*] Warnings will only appear with DEBUG_PER_CPU_MAPS enabled.
>
> This series address the issue for x86 and riscv, but from a quick
> grep of cpuinfo seq operations, I think at least openrisc, powerpc,
> and s390 also need an equivalent patch. While the test is simple (see
> next paragraph) I'm not equipped to test on each architecture.
>
> To test, just build a kernel with DEBUG_PER_CPU_MAPS enabled, boot to
> a shell, do 'cat /proc/cpuinfo', and look for a kernel warning.
>
> While the patches are being posted together in a series since they're
> for two different architectures they don't necessarily need to go
> through the same tree.
>
> v3:
> - Change condition from >= to == in order to still get a warning
> for > as that's unexpected. [Yury]
> - Picked up tags on the riscv patch
>
> v2:
> - Added all the information I should have in the first place
> to the commit message [Boris]
> - Changed style of fix [Boris]
>
> Andrew Jones (2):
> RISC-V: Fix /proc/cpuinfo cpumask warning
I just took the RISC-V fix, might be worth re-sending the x86 one alone
as nobody's replied over there so it may be lost.
Thanks!
> x86: Fix /proc/cpuinfo cpumask warning
>
> arch/riscv/kernel/cpu.c | 3 +++
> arch/x86/kernel/cpu/proc.c | 3 +++
> 2 files changed, 6 insertions(+)
On Thu, Oct 27, 2022 at 04:07:18PM -0700, Palmer Dabbelt wrote:
> On Fri, 14 Oct 2022 08:58:43 PDT (-0700), [email protected] wrote:
> > Commit 78e5a3399421 ("cpumask: fix checking valid cpu range") has
> > started issuing warnings[*] when cpu indices equal to nr_cpu_ids - 1
> > are passed to cpumask_next* functions. seq_read_iter() and cpuinfo's
> > start and next seq operations implement a pattern like
> >
> > n = cpumask_next(n - 1, mask);
> > show(n);
> > while (1) {
> > ++n;
> > n = cpumask_next(n - 1, mask);
> > if (n >= nr_cpu_ids)
> > break;
> > show(n);
> > }
> >
> > which will issue the warning when reading /proc/cpuinfo.
> >
> > [*] Warnings will only appear with DEBUG_PER_CPU_MAPS enabled.
> >
> > This series address the issue for x86 and riscv, but from a quick
> > grep of cpuinfo seq operations, I think at least openrisc, powerpc,
> > and s390 also need an equivalent patch. While the test is simple (see
> > next paragraph) I'm not equipped to test on each architecture.
> >
> > To test, just build a kernel with DEBUG_PER_CPU_MAPS enabled, boot to
> > a shell, do 'cat /proc/cpuinfo', and look for a kernel warning.
> >
> > While the patches are being posted together in a series since they're
> > for two different architectures they don't necessarily need to go
> > through the same tree.
> >
> > v3:
> > - Change condition from >= to == in order to still get a warning
> > for > as that's unexpected. [Yury]
> > - Picked up tags on the riscv patch
> >
> > v2:
> > - Added all the information I should have in the first place
> > to the commit message [Boris]
> > - Changed style of fix [Boris]
> >
> > Andrew Jones (2):
> > RISC-V: Fix /proc/cpuinfo cpumask warning
>
> I just took the RISC-V fix, might be worth re-sending the x86 one alone as
> nobody's replied over there so it may be lost.
Thanks Palmer. I still believe this fix is a good idea, or at least
not wrong, but as the cpumask change which started the warnings was
reverted (commit 80493877d7d0 ("Revert "cpumask: fix checking valid
cpu range".")) it seems the urgency for fixes like this one was
reduced. I'll ping the x86 patch to see if it's still of interest
or not.
Thanks,
drew
On Fri, Oct 14, 2022 at 05:58:45PM +0200, Andrew Jones wrote:
> Commit 78e5a3399421 ("cpumask: fix checking valid cpu range") has
> started issuing warnings[*] when cpu indices equal to nr_cpu_ids - 1
> are passed to cpumask_next* functions. seq_read_iter() and cpuinfo's
> start and next seq operations implement a pattern like
>
> n = cpumask_next(n - 1, mask);
> show(n);
> while (1) {
> ++n;
> n = cpumask_next(n - 1, mask);
> if (n >= nr_cpu_ids)
> break;
> show(n);
> }
>
> which will issue the warning when reading /proc/cpuinfo. Ensure no
> warning is generated by validating the cpu index before calling
> cpumask_next().
>
> [*] Warnings will only appear with DEBUG_PER_CPU_MAPS enabled.
>
> Signed-off-by: Andrew Jones <[email protected]>
> Cc: Yury Norov <[email protected]>
> ---
> arch/x86/kernel/cpu/proc.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
> index 099b6f0d96bd..de3f93ac6e49 100644
> --- a/arch/x86/kernel/cpu/proc.c
> +++ b/arch/x86/kernel/cpu/proc.c
> @@ -153,6 +153,9 @@ static int show_cpuinfo(struct seq_file *m, void *v)
>
> static void *c_start(struct seq_file *m, loff_t *pos)
> {
> + if (*pos == nr_cpu_ids)
> + return NULL;
> +
> *pos = cpumask_next(*pos - 1, cpu_online_mask);
> if ((*pos) < nr_cpu_ids)
> return &cpu_data(*pos);
> --
> 2.37.3
>
Hi x86 maintainers,
I realize 78e5a3399421 has now been reverted, so this fix is no longer
urgent. I don't believe it's wrong, though, so if it's still of interest,
then please consider this a friendly ping.
Thanks,
drew
On Fri, Oct 28, 2022 at 09:48:28AM +0200, Andrew Jones wrote:
> Hi x86 maintainers,
>
> I realize 78e5a3399421 has now been reverted, so this fix is no longer
> urgent. I don't believe it's wrong, though, so if it's still of interest,
> then please consider this a friendly ping.
>
> Thanks,
> drew
Hi Andrew,
I'll take it in bitmap-for-next this weekend.
Thanks,
Yury
On Fri, Oct 28, 2022 at 07:46:08AM -0700, Yury Norov wrote:
> I'll take it in bitmap-for-next this weekend.
Why?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Fri, Oct 28, 2022 at 10:13:28AM -0500, Yury Norov wrote:
> Because it's related to bitmap API usage and has been revealed after
> some work in bitmaps.
So first of all, that "fix" needs to explain what exactly it is fixing.
Not "it fixes this and that warning" but why the input arg to
cpumask_next() cannot be nr_cpu_ids because... yadda yadda...
> And because nobody else cares.
Why do you assume that?
> If you're willing to move it yourself please go ahead.
If it fixes a real issue, we are taking it. And pls note that x86
patches go through the tip tree.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Fri, Oct 28, 2022 at 06:06:41PM +0200, Borislav Petkov wrote:
> On Fri, Oct 28, 2022 at 10:13:28AM -0500, Yury Norov wrote:
> > Because it's related to bitmap API usage and has been revealed after
> > some work in bitmaps.
>
> So first of all, that "fix" needs to explain what exactly it is fixing.
> Not "it fixes this and that warning" but why the input arg to
> cpumask_next() cannot be nr_cpu_ids because... yadda yadda...
Hi Boris,
I didn't realize you were still looking for improvements to the commit
message for this patch. I could add something like,
The valid cpumask range is [0, nr_cpu_ids) and cpumask_next() always
returns a CPU ID greater than its input, which results in its input
range being [-1, nr_cpu_ids - 1). Ensure showing CPU info avoids
triggering error conditions in cpumask_next() by stopping its loop
over CPUs when its input would be invalid.
Thanks,
drew
>
> > And because nobody else cares.
>
> Why do you assume that?
>
> > If you're willing to move it yourself please go ahead.
>
> If it fixes a real issue, we are taking it. And pls note that x86
> patches go through the tip tree.
>
> Thx.
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Oct 31, 2022 at 09:06:04AM +0100, Andrew Jones wrote:
> The valid cpumask range is [0, nr_cpu_ids) and cpumask_next() always
> returns a CPU ID greater than its input, which results in its input
> range being [-1, nr_cpu_ids - 1). Ensure showing CPU info avoids
> triggering error conditions in cpumask_next() by stopping its loop
What error conditions?
What would happen if @n is outside of the valid range?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Oct 31, 2022 at 09:58:57AM +0100, Borislav Petkov wrote:
> On Mon, Oct 31, 2022 at 09:06:04AM +0100, Andrew Jones wrote:
> > The valid cpumask range is [0, nr_cpu_ids) and cpumask_next() always
> > returns a CPU ID greater than its input, which results in its input
> > range being [-1, nr_cpu_ids - 1). Ensure showing CPU info avoids
> > triggering error conditions in cpumask_next() by stopping its loop
>
> What error conditions?
>
> What would happen if @n is outside of the valid range?
Currently (after the revert of 78e5a3399421) with DEBUG_PER_CPU_MAPS we'll
get a warning splat when the cpu is outside the range [-1, nr_cpu_ids) and
cpumask_next() will call find_next_bit() with the input plus one anyway.
find_next_bit() doesn't explicity document what happens when an input is
outside the range, but it currently returns the bitmap size without any
side effects, which means cpumask_next() will return nr_cpu_ids.
show_cpuinfo() doesn't try to show anything in that case and stops its
loop, or, IOW, things work fine now with an input of nr_cpu_ids - 1. But,
show_cpuinfo() is just getting away with a violated cpumask_next()
contract, which 78e5a3399421 exposed. How about a new commit message like
this
seq_read_iter() and cpuinfo's start and next seq operations implement a
pattern like
n = cpumask_next(n - 1, mask);
show(n);
while (1) {
++n;
n = cpumask_next(n - 1, mask);
if (n >= nr_cpu_ids)
break;
show(n);
}
which loops until cpumask_next() identifies its CPU ID input is out of
its valid range, [-1, nr_cpu_ids - 1). seq_read_iter() assumes the
result of an invalid input is to return nr_cpu_ids or larger without any
side effects, however the cpumask API does not document that and it
reserves the right to change how it responds to invalid inputs. Ensure
inputs from seq_read_iter() are valid.
Thanks,
drew
On Mon, Oct 31, 2022 at 11:03:27AM +0100, Andrew Jones wrote:
> Currently (after the revert of 78e5a3399421)
After the revert?
That commit is still in the latest Linus tree.
> with DEBUG_PER_CPU_MAPS we'll get a warning splat when the cpu is
> outside the range [-1, nr_cpu_ids)
Yah, that range makes sense.
> and cpumask_next() will call find_next_bit() with the input plus one anyway.
> find_next_bit() doesn't explicity document what happens when an input is
> outside the range, but it currently returns the bitmap size without any
> side effects, which means cpumask_next() will return nr_cpu_ids.
That is good to have in the commit message.
> show_cpuinfo() doesn't try to show anything in that case and stops its
> loop, or, IOW, things work fine now with an input of nr_cpu_ids - 1. But,
> show_cpuinfo() is just getting away with a violated cpumask_next()
> contract, which 78e5a3399421 exposed. How about a new commit message like
> this
You're making it sound more complex than it is. All you wanna say is:
"Filter out invalid cpumask_next() inputs by checking its first argument
against nr_cpu_ids because cpumask_next() will call find_next_bit() with
the input plus one but the valid range for n is [-1, nr_cpu_ids)."
But that thing with the revert above needs to be clarified first.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Nov 02, 2022 at 07:44:02PM +0100, Borislav Petkov wrote:
> On Mon, Oct 31, 2022 at 11:03:27AM +0100, Andrew Jones wrote:
> > Currently (after the revert of 78e5a3399421)
>
> After the revert?
>
> That commit is still in the latest Linus tree.
The revert commit is 80493877d7d0 ("Revert "cpumask: fix checking valid
cpu range".")
>
> > with DEBUG_PER_CPU_MAPS we'll get a warning splat when the cpu is
> > outside the range [-1, nr_cpu_ids)
>
> Yah, that range makes sense.
>
> > and cpumask_next() will call find_next_bit() with the input plus one anyway.
> > find_next_bit() doesn't explicity document what happens when an input is
> > outside the range, but it currently returns the bitmap size without any
> > side effects, which means cpumask_next() will return nr_cpu_ids.
>
> That is good to have in the commit message.
>
> > show_cpuinfo() doesn't try to show anything in that case and stops its
> > loop, or, IOW, things work fine now with an input of nr_cpu_ids - 1. But,
> > show_cpuinfo() is just getting away with a violated cpumask_next()
> > contract, which 78e5a3399421 exposed. How about a new commit message like
> > this
>
> You're making it sound more complex than it is. All you wanna say is:
>
> "Filter out invalid cpumask_next() inputs by checking its first argument
> against nr_cpu_ids because cpumask_next() will call find_next_bit() with
> the input plus one but the valid range for n is [-1, nr_cpu_ids)."
The patch I'm proposing ensures cpumask_next()'s range, which is actually
[-1, nr_cpus_ids - 1), isn't violated. Violating that range will generate
the warning for kernels which have commit 78e5a3399421 ("cpumask: fix
checking valid cpu range"), but not its revert.
Since 78e5a3399421 has been reverted, the value of this proposed fix is
less, and indeed the warning may even go away completely for these types
of cpumask calls[1]. However, it seems reasonable for callers to implement
their own checks until the cpumask API has documented what they should
expect.
[1] https://lore.kernel.org/all/CAHk-=wihz-GXx66MmEyaADgS1fQE_LDcB9wrHAmkvXkd8nx9tA@mail.gmail.com/
>
> But that thing with the revert above needs to be clarified first.
I'll send a v4 with another stab at the commit message.
Thanks,
drew
>
> Thx.
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Nov 03, 2022 at 01:59:45PM +0100, Andrew Jones wrote:
> The patch I'm proposing ensures cpumask_next()'s range, which is actually
> [-1, nr_cpus_ids - 1),
Lemme make sure I understand it correctly: on the upper boundary, if you
supply for n the value nr_cpu_ids - 2, then it will return potentially
the last bit if the mask is set, i.e., the one at position (nr_cpu_ids - 1).
If you supply nr_cpus_ids - 1, then it'll return nr_cpu_ids to signal no
further bits set.
Yes, no?
> I'll send a v4 with another stab at the commit message.
Yes, and it is still an unreadable mess: "A kernel compiled with commit
... but not its revert... " Nope.
First make sure cpumask_next()'s valid accepted range has been settled
upon, has been explicitly documented in a comment above it and then I'll
take a patch that fixes whatever is there to fix.
Callers should not have to filter values before passing them in - the
function either returns an error or returns the next bit in the mask.
This thing:
if (*pos == nr_cpu_ids)
but then to pass in pos - 1:
*pos = cpumask_next(*pos - 1
looks to me like the interface needs more cooking.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Nov 03, 2022 at 04:02:12PM +0100, Borislav Petkov wrote:
> On Thu, Nov 03, 2022 at 01:59:45PM +0100, Andrew Jones wrote:
> > The patch I'm proposing ensures cpumask_next()'s range, which is actually
> > [-1, nr_cpus_ids - 1),
>
> Lemme make sure I understand it correctly: on the upper boundary, if you
> supply for n the value nr_cpu_ids - 2, then it will return potentially
> the last bit if the mask is set, i.e., the one at position (nr_cpu_ids - 1).
>
> If you supply nr_cpus_ids - 1, then it'll return nr_cpu_ids to signal no
> further bits set.
>
> Yes, no?
Yes
>
> > I'll send a v4 with another stab at the commit message.
>
> Yes, and it is still an unreadable mess: "A kernel compiled with commit
> ... but not its revert... " Nope.
>
> First make sure cpumask_next()'s valid accepted range has been settled
> upon, has been explicitly documented in a comment above it and then I'll
> take a patch that fixes whatever is there to fix.
That's fair, but I'll leave that to Yury.
>
> Callers should not have to filter values before passing them in - the
> function either returns an error or returns the next bit in the mask.
That's reasonable, but cpumask folk probably need to discuss it because
not all cpumask functions have a return value where an error may be
placed.
>
> This thing:
>
> if (*pos == nr_cpu_ids)
>
> but then to pass in pos - 1:
>
> *pos = cpumask_next(*pos - 1
>
> looks to me like the interface needs more cooking.
Indeed, but that's less of an issue with cpumask_next() than with
the way cpuinfo implements its start and next seq ops (next
unconditionally increments *pos and then calls start and start
must use *pos - 1 since the first time its called it needs to use
-1).
Thanks,
drew
On Thu, Nov 03, 2022 at 04:34:04PM +0100, Andrew Jones wrote:
> Indeed, but that's less of an issue with cpumask_next() than with
> the way cpuinfo implements its start and next seq ops (next
> unconditionally increments *pos and then calls start and start
> must use *pos - 1 since the first time its called it needs to use
> -1).
Maybe because those are done wrongly...
A ->next() function should not call the ->start() function. A ->start()
function should, well, only start and nothing else.
And a ->stop() function should maybe check *pos and say whether one
should stop or not.
But I haven't looked at seq_ops at least in a decade and I have no clue
whether that would work.
I'm just looking at the function pointers and am trying to spell out
what looks most natural IMO.
IOW, maybe this should be fixed "right" and not only "made to work".
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Nov 03, 2022 at 04:34:04PM +0100, Andrew Jones wrote:
> On Thu, Nov 03, 2022 at 04:02:12PM +0100, Borislav Petkov wrote:
> > On Thu, Nov 03, 2022 at 01:59:45PM +0100, Andrew Jones wrote:
> > > The patch I'm proposing ensures cpumask_next()'s range, which is actually
> > > [-1, nr_cpus_ids - 1),
> >
> > Lemme make sure I understand it correctly: on the upper boundary, if you
> > supply for n the value nr_cpu_ids - 2, then it will return potentially
> > the last bit if the mask is set, i.e., the one at position (nr_cpu_ids - 1).
> >
> > If you supply nr_cpus_ids - 1, then it'll return nr_cpu_ids to signal no
> > further bits set.
> >
> > Yes, no?
>
> Yes
>
> >
> > > I'll send a v4 with another stab at the commit message.
> >
> > Yes, and it is still an unreadable mess: "A kernel compiled with commit
> > ... but not its revert... " Nope.
> >
> > First make sure cpumask_next()'s valid accepted range has been settled
> > upon, has been explicitly documented in a comment above it and then I'll
> > take a patch that fixes whatever is there to fix.
>
> That's fair, but I'll leave that to Yury.
I'll take care of it.
> > Callers should not have to filter values before passing them in - the
> > function either returns an error or returns the next bit in the mask.
>
> That's reasonable, but cpumask folk probably need to discuss it because
> not all cpumask functions have a return value where an error may be
> placed.
Callers should pass sane arguments into internal functions if they
expect sane output. The API not exported to userspace shouldn't
sanity-check all inputs arguments. For example, cpumask_next() doesn't
check srcp for NULL.
However, cpumask API is exposed to drivers, and that's why optional
cpumask_check() exists. (Probably. It has been done long before I took
over this.)
Current *generic* implementation guarantees that out-of-region offset
would prevent cpumask_next() from dereferencing srcp, and makes it
returning nr_cpu_ids. This behavior is expected by many callers. However,
there is a couple of non-generic cpumask implementations, and one of
them is written in assembler. So, the portable code shouldn't expect
from cpumasks more than documentation said: for a _valid_ offset
cpumask_next() returns next set bit or >= nr_cpu_ids.
cpumask_check() has been broken for years. Attempting to fix it faced
so much resistance, that I had to revert the patch. Now there's
ongoing discussion whether we need this check at all. My opinion is
that if all implementations of cpumask (more precisely, underlying
bitmap API) are safe against out-of-range offset, we can simply remove
cpumask_check(). Those users, like cpuinfo, who waste time on useless
last iteration will bear it themselves.
Thanks,
Yury
On Thu, Nov 03, 2022 at 09:30:54AM -0700, [email protected] wrote:a
> Callers should pass sane arguments into internal functions if they
> expect sane output.
What internal function? It's in a global header.
> The API not exported to userspace shouldn't sanity-check all inputs
> arguments.
That doesn't have anything to do with userspace at all.
APIs exported to the rest of the kernel should very well check their
inputs. Otherwise they're not APIs - just some random functions which
are visible to the compiler.
> So, the portable code shouldn't expect from cpumasks more than
> documentation said: for a _valid_ offset cpumask_next() returns next
> set bit or >= nr_cpu_ids.
Lemme quote from my previous mail:
"First make sure cpumask_next()'s valid accepted range has been settled
upon, has been explicitly documented"
So where is that valid range documented?
> cpumask_check() has been broken for years. Attempting to fix it faced
> so much resistance, that I had to revert the patch.
The suggestion on that thread made sense: you first fix the callers and
then the interface. Just like any other "broken" kernel API.
Nothing's stopping you from fixing it properly - it'll just take a while
and if it is such a widely used interface, you probably should come up
with a strategy first how to fix it without impacting current use.
Interfaces and their in-kernel users get refactored constantly.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Nov 03, 2022 at 05:49:06PM +0100, Borislav Petkov wrote:
> On Thu, Nov 03, 2022 at 09:30:54AM -0700, [email protected] wrote:a
> > Callers should pass sane arguments into internal functions if they
> > expect sane output.
>
> What internal function? It's in a global header.
>
> > The API not exported to userspace shouldn't sanity-check all inputs
> > arguments.
>
> That doesn't have anything to do with userspace at all.
>
> APIs exported to the rest of the kernel should very well check their
> inputs. Otherwise they're not APIs - just some random functions which
> are visible to the compiler.
Let's take for example cpu_llc_shared_mask() added by you in
arch/x86/include/asm/smp.h recently:
static inline struct cpumask *cpu_llc_shared_mask(int cpu)
{
return per_cpu(cpu_llc_shared_map, cpu);
}
It's in a global header and available to the rest of the kernel, just as
well. How does it check its input? Maybe I lost something important in
per_cpu() internals, but at the first glance, there's no any protection
against -1, nr_cpu_ids, and other out-of-range arguments.
On Thu, Nov 03, 2022 at 10:31:30AM -0700, Yury Norov wrote:
> Let's take for example cpu_llc_shared_mask() added by you in
> arch/x86/include/asm/smp.h recently:
>
> static inline struct cpumask *cpu_llc_shared_mask(int cpu)
> {
> return per_cpu(cpu_llc_shared_map, cpu);
> }
>
> It's in a global header and available to the rest of the kernel, just as
> well.
Just like
static inline struct cpumask *cpu_l2c_shared_mask(int cpu)
{
return per_cpu(cpu_l2c_shared_map, cpu);
}
should check != must check.
But it's perfectly fine if you're going to attempt to prove some bogus
argument of yours - I can safely ignore you.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette