Re: [m68k:master 1174/1174] arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7]

Re: [m68k:master 1174/1174] arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7]

[Geert Uytterhoeven]

On Mon, Mar 4, 2019 at 1:44 PM kbuild test robot <lkp@intel.com> wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k.git master
> head:   e223cadc191661c67cb419b3a53c7854ecc39e8b
> commit: e223cadc191661c67cb419b3a53c7854ecc39e8b [1174/1174] Merge tag 'v5.0'
> config: m68k-allmodconfig (attached as .config)
> compiler: m68k-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         git checkout e223cadc191661c67cb419b3a53c7854ecc39e8b
>         # save the attached .config to linux build tree
>         GCC_VERSION=8.2.0 make.cross ARCH=m68k
>
> All warnings (new ones prefixed by >>):
>
>    In file included from include/linux/string.h:20,
>                     from include/linux/bitmap.h:9,
>                     from include/linux/nodemask.h:95,
>                     from include/linux/mmzone.h:17,
>                     from include/linux/gfp.h:6,
>                     from include/linux/umh.h:4,
>                     from include/linux/kmod.h:22,
>                     from include/linux/module.h:13,
>                     from drivers/nvme/target/admin-cmd.c:15:
>    In function 'memcpy_and_pad',
>        inlined from 'nvmet_execute_identify_ctrl' at drivers/nvme/target/admin-cmd.c:309:2:
> >> arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7] [-Warray-bounds]
>     #define memcpy(d, s, n) __builtin_memcpy(d, s, n)
>                             ^~~~~~~~~~~~~~~~~~~~~~~~~
>    include/linux/string.h:456:3: note: in expansion of macro 'memcpy'
>       memcpy(dest, src, dest_len);
>       ^~~~~~
>
> vim +/__builtin_memcpy +72 arch/m68k/include/asm/string.h
>
> ea61bc46 Greg Ungerer 2010-09-07  69
> ea61bc46 Greg Ungerer 2010-09-07  70  #define __HAVE_ARCH_MEMCPY
> ea61bc46 Greg Ungerer 2010-09-07  71  extern void *memcpy(void *, const void *, __kernel_size_t);
> ea61bc46 Greg Ungerer 2010-09-07 @72  #define memcpy(d, s, n) __builtin_memcpy(d, s, n)
> ea61bc46 Greg Ungerer 2010-09-07  73
>
> :::::: The code at line 72 was first introduced by commit
> :::::: ea61bc461d09e8d331a307916530aaae808c72a2 m68k/m68knommu: merge MMU and non-MMU string.h
>
> :::::: TO: Greg Ungerer <gerg@snapgear.com>
> :::::: CC: Geert Uytterhoeven <geert@linux-m68k.org>

Yep, that's a funny one, which I saw myself this morning, and decided
to dive into.
Apparently this is due 1) the recently added -ffreestanding and
2) the kernel version being shorter than 8 characters (indeed, it
didn't happen with allmodconfig on v5.0.0-rcX).

The offending code is:

        memcpy_and_pad(id->fr, sizeof(id->fr),
                       UTS_RELEASE, strlen(UTS_RELEASE), ' ');

with UTS_RELEASE being "5.0.0+", which calls into:

    static inline void memcpy_and_pad(void *dest, size_t dest_len,
                                      const void *src, size_t count, int pad)
    {
            if (dest_len > count) {

Of course this branch is taken, right?

                    memcpy(dest, src, count);
                    memset(dest + count, pad,  dest_len - count);
            } else
                    memcpy(dest, src, dest_len);
    }

This assembles to:

    .LC1:
            .string     "5.0.0+"

    | drivers/nvme/target/admin-cmd.c:311:      memcpy_and_pad(id->fr,
sizeof(id->fr),
            pea .LC1            |
            jsr strlen          |

Woops, gcc no longer optimizes this away, due to -ffreestanding.

    | drivers/nvme/target/admin-cmd.c:311:      memcpy_and_pad(id->fr,
sizeof(id->fr),
            lea (64,%a3),%a1    |, ret, _7
    | include/linux/string.h:452:       if (dest_len > count) {
            lea (16,%sp),%sp    |,
            moveq #7,%d1        |,
            cmp.l %d0,%d1       | _6,
            jcs .L53            |

And we end up with retaining both branches:

    | include/linux/string.h:453:               memcpy(dest, src, count);
            move.l %d0,-(%sp)   | _6,
            pea .LC1            |
            move.l %a1,-(%sp)   | _7,
            move.l %d0,-12(%fp) |,
            move.l %a1,-16(%fp) |,
            jsr memcpy          |
    | include/linux/string.h:454:               memset(dest + count,
pad,  dest_len - count);
            move.l -12(%fp),%d0 |,
            moveq #8,%d1        |,
            sub.l %d0,%d1       | _6,
            move.l %d1,-(%sp)   |,
            pea 32.w            |
            move.l -16(%fp),%a1 |,
            pea (%a1,%d0.l)             |
            jsr memset          |
            lea (24,%sp),%sp    |,
            jra .L54            |
    .L53:
    | include/linux/string.h:456:               memcpy(dest, src, dest_len);
            move.l .LC1,(%a1)   | MEM[(void *)"5.0.0+"], MEM[(void *)_7]
            move.l .LC1+4,4(%a1)        | MEM[(void *)"5.0.0+"], MEM[(void *)_7]

But given the warning, the compiler must have devised that taking the
second branch would read beyond the source buffer???

    .L54:


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: [m68k:master 1174/1174] arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7]

[Finn Thain]

On Mon, 4 Mar 2019, Geert Uytterhoeven wrote:

> On Mon, Mar 4, 2019 at 1:44 PM kbuild test robot <lkp@intel.com> wrote:
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k.git master
> > head:   e223cadc191661c67cb419b3a53c7854ecc39e8b
> > commit: e223cadc191661c67cb419b3a53c7854ecc39e8b [1174/1174] Merge tag 'v5.0'
> > config: m68k-allmodconfig (attached as .config)
> > compiler: m68k-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
> > reproduce:
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         git checkout e223cadc191661c67cb419b3a53c7854ecc39e8b
> >         # save the attached .config to linux build tree
> >         GCC_VERSION=8.2.0 make.cross ARCH=m68k
> >
> > All warnings (new ones prefixed by >>):
> >
> >    In file included from include/linux/string.h:20,
> >                     from include/linux/bitmap.h:9,
> >                     from include/linux/nodemask.h:95,
> >                     from include/linux/mmzone.h:17,
> >                     from include/linux/gfp.h:6,
> >                     from include/linux/umh.h:4,
> >                     from include/linux/kmod.h:22,
> >                     from include/linux/module.h:13,
> >                     from drivers/nvme/target/admin-cmd.c:15:
> >    In function 'memcpy_and_pad',
> >        inlined from 'nvmet_execute_identify_ctrl' at drivers/nvme/target/admin-cmd.c:309:2:
> > >> arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7] [-Warray-bounds]
> >     #define memcpy(d, s, n) __builtin_memcpy(d, s, n)
> >                             ^~~~~~~~~~~~~~~~~~~~~~~~~
> >    include/linux/string.h:456:3: note: in expansion of macro 'memcpy'
> >       memcpy(dest, src, dest_len);
> >       ^~~~~~
> >
> > vim +/__builtin_memcpy +72 arch/m68k/include/asm/string.h
> >
> > ea61bc46 Greg Ungerer 2010-09-07  69
> > ea61bc46 Greg Ungerer 2010-09-07  70  #define __HAVE_ARCH_MEMCPY
> > ea61bc46 Greg Ungerer 2010-09-07  71  extern void *memcpy(void *, const void *, __kernel_size_t);
> > ea61bc46 Greg Ungerer 2010-09-07 @72  #define memcpy(d, s, n) __builtin_memcpy(d, s, n)
> > ea61bc46 Greg Ungerer 2010-09-07  73
> >
> > :::::: The code at line 72 was first introduced by commit
> > :::::: ea61bc461d09e8d331a307916530aaae808c72a2 m68k/m68knommu: merge MMU and non-MMU string.h
> >
> > :::::: TO: Greg Ungerer <gerg@snapgear.com>
> > :::::: CC: Geert Uytterhoeven <geert@linux-m68k.org>
> 
> Yep, that's a funny one, which I saw myself this morning, and decided
> to dive into.
> Apparently this is due 1) the recently added -ffreestanding and
> 2) the kernel version being shorter than 8 characters (indeed, it
> didn't happen with allmodconfig on v5.0.0-rcX).
> 
> The offending code is:
> 
>         memcpy_and_pad(id->fr, sizeof(id->fr),
>                        UTS_RELEASE, strlen(UTS_RELEASE), ' ');
> 
> with UTS_RELEASE being "5.0.0+", which calls into:
> 
>     static inline void memcpy_and_pad(void *dest, size_t dest_len,
>                                       const void *src, size_t count, int pad)
>     {
>             if (dest_len > count) {
> 
> Of course this branch is taken, right?
> 

Apparently that's not known at compile time. The compiler knows dest_len 
is 8 but apparently doesn't know what count is, because -ffreestanding 
means it can't evaluate strlen(UTS_RELEASE). If you change that to 
__builtin_strlen(UTS_RELEASE), the problem goes away.

>                     memcpy(dest, src, count);
>                     memset(dest + count, pad,  dest_len - count);
>             } else
>                     memcpy(dest, src, dest_len);
>     }
> 
> This assembles to:
> 
>     .LC1:
>             .string     "5.0.0+"
> 
>     | drivers/nvme/target/admin-cmd.c:311:      memcpy_and_pad(id->fr,
> sizeof(id->fr),
>             pea .LC1            |
>             jsr strlen          |
> 
> Woops, gcc no longer optimizes this away, due to -ffreestanding.
> 
>     | drivers/nvme/target/admin-cmd.c:311:      memcpy_and_pad(id->fr,
> sizeof(id->fr),
>             lea (64,%a3),%a1    |, ret, _7
>     | include/linux/string.h:452:       if (dest_len > count) {
>             lea (16,%sp),%sp    |,
>             moveq #7,%d1        |,
>             cmp.l %d0,%d1       | _6,
>             jcs .L53            |
> 
> And we end up with retaining both branches:
> 
>     | include/linux/string.h:453:               memcpy(dest, src, count);
>             move.l %d0,-(%sp)   | _6,
>             pea .LC1            |
>             move.l %a1,-(%sp)   | _7,
>             move.l %d0,-12(%fp) |,
>             move.l %a1,-16(%fp) |,
>             jsr memcpy          |
>     | include/linux/string.h:454:               memset(dest + count,
> pad,  dest_len - count);
>             move.l -12(%fp),%d0 |,
>             moveq #8,%d1        |,
>             sub.l %d0,%d1       | _6,
>             move.l %d1,-(%sp)   |,
>             pea 32.w            |
>             move.l -16(%fp),%a1 |,
>             pea (%a1,%d0.l)             |
>             jsr memset          |
>             lea (24,%sp),%sp    |,
>             jra .L54            |
>     .L53:
>     | include/linux/string.h:456:               memcpy(dest, src, dest_len);
>             move.l .LC1,(%a1)   | MEM[(void *)"5.0.0+"], MEM[(void *)_7]
>             move.l .LC1+4,4(%a1)        | MEM[(void *)"5.0.0+"], MEM[(void *)_7]
> 
> But given the warning, the compiler must have devised that taking the
> second branch would read beyond the source buffer???
> 

Looks bogus to me.

If you change memcpy to __builtin_memcpy, then we avoid the macro and the 
warning changes to,

./include/linux/string.h:456:3: warning: '__builtin_memcpy' forming offset [7, 8] is out of the bounds [0, 6] [-Warray-bounds]
   __builtin_memcpy(dest, src, dest_len);

The compiler has nothing to complain about here. dest is known to be 
id->fr and dest_len is known to be sizeof(id->fr).

The error message indicates that gcc has applied the bounds [0, 6] to dest 
when in fact those are the bounds for src.

-- 

>     .L54:
> 
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 

Re: [m68k:master 1174/1174] arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7]

[Finn Thain]

On Tue, 5 Mar 2019, Finn Thain wrote:

> 
> Looks bogus to me.
> 
> If you change memcpy to __builtin_memcpy, then we avoid the macro and the 
> warning changes to,
> 
> ./include/linux/string.h:456:3: warning: '__builtin_memcpy' forming offset [7, 8] is out of the bounds [0, 6] [-Warray-bounds]
>    __builtin_memcpy(dest, src, dest_len);
> 
> The compiler has nothing to complain about here. dest is known to be 
> id->fr and dest_len is known to be sizeof(id->fr).
> 
> The error message indicates that gcc has applied the bounds [0, 6] to dest 
> when in fact those are the bounds for src.
> 

My mistake. GCC is right, it seems memcpy will read past the end of 
"5.0.0+".

-- 

Re: [m68k:master 1174/1174] arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7]

[Geert Uytterhoeven]

Hi Finn,

On Tue, Mar 5, 2019 at 3:58 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> On Tue, 5 Mar 2019, Finn Thain wrote:
> > Looks bogus to me.
> >
> > If you change memcpy to __builtin_memcpy, then we avoid the macro and the
> > warning changes to,
> >
> > ./include/linux/string.h:456:3: warning: '__builtin_memcpy' forming offset [7, 8] is out of the bounds [0, 6] [-Warray-bounds]
> >    __builtin_memcpy(dest, src, dest_len);
> >
> > The compiler has nothing to complain about here. dest is known to be
> > id->fr and dest_len is known to be sizeof(id->fr).
> >
> > The error message indicates that gcc has applied the bounds [0, 6] to dest
> > when in fact those are the bounds for src.
> >
>
> My mistake. GCC is right, it seems memcpy will read past the end of
> "5.0.0+".

But only if the else branch is taken, which is not the case.

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: [m68k:master 1174/1174] arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7]

[Finn Thain]

On Tue, 5 Mar 2019, Geert Uytterhoeven wrote:

> On Tue, Mar 5, 2019 at 3:58 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> > On Tue, 5 Mar 2019, Finn Thain wrote:
> > > Looks bogus to me.
> > >
> > > If you change memcpy to __builtin_memcpy, then we avoid the macro and the
> > > warning changes to,
> > >
> > > ./include/linux/string.h:456:3: warning: '__builtin_memcpy' forming offset [7, 8] is out of the bounds [0, 6] [-Warray-bounds]
> > >    __builtin_memcpy(dest, src, dest_len);
> > >
> > > The compiler has nothing to complain about here. dest is known to be
> > > id->fr and dest_len is known to be sizeof(id->fr).
> > >
> > > The error message indicates that gcc has applied the bounds [0, 6] to dest
> > > when in fact those are the bounds for src.
> > >
> >
> > My mistake. GCC is right, it seems memcpy will read past the end of
> > "5.0.0+".
> 
> But only if the else branch is taken, which is not the case.
> 

You and I know that, because we can see what values get passed to 
memcpy_and_pad(). But how is gcc to know that?

If we replace strlen with __builtin_strlen, this problem goes away. It's 
interesting that the kernel's strlen implementation in 
include/linux/string.h can't achieve this.

-- 

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

Re: [m68k:master 1174/1174] arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7]

[Geert Uytterhoeven]

Hi Finn,

On Tue, Mar 5, 2019 at 9:58 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> On Tue, 5 Mar 2019, Geert Uytterhoeven wrote:
> > On Tue, Mar 5, 2019 at 3:58 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> > > On Tue, 5 Mar 2019, Finn Thain wrote:
> > > > Looks bogus to me.
> > > >
> > > > If you change memcpy to __builtin_memcpy, then we avoid the macro and the
> > > > warning changes to,
> > > >
> > > > ./include/linux/string.h:456:3: warning: '__builtin_memcpy' forming offset [7, 8] is out of the bounds [0, 6] [-Warray-bounds]
> > > >    __builtin_memcpy(dest, src, dest_len);
> > > >
> > > > The compiler has nothing to complain about here. dest is known to be
> > > > id->fr and dest_len is known to be sizeof(id->fr).
> > > >
> > > > The error message indicates that gcc has applied the bounds [0, 6] to dest
> > > > when in fact those are the bounds for src.
> > > >
> > >
> > > My mistake. GCC is right, it seems memcpy will read past the end of
> > > "5.0.0+".
> >
> > But only if the else branch is taken, which is not the case.
> >
>
> You and I know that, because we can see what values get passed to
> memcpy_and_pad(). But how is gcc to know that?

Gcc also sees (partly) what values get passed, else it would not give that
warning.

Still, should gcc give warnings based on branches that may or may not be
taken? I guess there are lots of cases in the kernel where this could lead
to false positives.

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: [m68k:master 1174/1174] arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7]

[Andreas Schwab]

On Mär 05 2019, Finn Thain <fthain@telegraphics.com.au> wrote:

> interesting that the kernel's strlen implementation in 
> include/linux/string.h can't achieve this.

This implementation is only available if ARCH_HAS_FORTIFY_SOURCE.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [m68k:master 1174/1174] arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7]

[Finn Thain]

On Tue, 5 Mar 2019, Andreas Schwab wrote:

> On Mar 05 2019, Finn Thain <fthain@telegraphics.com.au> wrote:
> 
> > interesting that the kernel's strlen implementation in 
> > include/linux/string.h can't achieve this.
> 
> This implementation is only available if ARCH_HAS_FORTIFY_SOURCE.
> 

I see. Perhaps we could add another definition to that file:

#if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE)
...
#else
__FORTIFY_INLINE __kernel_size_t strlen(const char *p)
{
	return __builtin_strlen(p);
}
#endif

I didn't test that. But the following patch seems to work...

diff --git a/arch/m68k/include/asm/string.h b/arch/m68k/include/asm/string.h
index f759d944c449..3cff6b128ed3 100644
--- a/arch/m68k/include/asm/string.h
+++ b/arch/m68k/include/asm/string.h
@@ -71,4 +71,6 @@ extern void *memset(void *, int, __kernel_size_t);
 extern void *memcpy(void *, const void *, __kernel_size_t);
 #define memcpy(d, s, n) __builtin_memcpy(d, s, n)
 
+#define strlen(s) __builtin_strlen(s)
+
 #endif /* _M68K_STRING_H_ */
diff --git a/lib/string.c b/lib/string.c
index 38e4ca08e757..fe970f2160e5 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -472,6 +472,7 @@ char *strim(char *s)
 EXPORT_SYMBOL(strim);
 
 #ifndef __HAVE_ARCH_STRLEN
+#undef strlen
 /**
  * strlen - Find the length of a string
  * @s: The string to be sized

-- 

> Andreas.
> 
> 

Re: [m68k:master 1174/1174] arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7]

[Geert Uytterhoeven]

Hi Finn,

On Thu, Mar 7, 2019 at 3:59 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> On Tue, 5 Mar 2019, Andreas Schwab wrote:
> > On Mar 05 2019, Finn Thain <fthain@telegraphics.com.au> wrote:
> >
> > > interesting that the kernel's strlen implementation in
> > > include/linux/string.h can't achieve this.
> >
> > This implementation is only available if ARCH_HAS_FORTIFY_SOURCE.
> >
>
> I see. Perhaps we could add another definition to that file:
>
> #if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE)
> ...
> #else
> __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
> {
>         return __builtin_strlen(p);
> }
> #endif
>
> I didn't test that. But the following patch seems to work...
>
> diff --git a/arch/m68k/include/asm/string.h b/arch/m68k/include/asm/string.h
> index f759d944c449..3cff6b128ed3 100644
> --- a/arch/m68k/include/asm/string.h
> +++ b/arch/m68k/include/asm/string.h
> @@ -71,4 +71,6 @@ extern void *memset(void *, int, __kernel_size_t);
>  extern void *memcpy(void *, const void *, __kernel_size_t);
>  #define memcpy(d, s, n) __builtin_memcpy(d, s, n)
>
> +#define strlen(s) __builtin_strlen(s)

Shouldn't you add

    #define __HAVE_ARCH_STRLEN

here...

> +
>  #endif /* _M68K_STRING_H_ */
> diff --git a/lib/string.c b/lib/string.c
> index 38e4ca08e757..fe970f2160e5 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -472,6 +472,7 @@ char *strim(char *s)
>  EXPORT_SYMBOL(strim);
>
>  #ifndef __HAVE_ARCH_STRLEN
> +#undef strlen

... so you can drop this change?

>  /**
>   * strlen - Find the length of a string
>   * @s: The string to be sized

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: [m68k:master 1174/1174] arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7]

[Finn Thain]

On Thu, 7 Mar 2019, Geert Uytterhoeven wrote:

> On Thu, Mar 7, 2019 at 3:59 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> > On Tue, 5 Mar 2019, Andreas Schwab wrote:
> > > On Mar 05 2019, Finn Thain <fthain@telegraphics.com.au> wrote:
> > >
> > > > interesting that the kernel's strlen implementation in
> > > > include/linux/string.h can't achieve this.
> > >
> > > This implementation is only available if ARCH_HAS_FORTIFY_SOURCE.
> > >
> >
> > I see. Perhaps we could add another definition to that file:
> >
> > #if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE)
> > ...
> > #else
> > __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
> > {
> >         return __builtin_strlen(p);
> > }
> > #endif
> >
> > I didn't test that.

I've tested it now, it works too. This may be a better solution than 
defining a strlen macro.

diff --git a/include/linux/string.h b/include/linux/string.h
index 7927b875f80c..ec9c0a206bd3 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -436,6 +436,13 @@ __FORTIFY_INLINE char *strcpy(char *p, const char *q)
 	return p;
 }
 
+#else
+
+__FORTIFY_INLINE __kernel_size_t strlen(const char *p)
+{
+	return __builtin_strlen(p);
+}
+
 #endif
 
 /**

> But the following patch seems to work...
> >
> > diff --git a/arch/m68k/include/asm/string.h b/arch/m68k/include/asm/string.h
> > index f759d944c449..3cff6b128ed3 100644
> > --- a/arch/m68k/include/asm/string.h
> > +++ b/arch/m68k/include/asm/string.h
> > @@ -71,4 +71,6 @@ extern void *memset(void *, int, __kernel_size_t);
> >  extern void *memcpy(void *, const void *, __kernel_size_t);
> >  #define memcpy(d, s, n) __builtin_memcpy(d, s, n)
> >
> > +#define strlen(s) __builtin_strlen(s)
> 
> Shouldn't you add
> 
>     #define __HAVE_ARCH_STRLEN
> 
> here...
> 

No, the link fails because the compiler still emits some references to 
strlen().

-- 

> > +
> >  #endif /* _M68K_STRING_H_ */
> > diff --git a/lib/string.c b/lib/string.c
> > index 38e4ca08e757..fe970f2160e5 100644
> > --- a/lib/string.c
> > +++ b/lib/string.c
> > @@ -472,6 +472,7 @@ char *strim(char *s)
> >  EXPORT_SYMBOL(strim);
> >
> >  #ifndef __HAVE_ARCH_STRLEN
> > +#undef strlen
> 
> ... so you can drop this change?
> 
> >  /**
> >   * strlen - Find the length of a string
> >   * @s: The string to be sized
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 

Re: [m68k:master 1174/1174] arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7]

[Geert Uytterhoeven]

Hi Finn,

On Thu, Mar 7, 2019 at 10:42 PM Finn Thain <fthain@telegraphics.com.au> wrote:
> On Thu, 7 Mar 2019, Geert Uytterhoeven wrote:
> > On Thu, Mar 7, 2019 at 3:59 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> > > On Tue, 5 Mar 2019, Andreas Schwab wrote:
> > > > On Mar 05 2019, Finn Thain <fthain@telegraphics.com.au> wrote:
> > > >
> > > > > interesting that the kernel's strlen implementation in
> > > > > include/linux/string.h can't achieve this.
> > > >
> > > > This implementation is only available if ARCH_HAS_FORTIFY_SOURCE.
> > > >
> > >
> > > I see. Perhaps we could add another definition to that file:
> > >
> > > #if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE)
> > > ...
> > > #else
> > > __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
> > > {
> > >         return __builtin_strlen(p);
> > > }
> > > #endif
> > >
> > > I didn't test that.
>
> I've tested it now, it works too. This may be a better solution than
> defining a strlen macro.
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 7927b875f80c..ec9c0a206bd3 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -436,6 +436,13 @@ __FORTIFY_INLINE char *strcpy(char *p, const char *q)
>         return p;
>  }
>
> +#else
> +
> +__FORTIFY_INLINE __kernel_size_t strlen(const char *p)
> +{
> +       return __builtin_strlen(p);
> +}
> +
>  #endif
>
>  /**
>
> > But the following patch seems to work...
> > >
> > > diff --git a/arch/m68k/include/asm/string.h b/arch/m68k/include/asm/string.h
> > > index f759d944c449..3cff6b128ed3 100644
> > > --- a/arch/m68k/include/asm/string.h
> > > +++ b/arch/m68k/include/asm/string.h
> > > @@ -71,4 +71,6 @@ extern void *memset(void *, int, __kernel_size_t);
> > >  extern void *memcpy(void *, const void *, __kernel_size_t);
> > >  #define memcpy(d, s, n) __builtin_memcpy(d, s, n)
> > >
> > > +#define strlen(s) __builtin_strlen(s)
> >
> > Shouldn't you add
> >
> >     #define __HAVE_ARCH_STRLEN
> >
> > here...
> >
>
> No, the link fails because the compiler still emits some references to
> strlen().

Despite -ffreestanding?!?

> > > --- a/lib/string.c
> > > +++ b/lib/string.c
> > > @@ -472,6 +472,7 @@ char *strim(char *s)
> > >  EXPORT_SYMBOL(strim);
> > >
> > >  #ifndef __HAVE_ARCH_STRLEN
> > > +#undef strlen
> >
> > ... so you can drop this change?

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: [m68k:master 1174/1174] arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7]

[Andreas Schwab]

On Mär 11 2019, Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Finn,
>
> On Thu, Mar 7, 2019 at 10:42 PM Finn Thain <fthain@telegraphics.com.au> wrote:
>> No, the link fails because the compiler still emits some references to
>> strlen().
>
> Despite -ffreestanding?!?

*Because* of -ffreestanding.  Without that, strlen would be recognized
and turned into __builtin_strlen.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [m68k:master 1174/1174] arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7]

[Geert Uytterhoeven]

Hi Andreas,

On Mon, Mar 11, 2019 at 10:56 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
> On Mär 11 2019, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, Mar 7, 2019 at 10:42 PM Finn Thain <fthain@telegraphics.com.au> wrote:
> >> No, the link fails because the compiler still emits some references to
> >> strlen().
> >
> > Despite -ffreestanding?!?
>
> *Because* of -ffreestanding.  Without that, strlen would be recognized
> and turned into __builtin_strlen.

Now I'm confused: if we have a static inline or #define for strlen(), why
would the compiler still emit references to the strlen() symbol?

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: [m68k:master 1174/1174] arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7]

[Andreas Schwab]

On Mär 11 2019, Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Andreas,
>
> On Mon, Mar 11, 2019 at 10:56 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
>> On Mär 11 2019, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> > On Thu, Mar 7, 2019 at 10:42 PM Finn Thain <fthain@telegraphics.com.au> wrote:
>> >> No, the link fails because the compiler still emits some references to
>> >> strlen().
>> >
>> > Despite -ffreestanding?!?
>>
>> *Because* of -ffreestanding.  Without that, strlen would be recognized
>> and turned into __builtin_strlen.
>
> Now I'm confused: if we have a static inline or #define for strlen(),

Do you?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [m68k:master 1174/1174] arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7]

[Geert Uytterhoeven]

Hi Andreas,

On Mon, Mar 11, 2019 at 11:13 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
> On Mär 11 2019, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Mon, Mar 11, 2019 at 10:56 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
> >> On Mär 11 2019, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >> > On Thu, Mar 7, 2019 at 10:42 PM Finn Thain <fthain@telegraphics.com.au> wrote:
> >> >> No, the link fails because the compiler still emits some references to
> >> >> strlen().
> >> >
> >> > Despite -ffreestanding?!?
> >>
> >> *Because* of -ffreestanding.  Without that, strlen would be recognized
> >> and turned into __builtin_strlen.
> >
> > Now I'm confused: if we have a static inline or #define for strlen(),
>
> Do you?

I don't, but Finn's patch has, IINM.

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: [m68k:master 1174/1174] arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7]

[Finn Thain]

On Mon, 11 Mar 2019, Geert Uytterhoeven wrote:

> On Mon, Mar 11, 2019 at 11:13 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
> > On M?r 11 2019, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Mon, Mar 11, 2019 at 10:56 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
> > >> On M?r 11 2019, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > >> > On Thu, Mar 7, 2019 at 10:42 PM Finn Thain <fthain@telegraphics.com.au> wrote:
> > >> >> No, the link fails because the compiler still emits some references to
> > >> >> strlen().
> > >> >
> > >> > Despite -ffreestanding?!?
> > >>
> > >> *Because* of -ffreestanding.  Without that, strlen would be recognized
> > >> and turned into __builtin_strlen.
> > >
> > > Now I'm confused: if we have a static inline or #define for strlen(),
> >
> > Do you?
> 
> I don't, but Finn's patch has, IINM.
> 

You're mixing up two separate patches there. One uses a #define and the 
other uses a forced inline function. We were discussing the former patch 
when I answered your question about __HAVE_ARCH_STRLEN (which got 
snipped).

m68k doesn't define __HAVE_ARCH_STRLEN and relies on the strlen() 
implementation in lib/string.c. The former patch doesn't alter this but 
reduces the number of callers because some call sites get optimized away. 
That's how it avoids the warning you raised.

Anyway, I don't like pre-processor kludges. So I did another experiment 
with the latter (forced inline) approach, to see if some optimizations can 
still be used with -ffreestanding.

diff --git a/include/linux/string.h b/include/linux/string.h
index 7927b875f80c..25b5bf689018 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -436,6 +436,58 @@ __FORTIFY_INLINE char *strcpy(char *p, const char *q)
 	return p;
 }
 
+#else
+
+//__FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size)
+//{
+//	return __builtin_strncpy(p, q, size);
+//}
+
+__FORTIFY_INLINE char *strcat(char *p, const char *q)
+{
+	return __builtin_strcat(p, q);
+}
+
+__FORTIFY_INLINE __kernel_size_t strlen(const char *p)
+{
+	return __builtin_strlen(p);
+}
+
+__FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t count)
+{
+	return __builtin_strncat(p, q, count);
+}
+
+__FORTIFY_INLINE void *memset(void *p, int c, __kernel_size_t size)
+{
+	return __builtin_memset(p, c, size);
+}
+
+//__FORTIFY_INLINE void *memcpy(void *p, const void *q, __kernel_size_t size)
+//{
+//	return __builtin_memcpy(p, q, size);
+//}
+
+__FORTIFY_INLINE void *memmove(void *p, const void *q, __kernel_size_t size)
+{
+	return __builtin_memmove(p, q, size);
+}
+
+__FORTIFY_INLINE int memcmp(const void *p, const void *q, __kernel_size_t size)
+{
+	return __builtin_memcmp(p, q, size);
+}
+
+__FORTIFY_INLINE void *memchr(const void *p, int c, __kernel_size_t size)
+{
+	return __builtin_memchr(p, c, size);
+}
+
+__FORTIFY_INLINE char *strcpy(char *p, const char *q)
+{
+	return __builtin_strcpy(p, q);
+}
+
 #endif
 
 /**


The result of this patch really is confusing. It still suppresses the 
warning you raised:

arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming
offset 8 is out of the bounds [0, 7] [-Warray-bounds]
    #define memcpy(d, s, n) __builtin_memcpy(d, s, n)
                            ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:456:3: note: in expansion of macro 'memcpy'
      memcpy(dest, src, dest_len);
      ^~~~~~

But it also causes new ones, because of __builtin_memset():

drivers/video/fbdev/core/fbcvt.c: In function 'fb_find_mode_cvt':
drivers/video/fbdev/core/fbcvt.c:312:16: warning: 'cvt.flags' may be used uninit                                                                     ialized in this function [-Wmaybe-uninitialized]
      cvt.flags |= FB_CVT_FLAG_MARGINS;
                ^~

Apparently the compiler doesn't understand that __builtin_memset() has 
the effect of initialization. Weird.

-- 

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

Re: [m68k:master 1174/1174] arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7]

[Finn Thain]

On Tue, 12 Mar 2019, I wrote:

> ... I did another experiment with the latter (forced inline) approach, 
> to see if some optimizations can still be used with -ffreestanding.
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 7927b875f80c..25b5bf689018 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -436,6 +436,58 @@ __FORTIFY_INLINE char *strcpy(char *p, const char *q)
>  	return p;
>  }
>  
> +#else
> +
> +//__FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size)
> +//{
> +//	return __builtin_strncpy(p, q, size);
> +//}
> +
> +__FORTIFY_INLINE char *strcat(char *p, const char *q)
> +{
> +	return __builtin_strcat(p, q);
> +}
> +
> +__FORTIFY_INLINE __kernel_size_t strlen(const char *p)
> +{
> +	return __builtin_strlen(p);
> +}
> +
> +__FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t count)
> +{
> +	return __builtin_strncat(p, q, count);
> +}
> +
> +__FORTIFY_INLINE void *memset(void *p, int c, __kernel_size_t size)
> +{
> +	return __builtin_memset(p, c, size);
> +}
> +
> +//__FORTIFY_INLINE void *memcpy(void *p, const void *q, __kernel_size_t size)
> +//{
> +//	return __builtin_memcpy(p, q, size);
> +//}
> +
> +__FORTIFY_INLINE void *memmove(void *p, const void *q, __kernel_size_t size)
> +{
> +	return __builtin_memmove(p, q, size);
> +}
> +
> +__FORTIFY_INLINE int memcmp(const void *p, const void *q, __kernel_size_t size)
> +{
> +	return __builtin_memcmp(p, q, size);
> +}
> +
> +__FORTIFY_INLINE void *memchr(const void *p, int c, __kernel_size_t size)
> +{
> +	return __builtin_memchr(p, c, size);
> +}
> +
> +__FORTIFY_INLINE char *strcpy(char *p, const char *q)
> +{
> +	return __builtin_strcpy(p, q);
> +}
> +
>  #endif
>  
>  /**
> 
> 
> The result of this patch really is confusing. It still suppresses the 
> warning you raised:
> 
> arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming
> offset 8 is out of the bounds [0, 7] [-Warray-bounds]
>     #define memcpy(d, s, n) __builtin_memcpy(d, s, n)
>                             ^~~~~~~~~~~~~~~~~~~~~~~~~
>    include/linux/string.h:456:3: note: in expansion of macro 'memcpy'
>       memcpy(dest, src, dest_len);
>       ^~~~~~
> 
> But it also causes new ones, because of __builtin_memset():
> 
> drivers/video/fbdev/core/fbcvt.c: In function 'fb_find_mode_cvt':
> drivers/video/fbdev/core/fbcvt.c:312:16: warning: 'cvt.flags' may be used uninit                                                                     ialized in this function [-Wmaybe-uninitialized]
>       cvt.flags |= FB_CVT_FLAG_MARGINS;
>                 ^~
> 
> Apparently the compiler doesn't understand that __builtin_memset() has 
> the effect of initialization. Weird.
> 

The other weird thing is that this warning doesn't show up when 
CONFIG_FORTIFY_SOURCE=y, even though the technique is much the same, that 
is, __builtin_memset() gets wrapped in a static inline memset() function.

Anyway, a quick and dirty microbenchmark under qemu-m68k shows that this 
patch reduces system time for 'time find / -false' by approx. 10%.

Interestingly, the -ffreestanding option doesn't make much difference to 
this particular microbenchmark.

--