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: <071e9f32c19a007f4922903282c9121898641400.1681671848.git.christophe.jaillet@wanadoo.fr>
 <b5fea49d68e1e2a702b0050f73582526e205cfa2.camel@physik.fu-berlin.de>
In-Reply-To: <b5fea49d68e1e2a702b0050f73582526e205cfa2.camel@physik.fu-berlin.de>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Tue, 18 Apr 2023 09:14:25 +0200
Message-ID: <CAMuHMdWz7YJ4ifkxU2GGdoj46fTsjS5WE66R0YzvOYr1ZKY=4w@mail.gmail.com>
Subject: Re: [PATCH RESEND] sh: sq: Use the bitmap API when applicable
To:     John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
Cc:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
        Yoshinori Sato <ysato@users.sourceforge.jp>,
        Rich Felker <dalias@libc.org>, linux-kernel@vger.kernel.org,
        kernel-janitors@vger.kernel.org, linux-sh@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

Hi Adrian,

On Tue, Apr 18, 2023 at 8:36 AM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> Thanks for your patch. The changes look good to me. However, I have
> one question, see below.
>
> On Sun, 2023-04-16 at 21:05 +0200, Christophe JAILLET wrote:
> > Using the bitmap API is less verbose than hand writing them.
> > It also improves the semantic.
> >
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

> > --- a/arch/sh/kernel/cpu/sh4/sq.c
> > +++ b/arch/sh/kernel/cpu/sh4/sq.c
> > @@ -372,7 +372,6 @@ static struct subsys_interface sq_interface = {
> >  static int __init sq_api_init(void)
> >  {
> >       unsigned int nr_pages = 0x04000000 >> PAGE_SHIFT;
> > -     unsigned int size = (nr_pages + (BITS_PER_LONG - 1)) / BITS_PER_LONG;
> >       int ret = -ENOMEM;
> >
> >       printk(KERN_NOTICE "sq: Registering store queue API.\n");
> > @@ -382,7 +381,7 @@ static int __init sq_api_init(void)
> >       if (unlikely(!sq_cache))
> >               return ret;
> >
> > -     sq_bitmap = kzalloc(size, GFP_KERNEL);
> > +     sq_bitmap = bitmap_zalloc(nr_pages, GFP_KERNEL);
> >       if (unlikely(!sq_bitmap))
> >               goto out;
> >
>
> I have look through other patches where k{z,c,m}alloc() were replaced with
> bitmap_zalloc() and I noticed that in the other cases such as [1], kcalloc()
> was used instead of kzalloc() in our cases with the element size set to
> sizeof(long) while kzalloc() is using an element size equal to a byte.
>
> Wouldn't that mean that the current code in sq is allocating a buffer that is
> too small by a factor of 1/sizeof(long) or am I missing something?
>
> @Geert: Do you have any idea?

Nice catch!

Looking more deeply at the code, the intention is to allocate a bitmap
with nr_pages bits, so the code fater Christophe's patch is correct.
However, the old code is indeed wrong:

    (nr_pages + (BITS_PER_LONG - 1)) / BITS_PER_LONG

The aim is to calculate the size in bytes, rounded up to an integral
number of longs, but it lacks a final multiplication by BITS_PER_BYTE,
so it's off by a factor of 4.

Fixes: d7c30c682a278abe ("sh: Store Queue API rework.")

As we didn't have bitmap_zalloc() until commit c42b65e363ce97a8
("bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()")
in v4.19, it would be good to fix the bug first in a separate patch,
not using

BTW, interesting how this got missed when fixing the other out-of-range
bug in commit 9f650cf2b811cfb6 ("sh: Fix store queue bitmap end.",
s/marc.theaimsgroup.com/marc.info/ when following the link).

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds