Re: [PATCH 3/5] modpost: define TO_NATIVE() using bswap_* functions

[Masahiro Yamada]

On Tue, Oct 10, 2023 at 2:44 AM Nick Desaulniers
<ndesaulniers@xxxxxxxxxx> wrote:
>
> On Sat, Oct 7, 2023 at 10:05 AM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote:
> >
> > The current TO_NATIVE() has some limitations:
> >
> >  1) You cannot cast the argument.
> >
> >  2) You cannot pass a variable marked as 'const'.
> >
> >  3) Passing an array is a bug, but it is not detected.
> >
> > Impelement TO_NATIVE() using bswap_*() functions. These are GNU
> > extensions. If we face portability issues, we can port the code from
> > include/uapi/linux/swab.h.
> >
> > With this change, get_rel_type_and_sym() can be simplified by casting
> > the arguments directly.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>
> > ---
> >
> >  scripts/mod/modpost.c | 13 ++++---------
> >  scripts/mod/modpost.h | 25 ++++++++++++-------------
> >  2 files changed, 16 insertions(+), 22 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 2f3b0fe6f68d..99476a9695c5 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -1410,15 +1410,10 @@ static void get_rel_type_and_sym(struct elf_info *elf, uint64_t r_info,
> >                 return;
> >         }
> >
> > -       if (is_64bit) {
> > -               Elf64_Xword r_info64 = r_info;
> > -
> > -               r_info = TO_NATIVE(r_info64);
> > -       } else {
> > -               Elf32_Word r_info32 = r_info;
> > -
> > -               r_info = TO_NATIVE(r_info32);
> > -       }
> > +       if (is_64bit)
> > +               r_info = TO_NATIVE((Elf64_Xword)r_info);
> > +       else
> > +               r_info = TO_NATIVE((Elf32_Word)r_info);
> >
> >         *r_type = ELF_R_TYPE(r_info);
> >         *r_sym = ELF_R_SYM(r_info);
> > diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> > index 6413f26fcb6b..1392afec118c 100644
> > --- a/scripts/mod/modpost.h
> > +++ b/scripts/mod/modpost.h
> > @@ -1,4 +1,5 @@
> >  /* SPDX-License-Identifier: GPL-2.0 */
> > +#include <byteswap.h>
> >  #include <stdbool.h>
> >  #include <stdio.h>
> >  #include <stdlib.h>
> > @@ -51,21 +52,19 @@
> >  #define ELF_R_TYPE  ELF64_R_TYPE
> >  #endif
> >
> > +#define bswap(x) \
> > +({ \
> > +       _Static_assert(sizeof(x) == 1 || sizeof(x) == 2 || \
>
> Seems fine, but do we need to support folks trying to swap 1B values?
> i.e. is someone calling TO_NATIVE with 1B values?



Yes.

In scripts/mod/file2alias.c,
DEF_FIELD() calls TO_NATIVE().

DEF_FIELD() is also used to get access to 1-byte fields.
So, TO_NATIVE() needs to accept sizeof(x)==1.



>  Seems silly unless
> one of these types is variable length dependent on the target machine
> type?


You can use DEF_FIELD() without knowing the
field width. This is good.





-- 
Best Regards
Masahiro Yamada