Re: [PATCH RESEND] sh: sq: Use the bitmap API when applicable

Hi Adrian,

On Tue, Apr 18, 2023 at 8:36 AM John Paul Adrian Glaubitz
<[email protected]> 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 <[email protected]>

> > --- 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 -- [email protected]

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