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

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

[Christophe JAILLET]

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>
---
This is a resend of [1].

Now cross-compile tested with CONFIG_CPU_SUBTYPE_SH7770=y

[1]: https://lore.kernel.org/all/521788e22ad8f7a5058c154f068b061525321841.1656142814.git.christophe.jaillet@wanadoo.fr/
---
 arch/sh/kernel/cpu/sh4/sq.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/sh/kernel/cpu/sh4/sq.c b/arch/sh/kernel/cpu/sh4/sq.c
index 27f2e3da5aa2..d289e99dc118 100644
--- 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;
 
@@ -393,7 +392,7 @@ static int __init sq_api_init(void)
 	return 0;
 
 out:
-	kfree(sq_bitmap);
+	bitmap_free(sq_bitmap);
 	kmem_cache_destroy(sq_cache);
 
 	return ret;
@@ -402,7 +401,7 @@ static int __init sq_api_init(void)
 static void __exit sq_api_exit(void)
 {
 	subsys_interface_unregister(&sq_interface);
-	kfree(sq_bitmap);
+	bitmap_free(sq_bitmap);
 	kmem_cache_destroy(sq_cache);
 }
 
-- 
2.34.1

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

[John Paul Adrian Glaubitz]

Hi Christophe!

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>
> ---
> This is a resend of [1].
> 
> Now cross-compile tested with CONFIG_CPU_SUBTYPE_SH7770=y
> 
> [1]: https://lore.kernel.org/all/521788e22ad8f7a5058c154f068b061525321841.1656142814.git.christophe.jaillet@wanadoo.fr/
> ---
>  arch/sh/kernel/cpu/sh4/sq.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/sh/kernel/cpu/sh4/sq.c b/arch/sh/kernel/cpu/sh4/sq.c
> index 27f2e3da5aa2..d289e99dc118 100644
> --- 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;
>  
> @@ -393,7 +392,7 @@ static int __init sq_api_init(void)
>  	return 0;
>  
>  out:
> -	kfree(sq_bitmap);
> +	bitmap_free(sq_bitmap);
>  	kmem_cache_destroy(sq_cache);
>  
>  	return ret;
> @@ -402,7 +401,7 @@ static int __init sq_api_init(void)
>  static void __exit sq_api_exit(void)
>  {
>  	subsys_interface_unregister(&sq_interface);
> -	kfree(sq_bitmap);
> +	bitmap_free(sq_bitmap);
>  	kmem_cache_destroy(sq_cache);
>  }
>  

Thanks for resending this patch. I will try to review it next week
and also boot-test it on my SH7785LCR evaluation board.

I am not familiar with the bitmap API at the moment, so I might take
a little longer to review this patch.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

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

[Geert Uytterhoeven]

On Sun, Apr 16, 2023 at 9:10 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> 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>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

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

[Rob Landley]

On 4/16/23 14:05, 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>

Tested-by: Rob Landley <rob@landley.net>

It booted and ran for me.

Rob

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

[John Paul Adrian Glaubitz]

Hi Christophe!

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>
> ---
> This is a resend of [1].
> 
> Now cross-compile tested with CONFIG_CPU_SUBTYPE_SH7770=y
> 
> [1]: https://lore.kernel.org/all/521788e22ad8f7a5058c154f068b061525321841.1656142814.git.christophe.jaillet@wanadoo.fr/
> ---
>  arch/sh/kernel/cpu/sh4/sq.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/sh/kernel/cpu/sh4/sq.c b/arch/sh/kernel/cpu/sh4/sq.c
> index 27f2e3da5aa2..d289e99dc118 100644
> --- 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?

> @@ -393,7 +392,7 @@ static int __init sq_api_init(void)
>  	return 0;
>  
>  out:
> -	kfree(sq_bitmap);
> +	bitmap_free(sq_bitmap);
>  	kmem_cache_destroy(sq_cache);
>  
>  	return ret;
> @@ -402,7 +401,7 @@ static int __init sq_api_init(void)
>  static void __exit sq_api_exit(void)
>  {
>  	subsys_interface_unregister(&sq_interface);
> -	kfree(sq_bitmap);
> +	bitmap_free(sq_bitmap);
>  	kmem_cache_destroy(sq_cache);
>  }
>  

Adrian

> [1] https://lkml.org/lkml/2021/11/28/155

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

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

[Geert Uytterhoeven]

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

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

[John Paul Adrian Glaubitz]

Hi Geert!

On Tue, 2023-04-18 at 09:14 +0200, Geert Uytterhoeven wrote:
> 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.

Yeah, that's what I understood from reading the code which is why I was
wondering why the factor was missing.

> 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

I agree. Do you want to send a patch I can review?

> 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).

Yeah.

PS: Sorry for the slightly messy grammar in my previous mail, didn't have
    a coffee yet that early in the morning :-).

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

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

[Dan Carpenter]

On Tue, Apr 18, 2023 at 08:36:40AM +0200, John Paul Adrian Glaubitz wrote:
> Hi Christophe!
> 
> 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>
> > ---
> > This is a resend of [1].
> > 
> > Now cross-compile tested with CONFIG_CPU_SUBTYPE_SH7770=y
> > 
> > [1]: https://lore.kernel.org/all/521788e22ad8f7a5058c154f068b061525321841.1656142814.git.christophe.jaillet@wanadoo.fr/
> > ---
> >  arch/sh/kernel/cpu/sh4/sq.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/sh/kernel/cpu/sh4/sq.c b/arch/sh/kernel/cpu/sh4/sq.c
> > index 27f2e3da5aa2..d289e99dc118 100644
> > --- 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?

Yes.  You are correct.  The original code is buggy.

size is the number of longs we need so kzalloc() is allocating a too
small buffer.  It should be kzalloc(size * sizeof(long), ... etc as you
say.

I have some unpublished Smatch stuff which tries to track "variable x
is in terms of bit units or byte units etc."  I will try to make a
static checker rule for this.

regards,
dan carpenter

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

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 451 bytes --]

On Tue, Apr 18, 2023 at 10:30:01AM +0300, Dan Carpenter wrote:
> I have some unpublished Smatch stuff which tries to track "variable x
> is in terms of bit units or byte units etc."  I will try to make a
> static checker rule for this.

Attached.  It prints a warning like this:

drivers/net/ethernet/broadcom/cnic.c:667 cnic_init_id_tbl() warn: allocating units of longs instead of bytes 'test_var'

I'll test it out tonight.

regards,
dan carpenter

[-- Attachment #2: check_allocating_longs.c --]
[-- Type: text/x-csrc, Size: 1261 bytes --]

/*
 * Copyright 2023 Linaro Ltd.
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
 */

#include "smatch.h"
#include "smatch_slist.h"

static int my_id;

static void match_allocation(struct expression *expr,
			     const char *name, struct symbol *sym,
			     struct allocation_info *info)
{
	struct smatch_state *units;
	char *str;

	units = get_units(info->total_size);
	if (!units || strcmp(units->name, "longs") != 0)
		return;

	str = expr_to_str(info->total_size);
	sm_warning("allocating units of longs instead of bytes '%s'", str);
	free_string(str);
}

void check_allocating_longs(int id)
{
	my_id = id;

	add_allocation_hook(&match_allocation);
}

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

[John Paul Adrian Glaubitz]

Hi Dan!

On Tue, 2023-04-18 at 12:39 +0300, Dan Carpenter wrote:
> On Tue, Apr 18, 2023 at 10:30:01AM +0300, Dan Carpenter wrote:
> > I have some unpublished Smatch stuff which tries to track "variable x
> > is in terms of bit units or byte units etc."  I will try to make a
> > static checker rule for this.
> 
> Attached.  It prints a warning like this:
> 
> drivers/net/ethernet/broadcom/cnic.c:667 cnic_init_id_tbl() warn: allocating units of longs instead of bytes 'test_var'
> 
> I'll test it out tonight.

Nice job, thanks for creating this very handy script!

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

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

[Christophe JAILLET]

Le 18/04/2023 à 09:14, Geert Uytterhoeven a écrit :
> 
> 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).

So, this means that this got unnoticed for 16 years?
Waouh!

I would never have thought that a "trivial" clean-up that I took time to 
repost could trigger such a thing!

Again.
Waouh!


Maybe, 0x04000000 is way to big?
Anyone knows where this value comes from?

Could there have been some memory corruption in real world application?

CJ

> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

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

[Dan Carpenter]

On Tue, Apr 18, 2023 at 12:39:43PM +0300, Dan Carpenter wrote:
> On Tue, Apr 18, 2023 at 10:30:01AM +0300, Dan Carpenter wrote:
> > I have some unpublished Smatch stuff which tries to track "variable x
> > is in terms of bit units or byte units etc."  I will try to make a
> > static checker rule for this.
> 
> Attached.  It prints a warning like this:
> 
> drivers/net/ethernet/broadcom/cnic.c:667 cnic_init_id_tbl() warn: allocating units of longs instead of bytes 'test_var'
> 
> I'll test it out tonight.

It didn't find any bugs.  (Which is hardly surprising because most would
be caught in testing).  No false postives either.  I imagine that people
will introduce bugs like this in the future and now we'll see it when
that happens.

regards,
dan carpenter

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

[John Paul Adrian Glaubitz]

Hi Christophe!

On Tue, 2023-04-18 at 20:05 +0200, Christophe JAILLET wrote:
> Le 18/04/2023 à 09:14, Geert Uytterhoeven a écrit :
> > 
> > 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).
> 
> So, this means that this got unnoticed for 16 years?
> Waouh!
> 
> I would never have thought that a "trivial" clean-up that I took time to 
> repost could trigger such a thing!

I have fixed the original bug in my for-next branch [1] now. Would you mind
rebasing your patch on top of that branch and resend it?

The reason why we're doing this is because we want to be able to backport the
fix to older kernel versions such as 4.14 which don't have the bitmap API yet.

Thanks,
Adrian

> [1] https://git.kernel.org/pub/scm/linux/kernel/git/glaubitz/sh-linux.git/log/?h=for-next

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913