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;
Date:   Wed, 12 Oct 2022 15:13:25 +0200
From:   Andrew Jones <ajones@ventanamicro.com>
To:     Yury Norov <yury.norov@gmail.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>
Subject: Re: [PATCH v2] RISC-V: Fix /proc/cpuinfo cpumask warning
Message-ID: <20221012131325.oc7onxqo7t46wxq5@kamzik>
References: <20221012082949.1801222-1-ajones@ventanamicro.com>
 <Y0a5QYWzEwRlGtxf@yury-laptop>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y0a5QYWzEwRlGtxf@yury-laptop>
Precedence: bulk

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