| Message ID | 20231007170448.505487-3-masahiroy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] modpost: fix tee MODULE_DEVICE_TABLE built on big endian host | expand |
On Sat, Oct 7, 2023 at 10:05 AM Masahiro Yamada <masahiroy@kernel.org> 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@kernel.org> > --- > > 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? Seems silly unless one of these types is variable length dependent on the target machine type? Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > + sizeof(x) == 4 || sizeof(x) == 8, "bug"); \ > + (typeof(x))(sizeof(x) == 2 ? bswap_16(x) : \ > + sizeof(x) == 4 ? bswap_32(x) : \ > + sizeof(x) == 8 ? bswap_64(x) : \ > + x); \ > +}) > + > #if KERNEL_ELFDATA != HOST_ELFDATA > > -static inline void __endian(const void *src, void *dest, unsigned int size) > -{ > - unsigned int i; > - for (i = 0; i < size; i++) > - ((unsigned char*)dest)[i] = ((unsigned char*)src)[size - i-1]; > -} > - > -#define TO_NATIVE(x) \ > -({ \ > - typeof(x) __x; \ > - __endian(&(x), &(__x), sizeof(__x)); \ > - __x; \ > -}) > +#define TO_NATIVE(x) (bswap(x)) > > #else /* endianness matches */ > > -- > 2.39.2 >
On Tue, Oct 10, 2023 at 2:44 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Sat, Oct 7, 2023 at 10:05 AM Masahiro Yamada <masahiroy@kernel.org> 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@kernel.org> > > --- > > > > 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.
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 || \ + sizeof(x) == 4 || sizeof(x) == 8, "bug"); \ + (typeof(x))(sizeof(x) == 2 ? bswap_16(x) : \ + sizeof(x) == 4 ? bswap_32(x) : \ + sizeof(x) == 8 ? bswap_64(x) : \ + x); \ +}) + #if KERNEL_ELFDATA != HOST_ELFDATA -static inline void __endian(const void *src, void *dest, unsigned int size) -{ - unsigned int i; - for (i = 0; i < size; i++) - ((unsigned char*)dest)[i] = ((unsigned char*)src)[size - i-1]; -} - -#define TO_NATIVE(x) \ -({ \ - typeof(x) __x; \ - __endian(&(x), &(__x), sizeof(__x)); \ - __x; \ -}) +#define TO_NATIVE(x) (bswap(x)) #else /* endianness matches */
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@kernel.org> --- scripts/mod/modpost.c | 13 ++++--------- scripts/mod/modpost.h | 25 ++++++++++++------------- 2 files changed, 16 insertions(+), 22 deletions(-)