[PATCH v1 0/2] Add support to capture external module's SCM version

[PATCH v1 0/2] Add support to capture external module's SCM version

[Will McVicker]

These two patches add module support to capture an external module's SCM
version as a MODULE_INFO() attribute. This allows users to identity the SCM
version of a given kernel module by using the modinfo tool or on the device
via sysfs:

  cat /sys/module/<module>/scmversion

It's important to note that the sysfs node is necessary in order to get the SCM
version of modules that were loaded from the ramdisk in first stage init.
I have updated scripts/setlocalversion to support this for git, mercurial, and
subversion.

Here is the example output I used to test these patches with a simple module
versioned with git, hg, and svn:

  $ modinfo simple_module.ko | egrep 'scmversion|vermagic'
  scmversion:     gbf35fd9b6412
  vermagic:       5.10.0-rc4-00110-gd83461f36865 SMP mod_unload

  $ modinfo simple_module.ko | egrep 'scmversion|vermagic'
  scmversion:     hge5037af323b9
  vermagic:       5.10.0-rc4-00110-gd83461f36865 SMP mod_unload

  $ modinfo simple_module.ko | egrep 'scmversion|vermagic'
  scmversion:     svn1
  vermagic:       5.10.0-rc4-00110-gd83461f36865 SMP mod_unload

Will McVicker (2):
  scripts/setlocalversion: allow running in a subdir
  modules: add scmversion field

 include/linux/module.h   |  1 +
 kernel/module.c          | 20 ++++++++++++++++++++
 scripts/Makefile.modpost | 19 +++++++++++++++++--
 scripts/mod/modpost.c    | 28 +++++++++++++++++++++++++++-
 scripts/setlocalversion  |  5 ++---
 5 files changed, 67 insertions(+), 6 deletions(-)

-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v1 1/2] scripts/setlocalversion: allow running in a subdir

[Will McVicker]

Getting the scmversion using scripts/setlocalversion currently only
works when run at the root of a git or mecurial project. This was
introduced in commit 8558f59edf93 ("setlocalversion: Ignote SCMs above
the linux source tree") so that if one is building within a subdir of
a git tree that isn't the kernel git project, then the vermagic wouldn't
include that git sha1. However, the proper solution to that is to just
set this config in your defconfig:

  # CONFIG_LOCALVERSION_AUTO is not set

which is already the default in many defconfigs:

  $ grep -r "CONFIG_LOCALVERSION_AUTO is not set" arch/* | wc -l
  89

So let's bring back this functionality so that we can use
scripts/setlocalversion to capture the SCM version of external modules
that reside within subdirectories of an SCM project.

Signed-off-by: Will McVicker <willmcvicker@google.com>
---
 scripts/setlocalversion | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/scripts/setlocalversion b/scripts/setlocalversion
index bb709eda96cd..cd42009e675b 100755
--- a/scripts/setlocalversion
+++ b/scripts/setlocalversion
@@ -44,8 +44,7 @@ scm_version()
 	fi
 
 	# Check for git and a git repo.
-	if test -z "$(git rev-parse --show-cdup 2>/dev/null)" &&
-	   head=$(git rev-parse --verify HEAD 2>/dev/null); then
+	if head=$(git rev-parse --verify HEAD 2>/dev/null); then
 
 		# If we are at a tagged commit (like "v2.6.30-rc6"), we ignore
 		# it, because this version is defined in the top level Makefile.
@@ -102,7 +101,7 @@ scm_version()
 	fi
 
 	# Check for mercurial and a mercurial repo.
-	if test -d .hg && hgid=$(hg id 2>/dev/null); then
+	if hgid=$(hg id 2>/dev/null); then
 		# Do we have an tagged version?  If so, latesttagdistance == 1
 		if [ "$(hg log -r . --template '{latesttagdistance}')" = "1" ]; then
 			id=$(hg log -r . --template '{latesttag}')
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v1 2/2] modules: add scmversion field

[Will McVicker]

Add the modinfo field `scmversion` to include the SCM version of kernel
modules, e.g. git sha1. This allows one to identify the exact source
code version of a given kernel module.

You can retrieve it in two ways,

1) By using modinfo
    > modinfo -F scmversion <module_name>
2) By module sysfs node
    > cat /sys/module/<module_name>/scmversion

Signed-off-by: Will McVicker <willmcvicker@google.com>
---
 include/linux/module.h   |  1 +
 kernel/module.c          | 20 ++++++++++++++++++++
 scripts/Makefile.modpost | 19 +++++++++++++++++--
 scripts/mod/modpost.c    | 28 +++++++++++++++++++++++++++-
 4 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 6264617bab4d..63137ca5147b 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/kernel/module.c b/kernel/module.c
index a4fa44a652a7..be155ec80083 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -807,6 +807,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];
 
@@ -1265,10 +1266,29 @@ static ssize_t show_taint(struct module_attribute *mattr,
 static struct module_attribute modinfo_taint =
 	__ATTR(taint, 0444, show_taint, NULL);
 
+/**
+ * struct modinfo_attrs - Module attributes.
+ * @module_uevent: Used to notify udev of events.
+ * @modinfo_version: Module version.
+ * @modinfo_srcversion: Checksum of module source.
+ * @modinfo_scmversion: SCM version of module source.
+ * @modinfo_initstate: Module init state.
+ * @modinfo_coresize: Module core layout size.
+ * @modinfo_initsize: Module init layout size.
+ * @modinfo_taint: Indicates if the module is tainted.
+ * @modinfo_refcnt: Number of references in the kernel to the module.
+ *
+ * These are the module attributes accessible via the sysfs files
+ * /sys/module/<module_name>/<attribute>.
+ *
+ * The following subset of attributes can also be accessed via the modinfo tool
+ * as well: version, srcversion, and scmversion.
+ */
 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..4486eb72240e 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -77,8 +77,23 @@ src := $(obj)
 include $(if $(wildcard $(KBUILD_EXTMOD)/Kbuild), \
              $(KBUILD_EXTMOD)/Kbuild, $(KBUILD_EXTMOD)/Makefile)
 
-# modpost option for external modules
-MODPOST += -e
+# Get the external module's source path. KBUILD_EXTMOD could either be an
+# absolute path or relative path from $(srctree). This makes sure that we
+# aren't using a relative path from a separate working directory (O= or
+# KBUILD_OUTPUT) since that may not be the actual module's SCM project path. So
+# check the path relative to $(srctree) first.
+ifneq ($(realpath $(srctree)/$(KBUILD_EXTMOD) 2>/dev/null),)
+	module_srcpath := $(srctree)/$(KBUILD_EXTMOD)
+else
+	module_srcpath := $(KBUILD_EXTMOD)
+endif
+
+# Get the SCM version of the external module. Sed verifies setlocalversion
+# returns a proper revision based on the SCM type, e.g. git, mercurial, or svn.
+module_scmversion := $(shell $(srctree)/scripts/setlocalversion $(module_srcpath) | \
+	sed -n 's/.*-\(\(\(g\|hg\)[a-fA-F0-9]\+\|svn[0-9]\+\)\(-dirty\)\?\).*\?/\1/p')
+# modpost option for external modules.
+MODPOST += -e$(module_scmversion)
 
 input-symdump := Module.symvers $(KBUILD_EXTRA_SYMBOLS)
 output-symdump := $(KBUILD_EXTMOD)/Module.symvers
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index f882ce0d9327..db59eb2a880d 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 */
@@ -2272,6 +2274,27 @@ 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.
+ * @is_intree: Indicates if the module is in-tree or is an external module.
+ *
+ * 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.
+ *
+ * If it's an in-tree module, then the UTS_RELEASE version is used. Otherwise,
+ * the provided SCM version is used. If there was no SCM version provided to
+ * the script for an external module, then `scmversion` is omitted.
+ */
+static void add_scmversion(struct buffer *b, int is_intree)
+{
+	if (is_intree)
+		buf_printf(b, "\nMODULE_INFO(scmversion, UTS_RELEASE);\n");
+	else 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)
 {
@@ -2559,10 +2582,12 @@ 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, "e::i:mnT:o:awENd:")) != -1) {
 		switch (opt) {
 		case 'e':
 			external_module = 1;
+			if (optarg)
+				strncpy(module_scmversion, optarg, sizeof(module_scmversion) - 1);
 			break;
 		case 'i':
 			*dump_read_iter =
@@ -2645,6 +2670,7 @@ int main(int argc, char **argv)
 		add_depends(&buf, mod);
 		add_moddevtable(&buf, mod);
 		add_srcversion(&buf, mod);
+		add_scmversion(&buf, !external_module);
 
 		sprintf(fname, "%s.mod.c", mod->name);
 		write_if_changed(&buf, fname);
-- 
2.29.2.454.gaff20da3a2-goog

Re: [PATCH v1 2/2] modules: add scmversion field

[Greg KH]

On Sat, Nov 21, 2020 at 01:16:51AM +0000, Will McVicker wrote:
> Add the modinfo field `scmversion` to include the SCM version of kernel
> modules, e.g. git sha1. This allows one to identify the exact source
> code version of a given kernel module.
> 
> You can retrieve it in two ways,
> 
> 1) By using modinfo
>     > modinfo -F scmversion <module_name>
> 2) By module sysfs node
>     > cat /sys/module/<module_name>/scmversion

I agree with Christoph that this doesn't help any in-kernel stuff, so I
don't think it can be merged.  Why not just build these modules as part
of the normal kernel build process, the Android build system should
allow that, right?

But even if it was ok, you are adding new sysfs attributes without a
Documentation/ABI/ update, which is not ok either, so always remember
that for future patches.

thanks,

greg k-h

Re: [PATCH v1 2/2] modules: add scmversion field

[Greg KH]

On Sat, Nov 21, 2020 at 01:16:51AM +0000, Will McVicker wrote:
> +/**
> + * struct modinfo_attrs - Module attributes.
> + * @module_uevent: Used to notify udev of events.
> + * @modinfo_version: Module version.
> + * @modinfo_srcversion: Checksum of module source.
> + * @modinfo_scmversion: SCM version of module source.
> + * @modinfo_initstate: Module init state.
> + * @modinfo_coresize: Module core layout size.
> + * @modinfo_initsize: Module init layout size.
> + * @modinfo_taint: Indicates if the module is tainted.
> + * @modinfo_refcnt: Number of references in the kernel to the module.
> + *
> + * These are the module attributes accessible via the sysfs files
> + * /sys/module/<module_name>/<attribute>.
> + *
> + * The following subset of attributes can also be accessed via the modinfo tool
> + * as well: version, srcversion, and scmversion.
> + */
>  static struct module_attribute *modinfo_attrs[] = {
>  	&module_uevent,
>  	&modinfo_version,
>  	&modinfo_srcversion,
> +	&modinfo_scmversion,
>  	&modinfo_initstate,
>  	&modinfo_coresize,
>  	&modinfo_initsize,

This isn't the normal way to document an array, with kerneldoc, I don't
think I've seen that anywhere else in the kernel, have you?

Anyway, again, Documentation/ABI/ is the correct place for this.

thanks,

greg k-h

Re: [PATCH v1 0/2] Add support to capture external module's SCM version

[Christoph Hellwig]

On Sat, Nov 21, 2020 at 01:16:49AM +0000, Will McVicker wrote:
> These two patches add module support to capture an external module's SCM
> version as a MODULE_INFO() attribute. This allows users to identity the SCM
> version of a given kernel module by using the modinfo tool or on the device
> via sysfs:

As this obviously is of no use for in-tree modules it falls under the we
don't add code to support things that are not in tree rule and has no
business in the kernel.

Re: [PATCH v1 0/2] Add support to capture external module's SCM version

[William Mcvicker]

On Mon, Nov 23, 2020 at 09:02:57AM +0000, Christoph Hellwig wrote:
> On Sat, Nov 21, 2020 at 01:16:49AM +0000, Will McVicker wrote:
> > These two patches add module support to capture an external module's SCM
> > version as a MODULE_INFO() attribute. This allows users to identity the SCM
> > version of a given kernel module by using the modinfo tool or on the device
> > via sysfs:
> 
> As this obviously is of no use for in-tree modules it falls under the we
> don't add code to support things that are not in tree rule and has no
> business in the kernel.

Hi Christoph,

Ah sorry, I didn't intend this to come across as only for external modules.
That just seemed like the easiest way to explain how the scmversion attribute
can be different from the vermagic. We mainly need this for in-tree kernel
modules since that's where most our drivers are. Let me re-phrase this with
that in mind. Basically, I like to look at this as an improved version of the
existing srcversion module attribute since it allows you to easily identify the
module version with a quick SCM version string check instead of doing a full
checksum on the module source.

For example, we have a setup to test kernel changes on the hikey and db845c
devices without updating the kernel modules. Without this scmversion module
attribute, you can't identify the original module version using `uname
-r`. And for kernel modules in the initramfs, you can't even use modinfo to get
the module vermagic.  With this patch, you are able to get the SCM version for
*all* kernel modules (on disk and in the initramfs) via the sysfs node:
/sys/module/<mod>/scmversion. This also works the other way around when
developers update their kernel modules to fix some bug (like a security
vulnerability) but don't need to update the full kernel.

Regarding the documentation, Greg, thanks for pointing out Documentation/ABI/!
I seached high and low for documentation on the other module sysfs attributes,
but couldn't find anything. I'll update the proper documentation in the v2
patchset.

Thanks,
Will

Re: [PATCH v1 0/2] Add support to capture external module's SCM version

[Jessica Yu]

+++ William Mcvicker [23/11/20 14:13 -0800]:
>On Mon, Nov 23, 2020 at 09:02:57AM +0000, Christoph Hellwig wrote:
>> On Sat, Nov 21, 2020 at 01:16:49AM +0000, Will McVicker wrote:
>> > These two patches add module support to capture an external module's SCM
>> > version as a MODULE_INFO() attribute. This allows users to identity the SCM
>> > version of a given kernel module by using the modinfo tool or on the device
>> > via sysfs:
>>
>> As this obviously is of no use for in-tree modules it falls under the we
>> don't add code to support things that are not in tree rule and has no
>> business in the kernel.
>
>Hi Christoph,
>
>Ah sorry, I didn't intend this to come across as only for external modules.
>That just seemed like the easiest way to explain how the scmversion attribute
>can be different from the vermagic. We mainly need this for in-tree kernel
>modules since that's where most our drivers are. Let me re-phrase this with
>that in mind. Basically, I like to look at this as an improved version of the
>existing srcversion module attribute since it allows you to easily identify the
>module version with a quick SCM version string check instead of doing a full
>checksum on the module source.
>
>For example, we have a setup to test kernel changes on the hikey and db845c
>devices without updating the kernel modules. Without this scmversion module
>attribute, you can't identify the original module version using `uname
>-r`. And for kernel modules in the initramfs, you can't even use modinfo to get
>the module vermagic.  With this patch, you are able to get the SCM version for
>*all* kernel modules (on disk and in the initramfs) via the sysfs node:
>/sys/module/<mod>/scmversion. This also works the other way around when
>developers update their kernel modules to fix some bug (like a security
>vulnerability) but don't need to update the full kernel.

Hi Will,

If this were also intended for in-tree kernel modules, then why do
intree modules only get the UTS_RELEASE string in their scmversion
field, which basically already exists in the vermagic? Or do you plan
to change that?

Jessica

Re: [PATCH v1 0/2] Add support to capture external module's SCM version

[William Mcvicker]

On Tue, Nov 24, 2020 at 10:31:18AM +0100, Jessica Yu wrote:
> +++ William Mcvicker [23/11/20 14:13 -0800]:
> > On Mon, Nov 23, 2020 at 09:02:57AM +0000, Christoph Hellwig wrote:
> > > On Sat, Nov 21, 2020 at 01:16:49AM +0000, Will McVicker wrote:
> > > > These two patches add module support to capture an external module's SCM
> > > > version as a MODULE_INFO() attribute. This allows users to identity the SCM
> > > > version of a given kernel module by using the modinfo tool or on the device
> > > > via sysfs:
> > > 
> > > As this obviously is of no use for in-tree modules it falls under the we
> > > don't add code to support things that are not in tree rule and has no
> > > business in the kernel.
> > 
> > Hi Christoph,
> > 
> > Ah sorry, I didn't intend this to come across as only for external modules.
> > That just seemed like the easiest way to explain how the scmversion attribute
> > can be different from the vermagic. We mainly need this for in-tree kernel
> > modules since that's where most our drivers are. Let me re-phrase this with
> > that in mind. Basically, I like to look at this as an improved version of the
> > existing srcversion module attribute since it allows you to easily identify the
> > module version with a quick SCM version string check instead of doing a full
> > checksum on the module source.
> > 
> > For example, we have a setup to test kernel changes on the hikey and db845c
> > devices without updating the kernel modules. Without this scmversion module
> > attribute, you can't identify the original module version using `uname
> > -r`. And for kernel modules in the initramfs, you can't even use modinfo to get
> > the module vermagic.  With this patch, you are able to get the SCM version for
> > *all* kernel modules (on disk and in the initramfs) via the sysfs node:
> > /sys/module/<mod>/scmversion. This also works the other way around when
> > developers update their kernel modules to fix some bug (like a security
> > vulnerability) but don't need to update the full kernel.
> 
> Hi Will,
> 
> If this were also intended for in-tree kernel modules, then why do
> intree modules only get the UTS_RELEASE string in their scmversion
> field, which basically already exists in the vermagic? Or do you plan
> to change that?
> 
> Jessica

Hi Jessica,

Thanks for asking! The reason in-tree kernel modules get the UTS_RELEASE string
is for a few reasons:

(1) It contains the SCM version (since UTS_RELEASE has that).
(2) It allows you to get the SCM version via the sysfs node (useful for modules
in the initramfs).
(3) It helps identify that that particular kernel module was in-tree when
originally compiled.
(4) Using UTS_RELEASE also allows us to respect the privacy of kernels with
"# CONFIG_LOCALVERSION_AUTO is not set" by not including the SCM version in the
module scmversion attribute.

Now, if we don't care about knowing if a module was in-tree or not (since
we only care about in-tree modules here anyway), I can update the patch to have
a consistent format regardless of in-tree or external. Personally, I like the
UTS_RELEASE version better because it gives me more information from the sysfs
node which is useful when debugging issues related to modules loaded in
initramfs.

Thanks,
Will

Re: [PATCH v1 0/2] Add support to capture external module's SCM version

[Greg Kroah-Hartman]

On Tue, Nov 24, 2020 at 10:05:16AM -0800, William Mcvicker wrote:
> On Tue, Nov 24, 2020 at 10:31:18AM +0100, Jessica Yu wrote:
> > +++ William Mcvicker [23/11/20 14:13 -0800]:
> > > On Mon, Nov 23, 2020 at 09:02:57AM +0000, Christoph Hellwig wrote:
> > > > On Sat, Nov 21, 2020 at 01:16:49AM +0000, Will McVicker wrote:
> > > > > These two patches add module support to capture an external module's SCM
> > > > > version as a MODULE_INFO() attribute. This allows users to identity the SCM
> > > > > version of a given kernel module by using the modinfo tool or on the device
> > > > > via sysfs:
> > > > 
> > > > As this obviously is of no use for in-tree modules it falls under the we
> > > > don't add code to support things that are not in tree rule and has no
> > > > business in the kernel.
> > > 
> > > Hi Christoph,
> > > 
> > > Ah sorry, I didn't intend this to come across as only for external modules.
> > > That just seemed like the easiest way to explain how the scmversion attribute
> > > can be different from the vermagic. We mainly need this for in-tree kernel
> > > modules since that's where most our drivers are. Let me re-phrase this with
> > > that in mind. Basically, I like to look at this as an improved version of the
> > > existing srcversion module attribute since it allows you to easily identify the
> > > module version with a quick SCM version string check instead of doing a full
> > > checksum on the module source.
> > > 
> > > For example, we have a setup to test kernel changes on the hikey and db845c
> > > devices without updating the kernel modules. Without this scmversion module
> > > attribute, you can't identify the original module version using `uname
> > > -r`. And for kernel modules in the initramfs, you can't even use modinfo to get
> > > the module vermagic.  With this patch, you are able to get the SCM version for
> > > *all* kernel modules (on disk and in the initramfs) via the sysfs node:
> > > /sys/module/<mod>/scmversion. This also works the other way around when
> > > developers update their kernel modules to fix some bug (like a security
> > > vulnerability) but don't need to update the full kernel.
> > 
> > Hi Will,
> > 
> > If this were also intended for in-tree kernel modules, then why do
> > intree modules only get the UTS_RELEASE string in their scmversion
> > field, which basically already exists in the vermagic? Or do you plan
> > to change that?
> > 
> > Jessica
> 
> Hi Jessica,
> 
> Thanks for asking! The reason in-tree kernel modules get the UTS_RELEASE string
> is for a few reasons:
> 
> (1) It contains the SCM version (since UTS_RELEASE has that).
> (2) It allows you to get the SCM version via the sysfs node (useful for modules
> in the initramfs).
> (3) It helps identify that that particular kernel module was in-tree when
> originally compiled.
> (4) Using UTS_RELEASE also allows us to respect the privacy of kernels with
> "# CONFIG_LOCALVERSION_AUTO is not set" by not including the SCM version in the
> module scmversion attribute.
> 
> Now, if we don't care about knowing if a module was in-tree or not (since
> we only care about in-tree modules here anyway), I can update the patch to have
> a consistent format regardless of in-tree or external. Personally, I like the
> UTS_RELEASE version better because it gives me more information from the sysfs
> node which is useful when debugging issues related to modules loaded in
> initramfs.

We already know if a module was built in-or-out of tree, the "O" taint
flag is set, so that information is already in the module today, right?
Can't that be used somehow here?

thanks,

greg k-h

Re: [PATCH v1 0/2] Add support to capture external module's SCM version

[William Mcvicker]

On Tue, Nov 24, 2020 at 07:12:40PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Nov 24, 2020 at 10:05:16AM -0800, William Mcvicker wrote:
> > On Tue, Nov 24, 2020 at 10:31:18AM +0100, Jessica Yu wrote:
> > > +++ William Mcvicker [23/11/20 14:13 -0800]:
> > > > On Mon, Nov 23, 2020 at 09:02:57AM +0000, Christoph Hellwig wrote:
> > > > > On Sat, Nov 21, 2020 at 01:16:49AM +0000, Will McVicker wrote:
> > > > > > These two patches add module support to capture an external module's SCM
> > > > > > version as a MODULE_INFO() attribute. This allows users to identity the SCM
> > > > > > version of a given kernel module by using the modinfo tool or on the device
> > > > > > via sysfs:
> > > > > 
> > > > > As this obviously is of no use for in-tree modules it falls under the we
> > > > > don't add code to support things that are not in tree rule and has no
> > > > > business in the kernel.
> > > > 
> > > > Hi Christoph,
> > > > 
> > > > Ah sorry, I didn't intend this to come across as only for external modules.
> > > > That just seemed like the easiest way to explain how the scmversion attribute
> > > > can be different from the vermagic. We mainly need this for in-tree kernel
> > > > modules since that's where most our drivers are. Let me re-phrase this with
> > > > that in mind. Basically, I like to look at this as an improved version of the
> > > > existing srcversion module attribute since it allows you to easily identify the
> > > > module version with a quick SCM version string check instead of doing a full
> > > > checksum on the module source.
> > > > 
> > > > For example, we have a setup to test kernel changes on the hikey and db845c
> > > > devices without updating the kernel modules. Without this scmversion module
> > > > attribute, you can't identify the original module version using `uname
> > > > -r`. And for kernel modules in the initramfs, you can't even use modinfo to get
> > > > the module vermagic.  With this patch, you are able to get the SCM version for
> > > > *all* kernel modules (on disk and in the initramfs) via the sysfs node:
> > > > /sys/module/<mod>/scmversion. This also works the other way around when
> > > > developers update their kernel modules to fix some bug (like a security
> > > > vulnerability) but don't need to update the full kernel.
> > > 
> > > Hi Will,
> > > 
> > > If this were also intended for in-tree kernel modules, then why do
> > > intree modules only get the UTS_RELEASE string in their scmversion
> > > field, which basically already exists in the vermagic? Or do you plan
> > > to change that?
> > > 
> > > Jessica
> > 
> > Hi Jessica,
> > 
> > Thanks for asking! The reason in-tree kernel modules get the UTS_RELEASE string
> > is for a few reasons:
> > 
> > (1) It contains the SCM version (since UTS_RELEASE has that).
> > (2) It allows you to get the SCM version via the sysfs node (useful for modules
> > in the initramfs).
> > (3) It helps identify that that particular kernel module was in-tree when
> > originally compiled.
> > (4) Using UTS_RELEASE also allows us to respect the privacy of kernels with
> > "# CONFIG_LOCALVERSION_AUTO is not set" by not including the SCM version in the
> > module scmversion attribute.
> > 
> > Now, if we don't care about knowing if a module was in-tree or not (since
> > we only care about in-tree modules here anyway), I can update the patch to have
> > a consistent format regardless of in-tree or external. Personally, I like the
> > UTS_RELEASE version better because it gives me more information from the sysfs
> > node which is useful when debugging issues related to modules loaded in
> > initramfs.
> 
> We already know if a module was built in-or-out of tree, the "O" taint
> flag is set, so that information is already in the module today, right?
> Can't that be used somehow here?
> 
> thanks,
> 
> greg k-h
Hi Greg,

Let me prefix this with this, I do see the benefits of having a consistent
scmversion format for intree and out-of-tree modules. So I'm happy to fix that
in the next patchset.

Now, I could be wrong, but I believe the taint flag is only printed when the
module is loaded:

  XXX: loading out-of-tree module taints kernel.

or when there's a kernel WARNING or kernel crash. But that assumes you have the
full logs when the kernel booted or you have a full crash stack in the kernel.

Modinfo does have an attribute that indicates if the module is intree or
not:

$ modinfo -F intree out_dir/./net/netfilter/nf_log_common.ko
Y

But that is not queriable via sysfs. Ideally, we'd like to be able to get all
this information via sysfs so that it can be captured in our bug reports.

Thanks,
Will

Re: [PATCH v1 0/2] Add support to capture external module's SCM version

[Greg Kroah-Hartman]

On Tue, Nov 24, 2020 at 10:31:39AM -0800, William Mcvicker wrote:
> On Tue, Nov 24, 2020 at 07:12:40PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Nov 24, 2020 at 10:05:16AM -0800, William Mcvicker wrote:
> > > On Tue, Nov 24, 2020 at 10:31:18AM +0100, Jessica Yu wrote:
> > > > +++ William Mcvicker [23/11/20 14:13 -0800]:
> > > > > On Mon, Nov 23, 2020 at 09:02:57AM +0000, Christoph Hellwig wrote:
> > > > > > On Sat, Nov 21, 2020 at 01:16:49AM +0000, Will McVicker wrote:
> > > > > > > These two patches add module support to capture an external module's SCM
> > > > > > > version as a MODULE_INFO() attribute. This allows users to identity the SCM
> > > > > > > version of a given kernel module by using the modinfo tool or on the device
> > > > > > > via sysfs:
> > > > > > 
> > > > > > As this obviously is of no use for in-tree modules it falls under the we
> > > > > > don't add code to support things that are not in tree rule and has no
> > > > > > business in the kernel.
> > > > > 
> > > > > Hi Christoph,
> > > > > 
> > > > > Ah sorry, I didn't intend this to come across as only for external modules.
> > > > > That just seemed like the easiest way to explain how the scmversion attribute
> > > > > can be different from the vermagic. We mainly need this for in-tree kernel
> > > > > modules since that's where most our drivers are. Let me re-phrase this with
> > > > > that in mind. Basically, I like to look at this as an improved version of the
> > > > > existing srcversion module attribute since it allows you to easily identify the
> > > > > module version with a quick SCM version string check instead of doing a full
> > > > > checksum on the module source.
> > > > > 
> > > > > For example, we have a setup to test kernel changes on the hikey and db845c
> > > > > devices without updating the kernel modules. Without this scmversion module
> > > > > attribute, you can't identify the original module version using `uname
> > > > > -r`. And for kernel modules in the initramfs, you can't even use modinfo to get
> > > > > the module vermagic.  With this patch, you are able to get the SCM version for
> > > > > *all* kernel modules (on disk and in the initramfs) via the sysfs node:
> > > > > /sys/module/<mod>/scmversion. This also works the other way around when
> > > > > developers update their kernel modules to fix some bug (like a security
> > > > > vulnerability) but don't need to update the full kernel.
> > > > 
> > > > Hi Will,
> > > > 
> > > > If this were also intended for in-tree kernel modules, then why do
> > > > intree modules only get the UTS_RELEASE string in their scmversion
> > > > field, which basically already exists in the vermagic? Or do you plan
> > > > to change that?
> > > > 
> > > > Jessica
> > > 
> > > Hi Jessica,
> > > 
> > > Thanks for asking! The reason in-tree kernel modules get the UTS_RELEASE string
> > > is for a few reasons:
> > > 
> > > (1) It contains the SCM version (since UTS_RELEASE has that).
> > > (2) It allows you to get the SCM version via the sysfs node (useful for modules
> > > in the initramfs).
> > > (3) It helps identify that that particular kernel module was in-tree when
> > > originally compiled.
> > > (4) Using UTS_RELEASE also allows us to respect the privacy of kernels with
> > > "# CONFIG_LOCALVERSION_AUTO is not set" by not including the SCM version in the
> > > module scmversion attribute.
> > > 
> > > Now, if we don't care about knowing if a module was in-tree or not (since
> > > we only care about in-tree modules here anyway), I can update the patch to have
> > > a consistent format regardless of in-tree or external. Personally, I like the
> > > UTS_RELEASE version better because it gives me more information from the sysfs
> > > node which is useful when debugging issues related to modules loaded in
> > > initramfs.
> > 
> > We already know if a module was built in-or-out of tree, the "O" taint
> > flag is set, so that information is already in the module today, right?
> > Can't that be used somehow here?
> > 
> > thanks,
> > 
> > greg k-h
> Hi Greg,
> 
> Let me prefix this with this, I do see the benefits of having a consistent
> scmversion format for intree and out-of-tree modules. So I'm happy to fix that
> in the next patchset.
> 
> Now, I could be wrong, but I believe the taint flag is only printed when the
> module is loaded:
> 
>   XXX: loading out-of-tree module taints kernel.
> 
> or when there's a kernel WARNING or kernel crash. But that assumes you have the
> full logs when the kernel booted or you have a full crash stack in the kernel.
> 
> Modinfo does have an attribute that indicates if the module is intree or
> not:
> 
> $ modinfo -F intree out_dir/./net/netfilter/nf_log_common.ko
> Y
> 
> But that is not queriable via sysfs.

Look at the file in /sys/modules/MODULENAME/taint

That should show you this value.

> Ideally, we'd like to be able to get all
> this information via sysfs so that it can be captured in our bug reports.

I think you already have it :)

This is independent of your "source code id value" idea though...

thanks,

greg k-h

Re: [PATCH v1 0/2] Add support to capture external module's SCM version

[William Mcvicker]

On Tue, Nov 24, 2020 at 09:24:26PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Nov 24, 2020 at 10:31:39AM -0800, William Mcvicker wrote:
> > On Tue, Nov 24, 2020 at 07:12:40PM +0100, Greg Kroah-Hartman wrote:
> > > On Tue, Nov 24, 2020 at 10:05:16AM -0800, William Mcvicker wrote:
> > > > On Tue, Nov 24, 2020 at 10:31:18AM +0100, Jessica Yu wrote:
> > > > > +++ William Mcvicker [23/11/20 14:13 -0800]:
> > > > > > On Mon, Nov 23, 2020 at 09:02:57AM +0000, Christoph Hellwig wrote:
> > > > > > > On Sat, Nov 21, 2020 at 01:16:49AM +0000, Will McVicker wrote:
> > > > > > > > These two patches add module support to capture an external module's SCM
> > > > > > > > version as a MODULE_INFO() attribute. This allows users to identity the SCM
> > > > > > > > version of a given kernel module by using the modinfo tool or on the device
> > > > > > > > via sysfs:
> > > > > > > 
> > > > > > > As this obviously is of no use for in-tree modules it falls under the we
> > > > > > > don't add code to support things that are not in tree rule and has no
> > > > > > > business in the kernel.
> > > > > > 
> > > > > > Hi Christoph,
> > > > > > 
> > > > > > Ah sorry, I didn't intend this to come across as only for external modules.
> > > > > > That just seemed like the easiest way to explain how the scmversion attribute
> > > > > > can be different from the vermagic. We mainly need this for in-tree kernel
> > > > > > modules since that's where most our drivers are. Let me re-phrase this with
> > > > > > that in mind. Basically, I like to look at this as an improved version of the
> > > > > > existing srcversion module attribute since it allows you to easily identify the
> > > > > > module version with a quick SCM version string check instead of doing a full
> > > > > > checksum on the module source.
> > > > > > 
> > > > > > For example, we have a setup to test kernel changes on the hikey and db845c
> > > > > > devices without updating the kernel modules. Without this scmversion module
> > > > > > attribute, you can't identify the original module version using `uname
> > > > > > -r`. And for kernel modules in the initramfs, you can't even use modinfo to get
> > > > > > the module vermagic.  With this patch, you are able to get the SCM version for
> > > > > > *all* kernel modules (on disk and in the initramfs) via the sysfs node:
> > > > > > /sys/module/<mod>/scmversion. This also works the other way around when
> > > > > > developers update their kernel modules to fix some bug (like a security
> > > > > > vulnerability) but don't need to update the full kernel.
> > > > > 
> > > > > Hi Will,
> > > > > 
> > > > > If this were also intended for in-tree kernel modules, then why do
> > > > > intree modules only get the UTS_RELEASE string in their scmversion
> > > > > field, which basically already exists in the vermagic? Or do you plan
> > > > > to change that?
> > > > > 
> > > > > Jessica
> > > > 
> > > > Hi Jessica,
> > > > 
> > > > Thanks for asking! The reason in-tree kernel modules get the UTS_RELEASE string
> > > > is for a few reasons:
> > > > 
> > > > (1) It contains the SCM version (since UTS_RELEASE has that).
> > > > (2) It allows you to get the SCM version via the sysfs node (useful for modules
> > > > in the initramfs).
> > > > (3) It helps identify that that particular kernel module was in-tree when
> > > > originally compiled.
> > > > (4) Using UTS_RELEASE also allows us to respect the privacy of kernels with
> > > > "# CONFIG_LOCALVERSION_AUTO is not set" by not including the SCM version in the
> > > > module scmversion attribute.
> > > > 
> > > > Now, if we don't care about knowing if a module was in-tree or not (since
> > > > we only care about in-tree modules here anyway), I can update the patch to have
> > > > a consistent format regardless of in-tree or external. Personally, I like the
> > > > UTS_RELEASE version better because it gives me more information from the sysfs
> > > > node which is useful when debugging issues related to modules loaded in
> > > > initramfs.
> > > 
> > > We already know if a module was built in-or-out of tree, the "O" taint
> > > flag is set, so that information is already in the module today, right?
> > > Can't that be used somehow here?
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > Hi Greg,
> > 
> > Let me prefix this with this, I do see the benefits of having a consistent
> > scmversion format for intree and out-of-tree modules. So I'm happy to fix that
> > in the next patchset.
> > 
> > Now, I could be wrong, but I believe the taint flag is only printed when the
> > module is loaded:
> > 
> >   XXX: loading out-of-tree module taints kernel.
> > 
> > or when there's a kernel WARNING or kernel crash. But that assumes you have the
> > full logs when the kernel booted or you have a full crash stack in the kernel.
> > 
> > Modinfo does have an attribute that indicates if the module is intree or
> > not:
> > 
> > $ modinfo -F intree out_dir/./net/netfilter/nf_log_common.ko
> > Y
> > 
> > But that is not queriable via sysfs.
> 
> Look at the file in /sys/modules/MODULENAME/taint
> 
> That should show you this value.
> 
> > Ideally, we'd like to be able to get all
> > this information via sysfs so that it can be captured in our bug reports.
> 
> I think you already have it :)
> 
> This is independent of your "source code id value" idea though...
> 
> thanks,
> 
> greg k-h

Thanks for pointing out the taint sysfs node. With that, the only reason I can see
using UTS_RELEASE over always using the SCM version is to immediately get the
extra version information like the 5.10.0-rc4 part without having to extract
that from the SCM version. For scripting reasons and consistency I think it
would be best to just stick to using the SCM version alone and not UTS_RELEASE.
Unless someone objects, I'll update v2 to use the SCM version (not UTS_RELEASE)
always.

Thanks,
Will

Re: [PATCH v1 0/2] Add support to capture external module's SCM version

[Saravana Kannan]

On Tue, Nov 24, 2020 at 12:41 PM William Mcvicker
<willmcvicker@google.com> wrote:
>
> On Tue, Nov 24, 2020 at 09:24:26PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Nov 24, 2020 at 10:31:39AM -0800, William Mcvicker wrote:
> > > On Tue, Nov 24, 2020 at 07:12:40PM +0100, Greg Kroah-Hartman wrote:
> > > > On Tue, Nov 24, 2020 at 10:05:16AM -0800, William Mcvicker wrote:
> > > > > On Tue, Nov 24, 2020 at 10:31:18AM +0100, Jessica Yu wrote:
> > > > > > +++ William Mcvicker [23/11/20 14:13 -0800]:
> > > > > > > On Mon, Nov 23, 2020 at 09:02:57AM +0000, Christoph Hellwig wrote:
> > > > > > > > On Sat, Nov 21, 2020 at 01:16:49AM +0000, Will McVicker wrote:
> > > > > > > > > These two patches add module support to capture an external module's SCM
> > > > > > > > > version as a MODULE_INFO() attribute. This allows users to identity the SCM
> > > > > > > > > version of a given kernel module by using the modinfo tool or on the device
> > > > > > > > > via sysfs:
> > > > > > > >
> > > > > > > > As this obviously is of no use for in-tree modules it falls under the we
> > > > > > > > don't add code to support things that are not in tree rule and has no
> > > > > > > > business in the kernel.
> > > > > > >
> > > > > > > Hi Christoph,
> > > > > > >
> > > > > > > Ah sorry, I didn't intend this to come across as only for external modules.
> > > > > > > That just seemed like the easiest way to explain how the scmversion attribute
> > > > > > > can be different from the vermagic. We mainly need this for in-tree kernel
> > > > > > > modules since that's where most our drivers are. Let me re-phrase this with
> > > > > > > that in mind. Basically, I like to look at this as an improved version of the
> > > > > > > existing srcversion module attribute since it allows you to easily identify the
> > > > > > > module version with a quick SCM version string check instead of doing a full
> > > > > > > checksum on the module source.
> > > > > > >
> > > > > > > For example, we have a setup to test kernel changes on the hikey and db845c
> > > > > > > devices without updating the kernel modules. Without this scmversion module
> > > > > > > attribute, you can't identify the original module version using `uname
> > > > > > > -r`. And for kernel modules in the initramfs, you can't even use modinfo to get
> > > > > > > the module vermagic.  With this patch, you are able to get the SCM version for
> > > > > > > *all* kernel modules (on disk and in the initramfs) via the sysfs node:
> > > > > > > /sys/module/<mod>/scmversion. This also works the other way around when
> > > > > > > developers update their kernel modules to fix some bug (like a security
> > > > > > > vulnerability) but don't need to update the full kernel.
> > > > > >
> > > > > > Hi Will,
> > > > > >
> > > > > > If this were also intended for in-tree kernel modules, then why do
> > > > > > intree modules only get the UTS_RELEASE string in their scmversion
> > > > > > field, which basically already exists in the vermagic? Or do you plan
> > > > > > to change that?
> > > > > >
> > > > > > Jessica
> > > > >
> > > > > Hi Jessica,
> > > > >
> > > > > Thanks for asking! The reason in-tree kernel modules get the UTS_RELEASE string
> > > > > is for a few reasons:
> > > > >
> > > > > (1) It contains the SCM version (since UTS_RELEASE has that).
> > > > > (2) It allows you to get the SCM version via the sysfs node (useful for modules
> > > > > in the initramfs).
> > > > > (3) It helps identify that that particular kernel module was in-tree when
> > > > > originally compiled.
> > > > > (4) Using UTS_RELEASE also allows us to respect the privacy of kernels with
> > > > > "# CONFIG_LOCALVERSION_AUTO is not set" by not including the SCM version in the
> > > > > module scmversion attribute.
> > > > >
> > > > > Now, if we don't care about knowing if a module was in-tree or not (since
> > > > > we only care about in-tree modules here anyway), I can update the patch to have
> > > > > a consistent format regardless of in-tree or external. Personally, I like the
> > > > > UTS_RELEASE version better because it gives me more information from the sysfs
> > > > > node which is useful when debugging issues related to modules loaded in
> > > > > initramfs.
> > > >
> > > > We already know if a module was built in-or-out of tree, the "O" taint
> > > > flag is set, so that information is already in the module today, right?
> > > > Can't that be used somehow here?
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > > Hi Greg,
> > >
> > > Let me prefix this with this, I do see the benefits of having a consistent
> > > scmversion format for intree and out-of-tree modules. So I'm happy to fix that
> > > in the next patchset.
> > >
> > > Now, I could be wrong, but I believe the taint flag is only printed when the
> > > module is loaded:
> > >
> > >   XXX: loading out-of-tree module taints kernel.
> > >
> > > or when there's a kernel WARNING or kernel crash. But that assumes you have the
> > > full logs when the kernel booted or you have a full crash stack in the kernel.
> > >
> > > Modinfo does have an attribute that indicates if the module is intree or
> > > not:
> > >
> > > $ modinfo -F intree out_dir/./net/netfilter/nf_log_common.ko
> > > Y
> > >
> > > But that is not queriable via sysfs.
> >
> > Look at the file in /sys/modules/MODULENAME/taint
> >
> > That should show you this value.
> >
> > > Ideally, we'd like to be able to get all
> > > this information via sysfs so that it can be captured in our bug reports.
> >
> > I think you already have it :)
> >
> > This is independent of your "source code id value" idea though...
> >
> > thanks,
> >
> > greg k-h
>
> Thanks for pointing out the taint sysfs node. With that, the only reason I can see
> using UTS_RELEASE over always using the SCM version is to immediately get the
> extra version information like the 5.10.0-rc4 part without having to extract
> that from the SCM version. For scripting reasons and consistency I think it
> would be best to just stick to using the SCM version alone and not UTS_RELEASE.
> Unless someone objects, I'll update v2 to use the SCM version (not UTS_RELEASE)
> always.

sysfs files are supposed to be simple and follow one value per file in
general. Also, the documentation needs to be simple too. Documenting
two different formats for the same file would be very odd. So +1 to
what Jessica said and +1 to your decision to keep it consistent.

-Saravana

[PATCH v2 0/2] Adds support to capture module's SCM version

[Will McVicker]

Hi All,

I have updated the patchset to:

 *) Include Documentation.
 *) Use a consistent output pattern for the SCM version.

In my debugging, I found that the vermagic reported by modinfo can actually
vary based on how the module was loaded. For example, if you have a module in
the initramfs that is newer than the module on disk, then the initramfs module
will be loaded (not the one on disk) during boot. Then, when you run the
command:

  $ modinfo MODULENAME

The vermagic returned will actually be the vermagic of the module on disk and
not the one in the initramfs which was actually loaded. With that being said,
adding this scmversion attribute ensures that you can *always* get the correct
SCM version of the module that loaded.

Please take a look at the updated patch and provide any comments you find.

Thanks,
Will

Will McVicker (2):
  scripts/setlocalversion: allow running in a subdir
  modules: add scmversion field

 Documentation/ABI/stable/sysfs-module | 17 +++++++++++++++++
 include/linux/module.h                |  1 +
 kernel/module.c                       |  2 ++
 scripts/Makefile.modpost              | 20 ++++++++++++++++++++
 scripts/mod/modpost.c                 | 24 +++++++++++++++++++++++-
 scripts/setlocalversion               |  5 ++---
 6 files changed, 65 insertions(+), 4 deletions(-)

-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v2 1/2] scripts/setlocalversion: allow running in a subdir

[Will McVicker]

Getting the scmversion using scripts/setlocalversion currently only
works when run at the root of a git or mecurial project. This was
introduced in commit 8558f59edf93 ("setlocalversion: Ignote SCMs above
the linux source tree") so that if one is building within a subdir of
a git tree that isn't the kernel git project, then the vermagic wouldn't
include that git sha1. However, the proper solution to that is to just
set this config in your defconfig:

  # CONFIG_LOCALVERSION_AUTO is not set

which is already the default in many defconfigs:

  $ grep -r "CONFIG_LOCALVERSION_AUTO is not set" arch/* | wc -l
  89

So let's bring back this functionality so that we can use
scripts/setlocalversion to capture the SCM version of external modules
that reside within subdirectories of an SCM project.

Signed-off-by: Will McVicker <willmcvicker@google.com>
---
 scripts/setlocalversion | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/scripts/setlocalversion b/scripts/setlocalversion
index bb709eda96cd..cd42009e675b 100755
--- a/scripts/setlocalversion
+++ b/scripts/setlocalversion
@@ -44,8 +44,7 @@ scm_version()
 	fi
 
 	# Check for git and a git repo.
-	if test -z "$(git rev-parse --show-cdup 2>/dev/null)" &&
-	   head=$(git rev-parse --verify HEAD 2>/dev/null); then
+	if head=$(git rev-parse --verify HEAD 2>/dev/null); then
 
 		# If we are at a tagged commit (like "v2.6.30-rc6"), we ignore
 		# it, because this version is defined in the top level Makefile.
@@ -102,7 +101,7 @@ scm_version()
 	fi
 
 	# Check for mercurial and a mercurial repo.
-	if test -d .hg && hgid=$(hg id 2>/dev/null); then
+	if hgid=$(hg id 2>/dev/null); then
 		# Do we have an tagged version?  If so, latesttagdistance == 1
 		if [ "$(hg log -r . --template '{latesttagdistance}')" = "1" ]; then
 			id=$(hg log -r . --template '{latesttag}')
-- 
2.29.2.454.gaff20da3a2-goog

[PATCH v2 2/2] modules: add scmversion field

[Will McVicker]

Add the modinfo field `scmversion` to include the SCM version of kernel
modules, e.g. git sha1. This allows one to identify the exact source
code version of a given kernel module.

You can retrieve it in two ways,

1) By using modinfo
    > modinfo -F scmversion <module_name>
2) By module sysfs node
    > cat /sys/module/<module_name>/scmversion

Signed-off-by: Will McVicker <willmcvicker@google.com>
---
 Documentation/ABI/stable/sysfs-module | 17 +++++++++++++++++
 include/linux/module.h                |  1 +
 kernel/module.c                       |  2 ++
 scripts/Makefile.modpost              | 20 ++++++++++++++++++++
 scripts/mod/modpost.c                 | 24 +++++++++++++++++++++++-
 5 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/stable/sysfs-module b/Documentation/ABI/stable/sysfs-module
index 6272ae5fb366..46c99ec927ab 100644
--- a/Documentation/ABI/stable/sysfs-module
+++ b/Documentation/ABI/stable/sysfs-module
@@ -32,3 +32,20 @@ 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.10
+Contact:	Will McVicker <willmcvicker@google.com>
+Description:	This read-only file will appear if modpost was supplied with an
+		SCM version for the module. The SCM version is retrieved by
+		scripts/setlocalversion, which means that the presence of this
+		file depends on CONFIG_LOCALVERSION_AUTO=y or LOCALVERSION=.
+		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 6264617bab4d..63137ca5147b 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/kernel/module.c b/kernel/module.c
index a4fa44a652a7..a203dab4a03b 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -807,6 +807,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];
 
@@ -1269,6 +1270,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..fb4ddf2bf794 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
 
@@ -77,6 +78,17 @@ src := $(obj)
 include $(if $(wildcard $(KBUILD_EXTMOD)/Kbuild), \
              $(KBUILD_EXTMOD)/Kbuild, $(KBUILD_EXTMOD)/Makefile)
 
+# Get the external module's source path. KBUILD_EXTMOD could either be an
+# absolute path or relative path from $(srctree). This makes sure that we
+# aren't using a relative path from a separate working directory (O= or
+# KBUILD_OUTPUT) since that may not be the actual module's SCM project path. So
+# check the path relative to $(srctree) first.
+ifneq ($(realpath $(srctree)/$(KBUILD_EXTMOD) 2>/dev/null),)
+	module_srcpath := $(srctree)/$(KBUILD_EXTMOD)
+else
+	module_srcpath := $(KBUILD_EXTMOD)
+endif
+
 # modpost option for external modules
 MODPOST += -e
 
@@ -85,6 +97,14 @@ output-symdump := $(KBUILD_EXTMOD)/Module.symvers
 
 endif
 
+# 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.
+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
+
 # 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 f882ce0d9327..db71e0c9ab20 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 */
@@ -2272,6 +2274,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)
 {
@@ -2559,7 +2575,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;
@@ -2597,6 +2613,11 @@ int main(int argc, char **argv)
 		case 'd':
 			missing_namespace_deps = optarg;
 			break;
+		case 'v':
+			if (!optarg)
+				fatal("'-v' requires an argument defining the SCM version.");
+			strncpy(module_scmversion, optarg, sizeof(module_scmversion) - 1);
+			break;
 		default:
 			exit(1);
 		}
@@ -2645,6 +2666,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.29.2.454.gaff20da3a2-goog

Re: [PATCH v2 2/2] modules: add scmversion field

[Jessica Yu]

+++ Will McVicker [25/11/20 01:05 +0000]:
>Add the modinfo field `scmversion` to include the SCM version of kernel
>modules, e.g. git sha1. This allows one to identify the exact source
>code version of a given kernel module.
>
>You can retrieve it in two ways,
>
>1) By using modinfo
>    > modinfo -F scmversion <module_name>
>2) By module sysfs node
>    > cat /sys/module/<module_name>/scmversion
>
>Signed-off-by: Will McVicker <willmcvicker@google.com>

Hi Will, sorry for the delay.

First, what will help is to include a justification and a detailed
explanation of an example use-case (for in-tree modules) in the
changelog/commit message, which is scattered over thread replies but
seems to be omitted here. For example, your explanation here [1] was
pretty helpful and should be included in the changelog. Please
describe in the commit message how this new sysfs entry will be used
and why it is helpful to have this information available for in-tree
modules.

Second, this feature seems to be a debugging aid for distro
developers analogous to (or an alternative to) CONFIG_MODULE_SRCVERSION_ALL.
As opposed to including it by default (implicitly depending on
localversion config options), perhaps this feature should be
packaged in a config option i.e. CONFIG_MODULE_SCMVERSION with a
depends on CONFIG_LOCALVERSION_AUTO. That way the dependency is
explicitly specified and the option could be enabled for distros that
want this.

[1] https://lore.kernel.org/lkml/20201123221338.GA2726675@google.com/

Thanks,

Jessica
>---
> Documentation/ABI/stable/sysfs-module | 17 +++++++++++++++++
> include/linux/module.h                |  1 +
> kernel/module.c                       |  2 ++
> scripts/Makefile.modpost              | 20 ++++++++++++++++++++
> scripts/mod/modpost.c                 | 24 +++++++++++++++++++++++-
> 5 files changed, 63 insertions(+), 1 deletion(-)
>
>diff --git a/Documentation/ABI/stable/sysfs-module b/Documentation/ABI/stable/sysfs-module
>index 6272ae5fb366..46c99ec927ab 100644
>--- a/Documentation/ABI/stable/sysfs-module
>+++ b/Documentation/ABI/stable/sysfs-module
>@@ -32,3 +32,20 @@ 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.10
>+Contact:	Will McVicker <willmcvicker@google.com>
>+Description:	This read-only file will appear if modpost was supplied with an
>+		SCM version for the module. The SCM version is retrieved by
>+		scripts/setlocalversion, which means that the presence of this
>+		file depends on CONFIG_LOCALVERSION_AUTO=y or LOCALVERSION=.
>+		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 6264617bab4d..63137ca5147b 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/kernel/module.c b/kernel/module.c
>index a4fa44a652a7..a203dab4a03b 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -807,6 +807,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];
>
>@@ -1269,6 +1270,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..fb4ddf2bf794 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
>
>@@ -77,6 +78,17 @@ src := $(obj)
> include $(if $(wildcard $(KBUILD_EXTMOD)/Kbuild), \
>              $(KBUILD_EXTMOD)/Kbuild, $(KBUILD_EXTMOD)/Makefile)
>
>+# Get the external module's source path. KBUILD_EXTMOD could either be an
>+# absolute path or relative path from $(srctree). This makes sure that we
>+# aren't using a relative path from a separate working directory (O= or
>+# KBUILD_OUTPUT) since that may not be the actual module's SCM project path. So
>+# check the path relative to $(srctree) first.
>+ifneq ($(realpath $(srctree)/$(KBUILD_EXTMOD) 2>/dev/null),)
>+	module_srcpath := $(srctree)/$(KBUILD_EXTMOD)
>+else
>+	module_srcpath := $(KBUILD_EXTMOD)
>+endif
>+
> # modpost option for external modules
> MODPOST += -e
>
>@@ -85,6 +97,14 @@ output-symdump := $(KBUILD_EXTMOD)/Module.symvers
>
> endif
>
>+# 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.
>+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
>+
> # 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 f882ce0d9327..db71e0c9ab20 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 */
>@@ -2272,6 +2274,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)
> {
>@@ -2559,7 +2575,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;
>@@ -2597,6 +2613,11 @@ int main(int argc, char **argv)
> 		case 'd':
> 			missing_namespace_deps = optarg;
> 			break;
>+		case 'v':
>+			if (!optarg)
>+				fatal("'-v' requires an argument defining the SCM version.");
>+			strncpy(module_scmversion, optarg, sizeof(module_scmversion) - 1);
>+			break;
> 		default:
> 			exit(1);
> 		}
>@@ -2645,6 +2666,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.29.2.454.gaff20da3a2-goog
>

[PATCH v3 0/2] modules: add scmversion field

[Will McVicker]

Hi All,

Thanks Jessica for the feedback! I have updated the commit message to
include the justification and common use cases for the patch series. I
have also added the config MODULE_SCMVERSION so that this is not enabled
by default.

Please take a look and let me know of any concerns or issues found.

Thanks,
Will

Will McVicker (2):
  scripts/setlocalversion: allow running in a subdir
  modules: introduce the MODULE_SCMVERSION config

 Documentation/ABI/stable/sysfs-module | 18 ++++++++++++++++++
 include/linux/module.h                |  1 +
 init/Kconfig                          | 12 ++++++++++++
 kernel/module.c                       |  2 ++
 scripts/Makefile.modpost              | 22 ++++++++++++++++++++++
 scripts/mod/modpost.c                 | 24 +++++++++++++++++++++++-
 scripts/setlocalversion               |  5 ++---
 7 files changed, 80 insertions(+), 4 deletions(-)

-- 
2.29.2.576.ga3fc446d84-goog

[PATCH v3 1/2] scripts/setlocalversion: allow running in a subdir

[Will McVicker]

Getting the scmversion using scripts/setlocalversion currently only
works when run at the root of a git or mecurial project. This was
introduced in commit 8558f59edf93 ("setlocalversion: Ignote SCMs above
the linux source tree") so that if one is building within a subdir of
a git tree that isn't the kernel git project, then the vermagic wouldn't
include that git sha1. However, the proper solution to that is to just
set this config in your defconfig:

  # CONFIG_LOCALVERSION_AUTO is not set

which is already the default in many defconfigs:

  $ grep -r "CONFIG_LOCALVERSION_AUTO is not set" arch/* | wc -l
  89

So let's bring back this functionality so that we can use
scripts/setlocalversion to capture the SCM version of external modules
that reside within subdirectories of an SCM project.

Signed-off-by: Will McVicker <willmcvicker@google.com>
---
 scripts/setlocalversion | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/scripts/setlocalversion b/scripts/setlocalversion
index bb709eda96cd..cd42009e675b 100755
--- a/scripts/setlocalversion
+++ b/scripts/setlocalversion
@@ -44,8 +44,7 @@ scm_version()
 	fi
 
 	# Check for git and a git repo.
-	if test -z "$(git rev-parse --show-cdup 2>/dev/null)" &&
-	   head=$(git rev-parse --verify HEAD 2>/dev/null); then
+	if head=$(git rev-parse --verify HEAD 2>/dev/null); then
 
 		# If we are at a tagged commit (like "v2.6.30-rc6"), we ignore
 		# it, because this version is defined in the top level Makefile.
@@ -102,7 +101,7 @@ scm_version()
 	fi
 
 	# Check for mercurial and a mercurial repo.
-	if test -d .hg && hgid=$(hg id 2>/dev/null); then
+	if hgid=$(hg id 2>/dev/null); then
 		# Do we have an tagged version?  If so, latesttagdistance == 1
 		if [ "$(hg log -r . --template '{latesttagdistance}')" = "1" ]; then
 			id=$(hg log -r . --template '{latesttag}')
-- 
2.29.2.576.ga3fc446d84-goog

Re: [PATCH v3 1/2] scripts/setlocalversion: allow running in a subdir

[Jessica Yu]

Hi Will,

+++ Will McVicker [08/12/20 20:05 +0000]:
>Getting the scmversion using scripts/setlocalversion currently only
>works when run at the root of a git or mecurial project. This was
>introduced in commit 8558f59edf93 ("setlocalversion: Ignote SCMs above
>the linux source tree") so that if one is building within a subdir of
>a git tree that isn't the kernel git project, then the vermagic wouldn't
>include that git sha1. However, the proper solution to that is to just
>set this config in your defconfig:
>
>  # CONFIG_LOCALVERSION_AUTO is not set
>
>which is already the default in many defconfigs:
>
>  $ grep -r "CONFIG_LOCALVERSION_AUTO is not set" arch/* | wc -l
>  89
>
>So let's bring back this functionality so that we can use
>scripts/setlocalversion to capture the SCM version of external modules
>that reside within subdirectories of an SCM project.

Hm, this seems to essentially be a revert of commit 8558f59edf93.
AFAICT from light testing it also reintroduces the issue it was
originally trying to fix, no?

From the reporter:

    Dan McGee <dpmcgee@gmail.com> writes:
    > Note that when in git, you get the appended "+" sign. If
    > LOCALVERSION_AUTO is set, you will get something like
    > "eee-gb01b08c-dirty" (whereas the copy of the tree in /tmp still
    > returns "eee"). It doesn't matter whether the working tree is dirty or
    > clean.
    >
    > Is there a way to disable this? I'm building from a clean tarball that
    > just happens to be unpacked inside a git repository. One would think
    > setting LOCALVERSION_AUTO to false would do it, but no such luck...

Correct me if I'm wrong, but what I'm understanding is that the
original reporter was having trouble with setlocalversion appending
unwanted strings ("+" or "gXXXXXXX-dirty" etc) when building from a
clean tarball that happens to live inside a git repo. Even if
LOCALVERSION_AUTO is disabled it still appends the "+" string if the
SCM above the linux source tree is not at an annotated tag. Since
setlocalversion is getting confused by the presence of a different scm
that commit fixed this by confining the checks to the root of the
(possibly git managed) kernel source tree. Masahiro can probably
better comment since he maintains scripts/*.

In any case, this patch isn't of interest to in-tree modules, since we
can generate the scmversion perfectly fine without it, so I doubt it's
going to get any support here. Would you be fine with dropping the
first patch or would that pose issues?

>Signed-off-by: Will McVicker <willmcvicker@google.com>
>---
> scripts/setlocalversion | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
>diff --git a/scripts/setlocalversion b/scripts/setlocalversion
>index bb709eda96cd..cd42009e675b 100755
>--- a/scripts/setlocalversion
>+++ b/scripts/setlocalversion
>@@ -44,8 +44,7 @@ scm_version()
> 	fi
>
> 	# Check for git and a git repo.
>-	if test -z "$(git rev-parse --show-cdup 2>/dev/null)" &&
>-	   head=$(git rev-parse --verify HEAD 2>/dev/null); then
>+	if head=$(git rev-parse --verify HEAD 2>/dev/null); then
>
> 		# If we are at a tagged commit (like "v2.6.30-rc6"), we ignore
> 		# it, because this version is defined in the top level Makefile.
>@@ -102,7 +101,7 @@ scm_version()
> 	fi
>
> 	# Check for mercurial and a mercurial repo.
>-	if test -d .hg && hgid=$(hg id 2>/dev/null); then
>+	if hgid=$(hg id 2>/dev/null); then
> 		# Do we have an tagged version?  If so, latesttagdistance == 1
> 		if [ "$(hg log -r . --template '{latesttagdistance}')" = "1" ]; then
> 			id=$(hg log -r . --template '{latesttag}')
>-- 
>2.29.2.576.ga3fc446d84-goog
>

Re: [PATCH v3 1/2] scripts/setlocalversion: allow running in a subdir

[Will McVicker]

On Fri, Dec 11, 2020 at 04:33:59PM +0100, Jessica Yu wrote:
> Hi Will,
> 
> +++ Will McVicker [08/12/20 20:05 +0000]:
> > Getting the scmversion using scripts/setlocalversion currently only
> > works when run at the root of a git or mecurial project. This was
> > introduced in commit 8558f59edf93 ("setlocalversion: Ignote SCMs above
> > the linux source tree") so that if one is building within a subdir of
> > a git tree that isn't the kernel git project, then the vermagic wouldn't
> > include that git sha1. However, the proper solution to that is to just
> > set this config in your defconfig:
> > 
> >  # CONFIG_LOCALVERSION_AUTO is not set
> > 
> > which is already the default in many defconfigs:
> > 
> >  $ grep -r "CONFIG_LOCALVERSION_AUTO is not set" arch/* | wc -l
> >  89
> > 
> > So let's bring back this functionality so that we can use
> > scripts/setlocalversion to capture the SCM version of external modules
> > that reside within subdirectories of an SCM project.
> 
> Hm, this seems to essentially be a revert of commit 8558f59edf93.
> AFAICT from light testing it also reintroduces the issue it was
> originally trying to fix, no?
> 
> From the reporter:
> 
>    Dan McGee <dpmcgee@gmail.com> writes:
>    > Note that when in git, you get the appended "+" sign. If
>    > LOCALVERSION_AUTO is set, you will get something like
>    > "eee-gb01b08c-dirty" (whereas the copy of the tree in /tmp still
>    > returns "eee"). It doesn't matter whether the working tree is dirty or
>    > clean.
>    >
>    > Is there a way to disable this? I'm building from a clean tarball that
>    > just happens to be unpacked inside a git repository. One would think
>    > setting LOCALVERSION_AUTO to false would do it, but no such luck...
> 
> Correct me if I'm wrong, but what I'm understanding is that the
> original reporter was having trouble with setlocalversion appending
> unwanted strings ("+" or "gXXXXXXX-dirty" etc) when building from a
> clean tarball that happens to live inside a git repo. Even if
> LOCALVERSION_AUTO is disabled it still appends the "+" string if the
> SCM above the linux source tree is not at an annotated tag. Since
> setlocalversion is getting confused by the presence of a different scm
> that commit fixed this by confining the checks to the root of the
> (possibly git managed) kernel source tree. Masahiro can probably
> better comment since he maintains scripts/*.
> 
> In any case, this patch isn't of interest to in-tree modules, since we
> can generate the scmversion perfectly fine without it, so I doubt it's
> going to get any support here. Would you be fine with dropping the
> first patch or would that pose issues?
> 
> > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > ---
> > scripts/setlocalversion | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/scripts/setlocalversion b/scripts/setlocalversion
> > index bb709eda96cd..cd42009e675b 100755
> > --- a/scripts/setlocalversion
> > +++ b/scripts/setlocalversion
> > @@ -44,8 +44,7 @@ scm_version()
> > 	fi
> > 
> > 	# Check for git and a git repo.
> > -	if test -z "$(git rev-parse --show-cdup 2>/dev/null)" &&
> > -	   head=$(git rev-parse --verify HEAD 2>/dev/null); then
> > +	if head=$(git rev-parse --verify HEAD 2>/dev/null); then
> > 
> > 		# If we are at a tagged commit (like "v2.6.30-rc6"), we ignore
> > 		# it, because this version is defined in the top level Makefile.
> > @@ -102,7 +101,7 @@ scm_version()
> > 	fi
> > 
> > 	# Check for mercurial and a mercurial repo.
> > -	if test -d .hg && hgid=$(hg id 2>/dev/null); then
> > +	if hgid=$(hg id 2>/dev/null); then
> > 		# Do we have an tagged version?  If so, latesttagdistance == 1
> > 		if [ "$(hg log -r . --template '{latesttagdistance}')" = "1" ]; then
> > 			id=$(hg log -r . --template '{latesttag}')
> > -- 
> > 2.29.2.576.ga3fc446d84-goog
> > 
> 
> -- 
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
> 

Hi Jessica,

I'm okay with dropped this first patch since it does only related to external
modules. I'll upload v4 now with just the bits that related to in-tree modules.

Thanks,
Will

[PATCH v3 2/2] modules: introduce the MODULE_SCMVERSION config

[Will McVicker]

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>
---
 Documentation/ABI/stable/sysfs-module | 18 ++++++++++++++++++
 include/linux/module.h                |  1 +
 init/Kconfig                          | 12 ++++++++++++
 kernel/module.c                       |  2 ++
 scripts/Makefile.modpost              | 22 ++++++++++++++++++++++
 scripts/mod/modpost.c                 | 24 +++++++++++++++++++++++-
 6 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/stable/sysfs-module b/Documentation/ABI/stable/sysfs-module
index 6272ae5fb366..2ba731767737 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.11
+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 or LOCALVERSION=.
+		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 6264617bab4d..63137ca5147b 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 0872a5a2e759..7fc4be1ac62c 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -2131,6 +2131,18 @@ 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.
+
 config MODULE_SIG
 	bool "Module signature verification"
 	select MODULE_SIG_FORMAT
diff --git a/kernel/module.c b/kernel/module.c
index a4fa44a652a7..a203dab4a03b 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -807,6 +807,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];
 
@@ -1269,6 +1270,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..f1126b60adb7 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
 
@@ -77,6 +78,17 @@ src := $(obj)
 include $(if $(wildcard $(KBUILD_EXTMOD)/Kbuild), \
              $(KBUILD_EXTMOD)/Kbuild, $(KBUILD_EXTMOD)/Makefile)
 
+# Get the external module's source path. KBUILD_EXTMOD could either be an
+# absolute path or relative path from $(srctree). This makes sure that we
+# aren't using a relative path from a separate working directory (O= or
+# KBUILD_OUTPUT) since that may not be the actual module's SCM project path.
+# So check the path relative to $(srctree) first.
+ifneq ($(realpath $(srctree)/$(KBUILD_EXTMOD) 2>/dev/null),)
+	module_srcpath := $(srctree)/$(KBUILD_EXTMOD)
+else
+	module_srcpath := $(KBUILD_EXTMOD)
+endif
+
 # modpost option for external modules
 MODPOST += -e
 
@@ -85,6 +97,16 @@ output-symdump := $(KBUILD_EXTMOD)/Module.symvers
 
 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.
+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 f882ce0d9327..db71e0c9ab20 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 */
@@ -2272,6 +2274,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)
 {
@@ -2559,7 +2575,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;
@@ -2597,6 +2613,11 @@ int main(int argc, char **argv)
 		case 'd':
 			missing_namespace_deps = optarg;
 			break;
+		case 'v':
+			if (!optarg)
+				fatal("'-v' requires an argument defining the SCM version.");
+			strncpy(module_scmversion, optarg, sizeof(module_scmversion) - 1);
+			break;
 		default:
 			exit(1);
 		}
@@ -2645,6 +2666,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.29.2.576.ga3fc446d84-goog

Re: [PATCH v2 0/2] Adds support to capture module's SCM version

[William Mcvicker]

On Wed, Nov 25, 2020 at 01:05:39AM +0000, Will McVicker wrote:
> Hi All,
> 
> I have updated the patchset to:
> 
>  *) Include Documentation.
>  *) Use a consistent output pattern for the SCM version.
> 
> In my debugging, I found that the vermagic reported by modinfo can actually
> vary based on how the module was loaded. For example, if you have a module in
> the initramfs that is newer than the module on disk, then the initramfs module
> will be loaded (not the one on disk) during boot. Then, when you run the
> command:
> 
>   $ modinfo MODULENAME
> 
> The vermagic returned will actually be the vermagic of the module on disk and
> not the one in the initramfs which was actually loaded. With that being said,
> adding this scmversion attribute ensures that you can *always* get the correct
> SCM version of the module that loaded.
> 
> Please take a look at the updated patch and provide any comments you find.
> 
> Thanks,
> Will
> 
> Will McVicker (2):
>   scripts/setlocalversion: allow running in a subdir
>   modules: add scmversion field
> 
>  Documentation/ABI/stable/sysfs-module | 17 +++++++++++++++++
>  include/linux/module.h                |  1 +
>  kernel/module.c                       |  2 ++
>  scripts/Makefile.modpost              | 20 ++++++++++++++++++++
>  scripts/mod/modpost.c                 | 24 +++++++++++++++++++++++-
>  scripts/setlocalversion               |  5 ++---
>  6 files changed, 65 insertions(+), 4 deletions(-)
> 
> -- 
> 2.29.2.454.gaff20da3a2-goog
> 
Hi Jessica, Masahiro, and Michal,

Friendly reminder :)

Thanks,
Will

Re: [PATCH v2 0/2] Adds support to capture module's SCM version

[Christoph Hellwig]

I think your decription still shows absolutely no benefit for the
kernel, so I'not sure why anyone would want to waste time on this.

Re: [PATCH v2 0/2] Adds support to capture module's SCM version

[Will McVicker]

On Fri, Dec 04, 2020 at 07:51:59AM +0000, Christoph Hellwig wrote:
> I think your decription still shows absolutely no benefit for the
> kernel, so I'not sure why anyone would want to waste time on this.
Hi Christoph,

Did you get a chance to read my earlier responses regarding the uses for
in-tree modules?

The biggest benefit for the upstream community is being about to get the SCM
version for *any* module (including in-tree modules) in the initramfs via the
sysfs node. Currently there is no way to do that and there is no guarantee that
those modules in the initramfs were compiled with the running kernel. In fact,
running,

  modinfo -F vermagic MODULENAME

will return an invalid vermagic string if the same module with different
vermagic strings exists in the initramfs and on disk because modinfo only looks
at the module on disk (not in memory).

The second most useful benefit goes hand-in-hand with MODVERSIONS. The purpose
of MODVERSIONS is to create a stable interface that allows one to update the
kernel and kernel modules (including in-tree modules) independently. So when
developers do update their kernels independently (think for security bug
fixes), the `scmversion` attribute guarantees developers that they can still
identify the modules' or kernel's SCM version.

I hope that helps. If not, then please let me know why these reasons "show
absolutely no benefit for the kernel?"

Thanks,
Will

Re: [PATCH v2 0/2] Adds support to capture module's SCM version

[Christoph Hellwig]

On Fri, Dec 04, 2020 at 10:13:56AM -0800, Will McVicker wrote:
> On Fri, Dec 04, 2020 at 07:51:59AM +0000, Christoph Hellwig wrote:
> > I think your decription still shows absolutely no benefit for the
> > kernel, so I'not sure why anyone would want to waste time on this.
> Hi Christoph,
> 
> Did you get a chance to read my earlier responses regarding the uses for
> in-tree modules?
> 
> The biggest benefit for the upstream community is being about to get the SCM
> version for *any* module (including in-tree modules) in the initramfs via the
> sysfs node. Currently there is no way to do that and there is no guarantee that

That assumes the SCM version of a module has any kind of meaning for
an in-tree module.  Which it doesn't.  If you care about the SCM version
of an in-tree module the only thing we need is one single global sysfs
file.

Re: [PATCH v2 0/2] Adds support to capture module's SCM version

[Will McVicker]

On Fri, Dec 04, 2020 at 06:18:08PM +0000, Christoph Hellwig wrote:
> On Fri, Dec 04, 2020 at 10:13:56AM -0800, Will McVicker wrote:
> > On Fri, Dec 04, 2020 at 07:51:59AM +0000, Christoph Hellwig wrote:
> > > I think your decription still shows absolutely no benefit for the
> > > kernel, so I'not sure why anyone would want to waste time on this.
> > Hi Christoph,
> > 
> > Did you get a chance to read my earlier responses regarding the uses for
> > in-tree modules?
> > 
> > The biggest benefit for the upstream community is being about to get the SCM
> > version for *any* module (including in-tree modules) in the initramfs via the
> > sysfs node. Currently there is no way to do that and there is no guarantee that
> 
> That assumes the SCM version of a module has any kind of meaning for
> an in-tree module.  Which it doesn't.  If you care about the SCM version
> of an in-tree module the only thing we need is one single global sysfs
> file.
Why doesn't it have meaning? With MODVERSIONS, you are able to update in-tree
kernel modules independently of the kernel. That means you can update as many
in-tree modules as you want which would create many different SCM versions (1
per every module update). Also you can update the kernel independently of the
in-tree modules introducing another SCM version.

--Will