| Message ID | 20210121213641.3477522-1-willmcvicker@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6] modules: introduce the MODULE_SCMVERSION config | expand |
Hi Jessica and Masahiro, Could I get a final look at the latest patchset please? All the issues have been addressed thus far. Thanks, Will On Thu, Jan 21, 2021 at 1:36 PM Will McVicker <willmcvicker@google.com> wrote: > > Config MODULE_SCMVERSION introduces a new module attribute -- > `scmversion` -- which can be used to identify a given module's SCM > version. This is very useful for developers that update their kernel > independently from their kernel modules or vice-versa since the SCM > version provided by UTS_RELEASE (`uname -r`) will now differ from the > module's vermagic attribute. > > For example, we have a CI setup that tests new kernel changes on the > hikey960 and db845c devices without updating their kernel modules. When > these tests fail, we need to be able to identify the exact device > configuration the test was using. By including MODULE_SCMVERSION, we can > identify the exact kernel and modules' SCM versions for debugging the > failures. > > Additionally, by exposing the SCM version via the sysfs node > /sys/module/MODULENAME/scmversion, one can also verify the SCM versions > of the modules loaded from the initramfs. Currently, modinfo can only > retrieve module attributes from the module's ko on disk and not from the > actual module that is loaded in RAM. > > You can retrieve the SCM version in two ways, > > 1) By using modinfo: > > modinfo -F scmversion MODULENAME > 2) By module sysfs node: > > cat /sys/module/MODULENAME/scmversion > > Signed-off-by: Will McVicker <willmcvicker@google.com> > --- > Changelog since v5: > - Simplified scripts/Makefile.modpost to not check for a relative M= path > - Added space between -v and $(module_scmversion) > - Updated modpost.c to not check for a missing argument to -v > > Documentation/ABI/stable/sysfs-module | 18 ++++++++++++++++++ > include/linux/module.h | 1 + > init/Kconfig | 14 ++++++++++++++ > kernel/module.c | 2 ++ > scripts/Makefile.modpost | 14 ++++++++++++++ > scripts/mod/modpost.c | 22 +++++++++++++++++++++- > 6 files changed, 70 insertions(+), 1 deletion(-) > > diff --git a/Documentation/ABI/stable/sysfs-module b/Documentation/ABI/stable/sysfs-module > index 6272ae5fb366..a75d137e79f4 100644 > --- a/Documentation/ABI/stable/sysfs-module > +++ b/Documentation/ABI/stable/sysfs-module > @@ -32,3 +32,21 @@ Description: > Note: If the module is built into the kernel, or if the > CONFIG_MODULE_UNLOAD kernel configuration value is not enabled, > this file will not be present. > + > +What: /sys/module/MODULENAME/scmversion > +Date: November 2020 > +KernelVersion: 5.12 > +Contact: Will McVicker <willmcvicker@google.com> > +Description: This read-only file will appear if modpost was supplied with an > + SCM version for the module. It can be enabled with the config > + MODULE_SCMVERSION. The SCM version is retrieved by > + scripts/setlocalversion, which means that the presence of this > + file depends on CONFIG_LOCALVERSION_AUTO=y. When read, the SCM > + version that the module was compiled with is returned. The SCM > + version is returned in the following format:: > + > + === > + Git: g[a-f0-9]\+(-dirty)\? > + Mercurial: hg[a-f0-9]\+(-dirty)\? > + Subversion: svn[0-9]\+ > + === > diff --git a/include/linux/module.h b/include/linux/module.h > index 7a0bcb5b1ffc..3b1612193cf9 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -372,6 +372,7 @@ struct module { > struct module_attribute *modinfo_attrs; > const char *version; > const char *srcversion; > + const char *scmversion; > struct kobject *holders_dir; > > /* Exported symbols */ > diff --git a/init/Kconfig b/init/Kconfig > index b77c60f8b963..3d9dac3c4e8f 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -2131,6 +2131,20 @@ config MODULE_SRCVERSION_ALL > the version). With this option, such a "srcversion" field > will be created for all modules. If unsure, say N. > > +config MODULE_SCMVERSION > + bool "SCM version for modules" > + depends on LOCALVERSION_AUTO > + help > + This enables the module attribute "scmversion" which can be used > + by developers to identify the SCM version of a given module, e.g. > + git sha1 or hg sha1. The SCM version can be queried by modinfo or > + via the sysfs node: /sys/modules/MODULENAME/scmversion. This is > + useful when the kernel or kernel modules are updated separately > + since that causes the vermagic of the kernel and the module to > + differ. > + > + If unsure, say N. > + > config MODULE_SIG > bool "Module signature verification" > select MODULE_SIG_FORMAT > diff --git a/kernel/module.c b/kernel/module.c > index 4bf30e4b3eaa..d0b359c7e9c9 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -837,6 +837,7 @@ static struct module_attribute modinfo_##field = { \ > > MODINFO_ATTR(version); > MODINFO_ATTR(srcversion); > +MODINFO_ATTR(scmversion); > > static char last_unloaded_module[MODULE_NAME_LEN+1]; > > @@ -1298,6 +1299,7 @@ static struct module_attribute *modinfo_attrs[] = { > &module_uevent, > &modinfo_version, > &modinfo_srcversion, > + &modinfo_scmversion, > &modinfo_initstate, > &modinfo_coresize, > &modinfo_initsize, > diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost > index f54b6ac37ac2..13ec3e96650c 100644 > --- a/scripts/Makefile.modpost > +++ b/scripts/Makefile.modpost > @@ -66,6 +66,7 @@ ifeq ($(KBUILD_EXTMOD),) > > input-symdump := vmlinux.symvers > output-symdump := Module.symvers > +module_srcpath := $(srctree) > > else > > @@ -82,9 +83,22 @@ MODPOST += -e > > input-symdump := Module.symvers $(KBUILD_EXTRA_SYMBOLS) > output-symdump := $(KBUILD_EXTMOD)/Module.symvers > +module_srcpath := $(KBUILD_EXTMOD) > > endif > > +ifeq ($(CONFIG_MODULE_SCMVERSION),y) > +# Get the SCM version of the module. Sed verifies setlocalversion returns > +# a proper revision based on the SCM type, e.g. git, mercurial, or svn. > +# Note: relative M= paths are not supported when building the kernel out of the > +# srctree since setlocalversion won't be able to find the module srctree. > +module_scmversion := $(shell $(srctree)/scripts/setlocalversion $(module_srcpath) | \ > + sed -n 's/.*-\(\(g\|hg\)[a-fA-F0-9]\+\(-dirty\)\?\|svn[0-9]\+\).*/\1/p') > +ifneq ($(module_scmversion),) > +MODPOST += -v $(module_scmversion) > +endif > +endif > + > # modpost options for modules (both in-kernel and external) > MODPOST += \ > $(addprefix -i ,$(wildcard $(input-symdump))) \ > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > index d6c81657d695..92c4bd88f875 100644 > --- a/scripts/mod/modpost.c > +++ b/scripts/mod/modpost.c > @@ -30,6 +30,8 @@ static int have_vmlinux = 0; > static int all_versions = 0; > /* If we are modposting external module set to 1 */ > static int external_module = 0; > +#define MODULE_SCMVERSION_SIZE 64 > +static char module_scmversion[MODULE_SCMVERSION_SIZE]; > /* Only warn about unresolved symbols */ > static int warn_unresolved = 0; > /* How a symbol is exported */ > @@ -2264,6 +2266,20 @@ static void add_intree_flag(struct buffer *b, int is_intree) > buf_printf(b, "\nMODULE_INFO(intree, \"Y\");\n"); > } > > +/** > + * add_scmversion() - Adds the MODULE_INFO macro for the scmversion. > + * @b: Buffer to append to. > + * > + * This function fills in the module attribute `scmversion` for the kernel > + * module. This is useful for determining a given module's SCM version on > + * device via /sys/modules/<module>/scmversion and/or using the modinfo tool. > + */ > +static void add_scmversion(struct buffer *b) > +{ > + if (module_scmversion[0] != '\0') > + buf_printf(b, "\nMODULE_INFO(scmversion, \"%s\");\n", module_scmversion); > +} > + > /* Cannot check for assembler */ > static void add_retpoline(struct buffer *b) > { > @@ -2546,7 +2562,7 @@ int main(int argc, char **argv) > struct dump_list *dump_read_start = NULL; > struct dump_list **dump_read_iter = &dump_read_start; > > - while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) { > + while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:v:")) != -1) { > switch (opt) { > case 'e': > external_module = 1; > @@ -2584,6 +2600,9 @@ int main(int argc, char **argv) > case 'd': > missing_namespace_deps = optarg; > break; > + case 'v': > + strncpy(module_scmversion, optarg, sizeof(module_scmversion) - 1); > + break; > default: > exit(1); > } > @@ -2630,6 +2649,7 @@ int main(int argc, char **argv) > add_depends(&buf, mod); > add_moddevtable(&buf, mod); > add_srcversion(&buf, mod); > + add_scmversion(&buf); > > sprintf(fname, "%s.mod.c", mod->name); > write_if_changed(&buf, fname); > -- > 2.30.0.280.ga3ce27912f-goog >
Just to deposit my opposition again: I don't think this hack for your out of tree builds has any place in the kernel tree.
Hi Christoph, Did you see Saravana's response to you in version 5? I'd love to hear your thoughts. Thanks, Will On Mon, Feb 1, 2021 at 11:42 PM Christoph Hellwig <hch@infradead.org> wrote: > > Just to deposit my opposition again: I don't think this hack for your > out of tree builds has any place in the kernel tree.
+++ Will McVicker [21/01/21 21:36 +0000]: >Config MODULE_SCMVERSION introduces a new module attribute -- >`scmversion` -- which can be used to identify a given module's SCM >version. This is very useful for developers that update their kernel >independently from their kernel modules or vice-versa since the SCM >version provided by UTS_RELEASE (`uname -r`) will now differ from the >module's vermagic attribute. > >For example, we have a CI setup that tests new kernel changes on the >hikey960 and db845c devices without updating their kernel modules. When >these tests fail, we need to be able to identify the exact device >configuration the test was using. By including MODULE_SCMVERSION, we can >identify the exact kernel and modules' SCM versions for debugging the >failures. > >Additionally, by exposing the SCM version via the sysfs node >/sys/module/MODULENAME/scmversion, one can also verify the SCM versions >of the modules loaded from the initramfs. Currently, modinfo can only >retrieve module attributes from the module's ko on disk and not from the >actual module that is loaded in RAM. > >You can retrieve the SCM version in two ways, > >1) By using modinfo: > > modinfo -F scmversion MODULENAME >2) By module sysfs node: > > cat /sys/module/MODULENAME/scmversion Hi Will, First off, thanks for being patient and being responsive throughout the patch review process. Personally, I am rather neutral towards this feature. We already provide CONFIG_MODULE_SRCVERSION to provide a checksum of a module's source files and I think the SCMVERSION is a nicer alternative. I can see how an optional scmversion field might be helpful information for distro developers in testing environments and in situations where it is possible for the kernel and modules to be updated/packaged separately (for instance, the kernel selftest modules under lib/ are in-tree modules that are provided as a separate kernel module package in SLE). Generally, out of principle, I do not want to merge a patch that's been NAK'd repeatedly; even if I take the patch it'd likely be contested all the way up to the merge window. So this boils down to whether Christoph (and maybe Greg) are fine with this being a debug option that's not enabled by default. Thanks, Jessica
On Wed, Feb 03, 2021 at 04:46:16PM +0100, Jessica Yu wrote: > +++ Will McVicker [21/01/21 21:36 +0000]: > > Config MODULE_SCMVERSION introduces a new module attribute -- > > `scmversion` -- which can be used to identify a given module's SCM > > version. This is very useful for developers that update their kernel > > independently from their kernel modules or vice-versa since the SCM > > version provided by UTS_RELEASE (`uname -r`) will now differ from the > > module's vermagic attribute. > > > > For example, we have a CI setup that tests new kernel changes on the > > hikey960 and db845c devices without updating their kernel modules. When > > these tests fail, we need to be able to identify the exact device > > configuration the test was using. By including MODULE_SCMVERSION, we can > > identify the exact kernel and modules' SCM versions for debugging the > > failures. > > > > Additionally, by exposing the SCM version via the sysfs node > > /sys/module/MODULENAME/scmversion, one can also verify the SCM versions > > of the modules loaded from the initramfs. Currently, modinfo can only > > retrieve module attributes from the module's ko on disk and not from the > > actual module that is loaded in RAM. > > > > You can retrieve the SCM version in two ways, > > > > 1) By using modinfo: > > > modinfo -F scmversion MODULENAME > > 2) By module sysfs node: > > > cat /sys/module/MODULENAME/scmversion > > Hi Will, > > First off, thanks for being patient and being responsive throughout the patch > review process. > > Personally, I am rather neutral towards this feature. We already provide > CONFIG_MODULE_SRCVERSION to provide a checksum of a module's source files and I > think the SCMVERSION is a nicer alternative. I can see how an optional > scmversion field might be helpful information for distro developers in testing > environments and in situations where it is possible for the kernel and modules > to be updated/packaged separately (for instance, the kernel selftest modules > under lib/ are in-tree modules that are provided as a separate kernel module > package in SLE). > > Generally, out of principle, I do not want to merge a patch that's been NAK'd > repeatedly; even if I take the patch it'd likely be contested all the way up to > the merge window. So this boils down to whether Christoph (and maybe Greg) are > fine with this being a debug option that's not enabled by default. I am neutral on this, I don't care one way or the other. thanks, greg k-h
Thanks Jessica for all the reviews. I guess we can let this die here and I'll carry it downstream. At least it's all here for others to see. Thanks, Will On Wed, Feb 3, 2021 at 7:54 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Feb 03, 2021 at 04:46:16PM +0100, Jessica Yu wrote: > > +++ Will McVicker [21/01/21 21:36 +0000]: > > > Config MODULE_SCMVERSION introduces a new module attribute -- > > > `scmversion` -- which can be used to identify a given module's SCM > > > version. This is very useful for developers that update their kernel > > > independently from their kernel modules or vice-versa since the SCM > > > version provided by UTS_RELEASE (`uname -r`) will now differ from the > > > module's vermagic attribute. > > > > > > For example, we have a CI setup that tests new kernel changes on the > > > hikey960 and db845c devices without updating their kernel modules. When > > > these tests fail, we need to be able to identify the exact device > > > configuration the test was using. By including MODULE_SCMVERSION, we can > > > identify the exact kernel and modules' SCM versions for debugging the > > > failures. > > > > > > Additionally, by exposing the SCM version via the sysfs node > > > /sys/module/MODULENAME/scmversion, one can also verify the SCM versions > > > of the modules loaded from the initramfs. Currently, modinfo can only > > > retrieve module attributes from the module's ko on disk and not from the > > > actual module that is loaded in RAM. > > > > > > You can retrieve the SCM version in two ways, > > > > > > 1) By using modinfo: > > > > modinfo -F scmversion MODULENAME > > > 2) By module sysfs node: > > > > cat /sys/module/MODULENAME/scmversion > > > > Hi Will, > > > > First off, thanks for being patient and being responsive throughout the patch > > review process. > > > > Personally, I am rather neutral towards this feature. We already provide > > CONFIG_MODULE_SRCVERSION to provide a checksum of a module's source files and I > > think the SCMVERSION is a nicer alternative. I can see how an optional > > scmversion field might be helpful information for distro developers in testing > > environments and in situations where it is possible for the kernel and modules > > to be updated/packaged separately (for instance, the kernel selftest modules > > under lib/ are in-tree modules that are provided as a separate kernel module > > package in SLE). > > > > Generally, out of principle, I do not want to merge a patch that's been NAK'd > > repeatedly; even if I take the patch it'd likely be contested all the way up to > > the merge window. So this boils down to whether Christoph (and maybe Greg) are > > fine with this being a debug option that's not enabled by default. > > I am neutral on this, I don't care one way or the other. > > thanks, > > greg k-h
diff --git a/Documentation/ABI/stable/sysfs-module b/Documentation/ABI/stable/sysfs-module index 6272ae5fb366..a75d137e79f4 100644 --- a/Documentation/ABI/stable/sysfs-module +++ b/Documentation/ABI/stable/sysfs-module @@ -32,3 +32,21 @@ Description: Note: If the module is built into the kernel, or if the CONFIG_MODULE_UNLOAD kernel configuration value is not enabled, this file will not be present. + +What: /sys/module/MODULENAME/scmversion +Date: November 2020 +KernelVersion: 5.12 +Contact: Will McVicker <willmcvicker@google.com> +Description: This read-only file will appear if modpost was supplied with an + SCM version for the module. It can be enabled with the config + MODULE_SCMVERSION. The SCM version is retrieved by + scripts/setlocalversion, which means that the presence of this + file depends on CONFIG_LOCALVERSION_AUTO=y. When read, the SCM + version that the module was compiled with is returned. The SCM + version is returned in the following format:: + + === + Git: g[a-f0-9]\+(-dirty)\? + Mercurial: hg[a-f0-9]\+(-dirty)\? + Subversion: svn[0-9]\+ + === diff --git a/include/linux/module.h b/include/linux/module.h index 7a0bcb5b1ffc..3b1612193cf9 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -372,6 +372,7 @@ struct module { struct module_attribute *modinfo_attrs; const char *version; const char *srcversion; + const char *scmversion; struct kobject *holders_dir; /* Exported symbols */ diff --git a/init/Kconfig b/init/Kconfig index b77c60f8b963..3d9dac3c4e8f 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -2131,6 +2131,20 @@ config MODULE_SRCVERSION_ALL the version). With this option, such a "srcversion" field will be created for all modules. If unsure, say N. +config MODULE_SCMVERSION + bool "SCM version for modules" + depends on LOCALVERSION_AUTO + help + This enables the module attribute "scmversion" which can be used + by developers to identify the SCM version of a given module, e.g. + git sha1 or hg sha1. The SCM version can be queried by modinfo or + via the sysfs node: /sys/modules/MODULENAME/scmversion. This is + useful when the kernel or kernel modules are updated separately + since that causes the vermagic of the kernel and the module to + differ. + + If unsure, say N. + config MODULE_SIG bool "Module signature verification" select MODULE_SIG_FORMAT diff --git a/kernel/module.c b/kernel/module.c index 4bf30e4b3eaa..d0b359c7e9c9 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -837,6 +837,7 @@ static struct module_attribute modinfo_##field = { \ MODINFO_ATTR(version); MODINFO_ATTR(srcversion); +MODINFO_ATTR(scmversion); static char last_unloaded_module[MODULE_NAME_LEN+1]; @@ -1298,6 +1299,7 @@ static struct module_attribute *modinfo_attrs[] = { &module_uevent, &modinfo_version, &modinfo_srcversion, + &modinfo_scmversion, &modinfo_initstate, &modinfo_coresize, &modinfo_initsize, diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost index f54b6ac37ac2..13ec3e96650c 100644 --- a/scripts/Makefile.modpost +++ b/scripts/Makefile.modpost @@ -66,6 +66,7 @@ ifeq ($(KBUILD_EXTMOD),) input-symdump := vmlinux.symvers output-symdump := Module.symvers +module_srcpath := $(srctree) else @@ -82,9 +83,22 @@ MODPOST += -e input-symdump := Module.symvers $(KBUILD_EXTRA_SYMBOLS) output-symdump := $(KBUILD_EXTMOD)/Module.symvers +module_srcpath := $(KBUILD_EXTMOD) endif +ifeq ($(CONFIG_MODULE_SCMVERSION),y) +# Get the SCM version of the module. Sed verifies setlocalversion returns +# a proper revision based on the SCM type, e.g. git, mercurial, or svn. +# Note: relative M= paths are not supported when building the kernel out of the +# srctree since setlocalversion won't be able to find the module srctree. +module_scmversion := $(shell $(srctree)/scripts/setlocalversion $(module_srcpath) | \ + sed -n 's/.*-\(\(g\|hg\)[a-fA-F0-9]\+\(-dirty\)\?\|svn[0-9]\+\).*/\1/p') +ifneq ($(module_scmversion),) +MODPOST += -v $(module_scmversion) +endif +endif + # modpost options for modules (both in-kernel and external) MODPOST += \ $(addprefix -i ,$(wildcard $(input-symdump))) \ diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c index d6c81657d695..92c4bd88f875 100644 --- a/scripts/mod/modpost.c +++ b/scripts/mod/modpost.c @@ -30,6 +30,8 @@ static int have_vmlinux = 0; static int all_versions = 0; /* If we are modposting external module set to 1 */ static int external_module = 0; +#define MODULE_SCMVERSION_SIZE 64 +static char module_scmversion[MODULE_SCMVERSION_SIZE]; /* Only warn about unresolved symbols */ static int warn_unresolved = 0; /* How a symbol is exported */ @@ -2264,6 +2266,20 @@ static void add_intree_flag(struct buffer *b, int is_intree) buf_printf(b, "\nMODULE_INFO(intree, \"Y\");\n"); } +/** + * add_scmversion() - Adds the MODULE_INFO macro for the scmversion. + * @b: Buffer to append to. + * + * This function fills in the module attribute `scmversion` for the kernel + * module. This is useful for determining a given module's SCM version on + * device via /sys/modules/<module>/scmversion and/or using the modinfo tool. + */ +static void add_scmversion(struct buffer *b) +{ + if (module_scmversion[0] != '\0') + buf_printf(b, "\nMODULE_INFO(scmversion, \"%s\");\n", module_scmversion); +} + /* Cannot check for assembler */ static void add_retpoline(struct buffer *b) { @@ -2546,7 +2562,7 @@ int main(int argc, char **argv) struct dump_list *dump_read_start = NULL; struct dump_list **dump_read_iter = &dump_read_start; - while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) { + while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:v:")) != -1) { switch (opt) { case 'e': external_module = 1; @@ -2584,6 +2600,9 @@ int main(int argc, char **argv) case 'd': missing_namespace_deps = optarg; break; + case 'v': + strncpy(module_scmversion, optarg, sizeof(module_scmversion) - 1); + break; default: exit(1); } @@ -2630,6 +2649,7 @@ int main(int argc, char **argv) add_depends(&buf, mod); add_moddevtable(&buf, mod); add_srcversion(&buf, mod); + add_scmversion(&buf); sprintf(fname, "%s.mod.c", mod->name); write_if_changed(&buf, fname);