* [PATCH] x86: Fix /proc/cpuinfo cpumask warning
@ 2022-10-11 17:50 Andrew Jones
2022-10-11 18:01 ` Borislav Petkov
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Jones @ 2022-10-11 17:50 UTC (permalink / raw)
To: x86, linux-kernel
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, Yury Norov
Upcoming cpumask changes will start issuing warnings[*] when cpu
indices equal to nr_cpu_ids are passed to cpumask_next* functions.
Ensure we don't generate a warning when reading /proc/cpuinfo by
validating the cpu index before calling cpumask_next().
[*] Warnings will only appear with DEBUG_PER_CPU_MAPS enabled.
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Cc: Yury Norov <yury.norov@gmail.com>
---
arch/x86/kernel/cpu/proc.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
index 099b6f0d96bd..584ae6cb5b87 100644
--- a/arch/x86/kernel/cpu/proc.c
+++ b/arch/x86/kernel/cpu/proc.c
@@ -153,9 +153,12 @@ static int show_cpuinfo(struct seq_file *m, void *v)
static void *c_start(struct seq_file *m, loff_t *pos)
{
- *pos = cpumask_next(*pos - 1, cpu_online_mask);
- if ((*pos) < nr_cpu_ids)
- return &cpu_data(*pos);
+ if (*pos < nr_cpu_ids) {
+ *pos = cpumask_next(*pos - 1, cpu_online_mask);
+ if (*pos < nr_cpu_ids)
+ return &cpu_data(*pos);
+ }
+
return NULL;
}
--
2.37.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] x86: Fix /proc/cpuinfo cpumask warning
2022-10-11 17:50 [PATCH] x86: Fix /proc/cpuinfo cpumask warning Andrew Jones
@ 2022-10-11 18:01 ` Borislav Petkov
2022-10-11 18:17 ` Andrew Jones
0 siblings, 1 reply; 5+ messages in thread
From: Borislav Petkov @ 2022-10-11 18:01 UTC (permalink / raw)
To: Andrew Jones
Cc: x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Dave Hansen, Yury Norov
On Tue, Oct 11, 2022 at 07:50:31PM +0200, Andrew Jones wrote:
> Upcoming cpumask changes will start issuing warnings[*] when cpu
What upcoming changes?
This needs a concrete pointer to a commit or so.
> indices equal to nr_cpu_ids are passed to cpumask_next* functions.
How do those indices get passed here? I think you need to explain how
exactly this happens.
> Ensure we don't generate a warning when reading /proc/cpuinfo by
Please use passive voice in your commit message: no "we" or "I", etc,
and describe your changes in imperative mood.
> validating the cpu index before calling cpumask_next().
s/cpu/CPU/g
> [*] Warnings will only appear with DEBUG_PER_CPU_MAPS enabled.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> Cc: Yury Norov <yury.norov@gmail.com>
> ---
> arch/x86/kernel/cpu/proc.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
> index 099b6f0d96bd..584ae6cb5b87 100644
> --- a/arch/x86/kernel/cpu/proc.c
> +++ b/arch/x86/kernel/cpu/proc.c
> @@ -153,9 +153,12 @@ static int show_cpuinfo(struct seq_file *m, void *v)
>
> static void *c_start(struct seq_file *m, loff_t *pos)
> {
> - *pos = cpumask_next(*pos - 1, cpu_online_mask);
> - if ((*pos) < nr_cpu_ids)
> - return &cpu_data(*pos);
> + if (*pos < nr_cpu_ids) {
> + *pos = cpumask_next(*pos - 1, cpu_online_mask);
> + if (*pos < nr_cpu_ids)
> + return &cpu_data(*pos);
> + }
Simpler: on function entry:
if (*pos >= nr_cpu_ids)
return NULL;
/* the rest remains unchanged */
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86: Fix /proc/cpuinfo cpumask warning
2022-10-11 18:01 ` Borislav Petkov
@ 2022-10-11 18:17 ` Andrew Jones
2022-10-11 19:02 ` Yury Norov
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Jones @ 2022-10-11 18:17 UTC (permalink / raw)
To: Borislav Petkov
Cc: x86, linux-kernel, Thomas Gleixner, Ingo Molnar, Dave Hansen, Yury Norov
On Tue, Oct 11, 2022 at 08:01:03PM +0200, Borislav Petkov wrote:
> On Tue, Oct 11, 2022 at 07:50:31PM +0200, Andrew Jones wrote:
> > Upcoming cpumask changes will start issuing warnings[*] when cpu
>
> What upcoming changes?
>
> This needs a concrete pointer to a commit or so.
Hi Boris,
Sorry, I should have pointed this out. The upcoming change is
linux-next/master commit a314123c8bdb ("cpumask: fix checking valid cpu
range")
And also an ongoing discussion here
https://lore.kernel.org/lkml/20221011170949.upxk3tcfcwnkytwm@kamzik/
I'm hoping that Yury will pick these patches up and integrate
them at the front of his series when introducing the warnings.
I wasn't sure how to call that out other than with the generic
"upcoming change".
>
> > indices equal to nr_cpu_ids are passed to cpumask_next* functions.
>
> How do those indices get passed here? I think you need to explain how
> exactly this happens.
I took a stab at explaining it in the discussion thread[1] just now and I
can bring that explanation into the commit message for v2.
[1] https://lore.kernel.org/lkml/20221011180442.cwjtcvjioias3qt6@kamzik/
>
> > Ensure we don't generate a warning when reading /proc/cpuinfo by
>
> Please use passive voice in your commit message: no "we" or "I", etc,
> and describe your changes in imperative mood.
I'll change to "Ensure no warning is generated ..."
>
> > validating the cpu index before calling cpumask_next().
>
> s/cpu/CPU/g
>
> > [*] Warnings will only appear with DEBUG_PER_CPU_MAPS enabled.
> >
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > Cc: Yury Norov <yury.norov@gmail.com>
> > ---
> > arch/x86/kernel/cpu/proc.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
> > index 099b6f0d96bd..584ae6cb5b87 100644
> > --- a/arch/x86/kernel/cpu/proc.c
> > +++ b/arch/x86/kernel/cpu/proc.c
> > @@ -153,9 +153,12 @@ static int show_cpuinfo(struct seq_file *m, void *v)
> >
> > static void *c_start(struct seq_file *m, loff_t *pos)
> > {
> > - *pos = cpumask_next(*pos - 1, cpu_online_mask);
> > - if ((*pos) < nr_cpu_ids)
> > - return &cpu_data(*pos);
> > + if (*pos < nr_cpu_ids) {
> > + *pos = cpumask_next(*pos - 1, cpu_online_mask);
> > + if (*pos < nr_cpu_ids)
> > + return &cpu_data(*pos);
> > + }
>
> Simpler: on function entry:
>
> if (*pos >= nr_cpu_ids)
> return NULL;
>
> /* the rest remains unchanged */
Will do for v2.
Thanks,
drew
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86: Fix /proc/cpuinfo cpumask warning
2022-10-11 18:17 ` Andrew Jones
@ 2022-10-11 19:02 ` Yury Norov
2022-10-12 6:51 ` Andrew Jones
0 siblings, 1 reply; 5+ messages in thread
From: Yury Norov @ 2022-10-11 19:02 UTC (permalink / raw)
To: Andrew Jones
Cc: Borislav Petkov, x86, linux-kernel, Thomas Gleixner, Ingo Molnar,
Dave Hansen
On Tue, Oct 11, 2022 at 11:17 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Tue, Oct 11, 2022 at 08:01:03PM +0200, Borislav Petkov wrote:
> > On Tue, Oct 11, 2022 at 07:50:31PM +0200, Andrew Jones wrote:
> > > Upcoming cpumask changes will start issuing warnings[*] when cpu
> >
> > What upcoming changes?
> >
> > This needs a concrete pointer to a commit or so.
>
> Hi Boris,
>
> Sorry, I should have pointed this out. The upcoming change is
>
> linux-next/master commit a314123c8bdb ("cpumask: fix checking valid cpu
> range")
>
> And also an ongoing discussion here
> https://lore.kernel.org/lkml/20221011170949.upxk3tcfcwnkytwm@kamzik/
>
> I'm hoping that Yury will pick these patches up and integrate
> them at the front of his series when introducing the warnings.
> I wasn't sure how to call that out other than with the generic
> "upcoming change".
>
> >
> > > indices equal to nr_cpu_ids are passed to cpumask_next* functions.
> >
> > How do those indices get passed here? I think you need to explain how
> > exactly this happens.
The cpumask_check() fix is already in master. Because of some mess in
cpumask, cpumask_check() was broken for quite a long time, and didn't
bark when passed with an out-of-range CPU.
I fixed some false-positives and sent those fixes together with a314123c8bdb.
Now, I expect that people will see warnings generated by correct
cpumask_check().
This is actually the first sign.
Andrew, can you please answer Borislav's question:
> > How do those indices get passed here? I think you need to explain how
> > exactly this happens.
It might be either an expected behaviour, and then there should be a great
explanation on how and why things work in the subsystem.
Or it might be an error in the caller. In that case, the caller must be fixed.
Thanks,
Yury
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86: Fix /proc/cpuinfo cpumask warning
2022-10-11 19:02 ` Yury Norov
@ 2022-10-12 6:51 ` Andrew Jones
0 siblings, 0 replies; 5+ messages in thread
From: Andrew Jones @ 2022-10-12 6:51 UTC (permalink / raw)
To: Yury Norov
Cc: Borislav Petkov, x86, linux-kernel, Thomas Gleixner, Ingo Molnar,
Dave Hansen
On Tue, Oct 11, 2022 at 12:02:26PM -0700, Yury Norov wrote:
> On Tue, Oct 11, 2022 at 11:17 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Tue, Oct 11, 2022 at 08:01:03PM +0200, Borislav Petkov wrote:
> > > On Tue, Oct 11, 2022 at 07:50:31PM +0200, Andrew Jones wrote:
> > > > Upcoming cpumask changes will start issuing warnings[*] when cpu
> > >
> > > What upcoming changes?
> > >
> > > This needs a concrete pointer to a commit or so.
> >
> > Hi Boris,
> >
> > Sorry, I should have pointed this out. The upcoming change is
> >
> > linux-next/master commit a314123c8bdb ("cpumask: fix checking valid cpu
> > range")
> >
> > And also an ongoing discussion here
> > https://lore.kernel.org/lkml/20221011170949.upxk3tcfcwnkytwm@kamzik/
> >
> > I'm hoping that Yury will pick these patches up and integrate
> > them at the front of his series when introducing the warnings.
> > I wasn't sure how to call that out other than with the generic
> > "upcoming change".
> >
> > >
> > > > indices equal to nr_cpu_ids are passed to cpumask_next* functions.
> > >
> > > How do those indices get passed here? I think you need to explain how
> > > exactly this happens.
>
> The cpumask_check() fix is already in master. Because of some mess in
Ah, it's already in master. I should have checked that...
> cpumask, cpumask_check() was broken for quite a long time, and didn't
> bark when passed with an out-of-range CPU.
>
> I fixed some false-positives and sent those fixes together with a314123c8bdb.
> Now, I expect that people will see warnings generated by correct
> cpumask_check().
> This is actually the first sign.
>
> Andrew, can you please answer Borislav's question:
I answered in the discussion[1], but I'll send a v2 of this patch,
properly based on master, with a proper pointer to commit 78e5a3399421
("cpumask: fix checking valid cpu range"), and also with a condensed
analysis for justification in the commit message.
Thanks,
drew
>
> > > How do those indices get passed here? I think you need to explain how
> > > exactly this happens.
>
> It might be either an expected behaviour, and then there should be a great
> explanation on how and why things work in the subsystem.
>
> Or it might be an error in the caller. In that case, the caller must be fixed.
>
> Thanks,
> Yury
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-10-12 6:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-11 17:50 [PATCH] x86: Fix /proc/cpuinfo cpumask warning Andrew Jones
2022-10-11 18:01 ` Borislav Petkov
2022-10-11 18:17 ` Andrew Jones
2022-10-11 19:02 ` Yury Norov
2022-10-12 6:51 ` Andrew Jones
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).