[PATCH v5 00/13] module: core code clean up

[PATCH v5 00/13] module: core code clean up

[Aaron Tomlin]

Hi Luis,

As per your suggestion [1], this is an attempt to refactor and split
optional code out of core module support code into separate components.
This version is based on Linus' commit e6251ab4551f ("Merge tag
'nfs-for-5.17-2' of git://git.linux-nfs.org/projects/anna/linux-nfs").
Please let me know your thoughts. So far, no feedback from 0-day; albeit,
if I see something, I'll let you know.

Changes since v1 [2]:

  - Moved module version support code into a new file

Changes since v2 [3]:

 - Moved module decompress support to a separate file
 - Made check_modinfo_livepatch() generic (Petr Mladek)
 - Removed filename from each newly created file (Luis Chamberlain)
 - Addressed some (i.e. --ignore=ASSIGN_IN_IF,AVOID_BUG was used)
   minor scripts/checkpatch.pl concerns e.g., use strscpy over
   strlcpy and missing a blank line after declarations (Allen)

Changes since v3 [4]:

 - Refactored both is_livepatch_module() and set_livepatch_module(),
   respectively, to use IS_ENABLED(CONFIG_LIVEPATCH) (Joe Perches)
 - Addressed various compiler warnings e.g., no previous prototype (0-day)

Changes since v4 [5]:

 - Dropped RFC (Luis Chamberlain)
 - Moved is_livepatch_module() and set_livepatch_module() to
   kernel/module/livepatch.c; set_livepatch_module() will
   remain for internal use only
 - Addressed minor compiler warning concerning
   kernel/module/internal.h (0-day)
 - Resolved style violations reported by scripts/checkpatch.pl
 - Dropped patch 5 [6] so external patch [7] can be applied at
   a later date post merge into module-next (Christophe Leroy)

[1]: https://lore.kernel.org/lkml/YbEZ4HgSYQEPuRmS@bombadil.infradead.org/
[2]: https://lore.kernel.org/lkml/20211228213041.1356334-1-atomlin@redhat.com/
[3]: https://lore.kernel.org/lkml/20220106234319.2067842-1-atomlin@redhat.com/
[4]: https://lore.kernel.org/lkml/20220128203934.600247-1-atomlin@redhat.com/
[5]: https://lore.kernel.org/lkml/20220130213214.1042497-1-atomlin@redhat.com/
[6]: https://lore.kernel.org/lkml/20220130213214.1042497-6-atomlin@redhat.com/
[7]: https://lore.kernel.org/lkml/203348805c9ac9851d8939d15cb9802ef047b5e2.1643919758.git.christophe.leroy@csgroup.eu/


Aaron Tomlin (13):
  module: Move all into module/
  module: Simple refactor in preparation for split
  module: Make internal.h more compliant
  module: Move livepatch support to a separate file
  module: Move latched RB-tree support to a separate file
  module: Move strict rwx support to a separate file
  module: Move extra signature support out of core code
  module: Move kmemleak support to a separate file
  module: Move kallsyms support into a separate file
  module: Move procfs support into a separate file
  module: Move sysfs support into a separate file
  module: Move kdb_modules list out of core code
  module: Move version support into a separate file

 MAINTAINERS                                   |    2 +-
 include/linux/module.h                        |   10 +-
 kernel/Makefile                               |    5 +-
 kernel/debug/kdb/kdb_main.c                   |    5 +
 kernel/module-internal.h                      |   50 -
 kernel/module/Makefile                        |   19 +
 kernel/module/debug_kmemleak.c                |   30 +
 .../decompress.c}                             |    2 +-
 kernel/module/internal.h                      |  283 +++
 kernel/module/kallsyms.c                      |  502 +++++
 kernel/module/livepatch.c                     |   80 +
 kernel/{module.c => module/main.c}            | 1862 +----------------
 kernel/module/procfs.c                        |  142 ++
 .../signature.c}                              |    0
 kernel/module/signing.c                       |  120 ++
 kernel/module/strict_rwx.c                    |   84 +
 kernel/module/sysfs.c                         |  425 ++++
 kernel/module/tree_lookup.c                   |  109 +
 kernel/module/version.c                       |  110 +
 kernel/module_signing.c                       |   45 -
 20 files changed, 2009 insertions(+), 1876 deletions(-)
 delete mode 100644 kernel/module-internal.h
 create mode 100644 kernel/module/Makefile
 create mode 100644 kernel/module/debug_kmemleak.c
 rename kernel/{module_decompress.c => module/decompress.c} (99%)
 create mode 100644 kernel/module/internal.h
 create mode 100644 kernel/module/kallsyms.c
 create mode 100644 kernel/module/livepatch.c
 rename kernel/{module.c => module/main.c} (64%)
 create mode 100644 kernel/module/procfs.c
 rename kernel/{module_signature.c => module/signature.c} (100%)
 create mode 100644 kernel/module/signing.c
 create mode 100644 kernel/module/strict_rwx.c
 create mode 100644 kernel/module/sysfs.c
 create mode 100644 kernel/module/tree_lookup.c
 create mode 100644 kernel/module/version.c
 delete mode 100644 kernel/module_signing.c


base-commit: e6251ab4551f51fa4cee03523e08051898c3ce82
-- 
2.34.1

[PATCH v5 01/13] module: Move all into module/

[Aaron Tomlin]

No functional changes.

This patch moves all module related code into a separate directory,
modifies each file name and creates a new Makefile. Note: this effort
is in preparation to refactor core module code.

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 MAINTAINERS                                         | 2 +-
 kernel/Makefile                                     | 5 +----
 kernel/module/Makefile                              | 9 +++++++++
 kernel/{module_decompress.c => module/decompress.c} | 2 +-
 kernel/{module-internal.h => module/internal.h}     | 1 +
 kernel/{module.c => module/main.c}                  | 2 +-
 kernel/{module_signature.c => module/signature.c}   | 0
 kernel/{module_signing.c => module/signing.c}       | 2 +-
 8 files changed, 15 insertions(+), 8 deletions(-)
 create mode 100644 kernel/module/Makefile
 rename kernel/{module_decompress.c => module/decompress.c} (99%)
 rename kernel/{module-internal.h => module/internal.h} (99%)
 rename kernel/{module.c => module/main.c} (99%)
 rename kernel/{module_signature.c => module/signature.c} (100%)
 rename kernel/{module_signing.c => module/signing.c} (97%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3e461db9cd91..7e6232bd15f5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13001,7 +13001,7 @@ L:	linux-kernel@vger.kernel.org
 S:	Maintained
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git modules-next
 F:	include/linux/module.h
-F:	kernel/module.c
+F:	kernel/module/main.c
 
 MONOLITHIC POWER SYSTEM PMIC DRIVER
 M:	Saravanan Sekar <sravanhome@gmail.com>
diff --git a/kernel/Makefile b/kernel/Makefile
index 56f4ee97f328..3a6380975c57 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -53,6 +53,7 @@ obj-y += rcu/
 obj-y += livepatch/
 obj-y += dma/
 obj-y += entry/
+obj-y += module/
 
 obj-$(CONFIG_KCMP) += kcmp.o
 obj-$(CONFIG_FREEZER) += freezer.o
@@ -66,10 +67,6 @@ ifneq ($(CONFIG_SMP),y)
 obj-y += up.o
 endif
 obj-$(CONFIG_UID16) += uid16.o
-obj-$(CONFIG_MODULES) += module.o
-obj-$(CONFIG_MODULE_DECOMPRESS) += module_decompress.o
-obj-$(CONFIG_MODULE_SIG) += module_signing.o
-obj-$(CONFIG_MODULE_SIG_FORMAT) += module_signature.o
 obj-$(CONFIG_KALLSYMS) += kallsyms.o
 obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
 obj-$(CONFIG_CRASH_CORE) += crash_core.o
diff --git a/kernel/module/Makefile b/kernel/module/Makefile
new file mode 100644
index 000000000000..2902fc7d0ef1
--- /dev/null
+++ b/kernel/module/Makefile
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Makefile for linux kernel module support
+#
+
+obj-$(CONFIG_MODULES) += main.o
+obj-$(CONFIG_MODULE_DECOMPRESS) += decompress.o
+obj-$(CONFIG_MODULE_SIG) += signing.o
+obj-$(CONFIG_MODULE_SIG_FORMAT) += signature.o
diff --git a/kernel/module_decompress.c b/kernel/module/decompress.c
similarity index 99%
rename from kernel/module_decompress.c
rename to kernel/module/decompress.c
index b01c69c2ff99..c153fd8a4444 100644
--- a/kernel/module_decompress.c
+++ b/kernel/module/decompress.c
@@ -12,7 +12,7 @@
 #include <linux/sysfs.h>
 #include <linux/vmalloc.h>
 
-#include "module-internal.h"
+#include "internal.h"
 
 static int module_extend_max_pages(struct load_info *info, unsigned int extent)
 {
diff --git a/kernel/module-internal.h b/kernel/module/internal.h
similarity index 99%
rename from kernel/module-internal.h
rename to kernel/module/internal.h
index 8c381c99062f..c49896368f7f 100644
--- a/kernel/module-internal.h
+++ b/kernel/module/internal.h
@@ -44,6 +44,7 @@ static inline int module_decompress(struct load_info *info,
 {
 	return -EOPNOTSUPP;
 }
+
 static inline void module_decompress_cleanup(struct load_info *info)
 {
 }
diff --git a/kernel/module.c b/kernel/module/main.c
similarity index 99%
rename from kernel/module.c
rename to kernel/module/main.c
index 46a5c2ed1928..34a2b0cf3c3e 100644
--- a/kernel/module.c
+++ b/kernel/module/main.c
@@ -58,7 +58,7 @@
 #include <linux/dynamic_debug.h>
 #include <linux/audit.h>
 #include <uapi/linux/module.h>
-#include "module-internal.h"
+#include "internal.h"
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/module.h>
diff --git a/kernel/module_signature.c b/kernel/module/signature.c
similarity index 100%
rename from kernel/module_signature.c
rename to kernel/module/signature.c
diff --git a/kernel/module_signing.c b/kernel/module/signing.c
similarity index 97%
rename from kernel/module_signing.c
rename to kernel/module/signing.c
index 8723ae70ea1f..8aeb6d2ee94b 100644
--- a/kernel/module_signing.c
+++ b/kernel/module/signing.c
@@ -12,7 +12,7 @@
 #include <linux/string.h>
 #include <linux/verification.h>
 #include <crypto/public_key.h>
-#include "module-internal.h"
+#include "internal.h"
 
 /*
  * Verify the signature on a module.
-- 
2.34.1

Re: [PATCH v5 01/13] module: Move all into module/

[Christophe Leroy]

Le 09/02/2022 à 18:03, Aaron Tomlin a écrit :
> No functional changes.
> 
> This patch moves all module related code into a separate directory,
> modifies each file name and creates a new Makefile. Note: this effort
> is in preparation to refactor core module code.
> 
> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> ---
>   MAINTAINERS                                         | 2 +-
>   kernel/Makefile                                     | 5 +----
>   kernel/module/Makefile                              | 9 +++++++++
>   kernel/{module_decompress.c => module/decompress.c} | 2 +-
>   kernel/{module-internal.h => module/internal.h}     | 1 +
>   kernel/{module.c => module/main.c}                  | 2 +-
>   kernel/{module_signature.c => module/signature.c}   | 0
>   kernel/{module_signing.c => module/signing.c}       | 2 +-
>   8 files changed, 15 insertions(+), 8 deletions(-)
>   create mode 100644 kernel/module/Makefile
>   rename kernel/{module_decompress.c => module/decompress.c} (99%)
>   rename kernel/{module-internal.h => module/internal.h} (99%)
>   rename kernel/{module.c => module/main.c} (99%)
>   rename kernel/{module_signature.c => module/signature.c} (100%)
>   rename kernel/{module_signing.c => module/signing.c} (97%)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3e461db9cd91..7e6232bd15f5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13001,7 +13001,7 @@ L:	linux-kernel@vger.kernel.org
>   S:	Maintained
>   T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git modules-next
>   F:	include/linux/module.h
> -F:	kernel/module.c
> +F:	kernel/module/main.c

Shouldn't it be the entire directory ?

Otherwise you have to add all new files (and I didn't do it).

>   
>   MONOLITHIC POWER SYSTEM PMIC DRIVER
>   M:	Saravanan Sekar <sravanhome@gmail.com>
> diff --git a/kernel/Makefile b/kernel/Makefile
> index 56f4ee97f328..3a6380975c57 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -53,6 +53,7 @@ obj-y += rcu/
>   obj-y += livepatch/
>   obj-y += dma/
>   obj-y += entry/
> +obj-y += module/
>   
>   obj-$(CONFIG_KCMP) += kcmp.o
>   obj-$(CONFIG_FREEZER) += freezer.o
> @@ -66,10 +67,6 @@ ifneq ($(CONFIG_SMP),y)
>   obj-y += up.o
>   endif
>   obj-$(CONFIG_UID16) += uid16.o
> -obj-$(CONFIG_MODULES) += module.o
> -obj-$(CONFIG_MODULE_DECOMPRESS) += module_decompress.o
> -obj-$(CONFIG_MODULE_SIG) += module_signing.o
> -obj-$(CONFIG_MODULE_SIG_FORMAT) += module_signature.o
>   obj-$(CONFIG_KALLSYMS) += kallsyms.o
>   obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
>   obj-$(CONFIG_CRASH_CORE) += crash_core.o
> diff --git a/kernel/module/Makefile b/kernel/module/Makefile
> new file mode 100644
> index 000000000000..2902fc7d0ef1
> --- /dev/null
> +++ b/kernel/module/Makefile
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Makefile for linux kernel module support
> +#
> +
> +obj-$(CONFIG_MODULES) += main.o
> +obj-$(CONFIG_MODULE_DECOMPRESS) += decompress.o
> +obj-$(CONFIG_MODULE_SIG) += signing.o
> +obj-$(CONFIG_MODULE_SIG_FORMAT) += signature.o
> diff --git a/kernel/module_decompress.c b/kernel/module/decompress.c
> similarity index 99%
> rename from kernel/module_decompress.c
> rename to kernel/module/decompress.c
> index b01c69c2ff99..c153fd8a4444 100644
> --- a/kernel/module_decompress.c
> +++ b/kernel/module/decompress.c
> @@ -12,7 +12,7 @@
>   #include <linux/sysfs.h>
>   #include <linux/vmalloc.h>
>   
> -#include "module-internal.h"
> +#include "internal.h"
>   
>   static int module_extend_max_pages(struct load_info *info, unsigned int extent)
>   {
> diff --git a/kernel/module-internal.h b/kernel/module/internal.h
> similarity index 99%
> rename from kernel/module-internal.h
> rename to kernel/module/internal.h
> index 8c381c99062f..c49896368f7f 100644
> --- a/kernel/module-internal.h
> +++ b/kernel/module/internal.h
> @@ -44,6 +44,7 @@ static inline int module_decompress(struct load_info *info,
>   {
>   	return -EOPNOTSUPP;
>   }
> +

This new line should be in patch 3 instead.

>   static inline void module_decompress_cleanup(struct load_info *info)
>   {
>   }
> diff --git a/kernel/module.c b/kernel/module/main.c
> similarity index 99%
> rename from kernel/module.c
> rename to kernel/module/main.c
> index 46a5c2ed1928..34a2b0cf3c3e 100644
> --- a/kernel/module.c
> +++ b/kernel/module/main.c
> @@ -58,7 +58,7 @@
>   #include <linux/dynamic_debug.h>
>   #include <linux/audit.h>
>   #include <uapi/linux/module.h>
> -#include "module-internal.h"
> +#include "internal.h"
>   
>   #define CREATE_TRACE_POINTS
>   #include <trace/events/module.h>
> diff --git a/kernel/module_signature.c b/kernel/module/signature.c
> similarity index 100%
> rename from kernel/module_signature.c
> rename to kernel/module/signature.c
> diff --git a/kernel/module_signing.c b/kernel/module/signing.c
> similarity index 97%
> rename from kernel/module_signing.c
> rename to kernel/module/signing.c
> index 8723ae70ea1f..8aeb6d2ee94b 100644
> --- a/kernel/module_signing.c
> +++ b/kernel/module/signing.c
> @@ -12,7 +12,7 @@
>   #include <linux/string.h>
>   #include <linux/verification.h>
>   #include <crypto/public_key.h>
> -#include "module-internal.h"
> +#include "internal.h"
>   
>   /*
>    * Verify the signature on a module.

Re: [PATCH v5 01/13] module: Move all into module/

[Aaron Tomlin]

On Thu 2022-02-10 11:11 +0000, Christophe Leroy wrote:
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 3e461db9cd91..7e6232bd15f5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -13001,7 +13001,7 @@ L:    linux-kernel@vger.kernel.org
> >   S:    Maintained
> >   T:    git git://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git modules-next
> >   F:    include/linux/module.h
> > -F:    kernel/module.c
> > +F:    kernel/module/main.c
>
> Shouldn't it be the entire directory ?

Firstly, thank you for your feedback Christophe.

Indeed it should. Moving forward: kernel/module/*

> > @@ -44,6 +44,7 @@ static inline int module_decompress(struct load_info *info,
> >   {
> >       return -EOPNOTSUPP;
> >   }
> > +
>
> This new line should be in patch 3 instead.

Fair enough. Given that the purpose of this particular patch is a simple
migration, style violations e.g. "Please use a blank line after
function/struct/union/enum declarations", can be resolved at a later stage.


Kind regards,

-- 
Aaron Tomlin

Re: [PATCH v5 01/13] module: Move all into module/

[Joe Perches]

On Thu, 2022-02-10 at 14:45 +0000, Aaron Tomlin wrote:
> On Thu 2022-02-10 11:11 +0000, Christophe Leroy wrote:
> > > diff --git a/MAINTAINERS b/MAINTAINERS
[]
> > > @@ -13001,7 +13001,7 @@ L:    linux-kernel@vger.kernel.org
> > >   S:    Maintained
> > >   T:    git git://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git modules-next
> > >   F:    include/linux/module.h
> > > -F:    kernel/module.c
> > > +F:    kernel/module/main.c
> > 
> > Shouldn't it be the entire directory ?
> 
> Indeed it should. Moving forward: kernel/module/*

Better would be:

F:	kernel/module/

in case it ever gets subdirectories too.

Re: [PATCH v5 01/13] module: Move all into module/

[Aaron Tomlin]

On Thu 2022-02-10 09:02 -0800, Joe Perches wrote:
> On Thu, 2022-02-10 at 14:45 +0000, Aaron Tomlin wrote:
> Better would be:
>
> F:    kernel/module/
>
> in case it ever gets subdirectories too.

Agreed.

-- 
Aaron Tomlin

Re: [PATCH v5 01/13] module: Move all into module/

[Luis Chamberlain]

On Thu, Feb 10, 2022 at 09:02:09AM -0800, Joe Perches wrote:
> On Thu, 2022-02-10 at 14:45 +0000, Aaron Tomlin wrote:
> > On Thu 2022-02-10 11:11 +0000, Christophe Leroy wrote:
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> []
> > > > @@ -13001,7 +13001,7 @@ L:    linux-kernel@vger.kernel.org
> > > >   S:    Maintained
> > > >   T:    git git://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git modules-next
> > > >   F:    include/linux/module.h
> > > > -F:    kernel/module.c
> > > > +F:    kernel/module/main.c
> > > 
> > > Shouldn't it be the entire directory ?
> > 
> > Indeed it should. Moving forward: kernel/module/*
> 
> Better would be:
> 
> F:	kernel/module/
> 
> in case it ever gets subdirectories too.

Yes, that also later allows us to add entries for things like say
livepatching. Or we peg the livepatching file to the livepatching
tag. Either way, one of the side goals of the split is to eventually
help scale maintenance.

  Luis

[PATCH v5 02/13] module: Simple refactor in preparation for split

[Aaron Tomlin]

No functional change.

This patch makes it possible to move non-essential code
out of core module code.

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 kernel/module/internal.h | 22 ++++++++++++++++++++++
 kernel/module/main.c     | 23 ++---------------------
 2 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index c49896368f7f..1a4b33ce9f5f 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -7,6 +7,28 @@
 
 #include <linux/elf.h>
 #include <asm/module.h>
+#include <linux/mutex.h>
+
+#ifndef ARCH_SHF_SMALL
+#define ARCH_SHF_SMALL 0
+#endif
+
+/* If this is set, the section belongs in the init part of the module */
+#define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
+/* Maximum number of characters written by module_flags() */
+#define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
+#define MODULE_SECT_READ_SIZE (3 /* "0x", "\n" */ + (BITS_PER_LONG / 4))
+
+extern struct mutex module_mutex;
+extern struct list_head modules;
+
+/* Provided by the linker */
+extern const struct kernel_symbol __start___ksymtab[];
+extern const struct kernel_symbol __stop___ksymtab[];
+extern const struct kernel_symbol __start___ksymtab_gpl[];
+extern const struct kernel_symbol __stop___ksymtab_gpl[];
+extern const s32 __start___kcrctab[];
+extern const s32 __start___kcrctab_gpl[];
 
 struct load_info {
 	const char *name;
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 34a2b0cf3c3e..750e3ad28679 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -63,10 +63,6 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/module.h>
 
-#ifndef ARCH_SHF_SMALL
-#define ARCH_SHF_SMALL 0
-#endif
-
 /*
  * Modules' sections will be aligned on page boundaries
  * to ensure complete separation of code and data, but
@@ -78,9 +74,6 @@
 # define debug_align(X) (X)
 #endif
 
-/* If this is set, the section belongs in the init part of the module */
-#define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
-
 /*
  * Mutex protects:
  * 1) List of modules (also safely readable with preempt_disable),
@@ -88,8 +81,8 @@
  * 3) module_addr_min/module_addr_max.
  * (delete and add uses RCU list operations).
  */
-static DEFINE_MUTEX(module_mutex);
-static LIST_HEAD(modules);
+DEFINE_MUTEX(module_mutex);
+LIST_HEAD(modules);
 
 /* Work queue for freeing init sections in success case */
 static void do_free_init(struct work_struct *w);
@@ -408,14 +401,6 @@ static __maybe_unused void *any_section_objs(const struct load_info *info,
 	return (void *)info->sechdrs[sec].sh_addr;
 }
 
-/* Provided by the linker */
-extern const struct kernel_symbol __start___ksymtab[];
-extern const struct kernel_symbol __stop___ksymtab[];
-extern const struct kernel_symbol __start___ksymtab_gpl[];
-extern const struct kernel_symbol __stop___ksymtab_gpl[];
-extern const s32 __start___kcrctab[];
-extern const s32 __start___kcrctab_gpl[];
-
 #ifndef CONFIG_MODVERSIONS
 #define symversion(base, idx) NULL
 #else
@@ -1490,7 +1475,6 @@ struct module_sect_attrs {
 	struct module_sect_attr attrs[];
 };
 
-#define MODULE_SECT_READ_SIZE (3 /* "0x", "\n" */ + (BITS_PER_LONG / 4))
 static ssize_t module_sect_read(struct file *file, struct kobject *kobj,
 				struct bin_attribute *battr,
 				char *buf, loff_t pos, size_t count)
@@ -4542,9 +4526,6 @@ static void cfi_cleanup(struct module *mod)
 #endif
 }
 
-/* Maximum number of characters written by module_flags() */
-#define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
-
 /* Keep in sync with MODULE_FLAGS_BUF_SIZE !!! */
 static char *module_flags(struct module *mod, char *buf)
 {
-- 
2.34.1

Re: [PATCH v5 02/13] module: Simple refactor in preparation for split

[Christophe Leroy]

Le 09/02/2022 à 18:03, Aaron Tomlin a écrit :
> No functional change.
> 
> This patch makes it possible to move non-essential code
> out of core module code.
> 
> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> ---
>   kernel/module/internal.h | 22 ++++++++++++++++++++++
>   kernel/module/main.c     | 23 ++---------------------
>   2 files changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index c49896368f7f..1a4b33ce9f5f 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -7,6 +7,28 @@
>   
>   #include <linux/elf.h>
>   #include <asm/module.h>
> +#include <linux/mutex.h>
> +
> +#ifndef ARCH_SHF_SMALL
> +#define ARCH_SHF_SMALL 0
> +#endif
> +
> +/* If this is set, the section belongs in the init part of the module */
> +#define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
> +/* Maximum number of characters written by module_flags() */
> +#define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
> +#define MODULE_SECT_READ_SIZE (3 /* "0x", "\n" */ + (BITS_PER_LONG / 4))

This is used only in sysfs.c, why move it to internal.h ?


> +
> +extern struct mutex module_mutex;
> +extern struct list_head modules;
> +
> +/* Provided by the linker */
> +extern const struct kernel_symbol __start___ksymtab[];
> +extern const struct kernel_symbol __stop___ksymtab[];
> +extern const struct kernel_symbol __start___ksymtab_gpl[];
> +extern const struct kernel_symbol __stop___ksymtab_gpl[];
> +extern const s32 __start___kcrctab[];
> +extern const s32 __start___kcrctab_gpl[];
>   
>   struct load_info {
>   	const char *name;
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 34a2b0cf3c3e..750e3ad28679 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -63,10 +63,6 @@
>   #define CREATE_TRACE_POINTS
>   #include <trace/events/module.h>
>   
> -#ifndef ARCH_SHF_SMALL
> -#define ARCH_SHF_SMALL 0
> -#endif
> -
>   /*
>    * Modules' sections will be aligned on page boundaries
>    * to ensure complete separation of code and data, but
> @@ -78,9 +74,6 @@
>   # define debug_align(X) (X)
>   #endif
>   
> -/* If this is set, the section belongs in the init part of the module */
> -#define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
> -
>   /*
>    * Mutex protects:
>    * 1) List of modules (also safely readable with preempt_disable),
> @@ -88,8 +81,8 @@
>    * 3) module_addr_min/module_addr_max.
>    * (delete and add uses RCU list operations).
>    */
> -static DEFINE_MUTEX(module_mutex);
> -static LIST_HEAD(modules);
> +DEFINE_MUTEX(module_mutex);
> +LIST_HEAD(modules);
>   
>   /* Work queue for freeing init sections in success case */
>   static void do_free_init(struct work_struct *w);
> @@ -408,14 +401,6 @@ static __maybe_unused void *any_section_objs(const struct load_info *info,
>   	return (void *)info->sechdrs[sec].sh_addr;
>   }
>   
> -/* Provided by the linker */
> -extern const struct kernel_symbol __start___ksymtab[];
> -extern const struct kernel_symbol __stop___ksymtab[];
> -extern const struct kernel_symbol __start___ksymtab_gpl[];
> -extern const struct kernel_symbol __stop___ksymtab_gpl[];
> -extern const s32 __start___kcrctab[];
> -extern const s32 __start___kcrctab_gpl[];
> -
>   #ifndef CONFIG_MODVERSIONS
>   #define symversion(base, idx) NULL
>   #else
> @@ -1490,7 +1475,6 @@ struct module_sect_attrs {
>   	struct module_sect_attr attrs[];
>   };
>   
> -#define MODULE_SECT_READ_SIZE (3 /* "0x", "\n" */ + (BITS_PER_LONG / 4))

This is used only in sysfs.c, why move it to internal.h ?

>   static ssize_t module_sect_read(struct file *file, struct kobject *kobj,
>   				struct bin_attribute *battr,
>   				char *buf, loff_t pos, size_t count)
> @@ -4542,9 +4526,6 @@ static void cfi_cleanup(struct module *mod)
>   #endif
>   }
>   
> -/* Maximum number of characters written by module_flags() */
> -#define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
> -
>   /* Keep in sync with MODULE_FLAGS_BUF_SIZE !!! */
>   static char *module_flags(struct module *mod, char *buf)
>   {

Re: [PATCH v5 02/13] module: Simple refactor in preparation for split

[Aaron Tomlin]

On Thu 2022-02-10 11:18 +0000, Christophe Leroy wrote:
> > +#define MODULE_SECT_READ_SIZE (3 /* "0x", "\n" */ + (BITS_PER_LONG / 4))
>
> This is used only in sysfs.c, why move it to internal.h ?

Agreed. Since use is exclusive to kernel/module/sysfs.c it should be moved.

Kind regards,

-- 
Aaron Tomlin

[PATCH v5 03/13] module: Make internal.h more compliant

[Aaron Tomlin]

This patch will address the following warning and style violations
generated by ./scripts/checkpatch.pl:

  WARNING: Use #include <linux/module.h> instead of <asm/module.h>
  #10: FILE: kernel/module/internal.h:10:
  +#include <asm/module.h>

  CHECK: spaces preferred around that '-' (ctx:VxV)
  #18: FILE: kernel/module/internal.h:18:
  +#define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
						   ^
  CHECK: extern prototypes should be avoided in .h files
  #84: FILE: kernel/module/internal.h:84:
  +extern int mod_verify_sig(const void *mod, struct load_info *info);

Note: Fortunately, the multiple-include optimisation found in
include/linux/module.h will prevent duplication/or inclusion more than
once.

Fixes: f314dfea16a ("modsign: log module name in the event of an error")
Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 kernel/module/internal.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index 1a4b33ce9f5f..1cf5d6dabc97 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -6,7 +6,8 @@
  */
 
 #include <linux/elf.h>
-#include <asm/module.h>
+#include <linux/compiler.h>
+#include <linux/module.h>
 #include <linux/mutex.h>
 
 #ifndef ARCH_SHF_SMALL
@@ -14,7 +15,7 @@
 #endif
 
 /* If this is set, the section belongs in the init part of the module */
-#define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
+#define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG - 1))
 /* Maximum number of characters written by module_flags() */
 #define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
 #define MODULE_SECT_READ_SIZE (3 /* "0x", "\n" */ + (BITS_PER_LONG / 4))
@@ -55,7 +56,7 @@ struct load_info {
 	} index;
 };
 
-extern int mod_verify_sig(const void *mod, struct load_info *info);
+int mod_verify_sig(const void *mod, struct load_info *info);
 
 #ifdef CONFIG_MODULE_DECOMPRESS
 int module_decompress(struct load_info *info, const void *buf, size_t size);
-- 
2.34.1

Re: [PATCH v5 03/13] module: Make internal.h more compliant

[Christophe Leroy]

Le 09/02/2022 à 18:03, Aaron Tomlin a écrit :
> This patch will address the following warning and style violations
> generated by ./scripts/checkpatch.pl:
> 
>    WARNING: Use #include <linux/module.h> instead of <asm/module.h>
>    #10: FILE: kernel/module/internal.h:10:
>    +#include <asm/module.h>
> 
>    CHECK: spaces preferred around that '-' (ctx:VxV)
>    #18: FILE: kernel/module/internal.h:18:
>    +#define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
> 						   ^
>    CHECK: extern prototypes should be avoided in .h files
>    #84: FILE: kernel/module/internal.h:84:
>    +extern int mod_verify_sig(const void *mod, struct load_info *info);
> 
> Note: Fortunately, the multiple-include optimisation found in
> include/linux/module.h will prevent duplication/or inclusion more than
> once.
> 
> Fixes: f314dfea16a ("modsign: log module name in the event of an error")
> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   kernel/module/internal.h | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index 1a4b33ce9f5f..1cf5d6dabc97 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -6,7 +6,8 @@
>    */
>   
>   #include <linux/elf.h>
> -#include <asm/module.h>
> +#include <linux/compiler.h>
> +#include <linux/module.h>
>   #include <linux/mutex.h>
>   
>   #ifndef ARCH_SHF_SMALL
> @@ -14,7 +15,7 @@
>   #endif
>   
>   /* If this is set, the section belongs in the init part of the module */
> -#define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
> +#define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG - 1))
>   /* Maximum number of characters written by module_flags() */
>   #define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
>   #define MODULE_SECT_READ_SIZE (3 /* "0x", "\n" */ + (BITS_PER_LONG / 4))
> @@ -55,7 +56,7 @@ struct load_info {
>   	} index;
>   };
>   
> -extern int mod_verify_sig(const void *mod, struct load_info *info);
> +int mod_verify_sig(const void *mod, struct load_info *info);
>   
>   #ifdef CONFIG_MODULE_DECOMPRESS
>   int module_decompress(struct load_info *info, const void *buf, size_t size);

[PATCH v5 04/13] module: Move livepatch support to a separate file

[Aaron Tomlin]

No functional change.

This patch migrates livepatch support (i.e. used during module
add/or load and remove/or deletion) from core module code into
kernel/module/livepatch.c. At the moment it contains code to
persist Elf information about a given livepatch module, only.

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 include/linux/module.h    |   5 +-
 kernel/module/Makefile    |   3 ++
 kernel/module/internal.h  |  18 +++++++
 kernel/module/livepatch.c |  80 ++++++++++++++++++++++++++++++
 kernel/module/main.c      | 102 ++++----------------------------------
 5 files changed, 112 insertions(+), 96 deletions(-)
 create mode 100644 kernel/module/livepatch.c

diff --git a/include/linux/module.h b/include/linux/module.h
index 1e135fd5c076..680b31ff57fa 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -664,10 +664,7 @@ static inline bool module_requested_async_probing(struct module *module)
 }
 
 #ifdef CONFIG_LIVEPATCH
-static inline bool is_livepatch_module(struct module *mod)
-{
-	return mod->klp;
-}
+bool is_livepatch_module(struct module *mod);
 #else /* !CONFIG_LIVEPATCH */
 static inline bool is_livepatch_module(struct module *mod)
 {
diff --git a/kernel/module/Makefile b/kernel/module/Makefile
index 2902fc7d0ef1..ee20d864ad19 100644
--- a/kernel/module/Makefile
+++ b/kernel/module/Makefile
@@ -7,3 +7,6 @@ obj-$(CONFIG_MODULES) += main.o
 obj-$(CONFIG_MODULE_DECOMPRESS) += decompress.o
 obj-$(CONFIG_MODULE_SIG) += signing.o
 obj-$(CONFIG_MODULE_SIG_FORMAT) += signature.o
+ifdef CONFIG_MODULES
+obj-$(CONFIG_LIVEPATCH) += livepatch.o
+endif
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index 1cf5d6dabc97..d252e0af1c54 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -58,6 +58,24 @@ struct load_info {
 
 int mod_verify_sig(const void *mod, struct load_info *info);
 
+#ifdef CONFIG_LIVEPATCH
+int copy_module_elf(struct module *mod, struct load_info *info);
+void free_module_elf(struct module *mod);
+bool set_livepatch_module(struct module *mod);
+#else /* !CONFIG_LIVEPATCH */
+static inline int copy_module_elf(struct module *mod, struct load_info *info)
+{
+	return 0;
+}
+
+static inline bool set_livepatch_module(struct module *mod)
+{
+	return false;
+}
+
+static inline void free_module_elf(struct module *mod) { }
+#endif /* CONFIG_LIVEPATCH */
+
 #ifdef CONFIG_MODULE_DECOMPRESS
 int module_decompress(struct load_info *info, const void *buf, size_t size);
 void module_decompress_cleanup(struct load_info *info);
diff --git a/kernel/module/livepatch.c b/kernel/module/livepatch.c
new file mode 100644
index 000000000000..7e9cf530c3f0
--- /dev/null
+++ b/kernel/module/livepatch.c
@@ -0,0 +1,80 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Module livepatch support
+ *
+ * Copyright (C) 2016 Jessica Yu <jeyu@redhat.com>
+ */
+
+#include <linux/module.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+#include "internal.h"
+
+/*
+ * Persist Elf information about a module. Copy the Elf header,
+ * section header table, section string table, and symtab section
+ * index from info to mod->klp_info.
+ */
+int copy_module_elf(struct module *mod, struct load_info *info)
+{
+	unsigned int size, symndx;
+	int ret;
+
+	size = sizeof(*mod->klp_info);
+	mod->klp_info = kmalloc(size, GFP_KERNEL);
+	if (mod->klp_info == NULL)
+		return -ENOMEM;
+
+	/* Elf header */
+	size = sizeof(mod->klp_info->hdr);
+	memcpy(&mod->klp_info->hdr, info->hdr, size);
+
+	/* Elf section header table */
+	size = sizeof(*info->sechdrs) * info->hdr->e_shnum;
+	mod->klp_info->sechdrs = kmemdup(info->sechdrs, size, GFP_KERNEL);
+	if (mod->klp_info->sechdrs == NULL) {
+		ret = -ENOMEM;
+		goto free_info;
+	}
+
+	/* Elf section name string table */
+	size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
+	mod->klp_info->secstrings = kmemdup(info->secstrings, size, GFP_KERNEL);
+	if (mod->klp_info->secstrings == NULL) {
+		ret = -ENOMEM;
+		goto free_sechdrs;
+	}
+
+	/* Elf symbol section index */
+	symndx = info->index.sym;
+	mod->klp_info->symndx = symndx;
+
+	/*
+	 * For livepatch modules, core_kallsyms.symtab is a complete
+	 * copy of the original symbol table. Adjust sh_addr to point
+	 * to core_kallsyms.symtab since the copy of the symtab in module
+	 * init memory is freed at the end of do_init_module().
+	 */
+	mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long) mod->core_kallsyms.symtab;
+
+	return 0;
+
+free_sechdrs:
+	kfree(mod->klp_info->sechdrs);
+free_info:
+	kfree(mod->klp_info);
+	return ret;
+}
+
+void free_module_elf(struct module *mod)
+{
+	kfree(mod->klp_info->sechdrs);
+	kfree(mod->klp_info->secstrings);
+	kfree(mod->klp_info);
+}
+
+inline bool set_livepatch_module(struct module *mod)
+{
+	mod->klp = true;
+	return true;
+}
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 750e3ad28679..5f5bd7152b55 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2042,81 +2042,6 @@ static int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
 }
 #endif /*  CONFIG_STRICT_MODULE_RWX */
 
-#ifdef CONFIG_LIVEPATCH
-/*
- * Persist Elf information about a module. Copy the Elf header,
- * section header table, section string table, and symtab section
- * index from info to mod->klp_info.
- */
-static int copy_module_elf(struct module *mod, struct load_info *info)
-{
-	unsigned int size, symndx;
-	int ret;
-
-	size = sizeof(*mod->klp_info);
-	mod->klp_info = kmalloc(size, GFP_KERNEL);
-	if (mod->klp_info == NULL)
-		return -ENOMEM;
-
-	/* Elf header */
-	size = sizeof(mod->klp_info->hdr);
-	memcpy(&mod->klp_info->hdr, info->hdr, size);
-
-	/* Elf section header table */
-	size = sizeof(*info->sechdrs) * info->hdr->e_shnum;
-	mod->klp_info->sechdrs = kmemdup(info->sechdrs, size, GFP_KERNEL);
-	if (mod->klp_info->sechdrs == NULL) {
-		ret = -ENOMEM;
-		goto free_info;
-	}
-
-	/* Elf section name string table */
-	size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
-	mod->klp_info->secstrings = kmemdup(info->secstrings, size, GFP_KERNEL);
-	if (mod->klp_info->secstrings == NULL) {
-		ret = -ENOMEM;
-		goto free_sechdrs;
-	}
-
-	/* Elf symbol section index */
-	symndx = info->index.sym;
-	mod->klp_info->symndx = symndx;
-
-	/*
-	 * For livepatch modules, core_kallsyms.symtab is a complete
-	 * copy of the original symbol table. Adjust sh_addr to point
-	 * to core_kallsyms.symtab since the copy of the symtab in module
-	 * init memory is freed at the end of do_init_module().
-	 */
-	mod->klp_info->sechdrs[symndx].sh_addr = \
-		(unsigned long) mod->core_kallsyms.symtab;
-
-	return 0;
-
-free_sechdrs:
-	kfree(mod->klp_info->sechdrs);
-free_info:
-	kfree(mod->klp_info);
-	return ret;
-}
-
-static void free_module_elf(struct module *mod)
-{
-	kfree(mod->klp_info->sechdrs);
-	kfree(mod->klp_info->secstrings);
-	kfree(mod->klp_info);
-}
-#else /* !CONFIG_LIVEPATCH */
-static int copy_module_elf(struct module *mod, struct load_info *info)
-{
-	return 0;
-}
-
-static void free_module_elf(struct module *mod)
-{
-}
-#endif /* CONFIG_LIVEPATCH */
-
 void __weak module_memfree(void *module_region)
 {
 	/*
@@ -3091,30 +3016,23 @@ static int copy_chunked_from_user(void *dst, const void __user *usrc, unsigned l
 	return 0;
 }
 
-#ifdef CONFIG_LIVEPATCH
 static int check_modinfo_livepatch(struct module *mod, struct load_info *info)
 {
-	if (get_modinfo(info, "livepatch")) {
-		mod->klp = true;
+	if (!get_modinfo(info, "livepatch"))
+		/* Nothing more to do */
+		return 0;
+
+	if (set_livepatch_module(mod)) {
 		add_taint_module(mod, TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
 		pr_notice_once("%s: tainting kernel with TAINT_LIVEPATCH\n",
-			       mod->name);
-	}
-
-	return 0;
-}
-#else /* !CONFIG_LIVEPATCH */
-static int check_modinfo_livepatch(struct module *mod, struct load_info *info)
-{
-	if (get_modinfo(info, "livepatch")) {
-		pr_err("%s: module is marked as livepatch module, but livepatch support is disabled",
-		       mod->name);
-		return -ENOEXEC;
+				mod->name);
+		return 0;
 	}
 
-	return 0;
+	pr_err("%s: module is marked as livepatch module, but livepatch support is disabled",
+		mod->name);
+	return -ENOEXEC;
 }
-#endif /* CONFIG_LIVEPATCH */
 
 static void check_modinfo_retpoline(struct module *mod, struct load_info *info)
 {
-- 
2.34.1

Re: [PATCH v5 04/13] module: Move livepatch support to a separate file

[Christophe Leroy]

Le 09/02/2022 à 18:03, Aaron Tomlin a écrit :
> No functional change.
> 
> This patch migrates livepatch support (i.e. used during module
> add/or load and remove/or deletion) from core module code into
> kernel/module/livepatch.c. At the moment it contains code to
> persist Elf information about a given livepatch module, only.

   LD      .tmp_vmlinux.kallsyms1
powerpc64-linux-ld: kernel/livepatch/core.o: in function `klp_enable_patch':
(.text+0x1a0c): undefined reference to `is_livepatch_module'
powerpc64-linux-ld: kernel/module/main.o: in function `free_module':
main.c:(.text+0x50f4): undefined reference to `is_livepatch_module'
powerpc64-linux-ld: kernel/module/main.o: in function `load_module':
main.c:(.text+0x6f20): undefined reference to `is_livepatch_module'
powerpc64-linux-ld: main.c:(.text+0x8390): undefined reference to 
`is_livepatch_module'
powerpc64-linux-ld: main.c:(.text+0x8d6c): undefined reference to 
`is_livepatch_module'
make: *** [Makefile:1155 : vmlinux] Erreur 1


I don't understand why you are uninlining that so simple function.
Such a function is likely a single instruction in assembly, it is 
definitely not worth inlining.

Christophe

> 
> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> ---
>   include/linux/module.h    |   5 +-
>   kernel/module/Makefile    |   3 ++
>   kernel/module/internal.h  |  18 +++++++
>   kernel/module/livepatch.c |  80 ++++++++++++++++++++++++++++++
>   kernel/module/main.c      | 102 ++++----------------------------------
>   5 files changed, 112 insertions(+), 96 deletions(-)
>   create mode 100644 kernel/module/livepatch.c
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 1e135fd5c076..680b31ff57fa 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -664,10 +664,7 @@ static inline bool module_requested_async_probing(struct module *module)
>   }
>   
>   #ifdef CONFIG_LIVEPATCH
> -static inline bool is_livepatch_module(struct module *mod)
> -{
> -	return mod->klp;
> -}
> +bool is_livepatch_module(struct module *mod);
>   #else /* !CONFIG_LIVEPATCH */
>   static inline bool is_livepatch_module(struct module *mod)
>   {
> diff --git a/kernel/module/Makefile b/kernel/module/Makefile
> index 2902fc7d0ef1..ee20d864ad19 100644
> --- a/kernel/module/Makefile
> +++ b/kernel/module/Makefile
> @@ -7,3 +7,6 @@ obj-$(CONFIG_MODULES) += main.o
>   obj-$(CONFIG_MODULE_DECOMPRESS) += decompress.o
>   obj-$(CONFIG_MODULE_SIG) += signing.o
>   obj-$(CONFIG_MODULE_SIG_FORMAT) += signature.o
> +ifdef CONFIG_MODULES
> +obj-$(CONFIG_LIVEPATCH) += livepatch.o
> +endif
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index 1cf5d6dabc97..d252e0af1c54 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -58,6 +58,24 @@ struct load_info {
>   
>   int mod_verify_sig(const void *mod, struct load_info *info);
>   
> +#ifdef CONFIG_LIVEPATCH
> +int copy_module_elf(struct module *mod, struct load_info *info);
> +void free_module_elf(struct module *mod);
> +bool set_livepatch_module(struct module *mod);
> +#else /* !CONFIG_LIVEPATCH */
> +static inline int copy_module_elf(struct module *mod, struct load_info *info)
> +{
> +	return 0;
> +}
> +
> +static inline bool set_livepatch_module(struct module *mod)
> +{
> +	return false;
> +}
> +
> +static inline void free_module_elf(struct module *mod) { }
> +#endif /* CONFIG_LIVEPATCH */
> +
>   #ifdef CONFIG_MODULE_DECOMPRESS
>   int module_decompress(struct load_info *info, const void *buf, size_t size);
>   void module_decompress_cleanup(struct load_info *info);
> diff --git a/kernel/module/livepatch.c b/kernel/module/livepatch.c
> new file mode 100644
> index 000000000000..7e9cf530c3f0
> --- /dev/null
> +++ b/kernel/module/livepatch.c
> @@ -0,0 +1,80 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Module livepatch support
> + *
> + * Copyright (C) 2016 Jessica Yu <jeyu@redhat.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include "internal.h"
> +
> +/*
> + * Persist Elf information about a module. Copy the Elf header,
> + * section header table, section string table, and symtab section
> + * index from info to mod->klp_info.
> + */
> +int copy_module_elf(struct module *mod, struct load_info *info)
> +{
> +	unsigned int size, symndx;
> +	int ret;
> +
> +	size = sizeof(*mod->klp_info);
> +	mod->klp_info = kmalloc(size, GFP_KERNEL);
> +	if (mod->klp_info == NULL)
> +		return -ENOMEM;
> +
> +	/* Elf header */
> +	size = sizeof(mod->klp_info->hdr);
> +	memcpy(&mod->klp_info->hdr, info->hdr, size);
> +
> +	/* Elf section header table */
> +	size = sizeof(*info->sechdrs) * info->hdr->e_shnum;
> +	mod->klp_info->sechdrs = kmemdup(info->sechdrs, size, GFP_KERNEL);
> +	if (mod->klp_info->sechdrs == NULL) {
> +		ret = -ENOMEM;
> +		goto free_info;
> +	}
> +
> +	/* Elf section name string table */
> +	size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
> +	mod->klp_info->secstrings = kmemdup(info->secstrings, size, GFP_KERNEL);
> +	if (mod->klp_info->secstrings == NULL) {
> +		ret = -ENOMEM;
> +		goto free_sechdrs;
> +	}
> +
> +	/* Elf symbol section index */
> +	symndx = info->index.sym;
> +	mod->klp_info->symndx = symndx;
> +
> +	/*
> +	 * For livepatch modules, core_kallsyms.symtab is a complete
> +	 * copy of the original symbol table. Adjust sh_addr to point
> +	 * to core_kallsyms.symtab since the copy of the symtab in module
> +	 * init memory is freed at the end of do_init_module().
> +	 */
> +	mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long) mod->core_kallsyms.symtab;
> +
> +	return 0;
> +
> +free_sechdrs:
> +	kfree(mod->klp_info->sechdrs);
> +free_info:
> +	kfree(mod->klp_info);
> +	return ret;
> +}
> +
> +void free_module_elf(struct module *mod)
> +{
> +	kfree(mod->klp_info->sechdrs);
> +	kfree(mod->klp_info->secstrings);
> +	kfree(mod->klp_info);
> +}
> +
> +inline bool set_livepatch_module(struct module *mod)
> +{
> +	mod->klp = true;
> +	return true;
> +}
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 750e3ad28679..5f5bd7152b55 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2042,81 +2042,6 @@ static int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
>   }
>   #endif /*  CONFIG_STRICT_MODULE_RWX */
>   
> -#ifdef CONFIG_LIVEPATCH
> -/*
> - * Persist Elf information about a module. Copy the Elf header,
> - * section header table, section string table, and symtab section
> - * index from info to mod->klp_info.
> - */
> -static int copy_module_elf(struct module *mod, struct load_info *info)
> -{
> -	unsigned int size, symndx;
> -	int ret;
> -
> -	size = sizeof(*mod->klp_info);
> -	mod->klp_info = kmalloc(size, GFP_KERNEL);
> -	if (mod->klp_info == NULL)
> -		return -ENOMEM;
> -
> -	/* Elf header */
> -	size = sizeof(mod->klp_info->hdr);
> -	memcpy(&mod->klp_info->hdr, info->hdr, size);
> -
> -	/* Elf section header table */
> -	size = sizeof(*info->sechdrs) * info->hdr->e_shnum;
> -	mod->klp_info->sechdrs = kmemdup(info->sechdrs, size, GFP_KERNEL);
> -	if (mod->klp_info->sechdrs == NULL) {
> -		ret = -ENOMEM;
> -		goto free_info;
> -	}
> -
> -	/* Elf section name string table */
> -	size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
> -	mod->klp_info->secstrings = kmemdup(info->secstrings, size, GFP_KERNEL);
> -	if (mod->klp_info->secstrings == NULL) {
> -		ret = -ENOMEM;
> -		goto free_sechdrs;
> -	}
> -
> -	/* Elf symbol section index */
> -	symndx = info->index.sym;
> -	mod->klp_info->symndx = symndx;
> -
> -	/*
> -	 * For livepatch modules, core_kallsyms.symtab is a complete
> -	 * copy of the original symbol table. Adjust sh_addr to point
> -	 * to core_kallsyms.symtab since the copy of the symtab in module
> -	 * init memory is freed at the end of do_init_module().
> -	 */
> -	mod->klp_info->sechdrs[symndx].sh_addr = \
> -		(unsigned long) mod->core_kallsyms.symtab;
> -
> -	return 0;
> -
> -free_sechdrs:
> -	kfree(mod->klp_info->sechdrs);
> -free_info:
> -	kfree(mod->klp_info);
> -	return ret;
> -}
> -
> -static void free_module_elf(struct module *mod)
> -{
> -	kfree(mod->klp_info->sechdrs);
> -	kfree(mod->klp_info->secstrings);
> -	kfree(mod->klp_info);
> -}
> -#else /* !CONFIG_LIVEPATCH */
> -static int copy_module_elf(struct module *mod, struct load_info *info)
> -{
> -	return 0;
> -}
> -
> -static void free_module_elf(struct module *mod)
> -{
> -}
> -#endif /* CONFIG_LIVEPATCH */
> -
>   void __weak module_memfree(void *module_region)
>   {
>   	/*
> @@ -3091,30 +3016,23 @@ static int copy_chunked_from_user(void *dst, const void __user *usrc, unsigned l
>   	return 0;
>   }
>   
> -#ifdef CONFIG_LIVEPATCH
>   static int check_modinfo_livepatch(struct module *mod, struct load_info *info)
>   {
> -	if (get_modinfo(info, "livepatch")) {
> -		mod->klp = true;
> +	if (!get_modinfo(info, "livepatch"))
> +		/* Nothing more to do */
> +		return 0;
> +
> +	if (set_livepatch_module(mod)) {
>   		add_taint_module(mod, TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
>   		pr_notice_once("%s: tainting kernel with TAINT_LIVEPATCH\n",
> -			       mod->name);
> -	}
> -
> -	return 0;
> -}
> -#else /* !CONFIG_LIVEPATCH */
> -static int check_modinfo_livepatch(struct module *mod, struct load_info *info)
> -{
> -	if (get_modinfo(info, "livepatch")) {
> -		pr_err("%s: module is marked as livepatch module, but livepatch support is disabled",
> -		       mod->name);
> -		return -ENOEXEC;
> +				mod->name);
> +		return 0;
>   	}
>   
> -	return 0;
> +	pr_err("%s: module is marked as livepatch module, but livepatch support is disabled",
> +		mod->name);
> +	return -ENOEXEC;
>   }
> -#endif /* CONFIG_LIVEPATCH */
>   
>   static void check_modinfo_retpoline(struct module *mod, struct load_info *info)
>   {

Re: [PATCH v5 04/13] module: Move livepatch support to a separate file

[Christophe Leroy]

Le 09/02/2022 à 18:03, Aaron Tomlin a écrit :
> No functional change.
> 
> This patch migrates livepatch support (i.e. used during module
> add/or load and remove/or deletion) from core module code into
> kernel/module/livepatch.c. At the moment it contains code to
> persist Elf information about a given livepatch module, only.
> 
> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> ---
>   include/linux/module.h    |   5 +-
>   kernel/module/Makefile    |   3 ++
>   kernel/module/internal.h  |  18 +++++++
>   kernel/module/livepatch.c |  80 ++++++++++++++++++++++++++++++
>   kernel/module/main.c      | 102 ++++----------------------------------
>   5 files changed, 112 insertions(+), 96 deletions(-)
>   create mode 100644 kernel/module/livepatch.c
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 1e135fd5c076..680b31ff57fa 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -664,10 +664,7 @@ static inline bool module_requested_async_probing(struct module *module)
>   }
>   
>   #ifdef CONFIG_LIVEPATCH
> -static inline bool is_livepatch_module(struct module *mod)
> -{
> -	return mod->klp;
> -}
> +bool is_livepatch_module(struct module *mod);

This change is wrong, build fails with it because is_livepatch_module() 
is nowhere defined.

You could move is_livepatch_module() to include/linux/livepatch.h but as 
a separate patch.

>   #else /* !CONFIG_LIVEPATCH */
>   static inline bool is_livepatch_module(struct module *mod)
>   {
> diff --git a/kernel/module/Makefile b/kernel/module/Makefile
> index 2902fc7d0ef1..ee20d864ad19 100644
> --- a/kernel/module/Makefile
> +++ b/kernel/module/Makefile
> @@ -7,3 +7,6 @@ obj-$(CONFIG_MODULES) += main.o
>   obj-$(CONFIG_MODULE_DECOMPRESS) += decompress.o
>   obj-$(CONFIG_MODULE_SIG) += signing.o
>   obj-$(CONFIG_MODULE_SIG_FORMAT) += signature.o
> +ifdef CONFIG_MODULES

CONFIG_LIVEPATCH depends on CONFIG_MODULES so this ifdef is not needed 
(See kernel/livepatch/Kconfig)

> +obj-$(CONFIG_LIVEPATCH) += livepatch.o
> +endif
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index 1cf5d6dabc97..d252e0af1c54 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -58,6 +58,24 @@ struct load_info {
>   
>   int mod_verify_sig(const void *mod, struct load_info *info);
>   
> +#ifdef CONFIG_LIVEPATCH
> +int copy_module_elf(struct module *mod, struct load_info *info);
> +void free_module_elf(struct module *mod);
> +bool set_livepatch_module(struct module *mod);
> +#else /* !CONFIG_LIVEPATCH */
> +static inline int copy_module_elf(struct module *mod, struct load_info *info)
> +{
> +	return 0;
> +}
> +
> +static inline bool set_livepatch_module(struct module *mod)
> +{
> +	return false;
> +}
> +
> +static inline void free_module_elf(struct module *mod) { }
> +#endif /* CONFIG_LIVEPATCH */
> +
>   #ifdef CONFIG_MODULE_DECOMPRESS
>   int module_decompress(struct load_info *info, const void *buf, size_t size);
>   void module_decompress_cleanup(struct load_info *info);
> diff --git a/kernel/module/livepatch.c b/kernel/module/livepatch.c
> new file mode 100644
> index 000000000000..7e9cf530c3f0
> --- /dev/null
> +++ b/kernel/module/livepatch.c

Checkpatch reports the following:

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#80:
new file mode 100644

CHECK: Comparison to NULL could be written "!mod->klp_info"
#109: FILE: kernel/module/livepatch.c:25:
+	if (mod->klp_info == NULL)

CHECK: Comparison to NULL could be written "!mod->klp_info->sechdrs"
#119: FILE: kernel/module/livepatch.c:35:
+	if (mod->klp_info->sechdrs == NULL) {

CHECK: Comparison to NULL could be written "!mod->klp_info->secstrings"
#127: FILE: kernel/module/livepatch.c:43:
+	if (mod->klp_info->secstrings == NULL) {

CHECK: No space is necessary after a cast
#142: FILE: kernel/module/livepatch.c:58:
+	mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long) 
mod->core_kallsyms.symtab;


> +inline bool set_livepatch_module(struct module *mod)

'inline' keyword is pointless here, as far as this function is in a .c 
and is not static, it won't be inlined.

Given how simple this function is, it should be a 'static inline' in 
internal.c

> +{
> +	mod->klp = true;
> +	return true;
> +}



> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 750e3ad28679..5f5bd7152b55 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c

> @@ -3091,30 +3016,23 @@ static int copy_chunked_from_user(void *dst, const void __user *usrc, unsigned l
>   	return 0;
>   }
>   
> -#ifdef CONFIG_LIVEPATCH
>   static int check_modinfo_livepatch(struct module *mod, struct load_info *info)
>   {
> -	if (get_modinfo(info, "livepatch")) {
> -		mod->klp = true;
> +	if (!get_modinfo(info, "livepatch"))
> +		/* Nothing more to do */
> +		return 0;
> +
> +	if (set_livepatch_module(mod)) {
>   		add_taint_module(mod, TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
>   		pr_notice_once("%s: tainting kernel with TAINT_LIVEPATCH\n",
> -			       mod->name);
> -	}
> -
> -	return 0;
> -}
> -#else /* !CONFIG_LIVEPATCH */
> -static int check_modinfo_livepatch(struct module *mod, struct load_info *info)
> -{
> -	if (get_modinfo(info, "livepatch")) {
> -		pr_err("%s: module is marked as livepatch module, but livepatch support is disabled",
> -		       mod->name);
> -		return -ENOEXEC;
> +				mod->name);

This change seems wrong, mod->name must remain aligned to open parenthesis.


> +		return 0;
>   	}
>   
> -	return 0;
> +	pr_err("%s: module is marked as livepatch module, but livepatch support is disabled",
> +		mod->name);

CHECK: Alignment should match open parenthesis
#285: FILE: kernel/module/main.c:3033:
+	pr_err("%s: module is marked as livepatch module, but livepatch 
support is disabled",
+		mod->name);

> +	return -ENOEXEC;
>   }
> -#endif /* CONFIG_LIVEPATCH */
>   
>   static void check_modinfo_retpoline(struct module *mod, struct load_info *info)
>   {

Re: [PATCH v5 04/13] module: Move livepatch support to a separate file

[Aaron Tomlin]

On Thu 2022-02-10 11:44 +0000, Christophe Leroy wrote:
> This change is wrong, build fails with it because is_livepatch_module()
> is nowhere defined.

Yes, sorry about that. This was an omission/or oversight during the rebase
attempt.

> You could move is_livepatch_module() to include/linux/livepatch.h but as
> a separate patch.

Fair enough. Albeit, I'd prefer to revert and keep is_livepatch_module()
in include/linux/module.h - this is likely the best solution.
Note: set_livepatch_module() will remain for internal use only.

> >   #else /* !CONFIG_LIVEPATCH */
> >   static inline bool is_livepatch_module(struct module *mod)
> >   {
> > diff --git a/kernel/module/Makefile b/kernel/module/Makefile
> > index 2902fc7d0ef1..ee20d864ad19 100644
> > --- a/kernel/module/Makefile
> > +++ b/kernel/module/Makefile
> > @@ -7,3 +7,6 @@ obj-$(CONFIG_MODULES) += main.o
> >   obj-$(CONFIG_MODULE_DECOMPRESS) += decompress.o
> >   obj-$(CONFIG_MODULE_SIG) += signing.o
> >   obj-$(CONFIG_MODULE_SIG_FORMAT) += signature.o
> > +ifdef CONFIG_MODULES
>
> CONFIG_LIVEPATCH depends on CONFIG_MODULES so this ifdef is not needed

Agreed.

> Checkpatch reports the following:
>
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #80:
> new file mode 100644
>
> CHECK: Comparison to NULL could be written "!mod->klp_info"
> #109: FILE: kernel/module/livepatch.c:25:
> +    if (mod->klp_info == NULL)
>
> CHECK: Comparison to NULL could be written "!mod->klp_info->sechdrs"
> #119: FILE: kernel/module/livepatch.c:35:
> +    if (mod->klp_info->sechdrs == NULL) {
>
> CHECK: Comparison to NULL could be written "!mod->klp_info->secstrings"
> #127: FILE: kernel/module/livepatch.c:43:
> +    if (mod->klp_info->secstrings == NULL) {
>
> CHECK: No space is necessary after a cast
> #142: FILE: kernel/module/livepatch.c:58:
> +    mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long)
> mod->core_kallsyms.symtab;

Ok.

> Given how simple this function is, it should be a 'static inline' in
> internal.c

Agreed.

> CHECK: Alignment should match open parenthesis
> #285: FILE: kernel/module/main.c:3033:
> +    pr_err("%s: module is marked as livepatch module, but livepatch
> support is disabled",
> +        mod->name);

Fair enough.


Kind regards,

-- 
Aaron Tomlin

[PATCH v5 05/13] module: Move latched RB-tree support to a separate file

[Aaron Tomlin]

No functional change.

This patch migrates module latched RB-tree support
(e.g. see __module_address()) from core module code
into kernel/module/tree_lookup.c.

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 include/linux/module.h      |   4 +-
 kernel/module/Makefile      |   1 +
 kernel/module/internal.h    |  34 ++++++++++
 kernel/module/main.c        | 129 +-----------------------------------
 kernel/module/tree_lookup.c | 109 ++++++++++++++++++++++++++++++
 5 files changed, 148 insertions(+), 129 deletions(-)
 create mode 100644 kernel/module/tree_lookup.c

diff --git a/include/linux/module.h b/include/linux/module.h
index 680b31ff57fa..fd6161d78127 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -342,9 +342,9 @@ struct module_layout {
 #ifdef CONFIG_MODULES_TREE_LOOKUP
 /* Only touch one cacheline for common rbtree-for-core-layout case. */
 #define __module_layout_align ____cacheline_aligned
-#else
+#else /* !CONFIG_MODULES_TREE_LOOKUP */
 #define __module_layout_align
-#endif
+#endif /* CONFIG_MODULES_TREE_LOOKUP */
 
 struct mod_kallsyms {
 	Elf_Sym *symtab;
diff --git a/kernel/module/Makefile b/kernel/module/Makefile
index ee20d864ad19..fc6d7a053a62 100644
--- a/kernel/module/Makefile
+++ b/kernel/module/Makefile
@@ -9,4 +9,5 @@ obj-$(CONFIG_MODULE_SIG) += signing.o
 obj-$(CONFIG_MODULE_SIG_FORMAT) += signature.o
 ifdef CONFIG_MODULES
 obj-$(CONFIG_LIVEPATCH) += livepatch.o
+obj-$(CONFIG_MODULES_TREE_LOOKUP) += tree_lookup.o
 endif
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index d252e0af1c54..08b6be037b72 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -9,6 +9,7 @@
 #include <linux/compiler.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/rculist.h>
 
 #ifndef ARCH_SHF_SMALL
 #define ARCH_SHF_SMALL 0
@@ -90,3 +91,36 @@ static inline void module_decompress_cleanup(struct load_info *info)
 {
 }
 #endif
+
+#ifdef CONFIG_MODULES_TREE_LOOKUP
+struct mod_tree_root {
+	struct latch_tree_root root;
+	unsigned long addr_min;
+	unsigned long addr_max;
+};
+
+extern struct mod_tree_root mod_tree;
+
+void mod_tree_insert(struct module *mod);
+void mod_tree_remove_init(struct module *mod);
+void mod_tree_remove(struct module *mod);
+struct module *mod_find(unsigned long addr);
+#else /* !CONFIG_MODULES_TREE_LOOKUP */
+static unsigned long module_addr_min = -1UL, module_addr_max;
+
+static void mod_tree_insert(struct module *mod) { }
+static void mod_tree_remove_init(struct module *mod) { }
+static void mod_tree_remove(struct module *mod) { }
+static struct module *mod_find(unsigned long addr)
+{
+	struct module *mod;
+
+	list_for_each_entry_rcu(mod, &modules, list,
+				lockdep_is_held(&module_mutex)) {
+		if (within_module(addr, mod))
+			return mod;
+	}
+
+	return NULL;
+}
+#endif /* CONFIG_MODULES_TREE_LOOKUP */
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 5f5bd7152b55..f733a719c65d 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -90,138 +90,13 @@ static DECLARE_WORK(init_free_wq, do_free_init);
 static LLIST_HEAD(init_free_list);
 
 #ifdef CONFIG_MODULES_TREE_LOOKUP
-
-/*
- * Use a latched RB-tree for __module_address(); this allows us to use
- * RCU-sched lookups of the address from any context.
- *
- * This is conditional on PERF_EVENTS || TRACING because those can really hit
- * __module_address() hard by doing a lot of stack unwinding; potentially from
- * NMI context.
- */
-
-static __always_inline unsigned long __mod_tree_val(struct latch_tree_node *n)
-{
-	struct module_layout *layout = container_of(n, struct module_layout, mtn.node);
-
-	return (unsigned long)layout->base;
-}
-
-static __always_inline unsigned long __mod_tree_size(struct latch_tree_node *n)
-{
-	struct module_layout *layout = container_of(n, struct module_layout, mtn.node);
-
-	return (unsigned long)layout->size;
-}
-
-static __always_inline bool
-mod_tree_less(struct latch_tree_node *a, struct latch_tree_node *b)
-{
-	return __mod_tree_val(a) < __mod_tree_val(b);
-}
-
-static __always_inline int
-mod_tree_comp(void *key, struct latch_tree_node *n)
-{
-	unsigned long val = (unsigned long)key;
-	unsigned long start, end;
-
-	start = __mod_tree_val(n);
-	if (val < start)
-		return -1;
-
-	end = start + __mod_tree_size(n);
-	if (val >= end)
-		return 1;
-
-	return 0;
-}
-
-static const struct latch_tree_ops mod_tree_ops = {
-	.less = mod_tree_less,
-	.comp = mod_tree_comp,
-};
-
-static struct mod_tree_root {
-	struct latch_tree_root root;
-	unsigned long addr_min;
-	unsigned long addr_max;
-} mod_tree __cacheline_aligned = {
+struct mod_tree_root mod_tree __cacheline_aligned = {
 	.addr_min = -1UL,
 };
 
 #define module_addr_min mod_tree.addr_min
 #define module_addr_max mod_tree.addr_max
-
-static noinline void __mod_tree_insert(struct mod_tree_node *node)
-{
-	latch_tree_insert(&node->node, &mod_tree.root, &mod_tree_ops);
-}
-
-static void __mod_tree_remove(struct mod_tree_node *node)
-{
-	latch_tree_erase(&node->node, &mod_tree.root, &mod_tree_ops);
-}
-
-/*
- * These modifications: insert, remove_init and remove; are serialized by the
- * module_mutex.
- */
-static void mod_tree_insert(struct module *mod)
-{
-	mod->core_layout.mtn.mod = mod;
-	mod->init_layout.mtn.mod = mod;
-
-	__mod_tree_insert(&mod->core_layout.mtn);
-	if (mod->init_layout.size)
-		__mod_tree_insert(&mod->init_layout.mtn);
-}
-
-static void mod_tree_remove_init(struct module *mod)
-{
-	if (mod->init_layout.size)
-		__mod_tree_remove(&mod->init_layout.mtn);
-}
-
-static void mod_tree_remove(struct module *mod)
-{
-	__mod_tree_remove(&mod->core_layout.mtn);
-	mod_tree_remove_init(mod);
-}
-
-static struct module *mod_find(unsigned long addr)
-{
-	struct latch_tree_node *ltn;
-
-	ltn = latch_tree_find((void *)addr, &mod_tree.root, &mod_tree_ops);
-	if (!ltn)
-		return NULL;
-
-	return container_of(ltn, struct mod_tree_node, node)->mod;
-}
-
-#else /* MODULES_TREE_LOOKUP */
-
-static unsigned long module_addr_min = -1UL, module_addr_max = 0;
-
-static void mod_tree_insert(struct module *mod) { }
-static void mod_tree_remove_init(struct module *mod) { }
-static void mod_tree_remove(struct module *mod) { }
-
-static struct module *mod_find(unsigned long addr)
-{
-	struct module *mod;
-
-	list_for_each_entry_rcu(mod, &modules, list,
-				lockdep_is_held(&module_mutex)) {
-		if (within_module(addr, mod))
-			return mod;
-	}
-
-	return NULL;
-}
-
-#endif /* MODULES_TREE_LOOKUP */
+#endif
 
 /*
  * Bounds of module text, for speeding up __module_address.
diff --git a/kernel/module/tree_lookup.c b/kernel/module/tree_lookup.c
new file mode 100644
index 000000000000..037d6eb2f56f
--- /dev/null
+++ b/kernel/module/tree_lookup.c
@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Modules tree lookup
+ *
+ * Copyright (C) 2015 Peter Zijlstra
+ * Copyright (C) 2015 Rusty Russell
+ */
+
+#include <linux/module.h>
+#include <linux/rbtree_latch.h>
+#include "internal.h"
+
+/*
+ * Use a latched RB-tree for __module_address(); this allows us to use
+ * RCU-sched lookups of the address from any context.
+ *
+ * This is conditional on PERF_EVENTS || TRACING because those can really hit
+ * __module_address() hard by doing a lot of stack unwinding; potentially from
+ * NMI context.
+ */
+
+__always_inline unsigned long __mod_tree_val(struct latch_tree_node *n)
+{
+	struct module_layout *layout = container_of(n, struct module_layout, mtn.node);
+
+	return (unsigned long)layout->base;
+}
+
+__always_inline unsigned long __mod_tree_size(struct latch_tree_node *n)
+{
+	struct module_layout *layout = container_of(n, struct module_layout, mtn.node);
+
+	return (unsigned long)layout->size;
+}
+
+__always_inline bool
+mod_tree_less(struct latch_tree_node *a, struct latch_tree_node *b)
+{
+	return __mod_tree_val(a) < __mod_tree_val(b);
+}
+
+__always_inline int
+mod_tree_comp(void *key, struct latch_tree_node *n)
+{
+	unsigned long val = (unsigned long)key;
+	unsigned long start, end;
+
+	start = __mod_tree_val(n);
+	if (val < start)
+		return -1;
+
+	end = start + __mod_tree_size(n);
+	if (val >= end)
+		return 1;
+
+	return 0;
+}
+
+const struct latch_tree_ops mod_tree_ops = {
+	.less = mod_tree_less,
+	.comp = mod_tree_comp,
+};
+
+static noinline void __mod_tree_insert(struct mod_tree_node *node)
+{
+	latch_tree_insert(&node->node, &mod_tree.root, &mod_tree_ops);
+}
+
+static void __mod_tree_remove(struct mod_tree_node *node)
+{
+	latch_tree_erase(&node->node, &mod_tree.root, &mod_tree_ops);
+}
+
+/*
+ * These modifications: insert, remove_init and remove; are serialized by the
+ * module_mutex.
+ */
+void mod_tree_insert(struct module *mod)
+{
+	mod->core_layout.mtn.mod = mod;
+	mod->init_layout.mtn.mod = mod;
+
+	__mod_tree_insert(&mod->core_layout.mtn);
+	if (mod->init_layout.size)
+		__mod_tree_insert(&mod->init_layout.mtn);
+}
+
+void mod_tree_remove_init(struct module *mod)
+{
+	if (mod->init_layout.size)
+		__mod_tree_remove(&mod->init_layout.mtn);
+}
+
+void mod_tree_remove(struct module *mod)
+{
+	__mod_tree_remove(&mod->core_layout.mtn);
+	mod_tree_remove_init(mod);
+}
+
+struct module *mod_find(unsigned long addr)
+{
+	struct latch_tree_node *ltn;
+
+	ltn = latch_tree_find((void *)addr, &mod_tree.root, &mod_tree_ops);
+	if (!ltn)
+		return NULL;
+
+	return container_of(ltn, struct mod_tree_node, node)->mod;
+}
-- 
2.34.1

Re: [PATCH v5 05/13] module: Move latched RB-tree support to a separate file

[Christophe Leroy]

Le 09/02/2022 à 18:03, Aaron Tomlin a écrit :
> No functional change.
> 
> This patch migrates module latched RB-tree support
> (e.g. see __module_address()) from core module code
> into kernel/module/tree_lookup.c.
> 
> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> ---
>   include/linux/module.h      |   4 +-
>   kernel/module/Makefile      |   1 +
>   kernel/module/internal.h    |  34 ++++++++++
>   kernel/module/main.c        | 129 +-----------------------------------
>   kernel/module/tree_lookup.c | 109 ++++++++++++++++++++++++++++++
>   5 files changed, 148 insertions(+), 129 deletions(-)
>   create mode 100644 kernel/module/tree_lookup.c
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 680b31ff57fa..fd6161d78127 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -342,9 +342,9 @@ struct module_layout {
>   #ifdef CONFIG_MODULES_TREE_LOOKUP
>   /* Only touch one cacheline for common rbtree-for-core-layout case. */
>   #define __module_layout_align ____cacheline_aligned
> -#else
> +#else /* !CONFIG_MODULES_TREE_LOOKUP */
>   #define __module_layout_align
> -#endif
> +#endif /* CONFIG_MODULES_TREE_LOOKUP */

What's the added value of those two changes ? That's a five lines #ifdef 
block without any other nested #ifdef.

Commenting an #else / #endif is only usefull when the block is more than 
one screen or when there are nested #ifdef inside the block.

Please keep changes at the minimum.

>   
>   struct mod_kallsyms {
>   	Elf_Sym *symtab;
> diff --git a/kernel/module/Makefile b/kernel/module/Makefile
> index ee20d864ad19..fc6d7a053a62 100644
> --- a/kernel/module/Makefile
> +++ b/kernel/module/Makefile
> @@ -9,4 +9,5 @@ obj-$(CONFIG_MODULE_SIG) += signing.o
>   obj-$(CONFIG_MODULE_SIG_FORMAT) += signature.o
>   ifdef CONFIG_MODULES
>   obj-$(CONFIG_LIVEPATCH) += livepatch.o
> +obj-$(CONFIG_MODULES_TREE_LOOKUP) += tree_lookup.o
>   endif
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index d252e0af1c54..08b6be037b72 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -9,6 +9,7 @@
>   #include <linux/compiler.h>
>   #include <linux/module.h>
>   #include <linux/mutex.h>
> +#include <linux/rculist.h>
>   
>   #ifndef ARCH_SHF_SMALL
>   #define ARCH_SHF_SMALL 0
> @@ -90,3 +91,36 @@ static inline void module_decompress_cleanup(struct load_info *info)
>   {
>   }
>   #endif
> +
> +#ifdef CONFIG_MODULES_TREE_LOOKUP
> +struct mod_tree_root {
> +	struct latch_tree_root root;
> +	unsigned long addr_min;
> +	unsigned long addr_max;
> +};
> +
> +extern struct mod_tree_root mod_tree;
> +
> +void mod_tree_insert(struct module *mod);
> +void mod_tree_remove_init(struct module *mod);
> +void mod_tree_remove(struct module *mod);
> +struct module *mod_find(unsigned long addr);
> +#else /* !CONFIG_MODULES_TREE_LOOKUP */
> +static unsigned long module_addr_min = -1UL, module_addr_max;

This is wrong to put that in a .h.

By chance module_addr_min re used only in main.c but if they were used 
in another file you would get two independant versions of it.

So leave it in main.c, anyway it's going away with my series.

> +
> +static void mod_tree_insert(struct module *mod) { }
> +static void mod_tree_remove_init(struct module *mod) { }
> +static void mod_tree_remove(struct module *mod) { }
> +static struct module *mod_find(unsigned long addr)

Also keep mod_find() in main.c, or make it a 'static inline'. Otherwise 
it will be duplicated in every file including internal.h

> +{
> +	struct module *mod;
> +
> +	list_for_each_entry_rcu(mod, &modules, list,
> +				lockdep_is_held(&module_mutex)) {
> +		if (within_module(addr, mod))
> +			return mod;
> +	}
> +
> +	return NULL;
> +}
> +#endif /* CONFIG_MODULES_TREE_LOOKUP */
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 5f5bd7152b55..f733a719c65d 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -90,138 +90,13 @@ static DECLARE_WORK(init_free_wq, do_free_init);
>   static LLIST_HEAD(init_free_list);
>   
>   #ifdef CONFIG_MODULES_TREE_LOOKUP
> -
> -/*
> - * Use a latched RB-tree for __module_address(); this allows us to use
> - * RCU-sched lookups of the address from any context.
> - *
> - * This is conditional on PERF_EVENTS || TRACING because those can really hit
> - * __module_address() hard by doing a lot of stack unwinding; potentially from
> - * NMI context.
> - */
> -
> -static __always_inline unsigned long __mod_tree_val(struct latch_tree_node *n)
> -{
> -	struct module_layout *layout = container_of(n, struct module_layout, mtn.node);
> -
> -	return (unsigned long)layout->base;
> -}
> -
> -static __always_inline unsigned long __mod_tree_size(struct latch_tree_node *n)
> -{
> -	struct module_layout *layout = container_of(n, struct module_layout, mtn.node);
> -
> -	return (unsigned long)layout->size;
> -}
> -
> -static __always_inline bool
> -mod_tree_less(struct latch_tree_node *a, struct latch_tree_node *b)
> -{
> -	return __mod_tree_val(a) < __mod_tree_val(b);
> -}
> -
> -static __always_inline int
> -mod_tree_comp(void *key, struct latch_tree_node *n)
> -{
> -	unsigned long val = (unsigned long)key;
> -	unsigned long start, end;
> -
> -	start = __mod_tree_val(n);
> -	if (val < start)
> -		return -1;
> -
> -	end = start + __mod_tree_size(n);
> -	if (val >= end)
> -		return 1;
> -
> -	return 0;
> -}
> -
> -static const struct latch_tree_ops mod_tree_ops = {
> -	.less = mod_tree_less,
> -	.comp = mod_tree_comp,
> -};
> -
> -static struct mod_tree_root {
> -	struct latch_tree_root root;
> -	unsigned long addr_min;
> -	unsigned long addr_max;
> -} mod_tree __cacheline_aligned = {
> +struct mod_tree_root mod_tree __cacheline_aligned = {
>   	.addr_min = -1UL,
>   };
>   
>   #define module_addr_min mod_tree.addr_min
>   #define module_addr_max mod_tree.addr_max
> -
> -static noinline void __mod_tree_insert(struct mod_tree_node *node)
> -{
> -	latch_tree_insert(&node->node, &mod_tree.root, &mod_tree_ops);
> -}
> -
> -static void __mod_tree_remove(struct mod_tree_node *node)
> -{
> -	latch_tree_erase(&node->node, &mod_tree.root, &mod_tree_ops);
> -}
> -
> -/*
> - * These modifications: insert, remove_init and remove; are serialized by the
> - * module_mutex.
> - */
> -static void mod_tree_insert(struct module *mod)
> -{
> -	mod->core_layout.mtn.mod = mod;
> -	mod->init_layout.mtn.mod = mod;
> -
> -	__mod_tree_insert(&mod->core_layout.mtn);
> -	if (mod->init_layout.size)
> -		__mod_tree_insert(&mod->init_layout.mtn);
> -}
> -
> -static void mod_tree_remove_init(struct module *mod)
> -{
> -	if (mod->init_layout.size)
> -		__mod_tree_remove(&mod->init_layout.mtn);
> -}
> -
> -static void mod_tree_remove(struct module *mod)
> -{
> -	__mod_tree_remove(&mod->core_layout.mtn);
> -	mod_tree_remove_init(mod);
> -}
> -
> -static struct module *mod_find(unsigned long addr)
> -{
> -	struct latch_tree_node *ltn;
> -
> -	ltn = latch_tree_find((void *)addr, &mod_tree.root, &mod_tree_ops);
> -	if (!ltn)
> -		return NULL;
> -
> -	return container_of(ltn, struct mod_tree_node, node)->mod;
> -}
> -
> -#else /* MODULES_TREE_LOOKUP */
> -
> -static unsigned long module_addr_min = -1UL, module_addr_max = 0;
> -
> -static void mod_tree_insert(struct module *mod) { }
> -static void mod_tree_remove_init(struct module *mod) { }
> -static void mod_tree_remove(struct module *mod) { }
> -
> -static struct module *mod_find(unsigned long addr)
> -{
> -	struct module *mod;
> -
> -	list_for_each_entry_rcu(mod, &modules, list,
> -				lockdep_is_held(&module_mutex)) {
> -		if (within_module(addr, mod))
> -			return mod;
> -	}
> -
> -	return NULL;
> -}
> -
> -#endif /* MODULES_TREE_LOOKUP */
> +#endif
>   
>   /*
>    * Bounds of module text, for speeding up __module_address.
> diff --git a/kernel/module/tree_lookup.c b/kernel/module/tree_lookup.c
> new file mode 100644
> index 000000000000..037d6eb2f56f
> --- /dev/null
> +++ b/kernel/module/tree_lookup.c
> @@ -0,0 +1,109 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Modules tree lookup
> + *
> + * Copyright (C) 2015 Peter Zijlstra
> + * Copyright (C) 2015 Rusty Russell
> + */
> +
> +#include <linux/module.h>
> +#include <linux/rbtree_latch.h>
> +#include "internal.h"
> +
> +/*
> + * Use a latched RB-tree for __module_address(); this allows us to use
> + * RCU-sched lookups of the address from any context.
> + *
> + * This is conditional on PERF_EVENTS || TRACING because those can really hit
> + * __module_address() hard by doing a lot of stack unwinding; potentially from
> + * NMI context.
> + */
> +
> +__always_inline unsigned long __mod_tree_val(struct latch_tree_node *n)

Should be static.


> +{
> +	struct module_layout *layout = container_of(n, struct module_layout, mtn.node);
> +
> +	return (unsigned long)layout->base;
> +}
> +
> +__always_inline unsigned long __mod_tree_size(struct latch_tree_node *n)

Should be static.


> +{
> +	struct module_layout *layout = container_of(n, struct module_layout, mtn.node);
> +
> +	return (unsigned long)layout->size;
> +}
> +
> +__always_inline bool
> +mod_tree_less(struct latch_tree_node *a, struct latch_tree_node *b)

Should be static.


> +{
> +	return __mod_tree_val(a) < __mod_tree_val(b);
> +}
> +
> +__always_inline int
> +mod_tree_comp(void *key, struct latch_tree_node *n)

Should be static.

> +{
> +	unsigned long val = (unsigned long)key;
> +	unsigned long start, end;
> +
> +	start = __mod_tree_val(n);
> +	if (val < start)
> +		return -1;
> +
> +	end = start + __mod_tree_size(n);
> +	if (val >= end)
> +		return 1;
> +
> +	return 0;
> +}
> +
> +const struct latch_tree_ops mod_tree_ops = {
> +	.less = mod_tree_less,
> +	.comp = mod_tree_comp,
> +};

Should be static.


> +
> +static noinline void __mod_tree_insert(struct mod_tree_node *node)
> +{
> +	latch_tree_insert(&node->node, &mod_tree.root, &mod_tree_ops);
> +}
> +
> +static void __mod_tree_remove(struct mod_tree_node *node)
> +{
> +	latch_tree_erase(&node->node, &mod_tree.root, &mod_tree_ops);
> +}
> +
> +/*
> + * These modifications: insert, remove_init and remove; are serialized by the
> + * module_mutex.
> + */
> +void mod_tree_insert(struct module *mod)
> +{
> +	mod->core_layout.mtn.mod = mod;
> +	mod->init_layout.mtn.mod = mod;
> +
> +	__mod_tree_insert(&mod->core_layout.mtn);
> +	if (mod->init_layout.size)
> +		__mod_tree_insert(&mod->init_layout.mtn);
> +}
> +
> +void mod_tree_remove_init(struct module *mod)
> +{
> +	if (mod->init_layout.size)
> +		__mod_tree_remove(&mod->init_layout.mtn);
> +}
> +
> +void mod_tree_remove(struct module *mod)
> +{
> +	__mod_tree_remove(&mod->core_layout.mtn);
> +	mod_tree_remove_init(mod);
> +}
> +
> +struct module *mod_find(unsigned long addr)
> +{
> +	struct latch_tree_node *ltn;
> +
> +	ltn = latch_tree_find((void *)addr, &mod_tree.root, &mod_tree_ops);
> +	if (!ltn)
> +		return NULL;
> +
> +	return container_of(ltn, struct mod_tree_node, node)->mod;
> +}

Re: [PATCH v5 05/13] module: Move latched RB-tree support to a separate file

[Aaron Tomlin]

On Thu 2022-02-10 12:03 +0000, Christophe Leroy wrote:
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 680b31ff57fa..fd6161d78127 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -342,9 +342,9 @@ struct module_layout {
> >   #ifdef CONFIG_MODULES_TREE_LOOKUP
> >   /* Only touch one cacheline for common rbtree-for-core-layout case. */
> >   #define __module_layout_align ____cacheline_aligned
> > -#else
> > +#else /* !CONFIG_MODULES_TREE_LOOKUP */
> >   #define __module_layout_align
> > -#endif
> > +#endif /* CONFIG_MODULES_TREE_LOOKUP */
> Commenting an #else / #endif is only usefull when the block is more than
> one screen or when there are nested #ifdef inside the block.

For me, this is a personal style preference. That being said, fair enough.

> > +#else /* !CONFIG_MODULES_TREE_LOOKUP */
> > +static unsigned long module_addr_min = -1UL, module_addr_max;
>
> This is wrong to put that in a .h.
>

I understand. This was an oversight. I'll move this to kernel/module/main.c
in preparation for your work.

> > +static void mod_tree_insert(struct module *mod) { }
> > +static void mod_tree_remove_init(struct module *mod) { }
> > +static void mod_tree_remove(struct module *mod) { }
> > +static struct module *mod_find(unsigned long addr)
>
> Also keep mod_find() in main.c, or make it a 'static inline'. Otherwise
> it will be duplicated in every file including internal.h

Agreed. This too was an oversight. I'll use the 'inline' keyword here.

> > diff --git a/kernel/module/tree_lookup.c b/kernel/module/tree_lookup.c
> > new file mode 100644
> > index 000000000000..037d6eb2f56f
> > --- /dev/null
> > +++ b/kernel/module/tree_lookup.c
> > @@ -0,0 +1,109 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Modules tree lookup
> > + *
> > + * Copyright (C) 2015 Peter Zijlstra
> > + * Copyright (C) 2015 Rusty Russell
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/rbtree_latch.h>
> > +#include "internal.h"
> > +
> > +/*
> > + * Use a latched RB-tree for __module_address(); this allows us to use
> > + * RCU-sched lookups of the address from any context.
> > + *
> > + * This is conditional on PERF_EVENTS || TRACING because those can really hit
> > + * __module_address() hard by doing a lot of stack unwinding; potentially from
> > + * NMI context.
> > + */
> > +
> > +__always_inline unsigned long __mod_tree_val(struct latch_tree_node *n)
>
> Should be static.
> > +__always_inline unsigned long __mod_tree_size(struct latch_tree_node *n)
>
> Should be static.
> > +__always_inline bool
> > +mod_tree_less(struct latch_tree_node *a, struct latch_tree_node *b)
>
> Should be static.
>
>
> > +__always_inline int
> > +mod_tree_comp(void *key, struct latch_tree_node *n)
>
> Should be static.
>
> > +const struct latch_tree_ops mod_tree_ops = {
> > +    .less = mod_tree_less,
> > +    .comp = mod_tree_comp,
> > +};
>
> Should be static.

Agreed. Only used in kernel/module/tree_lookup.c.


-- 
Aaron Tomlin

[PATCH v5 06/13] module: Move strict rwx support to a separate file

[Aaron Tomlin]

No functional change.

This patch migrates code that makes module text
and rodata memory read-only and non-text memory
non-executable from core module code into
kernel/module/strict_rwx.c.

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 kernel/module/Makefile     |  1 +
 kernel/module/internal.h   | 38 +++++++++++++++
 kernel/module/main.c       | 97 +-------------------------------------
 kernel/module/strict_rwx.c | 84 +++++++++++++++++++++++++++++++++
 4 files changed, 124 insertions(+), 96 deletions(-)
 create mode 100644 kernel/module/strict_rwx.c

diff --git a/kernel/module/Makefile b/kernel/module/Makefile
index fc6d7a053a62..8f2857d9ba1e 100644
--- a/kernel/module/Makefile
+++ b/kernel/module/Makefile
@@ -10,4 +10,5 @@ obj-$(CONFIG_MODULE_SIG_FORMAT) += signature.o
 ifdef CONFIG_MODULES
 obj-$(CONFIG_LIVEPATCH) += livepatch.o
 obj-$(CONFIG_MODULES_TREE_LOOKUP) += tree_lookup.o
+obj-$(CONFIG_STRICT_MODULE_RWX) += strict_rwx.o
 endif
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index 08b6be037b72..99204447ce86 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -21,6 +21,17 @@
 #define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
 #define MODULE_SECT_READ_SIZE (3 /* "0x", "\n" */ + (BITS_PER_LONG / 4))
 
+/*
+ * Modules' sections will be aligned on page boundaries
+ * to ensure complete separation of code and data, but
+ * only when CONFIG_ARCH_HAS_STRICT_MODULE_RWX=y
+ */
+#ifdef CONFIG_ARCH_HAS_STRICT_MODULE_RWX
+# define debug_align(X) ALIGN(X, PAGE_SIZE)
+#else
+# define debug_align(X) (X)
+#endif
+
 extern struct mutex module_mutex;
 extern struct list_head modules;
 
@@ -124,3 +135,30 @@ static struct module *mod_find(unsigned long addr)
 	return NULL;
 }
 #endif /* CONFIG_MODULES_TREE_LOOKUP */
+
+#ifdef CONFIG_ARCH_HAS_STRICT_MODULE_RWX
+void frob_text(const struct module_layout *layout, int (*set_memory)(unsigned long start,
+	       int num_pages));
+#endif /* CONFIG_ARCH_HAS_STRICT_MODULE_RWX */
+
+#ifdef CONFIG_STRICT_MODULE_RWX
+void frob_rodata(const struct module_layout *layout,
+			int (*set_memory)(unsigned long start, int num_pages));
+void frob_ro_after_init(const struct module_layout *layout,
+			       int (*set_memory)(unsigned long start, int num_pages));
+void frob_writable_data(const struct module_layout *layout,
+			       int (*set_memory)(unsigned long start, int num_pages));
+void module_enable_ro(const struct module *mod, bool after_init);
+void module_enable_nx(const struct module *mod);
+int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
+				       char *secstrings, struct module *mod);
+
+#else /* !CONFIG_STRICT_MODULE_RWX */
+static void module_enable_nx(const struct module *mod) { }
+static void module_enable_ro(const struct module *mod, bool after_init) {}
+static int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
+				       char *secstrings, struct module *mod)
+{
+	return 0;
+}
+#endif /* CONFIG_STRICT_MODULE_RWX */
diff --git a/kernel/module/main.c b/kernel/module/main.c
index f733a719c65d..abdedc15f4f1 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -63,17 +63,6 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/module.h>
 
-/*
- * Modules' sections will be aligned on page boundaries
- * to ensure complete separation of code and data, but
- * only when CONFIG_ARCH_HAS_STRICT_MODULE_RWX=y
- */
-#ifdef CONFIG_ARCH_HAS_STRICT_MODULE_RWX
-# define debug_align(X) ALIGN(X, PAGE_SIZE)
-#else
-# define debug_align(X) (X)
-#endif
-
 /*
  * Mutex protects:
  * 1) List of modules (also safely readable with preempt_disable),
@@ -1815,7 +1804,7 @@ static void mod_sysfs_teardown(struct module *mod)
  * whether we are strict.
  */
 #ifdef CONFIG_ARCH_HAS_STRICT_MODULE_RWX
-static void frob_text(const struct module_layout *layout,
+void frob_text(const struct module_layout *layout,
 		      int (*set_memory)(unsigned long start, int num_pages))
 {
 	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
@@ -1833,90 +1822,6 @@ static void module_enable_x(const struct module *mod)
 static void module_enable_x(const struct module *mod) { }
 #endif /* CONFIG_ARCH_HAS_STRICT_MODULE_RWX */
 
-#ifdef CONFIG_STRICT_MODULE_RWX
-static void frob_rodata(const struct module_layout *layout,
-			int (*set_memory)(unsigned long start, int num_pages))
-{
-	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
-	BUG_ON((unsigned long)layout->text_size & (PAGE_SIZE-1));
-	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
-	set_memory((unsigned long)layout->base + layout->text_size,
-		   (layout->ro_size - layout->text_size) >> PAGE_SHIFT);
-}
-
-static void frob_ro_after_init(const struct module_layout *layout,
-				int (*set_memory)(unsigned long start, int num_pages))
-{
-	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
-	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
-	BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
-	set_memory((unsigned long)layout->base + layout->ro_size,
-		   (layout->ro_after_init_size - layout->ro_size) >> PAGE_SHIFT);
-}
-
-static void frob_writable_data(const struct module_layout *layout,
-			       int (*set_memory)(unsigned long start, int num_pages))
-{
-	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
-	BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
-	BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1));
-	set_memory((unsigned long)layout->base + layout->ro_after_init_size,
-		   (layout->size - layout->ro_after_init_size) >> PAGE_SHIFT);
-}
-
-static void module_enable_ro(const struct module *mod, bool after_init)
-{
-	if (!rodata_enabled)
-		return;
-
-	set_vm_flush_reset_perms(mod->core_layout.base);
-	set_vm_flush_reset_perms(mod->init_layout.base);
-	frob_text(&mod->core_layout, set_memory_ro);
-
-	frob_rodata(&mod->core_layout, set_memory_ro);
-	frob_text(&mod->init_layout, set_memory_ro);
-	frob_rodata(&mod->init_layout, set_memory_ro);
-
-	if (after_init)
-		frob_ro_after_init(&mod->core_layout, set_memory_ro);
-}
-
-static void module_enable_nx(const struct module *mod)
-{
-	frob_rodata(&mod->core_layout, set_memory_nx);
-	frob_ro_after_init(&mod->core_layout, set_memory_nx);
-	frob_writable_data(&mod->core_layout, set_memory_nx);
-	frob_rodata(&mod->init_layout, set_memory_nx);
-	frob_writable_data(&mod->init_layout, set_memory_nx);
-}
-
-static int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
-				       char *secstrings, struct module *mod)
-{
-	const unsigned long shf_wx = SHF_WRITE|SHF_EXECINSTR;
-	int i;
-
-	for (i = 0; i < hdr->e_shnum; i++) {
-		if ((sechdrs[i].sh_flags & shf_wx) == shf_wx) {
-			pr_err("%s: section %s (index %d) has invalid WRITE|EXEC flags\n",
-				mod->name, secstrings + sechdrs[i].sh_name, i);
-			return -ENOEXEC;
-		}
-	}
-
-	return 0;
-}
-
-#else /* !CONFIG_STRICT_MODULE_RWX */
-static void module_enable_nx(const struct module *mod) { }
-static void module_enable_ro(const struct module *mod, bool after_init) {}
-static int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
-				       char *secstrings, struct module *mod)
-{
-	return 0;
-}
-#endif /*  CONFIG_STRICT_MODULE_RWX */
-
 void __weak module_memfree(void *module_region)
 {
 	/*
diff --git a/kernel/module/strict_rwx.c b/kernel/module/strict_rwx.c
new file mode 100644
index 000000000000..1933272056c7
--- /dev/null
+++ b/kernel/module/strict_rwx.c
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Module strict rwx
+ *
+ * Copyright (C) 2015 Rusty Russell
+ */
+
+#include <linux/module.h>
+#include <linux/vmalloc.h>
+#include <linux/set_memory.h>
+#include "internal.h"
+
+void frob_rodata(const struct module_layout *layout,
+			int (*set_memory)(unsigned long start, int num_pages))
+{
+	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
+	BUG_ON((unsigned long)layout->text_size & (PAGE_SIZE-1));
+	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
+	set_memory((unsigned long)layout->base + layout->text_size,
+		   (layout->ro_size - layout->text_size) >> PAGE_SHIFT);
+}
+
+void frob_ro_after_init(const struct module_layout *layout,
+				int (*set_memory)(unsigned long start, int num_pages))
+{
+	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
+	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
+	BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
+	set_memory((unsigned long)layout->base + layout->ro_size,
+		   (layout->ro_after_init_size - layout->ro_size) >> PAGE_SHIFT);
+}
+
+void frob_writable_data(const struct module_layout *layout,
+			       int (*set_memory)(unsigned long start, int num_pages))
+{
+	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
+	BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
+	BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1));
+	set_memory((unsigned long)layout->base + layout->ro_after_init_size,
+		   (layout->size - layout->ro_after_init_size) >> PAGE_SHIFT);
+}
+
+void module_enable_ro(const struct module *mod, bool after_init)
+{
+	if (!rodata_enabled)
+		return;
+
+	set_vm_flush_reset_perms(mod->core_layout.base);
+	set_vm_flush_reset_perms(mod->init_layout.base);
+	frob_text(&mod->core_layout, set_memory_ro);
+
+	frob_rodata(&mod->core_layout, set_memory_ro);
+	frob_text(&mod->init_layout, set_memory_ro);
+	frob_rodata(&mod->init_layout, set_memory_ro);
+
+	if (after_init)
+		frob_ro_after_init(&mod->core_layout, set_memory_ro);
+}
+
+void module_enable_nx(const struct module *mod)
+{
+	frob_rodata(&mod->core_layout, set_memory_nx);
+	frob_ro_after_init(&mod->core_layout, set_memory_nx);
+	frob_writable_data(&mod->core_layout, set_memory_nx);
+	frob_rodata(&mod->init_layout, set_memory_nx);
+	frob_writable_data(&mod->init_layout, set_memory_nx);
+}
+
+int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
+				       char *secstrings, struct module *mod)
+{
+	const unsigned long shf_wx = SHF_WRITE|SHF_EXECINSTR;
+	int i;
+
+	for (i = 0; i < hdr->e_shnum; i++) {
+		if ((sechdrs[i].sh_flags & shf_wx) == shf_wx) {
+			pr_err("%s: section %s (index %d) has invalid WRITE|EXEC flags\n",
+				mod->name, secstrings + sechdrs[i].sh_name, i);
+			return -ENOEXEC;
+		}
+	}
+
+	return 0;
+}
-- 
2.34.1

Re: [PATCH v5 06/13] module: Move strict rwx support to a separate file

[Christophe Leroy]

Le 09/02/2022 à 18:03, Aaron Tomlin a écrit :
> No functional change.
> 
> This patch migrates code that makes module text
> and rodata memory read-only and non-text memory
> non-executable from core module code into
> kernel/module/strict_rwx.c.
> 
> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> ---
>   kernel/module/Makefile     |  1 +
>   kernel/module/internal.h   | 38 +++++++++++++++
>   kernel/module/main.c       | 97 +-------------------------------------
>   kernel/module/strict_rwx.c | 84 +++++++++++++++++++++++++++++++++

This file generates many checkpatch WARNINGs and CHECKs.

Don't worry too much about the ones telling you to use WARN_ON() instead 
of BUG_ON() for the time being, but others should be handled.

>   4 files changed, 124 insertions(+), 96 deletions(-)
>   create mode 100644 kernel/module/strict_rwx.c
> 
> diff --git a/kernel/module/Makefile b/kernel/module/Makefile
> index fc6d7a053a62..8f2857d9ba1e 100644
> --- a/kernel/module/Makefile
> +++ b/kernel/module/Makefile
> @@ -10,4 +10,5 @@ obj-$(CONFIG_MODULE_SIG_FORMAT) += signature.o
>   ifdef CONFIG_MODULES
>   obj-$(CONFIG_LIVEPATCH) += livepatch.o
>   obj-$(CONFIG_MODULES_TREE_LOOKUP) += tree_lookup.o
> +obj-$(CONFIG_STRICT_MODULE_RWX) += strict_rwx.o
>   endif
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index 08b6be037b72..99204447ce86 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -21,6 +21,17 @@
>   #define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
>   #define MODULE_SECT_READ_SIZE (3 /* "0x", "\n" */ + (BITS_PER_LONG / 4))
>   
> +/*
> + * Modules' sections will be aligned on page boundaries
> + * to ensure complete separation of code and data, but
> + * only when CONFIG_ARCH_HAS_STRICT_MODULE_RWX=y
> + */
> +#ifdef CONFIG_ARCH_HAS_STRICT_MODULE_RWX
> +# define debug_align(X) ALIGN(X, PAGE_SIZE)

You can use PAGE_ALIGN() instead.

> +#else
> +# define debug_align(X) (X)
> +#endif
> +
>   extern struct mutex module_mutex;
>   extern struct list_head modules;
>   
> @@ -124,3 +135,30 @@ static struct module *mod_find(unsigned long addr)
>   	return NULL;
>   }
>   #endif /* CONFIG_MODULES_TREE_LOOKUP */
> +
> +#ifdef CONFIG_ARCH_HAS_STRICT_MODULE_RWX

This #ifdef is not needed, frob_text() always exists.

> +void frob_text(const struct module_layout *layout, int (*set_memory)(unsigned long start,
> +	       int num_pages));
> +#endif /* CONFIG_ARCH_HAS_STRICT_MODULE_RWX */
> +
> +#ifdef CONFIG_STRICT_MODULE_RWX
> +void frob_rodata(const struct module_layout *layout,
> +			int (*set_memory)(unsigned long start, int num_pages));
> +void frob_ro_after_init(const struct module_layout *layout,
> +			       int (*set_memory)(unsigned long start, int num_pages));
> +void frob_writable_data(const struct module_layout *layout,
> +			       int (*set_memory)(unsigned long start, int num_pages));
> +void module_enable_ro(const struct module *mod, bool after_init);
> +void module_enable_nx(const struct module *mod);
> +int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
> +				       char *secstrings, struct module *mod);
> +
> +#else /* !CONFIG_STRICT_MODULE_RWX */
> +static void module_enable_nx(const struct module *mod) { }
> +static void module_enable_ro(const struct module *mod, bool after_init) {}
> +static int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
> +				       char *secstrings, struct module *mod)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_STRICT_MODULE_RWX */
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index f733a719c65d..abdedc15f4f1 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -63,17 +63,6 @@
>   #define CREATE_TRACE_POINTS
>   #include <trace/events/module.h>
>   
> -/*
> - * Modules' sections will be aligned on page boundaries
> - * to ensure complete separation of code and data, but
> - * only when CONFIG_ARCH_HAS_STRICT_MODULE_RWX=y
> - */
> -#ifdef CONFIG_ARCH_HAS_STRICT_MODULE_RWX
> -# define debug_align(X) ALIGN(X, PAGE_SIZE)
> -#else
> -# define debug_align(X) (X)
> -#endif
> -
>   /*
>    * Mutex protects:
>    * 1) List of modules (also safely readable with preempt_disable),
> @@ -1815,7 +1804,7 @@ static void mod_sysfs_teardown(struct module *mod)
>    * whether we are strict.
>    */
>   #ifdef CONFIG_ARCH_HAS_STRICT_MODULE_RWX
> -static void frob_text(const struct module_layout *layout,
> +void frob_text(const struct module_layout *layout,
>   		      int (*set_memory)(unsigned long start, int num_pages))

The alignment of the second line is not correct.

>   {
>   	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
> @@ -1833,90 +1822,6 @@ static void module_enable_x(const struct module *mod)
>   static void module_enable_x(const struct module *mod) { }
>   #endif /* CONFIG_ARCH_HAS_STRICT_MODULE_RWX */
>   
> -#ifdef CONFIG_STRICT_MODULE_RWX
> -static void frob_rodata(const struct module_layout *layout,
> -			int (*set_memory)(unsigned long start, int num_pages))
> -{
> -	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
> -	BUG_ON((unsigned long)layout->text_size & (PAGE_SIZE-1));
> -	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
> -	set_memory((unsigned long)layout->base + layout->text_size,
> -		   (layout->ro_size - layout->text_size) >> PAGE_SHIFT);
> -}
> -
> -static void frob_ro_after_init(const struct module_layout *layout,
> -				int (*set_memory)(unsigned long start, int num_pages))
> -{
> -	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
> -	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
> -	BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
> -	set_memory((unsigned long)layout->base + layout->ro_size,
> -		   (layout->ro_after_init_size - layout->ro_size) >> PAGE_SHIFT);
> -}
> -
> -static void frob_writable_data(const struct module_layout *layout,
> -			       int (*set_memory)(unsigned long start, int num_pages))
> -{
> -	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
> -	BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
> -	BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1));
> -	set_memory((unsigned long)layout->base + layout->ro_after_init_size,
> -		   (layout->size - layout->ro_after_init_size) >> PAGE_SHIFT);
> -}
> -
> -static void module_enable_ro(const struct module *mod, bool after_init)
> -{
> -	if (!rodata_enabled)
> -		return;
> -
> -	set_vm_flush_reset_perms(mod->core_layout.base);
> -	set_vm_flush_reset_perms(mod->init_layout.base);
> -	frob_text(&mod->core_layout, set_memory_ro);
> -
> -	frob_rodata(&mod->core_layout, set_memory_ro);
> -	frob_text(&mod->init_layout, set_memory_ro);
> -	frob_rodata(&mod->init_layout, set_memory_ro);
> -
> -	if (after_init)
> -		frob_ro_after_init(&mod->core_layout, set_memory_ro);
> -}
> -
> -static void module_enable_nx(const struct module *mod)
> -{
> -	frob_rodata(&mod->core_layout, set_memory_nx);
> -	frob_ro_after_init(&mod->core_layout, set_memory_nx);
> -	frob_writable_data(&mod->core_layout, set_memory_nx);
> -	frob_rodata(&mod->init_layout, set_memory_nx);
> -	frob_writable_data(&mod->init_layout, set_memory_nx);
> -}
> -
> -static int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
> -				       char *secstrings, struct module *mod)
> -{
> -	const unsigned long shf_wx = SHF_WRITE|SHF_EXECINSTR;
> -	int i;
> -
> -	for (i = 0; i < hdr->e_shnum; i++) {
> -		if ((sechdrs[i].sh_flags & shf_wx) == shf_wx) {
> -			pr_err("%s: section %s (index %d) has invalid WRITE|EXEC flags\n",
> -				mod->name, secstrings + sechdrs[i].sh_name, i);
> -			return -ENOEXEC;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
> -#else /* !CONFIG_STRICT_MODULE_RWX */
> -static void module_enable_nx(const struct module *mod) { }
> -static void module_enable_ro(const struct module *mod, bool after_init) {}
> -static int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
> -				       char *secstrings, struct module *mod)
> -{
> -	return 0;
> -}
> -#endif /*  CONFIG_STRICT_MODULE_RWX */
> -
>   void __weak module_memfree(void *module_region)
>   {
>   	/*
> diff --git a/kernel/module/strict_rwx.c b/kernel/module/strict_rwx.c
> new file mode 100644
> index 000000000000..1933272056c7
> --- /dev/null
> +++ b/kernel/module/strict_rwx.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Module strict rwx
> + *
> + * Copyright (C) 2015 Rusty Russell
> + */
> +
> +#include <linux/module.h>
> +#include <linux/vmalloc.h>
> +#include <linux/set_memory.h>
> +#include "internal.h"
> +
> +void frob_rodata(const struct module_layout *layout,
> +			int (*set_memory)(unsigned long start, int num_pages))
> +{
> +	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));

Could be:

	BUG_ON(!PAGE_ALIGNED(layout->base));


Same for all others.

> +	BUG_ON((unsigned long)layout->text_size & (PAGE_SIZE-1));
> +	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
> +	set_memory((unsigned long)layout->base + layout->text_size,
> +		   (layout->ro_size - layout->text_size) >> PAGE_SHIFT);
> +}
> +
> +void frob_ro_after_init(const struct module_layout *layout,
> +				int (*set_memory)(unsigned long start, int num_pages))
> +{
> +	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
> +	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
> +	BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
> +	set_memory((unsigned long)layout->base + layout->ro_size,
> +		   (layout->ro_after_init_size - layout->ro_size) >> PAGE_SHIFT);
> +}
> +
> +void frob_writable_data(const struct module_layout *layout,
> +			       int (*set_memory)(unsigned long start, int num_pages))
> +{
> +	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
> +	BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
> +	BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1));
> +	set_memory((unsigned long)layout->base + layout->ro_after_init_size,
> +		   (layout->size - layout->ro_after_init_size) >> PAGE_SHIFT);
> +}
> +
> +void module_enable_ro(const struct module *mod, bool after_init)
> +{
> +	if (!rodata_enabled)
> +		return;
> +
> +	set_vm_flush_reset_perms(mod->core_layout.base);
> +	set_vm_flush_reset_perms(mod->init_layout.base);
> +	frob_text(&mod->core_layout, set_memory_ro);
> +
> +	frob_rodata(&mod->core_layout, set_memory_ro);
> +	frob_text(&mod->init_layout, set_memory_ro);
> +	frob_rodata(&mod->init_layout, set_memory_ro);
> +
> +	if (after_init)
> +		frob_ro_after_init(&mod->core_layout, set_memory_ro);
> +}
> +
> +void module_enable_nx(const struct module *mod)
> +{
> +	frob_rodata(&mod->core_layout, set_memory_nx);
> +	frob_ro_after_init(&mod->core_layout, set_memory_nx);
> +	frob_writable_data(&mod->core_layout, set_memory_nx);
> +	frob_rodata(&mod->init_layout, set_memory_nx);
> +	frob_writable_data(&mod->init_layout, set_memory_nx);
> +}
> +
> +int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
> +				       char *secstrings, struct module *mod)
> +{
> +	const unsigned long shf_wx = SHF_WRITE|SHF_EXECINSTR;
> +	int i;
> +
> +	for (i = 0; i < hdr->e_shnum; i++) {
> +		if ((sechdrs[i].sh_flags & shf_wx) == shf_wx) {
> +			pr_err("%s: section %s (index %d) has invalid WRITE|EXEC flags\n",
> +				mod->name, secstrings + sechdrs[i].sh_name, i);
> +			return -ENOEXEC;
> +		}
> +	}
> +
> +	return 0;
> +}

Re: [PATCH v5 06/13] module: Move strict rwx support to a separate file

[Aaron Tomlin]

On Thu 2022-02-10 12:21 +0000, Christophe Leroy wrote:
> This file generates many checkpatch WARNINGs and CHECKs.
> Don't worry too much about the ones telling you to use WARN_ON() instead
> of BUG_ON() for the time being, but others should be handled.

Yes, with ./scripts/checkpatch.pl --strict'.
Please note: I have used '--ignore=ASSIGN_IN_IF,AVOID_BUG' previously on
that file. Albeit, I will resolve the check violations e.g. "Alignment
should match open parenthesis" etc.

> > +# define debug_align(X) ALIGN(X, PAGE_SIZE)
>
> You can use PAGE_ALIGN() instead.

Agreed: PAGE_ALIGN(X) does expand to ALIGN(X, PAGE_SIZE)

> > +#ifdef CONFIG_ARCH_HAS_STRICT_MODULE_RWX
>
> This #ifdef is not needed, frob_text() always exists.

I will leave this for you to remove, in your patch [1].

> > +    BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
>
> Could be:
>
>     BUG_ON(!PAGE_ALIGNED(layout->base));
>
>
> Same for all others.

Agreed.

[1]: https://lore.kernel.org/lkml/203348805c9ac9851d8939d15cb9802ef047b5e2.1643919758.git.christophe.leroy@csgroup.eu/

Kind regards,
-- 
Aaron Tomlin