Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
MIME-Version: 1.0
References: <20231007170448.505487-1-masahiroy@kernel.org> <20231007170448.505487-3-masahiroy@kernel.org>
In-Reply-To: <20231007170448.505487-3-masahiroy@kernel.org>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Mon, 9 Oct 2023 10:43:59 -0700
Message-ID: <CAKwvOdkP-28Z51UZcDL4434Uns9pb0kYYFwzHmQCg7x2V0E_TQ@mail.gmail.com>
Subject: Re: [PATCH 3/5] modpost: define TO_NATIVE() using bswap_* functions
To:     Masahiro Yamada <masahiroy@kernel.org>
Cc:     linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org,
        Nathan Chancellor <nathan@kernel.org>,
        Nicolas Schier <nicolas@fjasle.eu>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Oct 7, 2023 at 10:05=E2=80=AFAM Masahiro Yamada <masahiroy@kernel.o=
rg> 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 =3D r_info;
> -
> -               r_info =3D TO_NATIVE(r_info64);
> -       } else {
> -               Elf32_Word r_info32 =3D r_info;
> -
> -               r_info =3D TO_NATIVE(r_info32);
> -       }
> +       if (is_64bit)
> +               r_info =3D TO_NATIVE((Elf64_Xword)r_info);
> +       else
> +               r_info =3D TO_NATIVE((Elf32_Word)r_info);
>
>         *r_type =3D ELF_R_TYPE(r_info);
>         *r_sym =3D 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) =3D=3D 1 || sizeof(x) =3D=3D 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) =3D=3D 4 || sizeof(x) =3D=3D 8, "bug"); =
\
> +       (typeof(x))(sizeof(x) =3D=3D 2 ? bswap_16(x) : \
> +                   sizeof(x) =3D=3D 4 ? bswap_32(x) : \
> +                   sizeof(x) =3D=3D 8 ? bswap_64(x) : \
> +                   x); \
> +})
> +
>  #if KERNEL_ELFDATA !=3D HOST_ELFDATA
>
> -static inline void __endian(const void *src, void *dest, unsigned int si=
ze)
> -{
> -       unsigned int i;
> -       for (i =3D 0; i < size; i++)
> -               ((unsigned char*)dest)[i] =3D ((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
>


--=20
Thanks,
~Nick Desaulniers