re: sh: Support for multiple nodes.

re: sh: Support for multiple nodes.

[Dan Carpenter]

Hello Paul Mundt,

The patch b241cb0c885e: "sh: Support for multiple nodes." from Jun 6,
2007, leads to the following static checker warning:

	arch/sh/mm/numa.c:47 setup_bootmem_node()
	error: buffer overflow 'node_data' 1024 <= 1024

arch/sh/mm/numa.c
    27  void __init setup_bootmem_node(int nid, unsigned long start, unsigned long end)
    28  {
    29          unsigned long bootmap_pages;
    30          unsigned long start_pfn, end_pfn;
    31          unsigned long bootmem_paddr;
    32  
    33          /* Don't allow bogus node assignment */
    34          BUG_ON(nid > MAX_NUMNODES || nid <= 0);
                       ^^^^^^^^^^^^^^^^^^

It's not very important but this check is off by one so static checkers
complain.

    35  
    36          start_pfn = start >> PAGE_SHIFT;
    37          end_pfn = end >> PAGE_SHIFT;
    38  
    39          pmb_bolt_mapping((unsigned long)__va(start), start, end - start,
    40                           PAGE_KERNEL);
    41  
    42          memblock_add(start, end - start);
    43  
    44          __add_active_range(nid, start_pfn, end_pfn);
    45  
    46          /* Node-local pgdat */
    47          NODE_DATA(nid) = __va(memblock_alloc_base(sizeof(struct pglist_data),
    48                                               SMP_CACHE_BYTES, end));
    49          memset(NODE_DATA(nid), 0, sizeof(struct pglist_data));
    50  

regards,
dan carpenter

Re: sh: Support for multiple nodes.

[Geert Uytterhoeven]

Hi Dan,

On Wed, Dec 3, 2014 at 1:39 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> The patch b241cb0c885e: "sh: Support for multiple nodes." from Jun 6,
> 2007, leads to the following static checker warning:
>
>         arch/sh/mm/numa.c:47 setup_bootmem_node()
>         error: buffer overflow 'node_data' 1024 <= 1024
>
> arch/sh/mm/numa.c
>     27  void __init setup_bootmem_node(int nid, unsigned long start, unsigned long end)
>     28  {
>     29          unsigned long bootmap_pages;
>     30          unsigned long start_pfn, end_pfn;
>     31          unsigned long bootmem_paddr;
>     32
>     33          /* Don't allow bogus node assignment */
>     34          BUG_ON(nid > MAX_NUMNODES || nid <= 0);
>                        ^^^^^^^^^^^^^^^^^^
>
> It's not very important but this check is off by one so static checkers
> complain.

Thanks for your analysis!

Please note that sh is orphaned. Andrew Morton does accept patches.

>     35
>     36          start_pfn = start >> PAGE_SHIFT;
>     37          end_pfn = end >> PAGE_SHIFT;
>     38
>     39          pmb_bolt_mapping((unsigned long)__va(start), start, end - start,
>     40                           PAGE_KERNEL);
>     41
>     42          memblock_add(start, end - start);
>     43
>     44          __add_active_range(nid, start_pfn, end_pfn);
>     45
>     46          /* Node-local pgdat */
>     47          NODE_DATA(nid) = __va(memblock_alloc_base(sizeof(struct pglist_data),
>     48                                               SMP_CACHE_BYTES, end));
>     49          memset(NODE_DATA(nid), 0, sizeof(struct pglist_data));
>     50

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

[patch] sh: off by one BUG_ON() in setup_bootmem_node()

[Dan Carpenter]

This off by one bug is harmless but it upsets the static checkers and
the code is obvious so it doesn't hurt to fix it.  The Smatch warning
is:

	arch/sh/mm/numa.c:47 setup_bootmem_node()
	error: buffer overflow 'node_data' 1024 <= 1024

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/arch/sh/mm/numa.c b/arch/sh/mm/numa.c
index 3d85225..bce52ba 100644
--- a/arch/sh/mm/numa.c
+++ b/arch/sh/mm/numa.c
@@ -31,7 +31,7 @@ void __init setup_bootmem_node(int nid, unsigned long start, unsigned long end)
 	unsigned long bootmem_paddr;
 
 	/* Don't allow bogus node assignment */
-	BUG_ON(nid > MAX_NUMNODES || nid <= 0);
+	BUG_ON(nid >= MAX_NUMNODES || nid <= 0);
 
 	start_pfn = start >> PAGE_SHIFT;
 	end_pfn = end >> PAGE_SHIFT;

Re: [patch] sh: off by one BUG_ON() in setup_bootmem_node()

[Geert Uytterhoeven]

On Thu, Dec 4, 2014 at 12:01 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> This off by one bug is harmless but it upsets the static checkers and
> the code is obvious so it doesn't hurt to fix it.  The Smatch warning
> is:
>
>         arch/sh/mm/numa.c:47 setup_bootmem_node()
>         error: buffer overflow 'node_data' 1024 <= 1024
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

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

> diff --git a/arch/sh/mm/numa.c b/arch/sh/mm/numa.c
> index 3d85225..bce52ba 100644
> --- a/arch/sh/mm/numa.c
> +++ b/arch/sh/mm/numa.c
> @@ -31,7 +31,7 @@ void __init setup_bootmem_node(int nid, unsigned long start, unsigned long end)
>         unsigned long bootmem_paddr;
>
>         /* Don't allow bogus node assignment */
> -       BUG_ON(nid > MAX_NUMNODES || nid <= 0);
> +       BUG_ON(nid >= MAX_NUMNODES || nid <= 0);
>
>         start_pfn = start >> PAGE_SHIFT;
>         end_pfn = end >> PAGE_SHIFT;

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