[PATCH v2] RISC-V: Fix /proc/cpuinfo cpumask warning

[PATCH v2] RISC-V: Fix /proc/cpuinfo cpumask warning

[Andrew Jones]

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 <ajones@ventanamicro.com>
Cc: Yury Norov <yury.norov@gmail.com>
---
v2:
  - Got comments on the x86 equivalent patch and made the same
    changes to this one
    - Added all the information I should have in the first place
      to the commit message [Boris]
    - Changed style of fix [Boris]


 arch/riscv/kernel/cpu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 4aa8cd749441..63138b880b92 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -166,6 +166,9 @@ static void print_mmu(struct seq_file *f)
 
 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 (void *)(uintptr_t)(1 + *pos);
-- 
2.37.3

Re: [PATCH v2] RISC-V: Fix /proc/cpuinfo cpumask warning

[Anup Patel]

On Wed, Oct 12, 2022 at 1:59 PM Andrew Jones <ajones@ventanamicro.com> 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 <ajones@ventanamicro.com>
> Cc: Yury Norov <yury.norov@gmail.com>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
> v2:
>   - Got comments on the x86 equivalent patch and made the same
>     changes to this one
>     - Added all the information I should have in the first place
>       to the commit message [Boris]
>     - Changed style of fix [Boris]
>
>
>  arch/riscv/kernel/cpu.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 4aa8cd749441..63138b880b92 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -166,6 +166,9 @@ static void print_mmu(struct seq_file *f)
>
>  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 (void *)(uintptr_t)(1 + *pos);
> --
> 2.37.3
>

Re: [PATCH v2] RISC-V: Fix /proc/cpuinfo cpumask warning

[Conor Dooley]

On Wed, Oct 12, 2022 at 10:29:49AM +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 <ajones@ventanamicro.com>
> Cc: Yury Norov <yury.norov@gmail.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Tested-by: Conor Dooley <conor.dooley@microchip.com>

Thanks

> ---
> v2:
>   - Got comments on the x86 equivalent patch and made the same
>     changes to this one
>     - Added all the information I should have in the first place
>       to the commit message [Boris]
>     - Changed style of fix [Boris]
> 
> 
>  arch/riscv/kernel/cpu.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 4aa8cd749441..63138b880b92 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -166,6 +166,9 @@ static void print_mmu(struct seq_file *f)
>  
>  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 (void *)(uintptr_t)(1 + *pos);
> -- 
> 2.37.3
> 

Re: [PATCH v2] RISC-V: Fix /proc/cpuinfo cpumask warning

[Yury Norov]

On Wed, Oct 12, 2022 at 10:29:49AM +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);
>   }

Can you instead of sudo-code print show the real control flow? What
function hosts the infinite loop?

> 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 <ajones@ventanamicro.com>
> Cc: Yury Norov <yury.norov@gmail.com>
> ---
> v2:
>   - Got comments on the x86 equivalent patch and made the same
>     changes to this one
>     - Added all the information I should have in the first place
>       to the commit message [Boris]
>     - Changed style of fix [Boris]
> 
> 
>  arch/riscv/kernel/cpu.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 4aa8cd749441..63138b880b92 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -166,6 +166,9 @@ static void print_mmu(struct seq_file *f)
>  
>  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 (void *)(uintptr_t)(1 + *pos);

OK, as far as I understood your explanations, *pos == nr_cpu_ids
is a valid index because it's used as stop-code for traversing.

However, you're completely silencing cpumask_check(), including
those cases where *pos > nr_cpu_ids. I suspect there's no valid
cases for it. If so, the patch should look like:

 +	if (*pos == nr_cpu_ids)
 +		return NULL;
 +

The same for x86 patch. 

If it comes to v3, can you send both as a series?

Thanks,
Yury

Re: [PATCH v2] RISC-V: Fix /proc/cpuinfo cpumask warning

[Andrew Jones]

On Wed, Oct 12, 2022 at 05:55:29AM -0700, Yury Norov wrote:
> On Wed, Oct 12, 2022 at 10:29:49AM +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);
> >   }
> 
> Can you instead of sudo-code print show the real control flow? What
> function hosts the infinite loop?

The function is seq_read_iter(), which is pointed out above. I'd rather
not reproduce / describe more than what I've done here, as the function
is large. I'd be happy for reviewers to double check my pseudocode to
make sure I got it and the analysis right, though.

> 
> > 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 <ajones@ventanamicro.com>
> > Cc: Yury Norov <yury.norov@gmail.com>
> > ---
> > v2:
> >   - Got comments on the x86 equivalent patch and made the same
> >     changes to this one
> >     - Added all the information I should have in the first place
> >       to the commit message [Boris]
> >     - Changed style of fix [Boris]
> > 
> > 
> >  arch/riscv/kernel/cpu.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > index 4aa8cd749441..63138b880b92 100644
> > --- a/arch/riscv/kernel/cpu.c
> > +++ b/arch/riscv/kernel/cpu.c
> > @@ -166,6 +166,9 @@ static void print_mmu(struct seq_file *f)
> >  
> >  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 (void *)(uintptr_t)(1 + *pos);
> 
> OK, as far as I understood your explanations, *pos == nr_cpu_ids
> is a valid index because it's used as stop-code for traversing.
> 
> However, you're completely silencing cpumask_check(), including
> those cases where *pos > nr_cpu_ids. I suspect there's no valid
> cases for it. If so, the patch should look like:
> 
>  +	if (*pos == nr_cpu_ids)
>  +		return NULL;
>  +

That makes sense and it's probably worth a v3. I'll wait and see if more
comments roll in before sending though.

> 
> The same for x86 patch. 
> 
> If it comes to v3, can you send both as a series?

OK. I'll write a cover letter trying to explain that I don't expect them
to both go through the same tree.

Thanks,
drew

Re: [PATCH v2] RISC-V: Fix /proc/cpuinfo cpumask warning

[Yury Norov]

On Wed, Oct 12, 2022 at 6:13 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Wed, Oct 12, 2022 at 05:55:29AM -0700, Yury Norov wrote:
> > On Wed, Oct 12, 2022 at 10:29:49AM +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);
> > >   }
> >
> > Can you instead of sudo-code print show the real control flow? What
> > function hosts the infinite loop?
>
> The function is seq_read_iter(), which is pointed out above. I'd rather
> not reproduce / describe more than what I've done here, as the function
> is large. I'd be happy for reviewers to double check my pseudocode to
> make sure I got it and the analysis right, though.
>
> >
> > > 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 <ajones@ventanamicro.com>
> > > Cc: Yury Norov <yury.norov@gmail.com>
> > > ---
> > > v2:
> > >   - Got comments on the x86 equivalent patch and made the same
> > >     changes to this one
> > >     - Added all the information I should have in the first place
> > >       to the commit message [Boris]
> > >     - Changed style of fix [Boris]
> > >
> > >
> > >  arch/riscv/kernel/cpu.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> > > index 4aa8cd749441..63138b880b92 100644
> > > --- a/arch/riscv/kernel/cpu.c
> > > +++ b/arch/riscv/kernel/cpu.c
> > > @@ -166,6 +166,9 @@ static void print_mmu(struct seq_file *f)
> > >
> > >  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 (void *)(uintptr_t)(1 + *pos);
> >
> > OK, as far as I understood your explanations, *pos == nr_cpu_ids
> > is a valid index because it's used as stop-code for traversing.
> >
> > However, you're completely silencing cpumask_check(), including
> > those cases where *pos > nr_cpu_ids. I suspect there's no valid
> > cases for it. If so, the patch should look like:
> >
> >  +    if (*pos == nr_cpu_ids)
> >  +            return NULL;
> >  +
>
> That makes sense and it's probably worth a v3. I'll wait and see if more
> comments roll in before sending though.
>
> >
> > The same for x86 patch.
> >
> > If it comes to v3, can you send both as a series?
>
> OK. I'll write a cover letter trying to explain that I don't expect them
> to both go through the same tree.

I can take it in my tree, if it helps.