live-patching.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 11/13] module: Move sysfs support into a separate file
@ 2022-02-09 17:11 Aaron Tomlin
  2022-02-09 17:11 ` [PATCH v5 12/13] module: Move kdb_modules list out of core code Aaron Tomlin
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Aaron Tomlin @ 2022-02-09 17:11 UTC (permalink / raw)
  To: mcgrof
  Cc: cl, pmladek, mbenes, akpm, jeyu, linux-kernel, linux-modules,
	live-patching, atomlin, ghalat, allen.lkml, void, joe,
	christophe.leroy, msuchanek, oleksandr

No functional change.

This patch migrates module sysfs support out of core code into
kernel/module/sysfs.c. In addition simple code refactoring to
make this possible.

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 kernel/module/Makefile   |   1 +
 kernel/module/internal.h |  24 ++
 kernel/module/main.c     | 458 +--------------------------------------
 kernel/module/sysfs.c    | 425 ++++++++++++++++++++++++++++++++++++
 4 files changed, 453 insertions(+), 455 deletions(-)
 create mode 100644 kernel/module/sysfs.c

diff --git a/kernel/module/Makefile b/kernel/module/Makefile
index c6be08060252..c30141c37eb3 100644
--- a/kernel/module/Makefile
+++ b/kernel/module/Makefile
@@ -14,4 +14,5 @@ obj-$(CONFIG_STRICT_MODULE_RWX) += strict_rwx.o
 obj-$(CONFIG_DEBUG_KMEMLEAK) += debug_kmemleak.o
 obj-$(CONFIG_KALLSYMS) += kallsyms.o
 obj-$(CONFIG_PROC_FS) += procfs.o
+obj-$(CONFIG_SYSFS) += sysfs.o
 endif
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index b67ce836746a..52d30bf6d6b0 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -35,6 +35,9 @@
 extern struct mutex module_mutex;
 extern struct list_head modules;
 
+extern struct module_attribute *modinfo_attrs[];
+extern size_t modinfo_attrs_count;
+
 /* Provided by the linker */
 extern const struct kernel_symbol __start___ksymtab[];
 extern const struct kernel_symbol __stop___ksymtab[];
@@ -206,3 +209,24 @@ static inline char *find_kallsyms_symbol(struct module *mod, unsigned long addr,
 	return NULL;
 }
 #endif /* CONFIG_KALLSYMS */
+
+#ifdef CONFIG_SYSFS
+int mod_sysfs_setup(struct module *mod, const struct load_info *info,
+			   struct kernel_param *kparam, unsigned int num_params);
+void mod_sysfs_fini(struct module *mod);
+void module_remove_modinfo_attrs(struct module *mod, int end);
+void del_usage_links(struct module *mod);
+void init_param_lock(struct module *mod);
+#else /* !CONFIG_SYSFS */
+static int mod_sysfs_setup(struct module *mod,
+			   const struct load_info *info,
+			   struct kernel_param *kparam,
+			   unsigned int num_params)
+{
+	return 0;
+}
+static inline void mod_sysfs_fini(struct module *mod) { }
+static inline void module_remove_modinfo_attrs(struct module *mod, int end) { }
+static inline void del_usage_links(struct module *mod) { }
+static inline void init_param_lock(struct module *mod) { }
+#endif /* CONFIG_SYSFS */
diff --git a/kernel/module/main.c b/kernel/module/main.c
index ff39c556bdf8..c2255954b7df 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -14,9 +14,7 @@
 #include <linux/init.h>
 #include <linux/kallsyms.h>
 #include <linux/buildid.h>
-#include <linux/file.h>
 #include <linux/fs.h>
-#include <linux/sysfs.h>
 #include <linux/kernel.h>
 #include <linux/kernel_read_file.h>
 #include <linux/slab.h>
@@ -995,7 +993,7 @@ static ssize_t show_taint(struct module_attribute *mattr,
 static struct module_attribute modinfo_taint =
 	__ATTR(taint, 0444, show_taint, NULL);
 
-static struct module_attribute *modinfo_attrs[] = {
+struct module_attribute *modinfo_attrs[] = {
 	&module_uevent,
 	&modinfo_version,
 	&modinfo_srcversion,
@@ -1009,6 +1007,8 @@ static struct module_attribute *modinfo_attrs[] = {
 	NULL,
 };
 
+size_t modinfo_attrs_count = ARRAY_SIZE(modinfo_attrs);
+
 static const char vermagic[] = VERMAGIC_STRING;
 
 static int try_to_force_load(struct module *mod, const char *reason)
@@ -1259,458 +1259,6 @@ resolve_symbol_wait(struct module *mod,
 	return ksym;
 }
 
-/*
- * /sys/module/foo/sections stuff
- * J. Corbet <corbet@lwn.net>
- */
-#ifdef CONFIG_SYSFS
-
-#ifdef CONFIG_KALLSYMS
-struct module_sect_attr {
-	struct bin_attribute battr;
-	unsigned long address;
-};
-
-struct module_sect_attrs {
-	struct attribute_group grp;
-	unsigned int nsections;
-	struct module_sect_attr attrs[];
-};
-
-static ssize_t module_sect_read(struct file *file, struct kobject *kobj,
-				struct bin_attribute *battr,
-				char *buf, loff_t pos, size_t count)
-{
-	struct module_sect_attr *sattr =
-		container_of(battr, struct module_sect_attr, battr);
-	char bounce[MODULE_SECT_READ_SIZE + 1];
-	size_t wrote;
-
-	if (pos != 0)
-		return -EINVAL;
-
-	/*
-	 * Since we're a binary read handler, we must account for the
-	 * trailing NUL byte that sprintf will write: if "buf" is
-	 * too small to hold the NUL, or the NUL is exactly the last
-	 * byte, the read will look like it got truncated by one byte.
-	 * Since there is no way to ask sprintf nicely to not write
-	 * the NUL, we have to use a bounce buffer.
-	 */
-	wrote = scnprintf(bounce, sizeof(bounce), "0x%px\n",
-			 kallsyms_show_value(file->f_cred)
-				? (void *)sattr->address : NULL);
-	count = min(count, wrote);
-	memcpy(buf, bounce, count);
-
-	return count;
-}
-
-static void free_sect_attrs(struct module_sect_attrs *sect_attrs)
-{
-	unsigned int section;
-
-	for (section = 0; section < sect_attrs->nsections; section++)
-		kfree(sect_attrs->attrs[section].battr.attr.name);
-	kfree(sect_attrs);
-}
-
-static void add_sect_attrs(struct module *mod, const struct load_info *info)
-{
-	unsigned int nloaded = 0, i, size[2];
-	struct module_sect_attrs *sect_attrs;
-	struct module_sect_attr *sattr;
-	struct bin_attribute **gattr;
-
-	/* Count loaded sections and allocate structures */
-	for (i = 0; i < info->hdr->e_shnum; i++)
-		if (!sect_empty(&info->sechdrs[i]))
-			nloaded++;
-	size[0] = ALIGN(struct_size(sect_attrs, attrs, nloaded),
-			sizeof(sect_attrs->grp.bin_attrs[0]));
-	size[1] = (nloaded + 1) * sizeof(sect_attrs->grp.bin_attrs[0]);
-	sect_attrs = kzalloc(size[0] + size[1], GFP_KERNEL);
-	if (sect_attrs == NULL)
-		return;
-
-	/* Setup section attributes. */
-	sect_attrs->grp.name = "sections";
-	sect_attrs->grp.bin_attrs = (void *)sect_attrs + size[0];
-
-	sect_attrs->nsections = 0;
-	sattr = &sect_attrs->attrs[0];
-	gattr = &sect_attrs->grp.bin_attrs[0];
-	for (i = 0; i < info->hdr->e_shnum; i++) {
-		Elf_Shdr *sec = &info->sechdrs[i];
-		if (sect_empty(sec))
-			continue;
-		sysfs_bin_attr_init(&sattr->battr);
-		sattr->address = sec->sh_addr;
-		sattr->battr.attr.name =
-			kstrdup(info->secstrings + sec->sh_name, GFP_KERNEL);
-		if (sattr->battr.attr.name == NULL)
-			goto out;
-		sect_attrs->nsections++;
-		sattr->battr.read = module_sect_read;
-		sattr->battr.size = MODULE_SECT_READ_SIZE;
-		sattr->battr.attr.mode = 0400;
-		*(gattr++) = &(sattr++)->battr;
-	}
-	*gattr = NULL;
-
-	if (sysfs_create_group(&mod->mkobj.kobj, &sect_attrs->grp))
-		goto out;
-
-	mod->sect_attrs = sect_attrs;
-	return;
-  out:
-	free_sect_attrs(sect_attrs);
-}
-
-static void remove_sect_attrs(struct module *mod)
-{
-	if (mod->sect_attrs) {
-		sysfs_remove_group(&mod->mkobj.kobj,
-				   &mod->sect_attrs->grp);
-		/*
-		 * We are positive that no one is using any sect attrs
-		 * at this point.  Deallocate immediately.
-		 */
-		free_sect_attrs(mod->sect_attrs);
-		mod->sect_attrs = NULL;
-	}
-}
-
-/*
- * /sys/module/foo/notes/.section.name gives contents of SHT_NOTE sections.
- */
-
-struct module_notes_attrs {
-	struct kobject *dir;
-	unsigned int notes;
-	struct bin_attribute attrs[];
-};
-
-static ssize_t module_notes_read(struct file *filp, struct kobject *kobj,
-				 struct bin_attribute *bin_attr,
-				 char *buf, loff_t pos, size_t count)
-{
-	/*
-	 * The caller checked the pos and count against our size.
-	 */
-	memcpy(buf, bin_attr->private + pos, count);
-	return count;
-}
-
-static void free_notes_attrs(struct module_notes_attrs *notes_attrs,
-			     unsigned int i)
-{
-	if (notes_attrs->dir) {
-		while (i-- > 0)
-			sysfs_remove_bin_file(notes_attrs->dir,
-					      &notes_attrs->attrs[i]);
-		kobject_put(notes_attrs->dir);
-	}
-	kfree(notes_attrs);
-}
-
-static void add_notes_attrs(struct module *mod, const struct load_info *info)
-{
-	unsigned int notes, loaded, i;
-	struct module_notes_attrs *notes_attrs;
-	struct bin_attribute *nattr;
-
-	/* failed to create section attributes, so can't create notes */
-	if (!mod->sect_attrs)
-		return;
-
-	/* Count notes sections and allocate structures.  */
-	notes = 0;
-	for (i = 0; i < info->hdr->e_shnum; i++)
-		if (!sect_empty(&info->sechdrs[i]) &&
-		    (info->sechdrs[i].sh_type == SHT_NOTE))
-			++notes;
-
-	if (notes == 0)
-		return;
-
-	notes_attrs = kzalloc(struct_size(notes_attrs, attrs, notes),
-			      GFP_KERNEL);
-	if (notes_attrs == NULL)
-		return;
-
-	notes_attrs->notes = notes;
-	nattr = &notes_attrs->attrs[0];
-	for (loaded = i = 0; i < info->hdr->e_shnum; ++i) {
-		if (sect_empty(&info->sechdrs[i]))
-			continue;
-		if (info->sechdrs[i].sh_type == SHT_NOTE) {
-			sysfs_bin_attr_init(nattr);
-			nattr->attr.name = mod->sect_attrs->attrs[loaded].battr.attr.name;
-			nattr->attr.mode = S_IRUGO;
-			nattr->size = info->sechdrs[i].sh_size;
-			nattr->private = (void *) info->sechdrs[i].sh_addr;
-			nattr->read = module_notes_read;
-			++nattr;
-		}
-		++loaded;
-	}
-
-	notes_attrs->dir = kobject_create_and_add("notes", &mod->mkobj.kobj);
-	if (!notes_attrs->dir)
-		goto out;
-
-	for (i = 0; i < notes; ++i)
-		if (sysfs_create_bin_file(notes_attrs->dir,
-					  &notes_attrs->attrs[i]))
-			goto out;
-
-	mod->notes_attrs = notes_attrs;
-	return;
-
-  out:
-	free_notes_attrs(notes_attrs, i);
-}
-
-static void remove_notes_attrs(struct module *mod)
-{
-	if (mod->notes_attrs)
-		free_notes_attrs(mod->notes_attrs, mod->notes_attrs->notes);
-}
-
-#else
-
-static inline void add_sect_attrs(struct module *mod,
-				  const struct load_info *info)
-{
-}
-
-static inline void remove_sect_attrs(struct module *mod)
-{
-}
-
-static inline void add_notes_attrs(struct module *mod,
-				   const struct load_info *info)
-{
-}
-
-static inline void remove_notes_attrs(struct module *mod)
-{
-}
-#endif /* CONFIG_KALLSYMS */
-
-static void del_usage_links(struct module *mod)
-{
-#ifdef CONFIG_MODULE_UNLOAD
-	struct module_use *use;
-
-	mutex_lock(&module_mutex);
-	list_for_each_entry(use, &mod->target_list, target_list)
-		sysfs_remove_link(use->target->holders_dir, mod->name);
-	mutex_unlock(&module_mutex);
-#endif
-}
-
-static int add_usage_links(struct module *mod)
-{
-	int ret = 0;
-#ifdef CONFIG_MODULE_UNLOAD
-	struct module_use *use;
-
-	mutex_lock(&module_mutex);
-	list_for_each_entry(use, &mod->target_list, target_list) {
-		ret = sysfs_create_link(use->target->holders_dir,
-					&mod->mkobj.kobj, mod->name);
-		if (ret)
-			break;
-	}
-	mutex_unlock(&module_mutex);
-	if (ret)
-		del_usage_links(mod);
-#endif
-	return ret;
-}
-
-static void module_remove_modinfo_attrs(struct module *mod, int end);
-
-static int module_add_modinfo_attrs(struct module *mod)
-{
-	struct module_attribute *attr;
-	struct module_attribute *temp_attr;
-	int error = 0;
-	int i;
-
-	mod->modinfo_attrs = kzalloc((sizeof(struct module_attribute) *
-					(ARRAY_SIZE(modinfo_attrs) + 1)),
-					GFP_KERNEL);
-	if (!mod->modinfo_attrs)
-		return -ENOMEM;
-
-	temp_attr = mod->modinfo_attrs;
-	for (i = 0; (attr = modinfo_attrs[i]); i++) {
-		if (!attr->test || attr->test(mod)) {
-			memcpy(temp_attr, attr, sizeof(*temp_attr));
-			sysfs_attr_init(&temp_attr->attr);
-			error = sysfs_create_file(&mod->mkobj.kobj,
-					&temp_attr->attr);
-			if (error)
-				goto error_out;
-			++temp_attr;
-		}
-	}
-
-	return 0;
-
-error_out:
-	if (i > 0)
-		module_remove_modinfo_attrs(mod, --i);
-	else
-		kfree(mod->modinfo_attrs);
-	return error;
-}
-
-static void module_remove_modinfo_attrs(struct module *mod, int end)
-{
-	struct module_attribute *attr;
-	int i;
-
-	for (i = 0; (attr = &mod->modinfo_attrs[i]); i++) {
-		if (end >= 0 && i > end)
-			break;
-		/* pick a field to test for end of list */
-		if (!attr->attr.name)
-			break;
-		sysfs_remove_file(&mod->mkobj.kobj, &attr->attr);
-		if (attr->free)
-			attr->free(mod);
-	}
-	kfree(mod->modinfo_attrs);
-}
-
-static void mod_kobject_put(struct module *mod)
-{
-	DECLARE_COMPLETION_ONSTACK(c);
-	mod->mkobj.kobj_completion = &c;
-	kobject_put(&mod->mkobj.kobj);
-	wait_for_completion(&c);
-}
-
-static int mod_sysfs_init(struct module *mod)
-{
-	int err;
-	struct kobject *kobj;
-
-	if (!module_sysfs_initialized) {
-		pr_err("%s: module sysfs not initialized\n", mod->name);
-		err = -EINVAL;
-		goto out;
-	}
-
-	kobj = kset_find_obj(module_kset, mod->name);
-	if (kobj) {
-		pr_err("%s: module is already loaded\n", mod->name);
-		kobject_put(kobj);
-		err = -EINVAL;
-		goto out;
-	}
-
-	mod->mkobj.mod = mod;
-
-	memset(&mod->mkobj.kobj, 0, sizeof(mod->mkobj.kobj));
-	mod->mkobj.kobj.kset = module_kset;
-	err = kobject_init_and_add(&mod->mkobj.kobj, &module_ktype, NULL,
-				   "%s", mod->name);
-	if (err)
-		mod_kobject_put(mod);
-
-out:
-	return err;
-}
-
-static int mod_sysfs_setup(struct module *mod,
-			   const struct load_info *info,
-			   struct kernel_param *kparam,
-			   unsigned int num_params)
-{
-	int err;
-
-	err = mod_sysfs_init(mod);
-	if (err)
-		goto out;
-
-	mod->holders_dir = kobject_create_and_add("holders", &mod->mkobj.kobj);
-	if (!mod->holders_dir) {
-		err = -ENOMEM;
-		goto out_unreg;
-	}
-
-	err = module_param_sysfs_setup(mod, kparam, num_params);
-	if (err)
-		goto out_unreg_holders;
-
-	err = module_add_modinfo_attrs(mod);
-	if (err)
-		goto out_unreg_param;
-
-	err = add_usage_links(mod);
-	if (err)
-		goto out_unreg_modinfo_attrs;
-
-	add_sect_attrs(mod, info);
-	add_notes_attrs(mod, info);
-
-	return 0;
-
-out_unreg_modinfo_attrs:
-	module_remove_modinfo_attrs(mod, -1);
-out_unreg_param:
-	module_param_sysfs_remove(mod);
-out_unreg_holders:
-	kobject_put(mod->holders_dir);
-out_unreg:
-	mod_kobject_put(mod);
-out:
-	return err;
-}
-
-static void mod_sysfs_fini(struct module *mod)
-{
-	remove_notes_attrs(mod);
-	remove_sect_attrs(mod);
-	mod_kobject_put(mod);
-}
-
-static void init_param_lock(struct module *mod)
-{
-	mutex_init(&mod->param_lock);
-}
-#else /* !CONFIG_SYSFS */
-
-static int mod_sysfs_setup(struct module *mod,
-			   const struct load_info *info,
-			   struct kernel_param *kparam,
-			   unsigned int num_params)
-{
-	return 0;
-}
-
-static void mod_sysfs_fini(struct module *mod)
-{
-}
-
-static void module_remove_modinfo_attrs(struct module *mod, int end)
-{
-}
-
-static void del_usage_links(struct module *mod)
-{
-}
-
-static void init_param_lock(struct module *mod)
-{
-}
-#endif /* CONFIG_SYSFS */
-
 static void mod_sysfs_teardown(struct module *mod)
 {
 	del_usage_links(mod);
diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
new file mode 100644
index 000000000000..f5c72c567e71
--- /dev/null
+++ b/kernel/module/sysfs.c
@@ -0,0 +1,425 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Module sysfs support
+ *
+ * Copyright (C) 2008 Rusty Russell
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/fs.h>
+#include <linux/sysfs.h>
+#include <linux/slab.h>
+#include <linux/kallsyms.h>
+#include <linux/mutex.h>
+#include "internal.h"
+
+/*
+ * /sys/module/foo/sections stuff
+ * J. Corbet <corbet@lwn.net>
+ */
+#ifdef CONFIG_KALLSYMS
+struct module_sect_attr {
+	struct bin_attribute battr;
+	unsigned long address;
+};
+
+struct module_sect_attrs {
+	struct attribute_group grp;
+	unsigned int nsections;
+	struct module_sect_attr attrs[];
+};
+
+static ssize_t module_sect_read(struct file *file, struct kobject *kobj,
+				struct bin_attribute *battr,
+				char *buf, loff_t pos, size_t count)
+{
+	struct module_sect_attr *sattr =
+		container_of(battr, struct module_sect_attr, battr);
+	char bounce[MODULE_SECT_READ_SIZE + 1];
+	size_t wrote;
+
+	if (pos != 0)
+		return -EINVAL;
+
+	/*
+	 * Since we're a binary read handler, we must account for the
+	 * trailing NUL byte that sprintf will write: if "buf" is
+	 * too small to hold the NUL, or the NUL is exactly the last
+	 * byte, the read will look like it got truncated by one byte.
+	 * Since there is no way to ask sprintf nicely to not write
+	 * the NUL, we have to use a bounce buffer.
+	 */
+	wrote = scnprintf(bounce, sizeof(bounce), "0x%px\n",
+			 kallsyms_show_value(file->f_cred)
+				? (void *)sattr->address : NULL);
+	count = min(count, wrote);
+	memcpy(buf, bounce, count);
+
+	return count;
+}
+
+static void free_sect_attrs(struct module_sect_attrs *sect_attrs)
+{
+	unsigned int section;
+
+	for (section = 0; section < sect_attrs->nsections; section++)
+		kfree(sect_attrs->attrs[section].battr.attr.name);
+	kfree(sect_attrs);
+}
+
+static void add_sect_attrs(struct module *mod, const struct load_info *info)
+{
+	unsigned int nloaded = 0, i, size[2];
+	struct module_sect_attrs *sect_attrs;
+	struct module_sect_attr *sattr;
+	struct bin_attribute **gattr;
+
+	/* Count loaded sections and allocate structures */
+	for (i = 0; i < info->hdr->e_shnum; i++)
+		if (!sect_empty(&info->sechdrs[i]))
+			nloaded++;
+	size[0] = ALIGN(struct_size(sect_attrs, attrs, nloaded),
+			sizeof(sect_attrs->grp.bin_attrs[0]));
+	size[1] = (nloaded + 1) * sizeof(sect_attrs->grp.bin_attrs[0]);
+	sect_attrs = kzalloc(size[0] + size[1], GFP_KERNEL);
+	if (sect_attrs == NULL)
+		return;
+
+	/* Setup section attributes. */
+	sect_attrs->grp.name = "sections";
+	sect_attrs->grp.bin_attrs = (void *)sect_attrs + size[0];
+
+	sect_attrs->nsections = 0;
+	sattr = &sect_attrs->attrs[0];
+	gattr = &sect_attrs->grp.bin_attrs[0];
+	for (i = 0; i < info->hdr->e_shnum; i++) {
+		Elf_Shdr *sec = &info->sechdrs[i];
+
+		if (sect_empty(sec))
+			continue;
+		sysfs_bin_attr_init(&sattr->battr);
+		sattr->address = sec->sh_addr;
+		sattr->battr.attr.name =
+			kstrdup(info->secstrings + sec->sh_name, GFP_KERNEL);
+		if (sattr->battr.attr.name == NULL)
+			goto out;
+		sect_attrs->nsections++;
+		sattr->battr.read = module_sect_read;
+		sattr->battr.size = MODULE_SECT_READ_SIZE;
+		sattr->battr.attr.mode = 0400;
+		*(gattr++) = &(sattr++)->battr;
+	}
+	*gattr = NULL;
+
+	if (sysfs_create_group(&mod->mkobj.kobj, &sect_attrs->grp))
+		goto out;
+
+	mod->sect_attrs = sect_attrs;
+	return;
+out:
+	free_sect_attrs(sect_attrs);
+}
+
+static void remove_sect_attrs(struct module *mod)
+{
+	if (mod->sect_attrs) {
+		sysfs_remove_group(&mod->mkobj.kobj,
+				   &mod->sect_attrs->grp);
+		/*
+		 * We are positive that no one is using any sect attrs
+		 * at this point.  Deallocate immediately.
+		 */
+		free_sect_attrs(mod->sect_attrs);
+		mod->sect_attrs = NULL;
+	}
+}
+
+/*
+ * /sys/module/foo/notes/.section.name gives contents of SHT_NOTE sections.
+ */
+
+struct module_notes_attrs {
+	struct kobject *dir;
+	unsigned int notes;
+	struct bin_attribute attrs[];
+};
+
+static ssize_t module_notes_read(struct file *filp, struct kobject *kobj,
+				 struct bin_attribute *bin_attr,
+				 char *buf, loff_t pos, size_t count)
+{
+	/*
+	 * The caller checked the pos and count against our size.
+	 */
+	memcpy(buf, bin_attr->private + pos, count);
+	return count;
+}
+
+static void free_notes_attrs(struct module_notes_attrs *notes_attrs,
+			     unsigned int i)
+{
+	if (notes_attrs->dir) {
+		while (i-- > 0)
+			sysfs_remove_bin_file(notes_attrs->dir,
+					      &notes_attrs->attrs[i]);
+		kobject_put(notes_attrs->dir);
+	}
+	kfree(notes_attrs);
+}
+
+static void add_notes_attrs(struct module *mod, const struct load_info *info)
+{
+	unsigned int notes, loaded, i;
+	struct module_notes_attrs *notes_attrs;
+	struct bin_attribute *nattr;
+
+	/* failed to create section attributes, so can't create notes */
+	if (!mod->sect_attrs)
+		return;
+
+	/* Count notes sections and allocate structures.  */
+	notes = 0;
+	for (i = 0; i < info->hdr->e_shnum; i++)
+		if (!sect_empty(&info->sechdrs[i]) &&
+		    (info->sechdrs[i].sh_type == SHT_NOTE))
+			++notes;
+
+	if (notes == 0)
+		return;
+
+	notes_attrs = kzalloc(struct_size(notes_attrs, attrs, notes),
+			      GFP_KERNEL);
+	if (notes_attrs == NULL)
+		return;
+
+	notes_attrs->notes = notes;
+	nattr = &notes_attrs->attrs[0];
+	for (loaded = i = 0; i < info->hdr->e_shnum; ++i) {
+		if (sect_empty(&info->sechdrs[i]))
+			continue;
+		if (info->sechdrs[i].sh_type == SHT_NOTE) {
+			sysfs_bin_attr_init(nattr);
+			nattr->attr.name = mod->sect_attrs->attrs[loaded].battr.attr.name;
+			nattr->attr.mode = 0444;
+			nattr->size = info->sechdrs[i].sh_size;
+			nattr->private = (void *) info->sechdrs[i].sh_addr;
+			nattr->read = module_notes_read;
+			++nattr;
+		}
+		++loaded;
+	}
+
+	notes_attrs->dir = kobject_create_and_add("notes", &mod->mkobj.kobj);
+	if (!notes_attrs->dir)
+		goto out;
+
+	for (i = 0; i < notes; ++i)
+		if (sysfs_create_bin_file(notes_attrs->dir,
+					  &notes_attrs->attrs[i]))
+			goto out;
+
+	mod->notes_attrs = notes_attrs;
+	return;
+
+out:
+	free_notes_attrs(notes_attrs, i);
+}
+
+static void remove_notes_attrs(struct module *mod)
+{
+	if (mod->notes_attrs)
+		free_notes_attrs(mod->notes_attrs, mod->notes_attrs->notes);
+}
+
+#else /* !CONFIG_KALLSYMS */
+static inline void add_sect_attrs(struct module *mod, const struct load_info *info) { }
+static inline void remove_sect_attrs(struct module *mod) { }
+static inline void add_notes_attrs(struct module *mod, const struct load_info *info) { }
+static inline void remove_notes_attrs(struct module *mod) { }
+#endif /* CONFIG_KALLSYMS */
+
+void del_usage_links(struct module *mod)
+{
+#ifdef CONFIG_MODULE_UNLOAD
+	struct module_use *use;
+
+	mutex_lock(&module_mutex);
+	list_for_each_entry(use, &mod->target_list, target_list)
+		sysfs_remove_link(use->target->holders_dir, mod->name);
+	mutex_unlock(&module_mutex);
+#endif
+}
+
+static int add_usage_links(struct module *mod)
+{
+	int ret = 0;
+#ifdef CONFIG_MODULE_UNLOAD
+	struct module_use *use;
+
+	mutex_lock(&module_mutex);
+	list_for_each_entry(use, &mod->target_list, target_list) {
+		ret = sysfs_create_link(use->target->holders_dir,
+					&mod->mkobj.kobj, mod->name);
+		if (ret)
+			break;
+	}
+	mutex_unlock(&module_mutex);
+	if (ret)
+		del_usage_links(mod);
+#endif
+	return ret;
+}
+
+static int module_add_modinfo_attrs(struct module *mod)
+{
+	struct module_attribute *attr;
+	struct module_attribute *temp_attr;
+	int error = 0;
+	int i;
+
+	mod->modinfo_attrs = kzalloc((sizeof(struct module_attribute) *
+					(modinfo_attrs_count + 1)),
+					GFP_KERNEL);
+	if (!mod->modinfo_attrs)
+		return -ENOMEM;
+
+	temp_attr = mod->modinfo_attrs;
+	for (i = 0; (attr = modinfo_attrs[i]); i++) {
+		if (!attr->test || attr->test(mod)) {
+			memcpy(temp_attr, attr, sizeof(*temp_attr));
+			sysfs_attr_init(&temp_attr->attr);
+			error = sysfs_create_file(&mod->mkobj.kobj,
+					&temp_attr->attr);
+			if (error)
+				goto error_out;
+			++temp_attr;
+		}
+	}
+
+	return 0;
+
+error_out:
+	if (i > 0)
+		module_remove_modinfo_attrs(mod, --i);
+	else
+		kfree(mod->modinfo_attrs);
+	return error;
+}
+
+void module_remove_modinfo_attrs(struct module *mod, int end)
+{
+	struct module_attribute *attr;
+	int i;
+
+	for (i = 0; (attr = &mod->modinfo_attrs[i]); i++) {
+		if (end >= 0 && i > end)
+			break;
+		/* pick a field to test for end of list */
+		if (!attr->attr.name)
+			break;
+		sysfs_remove_file(&mod->mkobj.kobj, &attr->attr);
+		if (attr->free)
+			attr->free(mod);
+	}
+	kfree(mod->modinfo_attrs);
+}
+
+static void mod_kobject_put(struct module *mod)
+{
+	DECLARE_COMPLETION_ONSTACK(c);
+
+	mod->mkobj.kobj_completion = &c;
+	kobject_put(&mod->mkobj.kobj);
+	wait_for_completion(&c);
+}
+
+static int mod_sysfs_init(struct module *mod)
+{
+	int err;
+	struct kobject *kobj;
+
+	if (!module_sysfs_initialized) {
+		pr_err("%s: module sysfs not initialized\n", mod->name);
+		err = -EINVAL;
+		goto out;
+	}
+
+	kobj = kset_find_obj(module_kset, mod->name);
+	if (kobj) {
+		pr_err("%s: module is already loaded\n", mod->name);
+		kobject_put(kobj);
+		err = -EINVAL;
+		goto out;
+	}
+
+	mod->mkobj.mod = mod;
+
+	memset(&mod->mkobj.kobj, 0, sizeof(mod->mkobj.kobj));
+	mod->mkobj.kobj.kset = module_kset;
+	err = kobject_init_and_add(&mod->mkobj.kobj, &module_ktype, NULL,
+				   "%s", mod->name);
+	if (err)
+		mod_kobject_put(mod);
+
+out:
+	return err;
+}
+
+int mod_sysfs_setup(struct module *mod,
+			   const struct load_info *info,
+			   struct kernel_param *kparam,
+			   unsigned int num_params)
+{
+	int err;
+
+	err = mod_sysfs_init(mod);
+	if (err)
+		goto out;
+
+	mod->holders_dir = kobject_create_and_add("holders", &mod->mkobj.kobj);
+	if (!mod->holders_dir) {
+		err = -ENOMEM;
+		goto out_unreg;
+	}
+
+	err = module_param_sysfs_setup(mod, kparam, num_params);
+	if (err)
+		goto out_unreg_holders;
+
+	err = module_add_modinfo_attrs(mod);
+	if (err)
+		goto out_unreg_param;
+
+	err = add_usage_links(mod);
+	if (err)
+		goto out_unreg_modinfo_attrs;
+
+	add_sect_attrs(mod, info);
+	add_notes_attrs(mod, info);
+
+	return 0;
+
+out_unreg_modinfo_attrs:
+	module_remove_modinfo_attrs(mod, -1);
+out_unreg_param:
+	module_param_sysfs_remove(mod);
+out_unreg_holders:
+	kobject_put(mod->holders_dir);
+out_unreg:
+	mod_kobject_put(mod);
+out:
+	return err;
+}
+
+void mod_sysfs_fini(struct module *mod)
+{
+	remove_notes_attrs(mod);
+	remove_sect_attrs(mod);
+	mod_kobject_put(mod);
+}
+
+void init_param_lock(struct module *mod)
+{
+	mutex_init(&mod->param_lock);
+}
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v5 12/13] module: Move kdb_modules list out of core code
  2022-02-09 17:11 [PATCH v5 11/13] module: Move sysfs support into a separate file Aaron Tomlin
@ 2022-02-09 17:11 ` Aaron Tomlin
  2022-02-10 14:05   ` Christophe Leroy
  2022-02-09 17:11 ` [PATCH v5 13/13] module: Move version support into a separate file Aaron Tomlin
  2022-02-10 14:00 ` [PATCH v5 11/13] module: Move sysfs " Christophe Leroy
  2 siblings, 1 reply; 14+ messages in thread
From: Aaron Tomlin @ 2022-02-09 17:11 UTC (permalink / raw)
  To: mcgrof
  Cc: cl, pmladek, mbenes, akpm, jeyu, linux-kernel, linux-modules,
	live-patching, atomlin, ghalat, allen.lkml, void, joe,
	christophe.leroy, msuchanek, oleksandr

No functional change.

This patch migrates kdb_modules list to core kdb code
since the list of added/or loaded modules is no longer
private.

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 kernel/debug/kdb/kdb_main.c | 5 +++++
 kernel/module/internal.h    | 1 +
 kernel/module/main.c        | 4 ----
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 0852a537dad4..f101f5f078f4 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -59,6 +59,11 @@ EXPORT_SYMBOL(kdb_grepping_flag);
 int kdb_grep_leading;
 int kdb_grep_trailing;
 
+#ifdef CONFIG_MODULES
+extern struct list_head modules;
+struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */
+#endif /* CONFIG_MODULES */
+
 /*
  * Kernel debugger state flags
  */
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index 52d30bf6d6b0..c49b4900b30b 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -225,6 +225,7 @@ static int mod_sysfs_setup(struct module *mod,
 {
 	return 0;
 }
+
 static inline void mod_sysfs_fini(struct module *mod) { }
 static inline void module_remove_modinfo_attrs(struct module *mod, int end) { }
 static inline void del_usage_links(struct module *mod) { }
diff --git a/kernel/module/main.c b/kernel/module/main.c
index c2255954b7df..519c5335f7a6 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -105,10 +105,6 @@ static void mod_update_bounds(struct module *mod)
 		__mod_update_bounds(mod->init_layout.base, mod->init_layout.size);
 }
 
-#ifdef CONFIG_KGDB_KDB
-struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */
-#endif /* CONFIG_KGDB_KDB */
-
 static void module_assert_mutex_or_preempt(void)
 {
 #ifdef CONFIG_LOCKDEP
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v5 13/13] module: Move version support into a separate file
  2022-02-09 17:11 [PATCH v5 11/13] module: Move sysfs support into a separate file Aaron Tomlin
  2022-02-09 17:11 ` [PATCH v5 12/13] module: Move kdb_modules list out of core code Aaron Tomlin
@ 2022-02-09 17:11 ` Aaron Tomlin
  2022-02-10 14:28   ` Christophe Leroy
  2022-02-10 14:00 ` [PATCH v5 11/13] module: Move sysfs " Christophe Leroy
  2 siblings, 1 reply; 14+ messages in thread
From: Aaron Tomlin @ 2022-02-09 17:11 UTC (permalink / raw)
  To: mcgrof
  Cc: cl, pmladek, mbenes, akpm, jeyu, linux-kernel, linux-modules,
	live-patching, atomlin, ghalat, allen.lkml, void, joe,
	christophe.leroy, msuchanek, oleksandr

No functional change.

This patch migrates module version support out of core code into
kernel/module/version.c. In addition simple code refactoring to
make this possible.

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 kernel/module/Makefile   |   1 +
 kernel/module/internal.h |  50 +++++++++++++
 kernel/module/main.c     | 150 +--------------------------------------
 kernel/module/version.c  | 110 ++++++++++++++++++++++++++++
 4 files changed, 163 insertions(+), 148 deletions(-)
 create mode 100644 kernel/module/version.c

diff --git a/kernel/module/Makefile b/kernel/module/Makefile
index c30141c37eb3..1f111aa47e88 100644
--- a/kernel/module/Makefile
+++ b/kernel/module/Makefile
@@ -15,4 +15,5 @@ obj-$(CONFIG_DEBUG_KMEMLEAK) += debug_kmemleak.o
 obj-$(CONFIG_KALLSYMS) += kallsyms.o
 obj-$(CONFIG_PROC_FS) += procfs.o
 obj-$(CONFIG_SYSFS) += sysfs.o
+obj-$(CONFIG_MODVERSIONS) += version.o
 endif
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index c49b4900b30b..475a66aa42f2 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -71,7 +71,31 @@ struct load_info {
 	} index;
 };
 
+struct symsearch {
+	const struct kernel_symbol *start, *stop;
+	const s32 *crcs;
+	enum mod_license {
+		NOT_GPL_ONLY,
+		GPL_ONLY,
+	} license;
+};
+
+struct find_symbol_arg {
+	/* Input */
+	const char *name;
+	bool gplok;
+	bool warn;
+
+	/* Output */
+	struct module *owner;
+	const s32 *crc;
+	const struct kernel_symbol *sym;
+	enum mod_license license;
+};
+
 int mod_verify_sig(const void *mod, struct load_info *info);
+int try_to_force_load(struct module *mod, const char *reason);
+bool find_symbol(struct find_symbol_arg *fsa);
 struct module *find_module_all(const char *name, size_t len, bool even_unformed);
 unsigned long kernel_symbol_value(const struct kernel_symbol *sym);
 int cmp_name(const void *name, const void *sym);
@@ -231,3 +255,29 @@ static inline void module_remove_modinfo_attrs(struct module *mod, int end) { }
 static inline void del_usage_links(struct module *mod) { }
 static inline void init_param_lock(struct module *mod) { }
 #endif /* CONFIG_SYSFS */
+
+#ifdef CONFIG_MODVERSIONS
+int check_version(const struct load_info *info,
+		  const char *symname, struct module *mod, const s32 *crc);
+int check_modstruct_version(const struct load_info *info, struct module *mod);
+int same_magic(const char *amagic, const char *bmagic, bool has_crcs);
+#else /* !CONFIG_MODVERSIONS */
+static inline int check_version(const struct load_info *info,
+				const char *symname,
+				struct module *mod,
+				const s32 *crc)
+{
+	return 1;
+}
+
+static inline int check_modstruct_version(const struct load_info *info,
+					  struct module *mod)
+{
+	return 1;
+}
+
+static inline int same_magic(const char *amagic, const char *bmagic, bool has_crcs)
+{
+	return strcmp(amagic, bmagic) == 0;
+}
+#endif /* CONFIG_MODVERSIONS */
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 519c5335f7a6..b65bf5f7d474 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -241,28 +241,6 @@ static __maybe_unused void *any_section_objs(const struct load_info *info,
 #define symversion(base, idx) ((base != NULL) ? ((base) + (idx)) : NULL)
 #endif
 
-struct symsearch {
-	const struct kernel_symbol *start, *stop;
-	const s32 *crcs;
-	enum mod_license {
-		NOT_GPL_ONLY,
-		GPL_ONLY,
-	} license;
-};
-
-struct find_symbol_arg {
-	/* Input */
-	const char *name;
-	bool gplok;
-	bool warn;
-
-	/* Output */
-	struct module *owner;
-	const s32 *crc;
-	const struct kernel_symbol *sym;
-	enum mod_license license;
-};
-
 static bool check_exported_symbol(const struct symsearch *syms,
 				  struct module *owner,
 				  unsigned int symnum, void *data)
@@ -333,7 +311,7 @@ static bool find_exported_symbol_in_section(const struct symsearch *syms,
  * Find an exported symbol and return it, along with, (optional) crc and
  * (optional) module which owns it.  Needs preempt disabled or module_mutex.
  */
-static bool find_symbol(struct find_symbol_arg *fsa)
+bool find_symbol(struct find_symbol_arg *fsa)
 {
 	static const struct symsearch arr[] = {
 		{ __start___ksymtab, __stop___ksymtab, __start___kcrctab,
@@ -1007,7 +985,7 @@ size_t modinfo_attrs_count = ARRAY_SIZE(modinfo_attrs);
 
 static const char vermagic[] = VERMAGIC_STRING;
 
-static int try_to_force_load(struct module *mod, const char *reason)
+int try_to_force_load(struct module *mod, const char *reason)
 {
 #ifdef CONFIG_MODULE_FORCE_LOAD
 	if (!test_taint(TAINT_FORCED_MODULE))
@@ -1019,115 +997,6 @@ static int try_to_force_load(struct module *mod, const char *reason)
 #endif
 }
 
-#ifdef CONFIG_MODVERSIONS
-
-static u32 resolve_rel_crc(const s32 *crc)
-{
-	return *(u32 *)((void *)crc + *crc);
-}
-
-static int check_version(const struct load_info *info,
-			 const char *symname,
-			 struct module *mod,
-			 const s32 *crc)
-{
-	Elf_Shdr *sechdrs = info->sechdrs;
-	unsigned int versindex = info->index.vers;
-	unsigned int i, num_versions;
-	struct modversion_info *versions;
-
-	/* Exporting module didn't supply crcs?  OK, we're already tainted. */
-	if (!crc)
-		return 1;
-
-	/* No versions at all?  modprobe --force does this. */
-	if (versindex == 0)
-		return try_to_force_load(mod, symname) == 0;
-
-	versions = (void *) sechdrs[versindex].sh_addr;
-	num_versions = sechdrs[versindex].sh_size
-		/ sizeof(struct modversion_info);
-
-	for (i = 0; i < num_versions; i++) {
-		u32 crcval;
-
-		if (strcmp(versions[i].name, symname) != 0)
-			continue;
-
-		if (IS_ENABLED(CONFIG_MODULE_REL_CRCS))
-			crcval = resolve_rel_crc(crc);
-		else
-			crcval = *crc;
-		if (versions[i].crc == crcval)
-			return 1;
-		pr_debug("Found checksum %X vs module %lX\n",
-			 crcval, versions[i].crc);
-		goto bad_version;
-	}
-
-	/* Broken toolchain. Warn once, then let it go.. */
-	pr_warn_once("%s: no symbol version for %s\n", info->name, symname);
-	return 1;
-
-bad_version:
-	pr_warn("%s: disagrees about version of symbol %s\n",
-	       info->name, symname);
-	return 0;
-}
-
-static inline int check_modstruct_version(const struct load_info *info,
-					  struct module *mod)
-{
-	struct find_symbol_arg fsa = {
-		.name	= "module_layout",
-		.gplok	= true,
-	};
-
-	/*
-	 * Since this should be found in kernel (which can't be removed), no
-	 * locking is necessary -- use preempt_disable() to placate lockdep.
-	 */
-	preempt_disable();
-	if (!find_symbol(&fsa)) {
-		preempt_enable();
-		BUG();
-	}
-	preempt_enable();
-	return check_version(info, "module_layout", mod, fsa.crc);
-}
-
-/* First part is kernel version, which we ignore if module has crcs. */
-static inline int same_magic(const char *amagic, const char *bmagic,
-			     bool has_crcs)
-{
-	if (has_crcs) {
-		amagic += strcspn(amagic, " ");
-		bmagic += strcspn(bmagic, " ");
-	}
-	return strcmp(amagic, bmagic) == 0;
-}
-#else
-static inline int check_version(const struct load_info *info,
-				const char *symname,
-				struct module *mod,
-				const s32 *crc)
-{
-	return 1;
-}
-
-static inline int check_modstruct_version(const struct load_info *info,
-					  struct module *mod)
-{
-	return 1;
-}
-
-static inline int same_magic(const char *amagic, const char *bmagic,
-			     bool has_crcs)
-{
-	return strcmp(amagic, bmagic) == 0;
-}
-#endif /* CONFIG_MODVERSIONS */
-
 static char *get_modinfo(const struct load_info *info, const char *tag);
 static char *get_next_modinfo(const struct load_info *info, const char *tag,
 			      char *prev);
@@ -3263,18 +3132,3 @@ void print_modules(void)
 		pr_cont(" [last unloaded: %s]", last_unloaded_module);
 	pr_cont("\n");
 }
-
-#ifdef CONFIG_MODVERSIONS
-/*
- * Generate the signature for all relevant module structures here.
- * If these change, we don't want to try to parse the module.
- */
-void module_layout(struct module *mod,
-		   struct modversion_info *ver,
-		   struct kernel_param *kp,
-		   struct kernel_symbol *ks,
-		   struct tracepoint * const *tp)
-{
-}
-EXPORT_SYMBOL(module_layout);
-#endif
diff --git a/kernel/module/version.c b/kernel/module/version.c
new file mode 100644
index 000000000000..10a1490d1b9e
--- /dev/null
+++ b/kernel/module/version.c
@@ -0,0 +1,110 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Module version support
+ *
+ * Copyright (C) 2008 Rusty Russell
+ */
+
+#include <linux/module.h>
+#include <linux/string.h>
+#include <linux/printk.h>
+#include "internal.h"
+
+static u32 resolve_rel_crc(const s32 *crc)
+{
+	return *(u32 *)((void *)crc + *crc);
+}
+
+int check_version(const struct load_info *info,
+			 const char *symname,
+			 struct module *mod,
+			 const s32 *crc)
+{
+	Elf_Shdr *sechdrs = info->sechdrs;
+	unsigned int versindex = info->index.vers;
+	unsigned int i, num_versions;
+	struct modversion_info *versions;
+
+	/* Exporting module didn't supply crcs?  OK, we're already tainted. */
+	if (!crc)
+		return 1;
+
+	/* No versions at all?  modprobe --force does this. */
+	if (versindex == 0)
+		return try_to_force_load(mod, symname) == 0;
+
+	versions = (void *) sechdrs[versindex].sh_addr;
+	num_versions = sechdrs[versindex].sh_size
+		/ sizeof(struct modversion_info);
+
+	for (i = 0; i < num_versions; i++) {
+		u32 crcval;
+
+		if (strcmp(versions[i].name, symname) != 0)
+			continue;
+
+		if (IS_ENABLED(CONFIG_MODULE_REL_CRCS))
+			crcval = resolve_rel_crc(crc);
+		else
+			crcval = *crc;
+		if (versions[i].crc == crcval)
+			return 1;
+		pr_debug("Found checksum %X vs module %lX\n",
+			 crcval, versions[i].crc);
+		goto bad_version;
+	}
+
+	/* Broken toolchain. Warn once, then let it go.. */
+	pr_warn_once("%s: no symbol version for %s\n", info->name, symname);
+	return 1;
+
+bad_version:
+	pr_warn("%s: disagrees about version of symbol %s\n",
+	       info->name, symname);
+	return 0;
+}
+
+inline int check_modstruct_version(const struct load_info *info,
+					  struct module *mod)
+{
+	struct find_symbol_arg fsa = {
+		.name	= "module_layout",
+		.gplok	= true,
+	};
+
+	/*
+	 * Since this should be found in kernel (which can't be removed), no
+	 * locking is necessary -- use preempt_disable() to placate lockdep.
+	 */
+	preempt_disable();
+	if (!find_symbol(&fsa)) {
+		preempt_enable();
+		BUG();
+	}
+	preempt_enable();
+	return check_version(info, "module_layout", mod, fsa.crc);
+}
+
+/* First part is kernel version, which we ignore if module has crcs. */
+inline int same_magic(const char *amagic, const char *bmagic,
+			     bool has_crcs)
+{
+	if (has_crcs) {
+		amagic += strcspn(amagic, " ");
+		bmagic += strcspn(bmagic, " ");
+	}
+	return strcmp(amagic, bmagic) == 0;
+}
+
+/*
+ * Generate the signature for all relevant module structures here.
+ * If these change, we don't want to try to parse the module.
+ */
+void module_layout(struct module *mod,
+		   struct modversion_info *ver,
+		   struct kernel_param *kp,
+		   struct kernel_symbol *ks,
+		   struct tracepoint * const *tp)
+{
+}
+EXPORT_SYMBOL(module_layout);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 11/13] module: Move sysfs support into a separate file
  2022-02-09 17:11 [PATCH v5 11/13] module: Move sysfs support into a separate file Aaron Tomlin
  2022-02-09 17:11 ` [PATCH v5 12/13] module: Move kdb_modules list out of core code Aaron Tomlin
  2022-02-09 17:11 ` [PATCH v5 13/13] module: Move version support into a separate file Aaron Tomlin
@ 2022-02-10 14:00 ` Christophe Leroy
  2022-02-13 13:32   ` Aaron Tomlin
  2 siblings, 1 reply; 14+ messages in thread
From: Christophe Leroy @ 2022-02-10 14:00 UTC (permalink / raw)
  To: mcgrof, Aaron Tomlin
  Cc: cl, pmladek, mbenes, akpm, jeyu, linux-kernel, linux-modules,
	live-patching, atomlin, ghalat, allen.lkml, void, joe, msuchanek,
	oleksandr



Le 09/02/2022 à 18:11, Aaron Tomlin a écrit :
> No functional change.
> 
> This patch migrates module sysfs support out of core code into
> kernel/module/sysfs.c. In addition simple code refactoring to
> make this possible.
> 
> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> ---
>   kernel/module/Makefile   |   1 +
>   kernel/module/internal.h |  24 ++
>   kernel/module/main.c     | 458 +--------------------------------------
>   kernel/module/sysfs.c    | 425 ++++++++++++++++++++++++++++++++++++
>   4 files changed, 453 insertions(+), 455 deletions(-)
>   create mode 100644 kernel/module/sysfs.c

Checkpatch:

	total: 0 errors, 2 warnings, 10 checks, 946 lines checked


> 
> diff --git a/kernel/module/Makefile b/kernel/module/Makefile
> index c6be08060252..c30141c37eb3 100644
> --- a/kernel/module/Makefile
> +++ b/kernel/module/Makefile
> @@ -14,4 +14,5 @@ obj-$(CONFIG_STRICT_MODULE_RWX) += strict_rwx.o
>   obj-$(CONFIG_DEBUG_KMEMLEAK) += debug_kmemleak.o
>   obj-$(CONFIG_KALLSYMS) += kallsyms.o
>   obj-$(CONFIG_PROC_FS) += procfs.o
> +obj-$(CONFIG_SYSFS) += sysfs.o
>   endif
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index b67ce836746a..52d30bf6d6b0 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -35,6 +35,9 @@
>   extern struct mutex module_mutex;
>   extern struct list_head modules;
>   
> +extern struct module_attribute *modinfo_attrs[];
> +extern size_t modinfo_attrs_count;

Can't this come in sysfs.c as well ?

> +
>   /* Provided by the linker */
>   extern const struct kernel_symbol __start___ksymtab[];
>   extern const struct kernel_symbol __stop___ksymtab[];
> @@ -206,3 +209,24 @@ static inline char *find_kallsyms_symbol(struct module *mod, unsigned long addr,
>   	return NULL;
>   }
>   #endif /* CONFIG_KALLSYMS */
> +
> +#ifdef CONFIG_SYSFS
> +int mod_sysfs_setup(struct module *mod, const struct load_info *info,
> +			   struct kernel_param *kparam, unsigned int num_params);
> +void mod_sysfs_fini(struct module *mod);
> +void module_remove_modinfo_attrs(struct module *mod, int end);
> +void del_usage_links(struct module *mod);
> +void init_param_lock(struct module *mod);

Why don't we move mod_sysfs_teardown() here as well ?

It looks strange to move mod_sysfs_setup() and not mod_sysfs_teardown()

> +#else /* !CONFIG_SYSFS */
> +static int mod_sysfs_setup(struct module *mod,
> +			   const struct load_info *info,
> +			   struct kernel_param *kparam,
> +			   unsigned int num_params)
> +{
> +	return 0;
> +}
> +static inline void mod_sysfs_fini(struct module *mod) { }
> +static inline void module_remove_modinfo_attrs(struct module *mod, int end) { }
> +static inline void del_usage_links(struct module *mod) { }
> +static inline void init_param_lock(struct module *mod) { }
> +#endif /* CONFIG_SYSFS */

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 12/13] module: Move kdb_modules list out of core code
  2022-02-09 17:11 ` [PATCH v5 12/13] module: Move kdb_modules list out of core code Aaron Tomlin
@ 2022-02-10 14:05   ` Christophe Leroy
  2022-02-13 14:14     ` Aaron Tomlin
  0 siblings, 1 reply; 14+ messages in thread
From: Christophe Leroy @ 2022-02-10 14:05 UTC (permalink / raw)
  To: mcgrof, Aaron Tomlin
  Cc: cl, pmladek, mbenes, akpm, jeyu, linux-kernel, linux-modules,
	live-patching, atomlin, ghalat, allen.lkml, void, joe, msuchanek,
	oleksandr



Le 09/02/2022 à 18:11, Aaron Tomlin a écrit :
> No functional change.
> 
> This patch migrates kdb_modules list to core kdb code
> since the list of added/or loaded modules is no longer
> private.
> 
> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> ---
>   kernel/debug/kdb/kdb_main.c | 5 +++++
>   kernel/module/internal.h    | 1 +
>   kernel/module/main.c        | 4 ----
>   3 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index 0852a537dad4..f101f5f078f4 100644
> --- a/kernel/debug/kdb/kdb_main.c
> +++ b/kernel/debug/kdb/kdb_main.c
> @@ -59,6 +59,11 @@ EXPORT_SYMBOL(kdb_grepping_flag);
>   int kdb_grep_leading;
>   int kdb_grep_trailing;
>   
> +#ifdef CONFIG_MODULES
> +extern struct list_head modules;

Should go in module.h

> +struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */

Should be static and should be removed from kernel/debug/kdb/kdb_private.h

> +#endif /* CONFIG_MODULES */
> +
>   /*
>    * Kernel debugger state flags
>    */
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index 52d30bf6d6b0..c49b4900b30b 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -225,6 +225,7 @@ static int mod_sysfs_setup(struct module *mod,
>   {
>   	return 0;
>   }
> +

This should go in previous patch if needed (patch 11 sysfs)


>   static inline void mod_sysfs_fini(struct module *mod) { }
>   static inline void module_remove_modinfo_attrs(struct module *mod, int end) { }
>   static inline void del_usage_links(struct module *mod) { }
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c2255954b7df..519c5335f7a6 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -105,10 +105,6 @@ static void mod_update_bounds(struct module *mod)
>   		__mod_update_bounds(mod->init_layout.base, mod->init_layout.size);
>   }
>   
> -#ifdef CONFIG_KGDB_KDB
> -struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */
> -#endif /* CONFIG_KGDB_KDB */
> -
>   static void module_assert_mutex_or_preempt(void)
>   {
>   #ifdef CONFIG_LOCKDEP

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 13/13] module: Move version support into a separate file
  2022-02-09 17:11 ` [PATCH v5 13/13] module: Move version support into a separate file Aaron Tomlin
@ 2022-02-10 14:28   ` Christophe Leroy
  2022-02-13 18:03     ` Aaron Tomlin
  0 siblings, 1 reply; 14+ messages in thread
From: Christophe Leroy @ 2022-02-10 14:28 UTC (permalink / raw)
  To: mcgrof, Aaron Tomlin
  Cc: cl, pmladek, mbenes, akpm, jeyu, linux-kernel, linux-modules,
	live-patching, atomlin, ghalat, allen.lkml, void, joe, msuchanek,
	oleksandr



Le 09/02/2022 à 18:11, Aaron Tomlin a écrit :
> No functional change.
> 
> This patch migrates module version support out of core code into
> kernel/module/version.c. In addition simple code refactoring to
> make this possible.
> 
> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> ---
>   kernel/module/Makefile   |   1 +
>   kernel/module/internal.h |  50 +++++++++++++
>   kernel/module/main.c     | 150 +--------------------------------------
>   kernel/module/version.c  | 110 ++++++++++++++++++++++++++++
>   4 files changed, 163 insertions(+), 148 deletions(-)
>   create mode 100644 kernel/module/version.c

Sparse reports:

   CHECK   kernel/module/version.c
kernel/module/version.c:103:6: warning: symbol 'module_layout' was not 
declared. Should it be static?


Checkpatch:

	total: 0 errors, 2 warnings, 3 checks, 337 lines checked


Note that everywhere you get a warning for alignment not matching open 
parenthesis, first action should be to check if we really need it to be 
on two lines. When it's shorted than 100 chars it is usually better to 
keep it on a single line.

> 
> diff --git a/kernel/module/Makefile b/kernel/module/Makefile
> index c30141c37eb3..1f111aa47e88 100644
> --- a/kernel/module/Makefile
> +++ b/kernel/module/Makefile
> @@ -15,4 +15,5 @@ obj-$(CONFIG_DEBUG_KMEMLEAK) += debug_kmemleak.o
>   obj-$(CONFIG_KALLSYMS) += kallsyms.o
>   obj-$(CONFIG_PROC_FS) += procfs.o
>   obj-$(CONFIG_SYSFS) += sysfs.o
> +obj-$(CONFIG_MODVERSIONS) += version.o
>   endif
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index c49b4900b30b..475a66aa42f2 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -71,7 +71,31 @@ struct load_info {
>   	} index;
>   };
>   
> +struct symsearch {
> +	const struct kernel_symbol *start, *stop;
> +	const s32 *crcs;
> +	enum mod_license {
> +		NOT_GPL_ONLY,
> +		GPL_ONLY,
> +	} license;
> +};

Why don't leave this in main.c ?

> +
> +struct find_symbol_arg {
> +	/* Input */
> +	const char *name;
> +	bool gplok;
> +	bool warn;
> +
> +	/* Output */
> +	struct module *owner;
> +	const s32 *crc;
> +	const struct kernel_symbol *sym;
> +	enum mod_license license;
> +};
> +
>   int mod_verify_sig(const void *mod, struct load_info *info);
> +int try_to_force_load(struct module *mod, const char *reason);
> +bool find_symbol(struct find_symbol_arg *fsa);
>   struct module *find_module_all(const char *name, size_t len, bool even_unformed);
>   unsigned long kernel_symbol_value(const struct kernel_symbol *sym);
>   int cmp_name(const void *name, const void *sym);
> @@ -231,3 +255,29 @@ static inline void module_remove_modinfo_attrs(struct module *mod, int end) { }
>   static inline void del_usage_links(struct module *mod) { }
>   static inline void init_param_lock(struct module *mod) { }
>   #endif /* CONFIG_SYSFS */
> +
> +#ifdef CONFIG_MODVERSIONS
> +int check_version(const struct load_info *info,
> +		  const char *symname, struct module *mod, const s32 *crc);
> +int check_modstruct_version(const struct load_info *info, struct module *mod);
> +int same_magic(const char *amagic, const char *bmagic, bool has_crcs);
> +#else /* !CONFIG_MODVERSIONS */
> +static inline int check_version(const struct load_info *info,
> +				const char *symname,
> +				struct module *mod,
> +				const s32 *crc)
> +{
> +	return 1;
> +}
> +
> +static inline int check_modstruct_version(const struct load_info *info,
> +					  struct module *mod)
> +{
> +	return 1;
> +}
> +
> +static inline int same_magic(const char *amagic, const char *bmagic, bool has_crcs)
> +{
> +	return strcmp(amagic, bmagic) == 0;
> +}
> +#endif /* CONFIG_MODVERSIONS */

> diff --git a/kernel/module/version.c b/kernel/module/version.c
> new file mode 100644
> index 000000000000..10a1490d1b9e
> --- /dev/null
> +++ b/kernel/module/version.c
> @@ -0,0 +1,110 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Module version support
> + *
> + * Copyright (C) 2008 Rusty Russell
> + */
> +
> +#include <linux/module.h>
> +#include <linux/string.h>
> +#include <linux/printk.h>
> +#include "internal.h"
> +
> +static u32 resolve_rel_crc(const s32 *crc)
> +{
> +	return *(u32 *)((void *)crc + *crc);
> +}
> +
> +int check_version(const struct load_info *info,
> +			 const char *symname,
> +			 struct module *mod,
> +			 const s32 *crc)
> +{
> +	Elf_Shdr *sechdrs = info->sechdrs;
> +	unsigned int versindex = info->index.vers;
> +	unsigned int i, num_versions;
> +	struct modversion_info *versions;
> +
> +	/* Exporting module didn't supply crcs?  OK, we're already tainted. */
> +	if (!crc)
> +		return 1;
> +
> +	/* No versions at all?  modprobe --force does this. */
> +	if (versindex == 0)
> +		return try_to_force_load(mod, symname) == 0;
> +
> +	versions = (void *) sechdrs[versindex].sh_addr;
> +	num_versions = sechdrs[versindex].sh_size
> +		/ sizeof(struct modversion_info);
> +
> +	for (i = 0; i < num_versions; i++) {
> +		u32 crcval;
> +
> +		if (strcmp(versions[i].name, symname) != 0)
> +			continue;
> +
> +		if (IS_ENABLED(CONFIG_MODULE_REL_CRCS))
> +			crcval = resolve_rel_crc(crc);
> +		else
> +			crcval = *crc;
> +		if (versions[i].crc == crcval)
> +			return 1;
> +		pr_debug("Found checksum %X vs module %lX\n",
> +			 crcval, versions[i].crc);
> +		goto bad_version;
> +	}
> +
> +	/* Broken toolchain. Warn once, then let it go.. */
> +	pr_warn_once("%s: no symbol version for %s\n", info->name, symname);
> +	return 1;
> +
> +bad_version:
> +	pr_warn("%s: disagrees about version of symbol %s\n",
> +	       info->name, symname);
> +	return 0;
> +}
> +
> +inline int check_modstruct_version(const struct load_info *info,
> +					  struct module *mod)

inline is pointless for a non static function


> +{
> +	struct find_symbol_arg fsa = {
> +		.name	= "module_layout",
> +		.gplok	= true,
> +	};
> +
> +	/*
> +	 * Since this should be found in kernel (which can't be removed), no
> +	 * locking is necessary -- use preempt_disable() to placate lockdep.
> +	 */
> +	preempt_disable();
> +	if (!find_symbol(&fsa)) {
> +		preempt_enable();
> +		BUG();
> +	}
> +	preempt_enable();
> +	return check_version(info, "module_layout", mod, fsa.crc);
> +}
> +
> +/* First part is kernel version, which we ignore if module has crcs. */
> +inline int same_magic(const char *amagic, const char *bmagic,
> +			     bool has_crcs)

Same, not point for inline keyword here.


> +{
> +	if (has_crcs) {
> +		amagic += strcspn(amagic, " ");
> +		bmagic += strcspn(bmagic, " ");
> +	}
> +	return strcmp(amagic, bmagic) == 0;
> +}
> +
> +/*
> + * Generate the signature for all relevant module structures here.
> + * If these change, we don't want to try to parse the module.
> + */
> +void module_layout(struct module *mod,
> +		   struct modversion_info *ver,
> +		   struct kernel_param *kp,
> +		   struct kernel_symbol *ks,
> +		   struct tracepoint * const *tp)
> +{
> +}
> +EXPORT_SYMBOL(module_layout);

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 11/13] module: Move sysfs support into a separate file
  2022-02-10 14:00 ` [PATCH v5 11/13] module: Move sysfs " Christophe Leroy
@ 2022-02-13 13:32   ` Aaron Tomlin
  0 siblings, 0 replies; 14+ messages in thread
From: Aaron Tomlin @ 2022-02-13 13:32 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: mcgrof, cl, pmladek, mbenes, akpm, jeyu, linux-kernel,
	linux-modules, live-patching, atomlin, ghalat, allen.lkml, void,
	joe, msuchanek, oleksandr

On Thu 2022-02-10 14:00 +0000, Christophe Leroy wrote:
> Checkpatch:
>
>     total: 0 errors, 2 warnings, 10 checks, 946 lines checked

Ok.

> > diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> > index b67ce836746a..52d30bf6d6b0 100644
> > --- a/kernel/module/internal.h
> > +++ b/kernel/module/internal.h
> > @@ -35,6 +35,9 @@
> >   extern struct mutex module_mutex;
> >   extern struct list_head modules;
> >
> > +extern struct module_attribute *modinfo_attrs[];
> > +extern size_t modinfo_attrs_count;
>
> Can't this come in sysfs.c as well ?

No.

> > +#ifdef CONFIG_SYSFS
> > +int mod_sysfs_setup(struct module *mod, const struct load_info *info,
> > +               struct kernel_param *kparam, unsigned int num_params);
> > +void mod_sysfs_fini(struct module *mod);
> > +void module_remove_modinfo_attrs(struct module *mod, int end);
> > +void del_usage_links(struct module *mod);
> > +void init_param_lock(struct module *mod);
>
> Why don't we move mod_sysfs_teardown() here as well ?

Agreed.


Kind regards,

-- 
Aaron Tomlin


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 12/13] module: Move kdb_modules list out of core code
  2022-02-10 14:05   ` Christophe Leroy
@ 2022-02-13 14:14     ` Aaron Tomlin
  0 siblings, 0 replies; 14+ messages in thread
From: Aaron Tomlin @ 2022-02-13 14:14 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: mcgrof, cl, pmladek, mbenes, akpm, jeyu, linux-kernel,
	linux-modules, live-patching, atomlin, ghalat, allen.lkml, void,
	joe, msuchanek, oleksandr

On Thu 2022-02-10 14:05 +0000, Christophe Leroy wrote:
> > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> > index 0852a537dad4..f101f5f078f4 100644
> > --- a/kernel/debug/kdb/kdb_main.c
> > +++ b/kernel/debug/kdb/kdb_main.c
> > @@ -59,6 +59,11 @@ EXPORT_SYMBOL(kdb_grepping_flag);
> >   int kdb_grep_leading;
> >   int kdb_grep_trailing;
> >
> > +#ifdef CONFIG_MODULES
> > +extern struct list_head modules;
>
> Should go in module.h

I disagree. Let's keep it restricted somewhat. The list of loaded/or added
modules is not widely used.

> > +struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */
>
> Should be static and should be removed from kernel/debug/kdb/kdb_private.h

Agreed. It is only used in kernel/debug/kdb/kdb_main.c.

> > diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> > index 52d30bf6d6b0..c49b4900b30b 100644
> > --- a/kernel/module/internal.h
> > +++ b/kernel/module/internal.h
> > @@ -225,6 +225,7 @@ static int mod_sysfs_setup(struct module *mod,
> >   {
> >       return 0;
> >   }
> > +
>
> This should go in previous patch if needed (patch 11 sysfs)

Agreed.


Kind regards,

-- 
Aaron Tomlin


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 13/13] module: Move version support into a separate file
  2022-02-10 14:28   ` Christophe Leroy
@ 2022-02-13 18:03     ` Aaron Tomlin
  2022-02-13 18:29       ` Christophe Leroy
  2022-02-17 15:51       ` Miroslav Benes
  0 siblings, 2 replies; 14+ messages in thread
From: Aaron Tomlin @ 2022-02-13 18:03 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: mcgrof, cl, pmladek, mbenes, akpm, jeyu, linux-kernel,
	linux-modules, live-patching, atomlin, ghalat, allen.lkml, void,
	joe, msuchanek, oleksandr

On Thu 2022-02-10 14:28 +0000, Christophe Leroy wrote:
>
>
> Le 09/02/2022 à 18:11, Aaron Tomlin a écrit :
> > No functional change.
> >
> > This patch migrates module version support out of core code into
> > kernel/module/version.c. In addition simple code refactoring to
> > make this possible.
> >
> > Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> > ---
> >   kernel/module/Makefile   |   1 +
> >   kernel/module/internal.h |  50 +++++++++++++
> >   kernel/module/main.c     | 150 +--------------------------------------
> >   kernel/module/version.c  | 110 ++++++++++++++++++++++++++++
> >   4 files changed, 163 insertions(+), 148 deletions(-)
> >   create mode 100644 kernel/module/version.c
>
> Sparse reports:
>
>    CHECK   kernel/module/version.c
> kernel/module/version.c:103:6: warning: symbol 'module_layout' was not
> declared. Should it be static?

The function module_layout() does not appear to be used. So, I've decided
to remove it.

> Checkpatch:
>
>     total: 0 errors, 2 warnings, 3 checks, 337 lines checked

Ok.

> > +struct symsearch {
> > +    const struct kernel_symbol *start, *stop;
> > +    const s32 *crcs;
> > +    enum mod_license {
> > +        NOT_GPL_ONLY,
> > +        GPL_ONLY,
> > +    } license;
> > +};
>
> Why don't leave this in main.c ?

Yes, struct 'symsearch' is not used outside of kernel/module/main.c.

> > +inline int check_modstruct_version(const struct load_info *info,
> > +                      struct module *mod)
>
> inline is pointless for a non static function

This was an unfortunate oversight.

> > +inline int same_magic(const char *amagic, const char *bmagic,
> > +                 bool has_crcs)
>
> Same, not point for inline keyword here.

Agreed.


Kind regards,

-- 
Aaron Tomlin


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 13/13] module: Move version support into a separate file
  2022-02-13 18:03     ` Aaron Tomlin
@ 2022-02-13 18:29       ` Christophe Leroy
  2022-02-15 15:05         ` Aaron Tomlin
  2022-02-17 15:51       ` Miroslav Benes
  1 sibling, 1 reply; 14+ messages in thread
From: Christophe Leroy @ 2022-02-13 18:29 UTC (permalink / raw)
  To: Aaron Tomlin
  Cc: mcgrof, cl, pmladek, mbenes, akpm, jeyu, linux-kernel,
	linux-modules, live-patching, atomlin, ghalat, allen.lkml, void,
	joe, msuchanek, oleksandr



Le 13/02/2022 à 19:03, Aaron Tomlin a écrit :
> On Thu 2022-02-10 14:28 +0000, Christophe Leroy wrote:
>>
>>
>> Le 09/02/2022 à 18:11, Aaron Tomlin a écrit :
>>> No functional change.
>>>
>>> This patch migrates module version support out of core code into
>>> kernel/module/version.c. In addition simple code refactoring to
>>> make this possible.
>>>
>>> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
>>> ---
>>>    kernel/module/Makefile   |   1 +
>>>    kernel/module/internal.h |  50 +++++++++++++
>>>    kernel/module/main.c     | 150 +--------------------------------------
>>>    kernel/module/version.c  | 110 ++++++++++++++++++++++++++++
>>>    4 files changed, 163 insertions(+), 148 deletions(-)
>>>    create mode 100644 kernel/module/version.c
>>
>> Sparse reports:
>>
>>     CHECK   kernel/module/version.c
>> kernel/module/version.c:103:6: warning: symbol 'module_layout' was not
>> declared. Should it be static?
> 
> The function module_layout() does not appear to be used. So, I've decided
> to remove it.

I'm not sure you can do that.

 From commit 8c8ef42aee8f ("module: include other structures in module 
version check") I understand that module_layout is there for some signature.


> 
>> Checkpatch:
>>
>>      total: 0 errors, 2 warnings, 3 checks, 337 lines checked
> 
> Ok.
> 
>>> +struct symsearch {
>>> +    const struct kernel_symbol *start, *stop;
>>> +    const s32 *crcs;
>>> +    enum mod_license {
>>> +        NOT_GPL_ONLY,
>>> +        GPL_ONLY,
>>> +    } license;
>>> +};
>>
>> Why don't leave this in main.c ?
> 
> Yes, struct 'symsearch' is not used outside of kernel/module/main.c.
> 
>>> +inline int check_modstruct_version(const struct load_info *info,
>>> +                      struct module *mod)
>>
>> inline is pointless for a non static function
> 
> This was an unfortunate oversight.
> 
>>> +inline int same_magic(const char *amagic, const char *bmagic,
>>> +                 bool has_crcs)
>>
>> Same, not point for inline keyword here.
> 
> Agreed.
> 
> 
> Kind regards,
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 13/13] module: Move version support into a separate file
  2022-02-13 18:29       ` Christophe Leroy
@ 2022-02-15 15:05         ` Aaron Tomlin
  2022-02-15 15:11           ` Christophe Leroy
  0 siblings, 1 reply; 14+ messages in thread
From: Aaron Tomlin @ 2022-02-15 15:05 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Aaron Tomlin, mcgrof, cl, pmladek, mbenes, akpm, jeyu,
	linux-kernel, linux-modules, live-patching, ghalat, allen.lkml,
	void, joe, msuchanek, oleksandr

On Sun 2022-02-13 18:29 +0000, Christophe Leroy wrote:
> I'm not sure you can do that.
> 
>  From commit 8c8ef42aee8f ("module: include other structures in module 
> version check") I understand that module_layout is there for some signature.

Hi Christophe,

I see. In which case, if I understand correctly, we'd have to ignore the
report from Sparse. I will add a comment to hopefully suggest against
removal.


Kind regards,

-- 
Aaron Tomlin

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 13/13] module: Move version support into a separate file
  2022-02-15 15:05         ` Aaron Tomlin
@ 2022-02-15 15:11           ` Christophe Leroy
  0 siblings, 0 replies; 14+ messages in thread
From: Christophe Leroy @ 2022-02-15 15:11 UTC (permalink / raw)
  To: Aaron Tomlin
  Cc: Aaron Tomlin, mcgrof, cl, pmladek, mbenes, akpm, jeyu,
	linux-kernel, linux-modules, live-patching, ghalat, allen.lkml,
	void, joe, msuchanek, oleksandr



Le 15/02/2022 à 16:05, Aaron Tomlin a écrit :
> On Sun 2022-02-13 18:29 +0000, Christophe Leroy wrote:
>> I'm not sure you can do that.
>>
>>   From commit 8c8ef42aee8f ("module: include other structures in module
>> version check") I understand that module_layout is there for some signature.
> 
> Hi Christophe,
> 
> I see. In which case, if I understand correctly, we'd have to ignore the
> report from Sparse. I will add a comment to hopefully suggest against
> removal.
> 
> 

I would add the function's prototype in internal.h to shut up sparse.

Christophe

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 13/13] module: Move version support into a separate file
  2022-02-13 18:03     ` Aaron Tomlin
  2022-02-13 18:29       ` Christophe Leroy
@ 2022-02-17 15:51       ` Miroslav Benes
  2022-02-17 16:26         ` Aaron Tomlin
  1 sibling, 1 reply; 14+ messages in thread
From: Miroslav Benes @ 2022-02-17 15:51 UTC (permalink / raw)
  To: Aaron Tomlin
  Cc: Christophe Leroy, mcgrof, cl, pmladek, akpm, jeyu, linux-kernel,
	linux-modules, live-patching, atomlin, ghalat, allen.lkml, void,
	joe, msuchanek, oleksandr

> > > +struct symsearch {
> > > +    const struct kernel_symbol *start, *stop;
> > > +    const s32 *crcs;
> > > +    enum mod_license {
> > > +        NOT_GPL_ONLY,
> > > +        GPL_ONLY,
> > > +    } license;
> > > +};
> >
> > Why don't leave this in main.c ?
> 
> Yes, struct 'symsearch' is not used outside of kernel/module/main.c.

It is not, but "struct find_symbol_arg", which you moved, uses "enum 
mod_license" defined above, so you can either leave it as it is, or carve 
"enum mod_license" definition out.

Miroslav

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 13/13] module: Move version support into a separate file
  2022-02-17 15:51       ` Miroslav Benes
@ 2022-02-17 16:26         ` Aaron Tomlin
  0 siblings, 0 replies; 14+ messages in thread
From: Aaron Tomlin @ 2022-02-17 16:26 UTC (permalink / raw)
  To: Miroslav Benes
  Cc: Christophe Leroy, mcgrof, cl, pmladek, akpm, jeyu, linux-kernel,
	linux-modules, live-patching, atomlin, ghalat, allen.lkml, void,
	joe, msuchanek, oleksandr

On Thu 2022-02-17 16:51 +0100, Miroslav Benes wrote:
> It is not, but "struct find_symbol_arg", which you moved, uses "enum
> mod_license" defined above, so you can either leave it as it is, or carve
> "enum mod_license" definition out.

Hi Miroslav,

I've done this already for v6. I hope to share the latest version for
review shortly.


Kind regards,

-- 
Aaron Tomlin


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2022-02-17 16:26 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-09 17:11 [PATCH v5 11/13] module: Move sysfs support into a separate file Aaron Tomlin
2022-02-09 17:11 ` [PATCH v5 12/13] module: Move kdb_modules list out of core code Aaron Tomlin
2022-02-10 14:05   ` Christophe Leroy
2022-02-13 14:14     ` Aaron Tomlin
2022-02-09 17:11 ` [PATCH v5 13/13] module: Move version support into a separate file Aaron Tomlin
2022-02-10 14:28   ` Christophe Leroy
2022-02-13 18:03     ` Aaron Tomlin
2022-02-13 18:29       ` Christophe Leroy
2022-02-15 15:05         ` Aaron Tomlin
2022-02-15 15:11           ` Christophe Leroy
2022-02-17 15:51       ` Miroslav Benes
2022-02-17 16:26         ` Aaron Tomlin
2022-02-10 14:00 ` [PATCH v5 11/13] module: Move sysfs " Christophe Leroy
2022-02-13 13:32   ` Aaron Tomlin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).