[v1] x86: Fix /proc/cpuinfo cpumask warning

[PATCH] x86: Fix /proc/cpuinfo cpumask warning

Posted by Andrew Jones 3 years, 5 months ago

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

Re: [PATCH] x86: Fix /proc/cpuinfo cpumask warning

Posted by Borislav Petkov 3 years, 5 months ago

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

Re: [PATCH] x86: Fix /proc/cpuinfo cpumask warning

Posted by Andrew Jones 3 years, 5 months ago

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

Re: [PATCH] x86: Fix /proc/cpuinfo cpumask warning

Posted by Yury Norov 3 years, 5 months ago

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

Re: [PATCH] x86: Fix /proc/cpuinfo cpumask warning

Posted by Andrew Jones 3 years, 5 months ago

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