2022-02-09 17:29:33

by Aaron Tomlin

[permalink] [raw]
Subject: [PATCH v5 13/13] module: Move version support into a separate file

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 <[email protected]>
---
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



2022-02-10 19:17:36

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH v5 13/13] module: Move version support into a separate file



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 <[email protected]>
> ---
> 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);

2022-02-13 21:58:24

by Aaron Tomlin

[permalink] [raw]
Subject: Re: [PATCH v5 13/13] module: Move version support into a separate file

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 <[email protected]>
> > ---
> > 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

2022-02-13 22:34:18

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH v5 13/13] module: Move version support into a separate file



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 <[email protected]>
>>> ---
>>> 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,
>

2022-02-15 15:34:20

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH v5 13/13] module: Move version support into a separate file



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

2022-02-16 07:04:41

by Aaron Tomlin

[permalink] [raw]
Subject: Re: [PATCH v5 13/13] module: Move version support into a separate file

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

2022-02-17 18:24:36

by Miroslav Benes

[permalink] [raw]
Subject: Re: [PATCH v5 13/13] module: Move version support into a separate file

> > > +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

2022-02-18 00:05:00

by Aaron Tomlin

[permalink] [raw]
Subject: Re: [PATCH v5 13/13] module: Move version support into a separate file

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