Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221012082949.1801222-1-ajones@ventanamicro.com>
 <Y0a5QYWzEwRlGtxf@yury-laptop> <20221012131325.oc7onxqo7t46wxq5@kamzik>
In-Reply-To: <20221012131325.oc7onxqo7t46wxq5@kamzik>
From:   Yury Norov <yury.norov@gmail.com>
Date:   Wed, 12 Oct 2022 06:41:16 -0700
Message-ID: <CAAH8bW9z6D_DRCAvQi5BUN9dh82LBZwGd-GWa8r+MFc=xs6xVw@mail.gmail.com>
Subject: Re: [PATCH v2] RISC-V: Fix /proc/cpuinfo cpumask warning
To:     Andrew Jones <ajones@ventanamicro.com>
Cc:     linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Albert Ou <aou@eecs.berkeley.edu>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.