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]>
---
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
On Wed, Oct 12, 2022 at 1:59 PM Andrew Jones <[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. 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]>
Looks good to me.
Reviewed-by: Anup Patel <[email protected]>
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
>
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 <[email protected]>
> Cc: Yury Norov <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
Tested-by: Conor Dooley <[email protected]>
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
>
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 <[email protected]>
> Cc: Yury Norov <[email protected]>
> ---
> 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
On Wed, Oct 12, 2022 at 6:13 AM Andrew Jones <[email protected]> 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 <[email protected]>
> > > Cc: Yury Norov <[email protected]>
> > > ---
> > > 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.
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 <[email protected]>
> > Cc: Yury Norov <[email protected]>
> > ---
> > 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