| Message ID | 20220218212511.887059-2-atomlin@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | module: core code clean up | expand |
Le 18/02/2022 à 22:24, 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} | 0 > 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, 14 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} (100%) > 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%) > I'm wondering whether we should avoid moving module_signature.c and leave it in kernel/ as this file is used even when CONFIG_MODULES is not selected, and he is the only one like this. Keeping it outside of kernel/module/ would allow to conditionaly build entire kernel/module/ based of CONFIG_MODULES and then avoid all checks against CONFIG_MODULES which look misleading at times. Christophe
Le 18/02/2022 à 22:24, 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} | 0 > 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, 14 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} (100%) > 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 bd86ed9fbc79..463bdb829db4 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -13012,7 +13012,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/ > > 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 This file also contains: KCOV_INSTRUMENT_module.o := n It needs to follow through into kernel/module/Makefile, with a copy of the comment. And then it needs to be taken care of while dismantling main.c > @@ -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
On Mon 2022-02-21 12:21 +0000, Christophe Leroy wrote: > > > Le 18/02/2022 à 22:24, 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} | 0 > > 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, 14 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} (100%) > > 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%) > > > > I'm wondering whether we should avoid moving module_signature.c and > leave it in kernel/ as this file is used even when CONFIG_MODULES is not > selected, and he is the only one like this. > > Keeping it outside of kernel/module/ would allow to conditionaly build > entire kernel/module/ based of CONFIG_MODULES and then avoid all checks > against CONFIG_MODULES which look misleading at times. Luis, What is your opinion on this? Indeed, mod_check_sig() is used by code outside of kernel/module/ too i.e. ima_read_modsig(); albeit, I believe it does make sense to keep it under kernel/module/ since the function in question is used to review a given module's signature anyway. Kind regards,
Le 22/02/2022 à 12:13, Aaron Tomlin a écrit : > On Mon 2022-02-21 13:13 +0000, Christophe Leroy wrote: >>> diff --git a/kernel/Makefile b/kernel/Makefile >>> index 56f4ee97f328..3a6380975c57 100644 >>> --- a/kernel/Makefile >>> +++ b/kernel/Makefile >> >> This file also contains: >> >> KCOV_INSTRUMENT_module.o := n >> >> It needs to follow through into kernel/module/Makefile, with a copy of >> the comment. And then it needs to be taken care of while dismantling main.c > > Christophe, > > I do not see why this is necessary. > > I don't know. The comment says: # These are called from save_stack_trace() on slub debug path, # and produce insane amounts of uninteresting coverage. This was brought by commit 5c9a8750a640 ("kernel: add kcov code coverage") Since then, some architectures don't have save_stack_trace() anymore as they were converted to generic ARCH_STACKWALK. It was done on powerpc by commit a1cdef04f22d ("powerpc: Convert stacktrace to generic ARCH_STACKWALK") Whatever you decide to do, you can't leave this in kernel/Makefile as module.o is gone. Christophe
On Tue 2022-02-22 11:20 +0000, Christophe Leroy wrote: > I don't know. > > The comment says: > > # These are called from save_stack_trace() on slub debug path, > # and produce insane amounts of uninteresting coverage. > > This was brought by commit 5c9a8750a640 ("kernel: add kcov code coverage") > > Since then, some architectures don't have save_stack_trace() anymore as > they were converted to generic ARCH_STACKWALK. > It was done on powerpc by commit a1cdef04f22d ("powerpc: Convert > stacktrace to generic ARCH_STACKWALK") > > Whatever you decide to do, you can't leave this in kernel/Makefile as > module.o is gone. Fair enough. I'll move it and copy the comment to kernel/module/Makefile.
On Tue, Feb 22, 2022 at 10:48:00AM +0000, Aaron Tomlin wrote: > On Mon 2022-02-21 12:21 +0000, Christophe Leroy wrote: > > Le 18/02/2022 à 22:24, Aaron Tomlin a écrit : > > > kernel/{module_signature.c => module/signature.c} | 0 > > Keeping it outside of kernel/module/ would allow to conditionaly build > > entire kernel/module/ based of CONFIG_MODULES and then avoid all checks > > against CONFIG_MODULES which look misleading at times. > > Luis, > > What is your opinion on this? Indeed, mod_check_sig() is used by code > outside of kernel/module/ too i.e. ima_read_modsig(); albeit, I believe it > does make sense to keep it under kernel/module/ since the function in > question is used to review a given module's signature anyway. How about: obj-$(CONFIG_MODULE_SIG_FORMAT) += module/module_signature.o Luis
On Tue 2022-02-22 17:16 -0800, Luis Chamberlain wrote: > How about: > > obj-$(CONFIG_MODULE_SIG_FORMAT) += module/module_signature.o > > Luis Hi Luis, Please see v8 [1]. [1]: https://lore.kernel.org/all/20220222141303.1392190-1-atomlin@redhat.com/ Kind regards,
diff --git a/MAINTAINERS b/MAINTAINERS index bd86ed9fbc79..463bdb829db4 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13012,7 +13012,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/ 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 ffef98a20320..d14d6443225a 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 100% rename from kernel/module-internal.h rename to kernel/module/internal.h 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.
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} | 0 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, 14 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} (100%) 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%)