On Mon, Mar 4, 2019 at 1:44 PM kbuild test robot <[email protected]> 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 <[email protected]>
> :::::: CC: Geert Uytterhoeven <[email protected]>
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 -- [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
On Mon, 4 Mar 2019, Geert Uytterhoeven wrote:
> On Mon, Mar 4, 2019 at 1:44 PM kbuild test robot <[email protected]> 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 <[email protected]>
> > :::::: CC: Geert Uytterhoeven <[email protected]>
>
> 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
>
>
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+".
--
Hi Finn,
On Tue, Mar 5, 2019 at 3:58 AM Finn Thain <[email protected]> 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 -- [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
On Tue, 5 Mar 2019, Geert Uytterhoeven wrote:
> On Tue, Mar 5, 2019 at 3:58 AM Finn Thain <[email protected]> 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
>
>
Hi Finn,
On Tue, Mar 5, 2019 at 9:58 AM Finn Thain <[email protected]> wrote:
> On Tue, 5 Mar 2019, Geert Uytterhoeven wrote:
> > On Tue, Mar 5, 2019 at 3:58 AM Finn Thain <[email protected]> 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 -- [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
On Mär 05 2019, Finn Thain <[email protected]> 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, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
On Tue, 5 Mar 2019, Andreas Schwab wrote:
> On Mar 05 2019, Finn Thain <[email protected]> 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.
>
>
Hi Finn,
On Thu, Mar 7, 2019 at 3:59 AM Finn Thain <[email protected]> wrote:
> On Tue, 5 Mar 2019, Andreas Schwab wrote:
> > On Mar 05 2019, Finn Thain <[email protected]> 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 -- [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
On Thu, 7 Mar 2019, Geert Uytterhoeven wrote:
> On Thu, Mar 7, 2019 at 3:59 AM Finn Thain <[email protected]> wrote:
> > On Tue, 5 Mar 2019, Andreas Schwab wrote:
> > > On Mar 05 2019, Finn Thain <[email protected]> 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
>
>
Hi Finn,
On Thu, Mar 7, 2019 at 10:42 PM Finn Thain <[email protected]> wrote:
> On Thu, 7 Mar 2019, Geert Uytterhoeven wrote:
> > On Thu, Mar 7, 2019 at 3:59 AM Finn Thain <[email protected]> wrote:
> > > On Tue, 5 Mar 2019, Andreas Schwab wrote:
> > > > On Mar 05 2019, Finn Thain <[email protected]> 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 -- [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
On Mär 11 2019, Geert Uytterhoeven <[email protected]> wrote:
> Hi Finn,
>
> On Thu, Mar 7, 2019 at 10:42 PM Finn Thain <[email protected]> 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, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
Hi Andreas,
On Mon, Mar 11, 2019 at 10:56 AM Andreas Schwab <[email protected]> wrote:
> On Mär 11 2019, Geert Uytterhoeven <[email protected]> wrote:
> > On Thu, Mar 7, 2019 at 10:42 PM Finn Thain <[email protected]> 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 -- [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
On Mär 11 2019, Geert Uytterhoeven <[email protected]> wrote:
> Hi Andreas,
>
> On Mon, Mar 11, 2019 at 10:56 AM Andreas Schwab <[email protected]> wrote:
>> On Mär 11 2019, Geert Uytterhoeven <[email protected]> wrote:
>> > On Thu, Mar 7, 2019 at 10:42 PM Finn Thain <[email protected]> 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, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
Hi Andreas,
On Mon, Mar 11, 2019 at 11:13 AM Andreas Schwab <[email protected]> wrote:
> On Mär 11 2019, Geert Uytterhoeven <[email protected]> wrote:
> > On Mon, Mar 11, 2019 at 10:56 AM Andreas Schwab <[email protected]> wrote:
> >> On Mär 11 2019, Geert Uytterhoeven <[email protected]> wrote:
> >> > On Thu, Mar 7, 2019 at 10:42 PM Finn Thain <[email protected]> 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 -- [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
On Mon, 11 Mar 2019, Geert Uytterhoeven wrote:
> On Mon, Mar 11, 2019 at 11:13 AM Andreas Schwab <[email protected]> wrote:
> > On M?r 11 2019, Geert Uytterhoeven <[email protected]> wrote:
> > > On Mon, Mar 11, 2019 at 10:56 AM Andreas Schwab <[email protected]> wrote:
> > >> On M?r 11 2019, Geert Uytterhoeven <[email protected]> wrote:
> > >> > On Thu, Mar 7, 2019 at 10:42 PM Finn Thain <[email protected]> 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
>
>
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.
--