[PATCH net-next v1 0/4] Remove vermagic header from global include folder

[PATCH net-next v1 0/4] Remove vermagic header from global include folder

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

Changelog:
v1:
 * Added tags
 * Updated patch #4 with test results
 * Changed scripts/mod/modpost.c to create inclusion of vermagic.h
   from kernel folder and not from general include/linux. This is
   needed to generate *.mod.c files, while building modules.
v0: https://lore.kernel.org/lkml/20200414155732.1236944-1-leon@kernel.org
----------------------------------------------------------------------------

Hi,

This is followup to the failure reported by Borislav [1] and suggested
fix later on [2].

The series removes all includes of linux/vermagic.h, updates hns and
nfp to use same kernel versioning scheme (exactly like we did for
other drivers in previous cycle) and removes vermagic.h from global
include folder.

[1] https://lore.kernel.org/lkml/20200411155623.GA22175@zn.tnic
[2] https://lore.kernel.org/lkml/20200413080452.GA3772@zn.tnic

------------------------------------------------------------
1. Honestly, I have no idea if it can go to net-rc, clearly not all my
patches are fixes, so I'm sending them to the net-next.
2. Still didn't get response from kbuild, but it passed my own
compilation tests.
https://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git/log/?h=vermagic

Thanks

Leon Romanovsky (4):
  drivers: Remove inclusion of vermagic header
  net/hns: Remove custom driver version in favour of global one
  net/nfp: Update driver to use global kernel version
  kernel/module: Hide vermagic header file from general use

 drivers/net/bonding/bonding_priv.h                   | 2 +-
 drivers/net/ethernet/3com/3c509.c                    | 1 -
 drivers/net/ethernet/3com/3c515.c                    | 1 -
 drivers/net/ethernet/adaptec/starfire.c              | 1 -
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c      | 3 ---
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.h      | 4 ----
 drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c   | 4 ----
 drivers/net/ethernet/netronome/nfp/nfp_main.c        | 3 ---
 drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c | 2 --
 drivers/net/ethernet/pensando/ionic/ionic_main.c     | 2 +-
 drivers/power/supply/test_power.c                    | 2 +-
 kernel/module.c                                      | 2 +-
 {include/linux => kernel}/vermagic.h                 | 0
 net/ethtool/ioctl.c                                  | 3 +--
 scripts/mod/modpost.c                                | 2 +-
 15 files changed, 6 insertions(+), 26 deletions(-)
 rename {include/linux => kernel}/vermagic.h (100%)

--
2.25.2

[PATCH net-next v1 4/4] kernel/module: Hide vermagic header file from general use

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

VERMAGIC* definitions are not supposed to be used by the drivers,
see this [1] bug report, so simply move this header file to be visible
to kernel/* and scripts files only.

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>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 kernel/module.c                      | 2 +-
 {include/linux => kernel}/vermagic.h | 0
 scripts/mod/modpost.c                | 2 +-
 3 files changed, 2 insertions(+), 2 deletions(-)
 rename {include/linux => kernel}/vermagic.h (100%)

diff --git a/kernel/module.c b/kernel/module.c
index 3447f3b74870..fce06095d341 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -29,7 +29,6 @@
 #include <linux/moduleparam.h>
 #include <linux/errno.h>
 #include <linux/err.h>
-#include <linux/vermagic.h>
 #include <linux/notifier.h>
 #include <linux/sched.h>
 #include <linux/device.h>
@@ -55,6 +54,7 @@
 #include <linux/audit.h>
 #include <uapi/linux/module.h>
 #include "module-internal.h"
+#include "vermagic.h"

 #define CREATE_TRACE_POINTS
 #include <trace/events/module.h>
diff --git a/include/linux/vermagic.h b/kernel/vermagic.h
similarity index 100%
rename from include/linux/vermagic.h
rename to kernel/vermagic.h
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 5c3c50c5ec52..91f86261bcfe 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2252,7 +2252,7 @@ static void add_header(struct buffer *b, struct module *mod)
 	 * inherit the definitions.
 	 */
 	buf_printf(b, "#include <linux/build-salt.h>\n");
-	buf_printf(b, "#include <linux/vermagic.h>\n");
+	buf_printf(b, "#include <../kernel/vermagic.h>\n");
 	buf_printf(b, "#include <linux/compiler.h>\n");
 	buf_printf(b, "\n");
 	buf_printf(b, "BUILD_SALT;\n");
--
2.25.2

Re: [PATCH net-next v1 4/4] kernel/module: Hide vermagic header file from general use

[Jessica Yu]

+++ Leon Romanovsky [15/04/20 16:36 +0300]:
>From: Leon Romanovsky <leonro@mellanox.com>
>
>VERMAGIC* definitions are not supposed to be used by the drivers,
>see this [1] bug report, so simply move this header file to be visible
>to kernel/* and scripts files only.
>
>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>
>Signed-off-by: Leon Romanovsky <leonro@mellanox.com>

Acked-by: Jessica Yu <jeyu@kernel.org>

Re: [PATCH net-next v1 4/4] kernel/module: Hide vermagic header file from general use

[Masahiro Yamada]

On Wed, Apr 15, 2020 at 10:37 PM 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 simply move this header file to be visible
> to kernel/* and scripts files only.
>
> 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>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
>  kernel/module.c                      | 2 +-
>  {include/linux => kernel}/vermagic.h | 0
>  scripts/mod/modpost.c                | 2 +-
>  3 files changed, 2 insertions(+), 2 deletions(-)
>  rename {include/linux => kernel}/vermagic.h (100%)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 3447f3b74870..fce06095d341 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -29,7 +29,6 @@
>  #include <linux/moduleparam.h>
>  #include <linux/errno.h>
>  #include <linux/err.h>
> -#include <linux/vermagic.h>
>  #include <linux/notifier.h>
>  #include <linux/sched.h>
>  #include <linux/device.h>
> @@ -55,6 +54,7 @@
>  #include <linux/audit.h>
>  #include <uapi/linux/module.h>
>  #include "module-internal.h"
> +#include "vermagic.h"
>
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/module.h>
> diff --git a/include/linux/vermagic.h b/kernel/vermagic.h
> similarity index 100%
> rename from include/linux/vermagic.h
> rename to kernel/vermagic.h
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 5c3c50c5ec52..91f86261bcfe 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -2252,7 +2252,7 @@ static void add_header(struct buffer *b, struct module *mod)
>          * inherit the definitions.
>          */
>         buf_printf(b, "#include <linux/build-salt.h>\n");
> -       buf_printf(b, "#include <linux/vermagic.h>\n");
> +       buf_printf(b, "#include <../kernel/vermagic.h>\n");


I hate this.


#error can break the build if the header is included in a wrong way.

For example, include/acpi/platform/aclinux.h



I prefer something like this if a big hammer is needed here.

diff --git a/include/linux/vermagic.h b/include/linux/vermagic.h
index 9aced11e9000..d69fa4661715 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"
+#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");




-- 
Best Regards
Masahiro Yamada

Re: [PATCH net-next v1 4/4] kernel/module: Hide vermagic header file from general use

[Leon Romanovsky]

On Fri, Apr 17, 2020 at 09:07:10PM +0900, Masahiro Yamada wrote:
> On Wed, Apr 15, 2020 at 10:37 PM 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 simply move this header file to be visible
> > to kernel/* and scripts files only.
> >
> > 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>
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > ---
> >  kernel/module.c                      | 2 +-
> >  {include/linux => kernel}/vermagic.h | 0
> >  scripts/mod/modpost.c                | 2 +-
> >  3 files changed, 2 insertions(+), 2 deletions(-)
> >  rename {include/linux => kernel}/vermagic.h (100%)
> >
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 3447f3b74870..fce06095d341 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -29,7 +29,6 @@
> >  #include <linux/moduleparam.h>
> >  #include <linux/errno.h>
> >  #include <linux/err.h>
> > -#include <linux/vermagic.h>
> >  #include <linux/notifier.h>
> >  #include <linux/sched.h>
> >  #include <linux/device.h>
> > @@ -55,6 +54,7 @@
> >  #include <linux/audit.h>
> >  #include <uapi/linux/module.h>
> >  #include "module-internal.h"
> > +#include "vermagic.h"
> >
> >  #define CREATE_TRACE_POINTS
> >  #include <trace/events/module.h>
> > diff --git a/include/linux/vermagic.h b/kernel/vermagic.h
> > similarity index 100%
> > rename from include/linux/vermagic.h
> > rename to kernel/vermagic.h
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 5c3c50c5ec52..91f86261bcfe 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -2252,7 +2252,7 @@ static void add_header(struct buffer *b, struct module *mod)
> >          * inherit the definitions.
> >          */
> >         buf_printf(b, "#include <linux/build-salt.h>\n");
> > -       buf_printf(b, "#include <linux/vermagic.h>\n");
> > +       buf_printf(b, "#include <../kernel/vermagic.h>\n");
>
>
> I hate this.

Why?

The "ifndef" solution won't achieve the main goal of this patch -
completely disallow usage of vermagic.h. Every kernel developer
knows that headers outside of include/ folder are not meant to be
used.

Once the vermagic.h inside kernel/ folder no one will even try
to include it.

But with new define, all reviewers will need to be aware that they
don't suppose to see it in "regular" patches.

So, instead of making everything clear by being explicit, we will
rely on developers + review + checkpatch and implicit logic.

>
>
> #error can break the build if the header is included in a wrong way.
>
> For example, include/acpi/platform/aclinux.h
>
>
>
> I prefer something like this if a big hammer is needed here.

If it is the only way to progress, I will change the series to use
this variant, however I don't like it both technically and aesthetically
by seeing machinery file with extremely narrow scope inside general include
header.

>
> diff --git a/include/linux/vermagic.h b/include/linux/vermagic.h
> index 9aced11e9000..d69fa4661715 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"
> +#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");
>
>
>
>
> --
> Best Regards
> Masahiro Yamada