[net-next,v2,4/4] kernel/module: Hide vermagic header file from general use
diff mbox series

Message ID 20200419155506.129392-1-leon@kernel.org
State New
Headers show
Series
  • Untitled series #273637
Related show

Commit Message

Leon Romanovsky April 19, 2020, 3:55 p.m. UTC
From: Leon Romanovsky <leonro@mellanox.com>

VERMAGIC* definitions are not supposed to be used by the drivers,
see this [1] bug report, so introduce special define to guard inclusion
of this header file and define it in kernel/modules.h and in internal
script that generates *.mod.c files.

In-tree module build:
➜  kernel git:(vermagic) ✗ make clean
➜  kernel git:(vermagic) ✗ make M=drivers/infiniband/hw/mlx5
➜  kernel git:(vermagic) ✗ modinfo drivers/infiniband/hw/mlx5/mlx5_ib.ko
filename:	/images/leonro/src/kernel/drivers/infiniband/hw/mlx5/mlx5_ib.ko
<...>
vermagic:       5.6.0+ SMP mod_unload modversions

Out-of-tree module build:
➜  mlx5 make -C /images/leonro/src/kernel clean M=/tmp/mlx5
➜  mlx5 make -C /images/leonro/src/kernel M=/tmp/mlx5
➜  mlx5 modinfo /tmp/mlx5/mlx5_ib.ko
filename:       /tmp/mlx5/mlx5_ib.ko
<...>
vermagic:       5.6.0+ SMP mod_unload modversions

[1] https://lore.kernel.org/lkml/20200411155623.GA22175@zn.tnic
Reported-by: Borislav Petkov <bp@suse.de>
Acked-by: Borislav Petkov <bp@suse.de>
Acked-by: Jessica Yu <jeyu@kernel.org>
Co-developed-by: Masahiro Yamada <masahiroy@kernel.org>
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 include/linux/vermagic.h | 5 +++++
 kernel/module.c          | 3 +++
 scripts/mod/modpost.c    | 1 +
 3 files changed, 9 insertions(+)

--
2.25.2

Comments

Masahiro Yamada April 19, 2020, 10:59 p.m. UTC | #1
Hi,

On Mon, Apr 20, 2020 at 12:55 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> From: Leon Romanovsky <leonro@mellanox.com>
>
> VERMAGIC* definitions are not supposed to be used by the drivers,
> see this [1] bug report, so introduce special define to guard inclusion
> of this header file and define it in kernel/modules.h and in internal
> script that generates *.mod.c files.
>
> In-tree module build:
> ➜  kernel git:(vermagic) ✗ make clean
> ➜  kernel git:(vermagic) ✗ make M=drivers/infiniband/hw/mlx5
> ➜  kernel git:(vermagic) ✗ modinfo drivers/infiniband/hw/mlx5/mlx5_ib.ko
> filename:       /images/leonro/src/kernel/drivers/infiniband/hw/mlx5/mlx5_ib.ko
> <...>
> vermagic:       5.6.0+ SMP mod_unload modversions
>
> Out-of-tree module build:
> ➜  mlx5 make -C /images/leonro/src/kernel clean M=/tmp/mlx5
> ➜  mlx5 make -C /images/leonro/src/kernel M=/tmp/mlx5
> ➜  mlx5 modinfo /tmp/mlx5/mlx5_ib.ko
> filename:       /tmp/mlx5/mlx5_ib.ko
> <...>
> vermagic:       5.6.0+ SMP mod_unload modversions
>
> [1] https://lore.kernel.org/lkml/20200411155623.GA22175@zn.tnic
> Reported-by: Borislav Petkov <bp@suse.de>
> Acked-by: Borislav Petkov <bp@suse.de>
> Acked-by: Jessica Yu <jeyu@kernel.org>
> Co-developed-by: Masahiro Yamada <masahiroy@kernel.org>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---


I did not read the full thread of [1],
and perhaps may miss something.

But, this series is trying to solve a different problem
"driver code should not include <linux/vermagic.h>"
isn't it?


IIUC, Borislav reported conflict of MODULE_ARCH_VERMAGIC
if <linux/vermagic.h> is included before <linux/module.h>.

With your cleanups, the include site of <linux/vermagic.h>
will be limited to kernel/module.c and scripts/mod/module.c

Assuming those two files include them in the *correct* order,
this problem will be suppressed.

But, I do not think it addresses the problem properly.


If
  #include <foo.h>
  #include <bar.h>

works, but

  #include <bar.h>
  #include <foo.h>

does not, the root cause is very likely
that <bar.h> is not self-contained.
The problem is solved by including <foo.h> from <bar.h>


Please see my thoughts in this:
https://lore.kernel.org/patchwork/patch/1227024/


Of course, we are solving different issues, so I think
we can merge both.


What do you think?




>  include/linux/vermagic.h | 5 +++++
>  kernel/module.c          | 3 +++
>  scripts/mod/modpost.c    | 1 +
>  3 files changed, 9 insertions(+)
>
> diff --git a/include/linux/vermagic.h b/include/linux/vermagic.h
> index 9aced11e9000..7768d20ada39 100644
> --- a/include/linux/vermagic.h
> +++ b/include/linux/vermagic.h
> @@ -1,4 +1,9 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef INCLUDE_VERMAGIC
> +#error "This header can be included from kernel/module.c or *.mod.c only"
> +#endif
> +
>  #include <generated/utsrelease.h>
>
>  /* Simply sanity version stamp for modules. */
> diff --git a/kernel/module.c b/kernel/module.c
> index 646f1e2330d2..8833e848b73c 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -4,6 +4,9 @@
>     Copyright (C) 2001 Rusty Russell, 2002, 2010 Rusty Russell IBM.
>
>  */
> +
> +#define INCLUDE_VERMAGIC
> +
>  #include <linux/export.h>
>  #include <linux/extable.h>
>  #include <linux/moduleloader.h>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 5c3c50c5ec52..7f7d4ee7b652 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -2251,6 +2251,7 @@ static void add_header(struct buffer *b, struct module *mod)
>          * Include build-salt.h after module.h in order to
>          * inherit the definitions.
>          */
> +       buf_printf(b, "#define INCLUDE_VERMAGIC\n");
>         buf_printf(b, "#include <linux/build-salt.h>\n");
>         buf_printf(b, "#include <linux/vermagic.h>\n");
>         buf_printf(b, "#include <linux/compiler.h>\n");
> --
> 2.25.2
>


--
Best Regards
Masahiro Yamada
Leon Romanovsky April 20, 2020, 8:05 a.m. UTC | #2
On Mon, Apr 20, 2020 at 07:59:35AM +0900, Masahiro Yamada wrote:
> Hi,
>
> On Mon, Apr 20, 2020 at 12:55 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > From: Leon Romanovsky <leonro@mellanox.com>
> >
> > VERMAGIC* definitions are not supposed to be used by the drivers,
> > see this [1] bug report, so introduce special define to guard inclusion
> > of this header file and define it in kernel/modules.h and in internal
> > script that generates *.mod.c files.
> >
> > In-tree module build:
> > ➜  kernel git:(vermagic) ✗ make clean
> > ➜  kernel git:(vermagic) ✗ make M=drivers/infiniband/hw/mlx5
> > ➜  kernel git:(vermagic) ✗ modinfo drivers/infiniband/hw/mlx5/mlx5_ib.ko
> > filename:       /images/leonro/src/kernel/drivers/infiniband/hw/mlx5/mlx5_ib.ko
> > <...>
> > vermagic:       5.6.0+ SMP mod_unload modversions
> >
> > Out-of-tree module build:
> > ➜  mlx5 make -C /images/leonro/src/kernel clean M=/tmp/mlx5
> > ➜  mlx5 make -C /images/leonro/src/kernel M=/tmp/mlx5
> > ➜  mlx5 modinfo /tmp/mlx5/mlx5_ib.ko
> > filename:       /tmp/mlx5/mlx5_ib.ko
> > <...>
> > vermagic:       5.6.0+ SMP mod_unload modversions
> >
> > [1] https://lore.kernel.org/lkml/20200411155623.GA22175@zn.tnic
> > Reported-by: Borislav Petkov <bp@suse.de>
> > Acked-by: Borislav Petkov <bp@suse.de>
> > Acked-by: Jessica Yu <jeyu@kernel.org>
> > Co-developed-by: Masahiro Yamada <masahiroy@kernel.org>
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > ---
>
>
> I did not read the full thread of [1],
> and perhaps may miss something.
>
> But, this series is trying to solve a different problem
> "driver code should not include <linux/vermagic.h>"
> isn't it?
>
>
> IIUC, Borislav reported conflict of MODULE_ARCH_VERMAGIC
> if <linux/vermagic.h> is included before <linux/module.h>.
>
> With your cleanups, the include site of <linux/vermagic.h>
> will be limited to kernel/module.c and scripts/mod/module.c
>
> Assuming those two files include them in the *correct* order,
> this problem will be suppressed.
>
> But, I do not think it addresses the problem properly.
>
>
> If
>   #include <foo.h>
>   #include <bar.h>
>
> works, but
>
>   #include <bar.h>
>   #include <foo.h>
>
> does not, the root cause is very likely
> that <bar.h> is not self-contained.
> The problem is solved by including <foo.h> from <bar.h>
>
>
> Please see my thoughts in this:
> https://lore.kernel.org/patchwork/patch/1227024/
>
>
> Of course, we are solving different issues, so I think
> we can merge both.
>
>
> What do you think?

The idea and rationale are right, include order should not be important.

Thanks

Patch
diff mbox series

diff --git a/include/linux/vermagic.h b/include/linux/vermagic.h
index 9aced11e9000..7768d20ada39 100644
--- a/include/linux/vermagic.h
+++ b/include/linux/vermagic.h
@@ -1,4 +1,9 @@ 
 /* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef INCLUDE_VERMAGIC
+#error "This header can be included from kernel/module.c or *.mod.c only"
+#endif
+
 #include <generated/utsrelease.h>

 /* Simply sanity version stamp for modules. */
diff --git a/kernel/module.c b/kernel/module.c
index 646f1e2330d2..8833e848b73c 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -4,6 +4,9 @@ 
    Copyright (C) 2001 Rusty Russell, 2002, 2010 Rusty Russell IBM.

 */
+
+#define INCLUDE_VERMAGIC
+
 #include <linux/export.h>
 #include <linux/extable.h>
 #include <linux/moduleloader.h>
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 5c3c50c5ec52..7f7d4ee7b652 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2251,6 +2251,7 @@  static void add_header(struct buffer *b, struct module *mod)
 	 * Include build-salt.h after module.h in order to
 	 * inherit the definitions.
 	 */
+	buf_printf(b, "#define INCLUDE_VERMAGIC\n");
 	buf_printf(b, "#include <linux/build-salt.h>\n");
 	buf_printf(b, "#include <linux/vermagic.h>\n");
 	buf_printf(b, "#include <linux/compiler.h>\n");