diff mbox series

[RFC,1/3] modpost: fix TO_NATIVE() with expressions and consts

Message ID 20220818115306.1109642-2-alexandr.lobakin@intel.com (mailing list archive)
State New, archived
Headers show
Series kallsyms: add option to include relative filepaths into kallsyms | expand

Commit Message

Alexander Lobakin Aug. 18, 2022, 11:53 a.m. UTC 
Macro TO_NATIVE() directly takes a reference to its argument @x
without making an intermediate variable. This makes compilers
emit build warnings and errors if @x is an expression or a deref
of a const pointer (when target Endianness != host Endianness):

>> scripts/mod/modpost.h:87:18: error: lvalue required as unary '&' operand
      87 |         __endian(&(x), &(__x), sizeof(__x));                    \
         |                  ^
   scripts/mod/sympath.c:19:25: note: in expansion of macro 'TO_NATIVE'
      19 | #define t(x)            TO_NATIVE(x)
         |                         ^~~~~~~~~
   scripts/mod/sympath.c:100:31: note: in expansion of macro 't'
     100 |                 eh->e_shoff = t(h(eh->e_shoff) + off);

>> scripts/mod/modpost.h:87:24: warning: passing argument 2 of '__endian'
discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
      87 |         __endian(&(x), &(__x), sizeof(__x));                    \
         |                        ^~~~~~
   scripts/mod/sympath.c:18:25: note: in expansion of macro 'TO_NATIVE'
      18 | #define h(x)            TO_NATIVE(x)
         |                         ^~~~~~~~~
   scripts/mod/sympath.c:178:48: note: in expansion of macro 'h'
     178 |              iter < end; iter = (void *)iter + h(eh->e_shentsize)) {

Create a temporary variable, assign @x to it and don't use @x after
that. This makes it possible to pass expressions as an argument.
Also, do a cast-away for the second argument when calling __endian()
to avoid 'discarded qualifiers' warning, as typeof() preserves
qualifiers and makes compilers think that we're passing pointer
to a const.

Reported-by: kernel test robot <lkp@intel.com>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org # 4.9+
Signed-off-by: Alexander Lobakin <alexandr.lobakin@intel.com>
---
 scripts/mod/modpost.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Greg Kroah-Hartman Aug. 18, 2022, 12:26 p.m. UTC | #1 
On Thu, Aug 18, 2022 at 01:53:04PM +0200, Alexander Lobakin wrote:
> Macro TO_NATIVE() directly takes a reference to its argument @x
> without making an intermediate variable. This makes compilers
> emit build warnings and errors if @x is an expression or a deref
> of a const pointer (when target Endianness != host Endianness):
> 
> >> scripts/mod/modpost.h:87:18: error: lvalue required as unary '&' operand
>       87 |         __endian(&(x), &(__x), sizeof(__x));                    \
>          |                  ^
>    scripts/mod/sympath.c:19:25: note: in expansion of macro 'TO_NATIVE'
>       19 | #define t(x)            TO_NATIVE(x)
>          |                         ^~~~~~~~~
>    scripts/mod/sympath.c:100:31: note: in expansion of macro 't'
>      100 |                 eh->e_shoff = t(h(eh->e_shoff) + off);
> 
> >> scripts/mod/modpost.h:87:24: warning: passing argument 2 of '__endian'
> discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>       87 |         __endian(&(x), &(__x), sizeof(__x));                    \
>          |                        ^~~~~~
>    scripts/mod/sympath.c:18:25: note: in expansion of macro 'TO_NATIVE'
>       18 | #define h(x)            TO_NATIVE(x)
>          |                         ^~~~~~~~~
>    scripts/mod/sympath.c:178:48: note: in expansion of macro 'h'
>      178 |              iter < end; iter = (void *)iter + h(eh->e_shentsize)) {

How come this hasn't shown up in cross-builds today?


> 
> Create a temporary variable, assign @x to it and don't use @x after
> that. This makes it possible to pass expressions as an argument.
> Also, do a cast-away for the second argument when calling __endian()
> to avoid 'discarded qualifiers' warning, as typeof() preserves
> qualifiers and makes compilers think that we're passing pointer
> to a const.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org # 4.9+

Where are these build warnings showing up at that we don't see them
today, yet this is needed to go back to all stable trees?

still confused,

greg k-h
Alexander Lobakin Aug. 18, 2022, 2:01 p.m. UTC | #2 
From: Greg KH <gregkh@linuxfoundation.org>
Date: Thu, 18 Aug 2022 14:26:14 +0200

> On Thu, Aug 18, 2022 at 01:53:04PM +0200, Alexander Lobakin wrote:
> > Macro TO_NATIVE() directly takes a reference to its argument @x
> > without making an intermediate variable. This makes compilers
> > emit build warnings and errors if @x is an expression or a deref
> > of a const pointer (when target Endianness != host Endianness):
> > 
> > >> scripts/mod/modpost.h:87:18: error: lvalue required as unary '&' operand
> >       87 |         __endian(&(x), &(__x), sizeof(__x));                    \
> >          |                  ^
> >    scripts/mod/sympath.c:19:25: note: in expansion of macro 'TO_NATIVE'
> >       19 | #define t(x)            TO_NATIVE(x)
> >          |                         ^~~~~~~~~
> >    scripts/mod/sympath.c:100:31: note: in expansion of macro 't'
> >      100 |                 eh->e_shoff = t(h(eh->e_shoff) + off);
> > 
> > >> scripts/mod/modpost.h:87:24: warning: passing argument 2 of '__endian'
> > discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
> >       87 |         __endian(&(x), &(__x), sizeof(__x));                    \
> >          |                        ^~~~~~
> >    scripts/mod/sympath.c:18:25: note: in expansion of macro 'TO_NATIVE'
> >       18 | #define h(x)            TO_NATIVE(x)
> >          |                         ^~~~~~~~~
> >    scripts/mod/sympath.c:178:48: note: in expansion of macro 'h'
> >      178 |              iter < end; iter = (void *)iter + h(eh->e_shentsize)) {
> 
> How come this hasn't shown up in cross-builds today?

It doesn't happen with the current code.

> 
> 
> > 
> > Create a temporary variable, assign @x to it and don't use @x after
> > that. This makes it possible to pass expressions as an argument.
> > Also, do a cast-away for the second argument when calling __endian()
> > to avoid 'discarded qualifiers' warning, as typeof() preserves
> > qualifiers and makes compilers think that we're passing pointer
> > to a const.
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: stable@vger.kernel.org # 4.9+
> 
> Where are these build warnings showing up at that we don't see them
> today, yet this is needed to go back to all stable trees?

I thought all fixes should go to the applicable stable trees, am I
wrong? If so, I'll drop the tag in the next spin.

I remember we had such discussion already regarding fixing stuff in
modpost, which can happen only with never mainlained GCC LTO or with
the in-dev code. At the end that fix made it into the stables IIRC.

> 
> still confused,
> 
> greg k-h

Thanks,
Olek
Greg Kroah-Hartman Aug. 18, 2022, 2:10 p.m. UTC | #3 
On Thu, Aug 18, 2022 at 04:01:53PM +0200, Alexander Lobakin wrote:
> From: Greg KH <gregkh@linuxfoundation.org>
> Date: Thu, 18 Aug 2022 14:26:14 +0200
> 
> > On Thu, Aug 18, 2022 at 01:53:04PM +0200, Alexander Lobakin wrote:
> > > Macro TO_NATIVE() directly takes a reference to its argument @x
> > > without making an intermediate variable. This makes compilers
> > > emit build warnings and errors if @x is an expression or a deref
> > > of a const pointer (when target Endianness != host Endianness):
> > > 
> > > >> scripts/mod/modpost.h:87:18: error: lvalue required as unary '&' operand
> > >       87 |         __endian(&(x), &(__x), sizeof(__x));                    \
> > >          |                  ^
> > >    scripts/mod/sympath.c:19:25: note: in expansion of macro 'TO_NATIVE'
> > >       19 | #define t(x)            TO_NATIVE(x)
> > >          |                         ^~~~~~~~~
> > >    scripts/mod/sympath.c:100:31: note: in expansion of macro 't'
> > >      100 |                 eh->e_shoff = t(h(eh->e_shoff) + off);
> > > 
> > > >> scripts/mod/modpost.h:87:24: warning: passing argument 2 of '__endian'
> > > discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
> > >       87 |         __endian(&(x), &(__x), sizeof(__x));                    \
> > >          |                        ^~~~~~
> > >    scripts/mod/sympath.c:18:25: note: in expansion of macro 'TO_NATIVE'
> > >       18 | #define h(x)            TO_NATIVE(x)
> > >          |                         ^~~~~~~~~
> > >    scripts/mod/sympath.c:178:48: note: in expansion of macro 'h'
> > >      178 |              iter < end; iter = (void *)iter + h(eh->e_shentsize)) {
> > 
> > How come this hasn't shown up in cross-builds today?
> 
> It doesn't happen with the current code.

Great, so there is no bug that you are trying to fix :)

> > > Create a temporary variable, assign @x to it and don't use @x after
> > > that. This makes it possible to pass expressions as an argument.
> > > Also, do a cast-away for the second argument when calling __endian()
> > > to avoid 'discarded qualifiers' warning, as typeof() preserves
> > > qualifiers and makes compilers think that we're passing pointer
> > > to a const.
> > > 
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > Cc: stable@vger.kernel.org # 4.9+
> > 
> > Where are these build warnings showing up at that we don't see them
> > today, yet this is needed to go back to all stable trees?
> 
> I thought all fixes should go to the applicable stable trees, am I
> wrong? If so, I'll drop the tag in the next spin.

But this isn't fixing a bug in the code today that anyone can hit, so
why would you mark it as such?

> I remember we had such discussion already regarding fixing stuff in
> modpost, which can happen only with never mainlained GCC LTO or with
> the in-dev code. At the end that fix made it into the stables IIRC.

I don't remember taking fixes for out-of-tree LTO stuff, but I shouldn't
have :)

thanks,

greg k-h
Alexander Lobakin Aug. 18, 2022, 3:07 p.m. UTC | #4 
From: Greg KH <gregkh@linuxfoundation.org>
Date: Thu, 18 Aug 2022 16:10:21 +0200

> On Thu, Aug 18, 2022 at 04:01:53PM +0200, Alexander Lobakin wrote:
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Date: Thu, 18 Aug 2022 14:26:14 +0200
> > 
> > > On Thu, Aug 18, 2022 at 01:53:04PM +0200, Alexander Lobakin wrote:
> > > > Macro TO_NATIVE() directly takes a reference to its argument @x
> > > > without making an intermediate variable. This makes compilers
> > > > emit build warnings and errors if @x is an expression or a deref
> > > > of a const pointer (when target Endianness != host Endianness):
> > > > 
> > > > >> scripts/mod/modpost.h:87:18: error: lvalue required as unary '&' operand
> > > >       87 |         __endian(&(x), &(__x), sizeof(__x));                    \
> > > >          |                  ^
> > > >    scripts/mod/sympath.c:19:25: note: in expansion of macro 'TO_NATIVE'
> > > >       19 | #define t(x)            TO_NATIVE(x)
> > > >          |                         ^~~~~~~~~
> > > >    scripts/mod/sympath.c:100:31: note: in expansion of macro 't'
> > > >      100 |                 eh->e_shoff = t(h(eh->e_shoff) + off);
> > > > 
> > > > >> scripts/mod/modpost.h:87:24: warning: passing argument 2 of '__endian'
> > > > discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
> > > >       87 |         __endian(&(x), &(__x), sizeof(__x));                    \
> > > >          |                        ^~~~~~
> > > >    scripts/mod/sympath.c:18:25: note: in expansion of macro 'TO_NATIVE'
> > > >       18 | #define h(x)            TO_NATIVE(x)
> > > >          |                         ^~~~~~~~~
> > > >    scripts/mod/sympath.c:178:48: note: in expansion of macro 'h'
> > > >      178 |              iter < end; iter = (void *)iter + h(eh->e_shentsize)) {
> > > 
> > > How come this hasn't shown up in cross-builds today?
> > 
> > It doesn't happen with the current code.
> 
> Great, so there is no bug that you are trying to fix :)
> 
> > > > Create a temporary variable, assign @x to it and don't use @x after
> > > > that. This makes it possible to pass expressions as an argument.
> > > > Also, do a cast-away for the second argument when calling __endian()
> > > > to avoid 'discarded qualifiers' warning, as typeof() preserves
> > > > qualifiers and makes compilers think that we're passing pointer
> > > > to a const.
> > > > 
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > Cc: stable@vger.kernel.org # 4.9+
> > > 
> > > Where are these build warnings showing up at that we don't see them
> > > today, yet this is needed to go back to all stable trees?
> > 
> > I thought all fixes should go to the applicable stable trees, am I
> > wrong? If so, I'll drop the tag in the next spin.
> 
> But this isn't fixing a bug in the code today that anyone can hit, so
> why would you mark it as such?

So do you mean that a fix is a fix not when it makes some wrong code
work properly, but only when there's a certain bug report and this
fix seems to resolve it?
I.e, if there are no ways to reach some code in which 2 + 2 == 5,
there is no bug? A loaded shotgun can't be considered loaded unless
someone shots his leg?

I mean, I understand the rule "don't touch if it works", but dunno,
I don't feel it's: 1) completely justified; 2) always followed in
the current stable trees.
But I'm not a -stable maintainer :)

> 
> > I remember we had such discussion already regarding fixing stuff in
> > modpost, which can happen only with never mainlained GCC LTO or with
> > the in-dev code. At the end that fix made it into the stables IIRC.
> 
> I don't remember taking fixes for out-of-tree LTO stuff, but I shouldn't
> have :)

This: [0]

There is no way to repro it on the stable kernels, but it's here
backported :)

> 
> thanks,
> 
> greg k-h

[0] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-4.19.y&id=03bd6eaab3e1cbd4e5060b36a67000165f6e0482

Thanks,
Olek
diff mbox series

Patch

diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index 1178f40a73f3..29f440f18414 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -83,9 +83,10 @@  static inline void __endian(const void *src, void *dest, unsigned int size)
 
 #define TO_NATIVE(x)						\
 ({								\
-	typeof(x) __x;						\
-	__endian(&(x), &(__x), sizeof(__x));			\
-	__x;							\
+	typeof(x) __src = (x);					\
+	typeof(__src) __dst;					\
+	__endian(&__src, (void *)&__dst, sizeof(__src));	\
+	__dst;							\
 })
 
 #else /* endianness matches */