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

[Christophe Leroy]

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@xxxxxxxxxx>
> ---
>   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);