[PATCH v3 0/2] module: Introduce module unload taint tracking

[PATCH v3 0/2] module: Introduce module unload taint tracking

[Aaron Tomlin]

Hi Luis,

This is based on the latest mcgrof/modules-next branch.  I have decided
still to use RCU even though no entry is ever removed from the unloaded
tainted modules list. That being said, if I understand correctly, it is not
safe in some instances to use 'module_mutex' in print_modules(). So instead
we disable preemption to ensure list traversal with concurrent list
manipulation e.g. list_add_rcu(), is safe too.

Changes since RFC v2 [1]
 - Dropped RFC from subject
 - Removed the newline i.e. "\n" in printk()
 - Always include the tainted module's unload count
 - Unconditionally display each unloaded tainted module

Please let me know your thoughts.

[1]: https://lore.kernel.org/all/20220419150334.3395019-1-atomlin@redhat.com/


Aaron Tomlin (2):
  module: Make module_flags_taint() accept a module's taints bitmap
    directly
  module: Introduce module unload taint tracking

 init/Kconfig         | 11 +++++++
 kernel/module/main.c | 73 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 80 insertions(+), 4 deletions(-)


base-commit: eeaec7801c421e17edda6e45a32d4a5596b633da
-- 
2.34.1

[PATCH v3 1/2] module: Make module_flags_taint() accept a module's taints bitmap directly

[Aaron Tomlin]

No functional change.

The purpose of this patch is to modify module_flags_taint() to accept a
module's taints bitmap as a parameter and modifies all users accordingly.
This is in preparation for module unload taint tracking support.

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 kernel/module/main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 05a42d8fcd7a..ea78cec316dd 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -890,13 +890,13 @@ static inline int module_unload_init(struct module *mod)
 }
 #endif /* CONFIG_MODULE_UNLOAD */
 
-static size_t module_flags_taint(struct module *mod, char *buf)
+static size_t module_flags_taint(unsigned long taints, char *buf)
 {
 	size_t l = 0;
 	int i;
 
 	for (i = 0; i < TAINT_FLAGS_COUNT; i++) {
-		if (taint_flags[i].module && test_bit(i, &mod->taints))
+		if (taint_flags[i].module && test_bit(i, &taints))
 			buf[l++] = taint_flags[i].c_true;
 	}
 
@@ -974,7 +974,7 @@ static ssize_t show_taint(struct module_attribute *mattr,
 {
 	size_t l;
 
-	l = module_flags_taint(mk->mod, buffer);
+	l = module_flags_taint(mk->mod->taints, buffer);
 	buffer[l++] = '\n';
 	return l;
 }
@@ -2993,7 +2993,7 @@ char *module_flags(struct module *mod, char *buf)
 	    mod->state == MODULE_STATE_GOING ||
 	    mod->state == MODULE_STATE_COMING) {
 		buf[bx++] = '(';
-		bx += module_flags_taint(mod, buf + bx);
+		bx += module_flags_taint(mod->taints, buf + bx);
 		/* Show a - for module-is-being-unloaded */
 		if (mod->state == MODULE_STATE_GOING)
 			buf[bx++] = '-';
-- 
2.34.1

[PATCH v3 2/2] module: Introduce module unload taint tracking

[Aaron Tomlin]

Currently, only the initial module that tainted the kernel is
recorded e.g. when an out-of-tree module is loaded.

The purpose of this patch is to allow the kernel to maintain a record of
each unloaded module that taints the kernel. So, in addition to
displaying a list of linked modules (see print_modules()) e.g. in the
event of a detected bad page, unloaded modules that carried a taint/or
taints are displayed too. A tainted module unload count is maintained.

The number of tracked modules is not fixed. This feature is disabled by
default.

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 init/Kconfig         | 11 ++++++++
 kernel/module/main.c | 65 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+)

diff --git a/init/Kconfig b/init/Kconfig
index ddcbefe535e9..6b30210f787d 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -2118,6 +2118,17 @@ config MODULE_FORCE_UNLOAD
 	  rmmod).  This is mainly for kernel developers and desperate users.
 	  If unsure, say N.
 
+config MODULE_UNLOAD_TAINT_TRACKING
+	bool "Tainted module unload tracking"
+	depends on MODULE_UNLOAD
+	default n
+	help
+	  This option allows you to maintain a record of each unloaded
+	  module that tainted the kernel. In addition to displaying a
+	  list of linked (or loaded) modules e.g. on detection of a bad
+	  page (see bad_page()), the aforementioned details are also
+	  shown. If unsure, say N.
+
 config MODVERSIONS
 	bool "Module versioning support"
 	help
diff --git a/kernel/module/main.c b/kernel/module/main.c
index ea78cec316dd..d7cc64172dd1 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -68,6 +68,16 @@
  */
 DEFINE_MUTEX(module_mutex);
 LIST_HEAD(modules);
+#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING
+static LIST_HEAD(unloaded_tainted_modules);
+
+struct mod_unload_taint {
+	struct list_head list;
+	char name[MODULE_NAME_LEN];
+	unsigned long taints;
+	u64 count;
+};
+#endif
 
 /* Work queue for freeing init sections in success case */
 static void do_free_init(struct work_struct *w);
@@ -150,6 +160,41 @@ int unregister_module_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL(unregister_module_notifier);
 
+#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING
+static int try_add_tainted_module(struct module *mod)
+{
+	struct mod_unload_taint *mod_taint;
+
+	module_assert_mutex_or_preempt();
+
+	list_for_each_entry_rcu(mod_taint, &unloaded_tainted_modules, list,
+				lockdep_is_held(&module_mutex)) {
+		size_t len = strlen(mod_taint->name);
+
+		if (len == strlen(mod->name) && !memcmp(mod_taint->name, mod->name, len) &&
+		    mod_taint->taints & mod->taints) {
+			mod_taint->count++;
+			goto out;
+		}
+	}
+
+	mod_taint = kmalloc(sizeof(*mod_taint), GFP_KERNEL);
+	if (unlikely(!mod_taint))
+		return -ENOMEM;
+	strscpy(mod_taint->name, mod->name, MODULE_NAME_LEN);
+	mod_taint->taints = mod->taints;
+	list_add_rcu(&mod_taint->list, &unloaded_tainted_modules);
+	mod_taint->count = 1;
+out:
+	return 0;
+}
+#else /* MODULE_UNLOAD_TAINT_TRACKING */
+static int try_add_tainted_module(struct module *mod)
+{
+	return 0;
+}
+#endif
+
 /*
  * We require a truly strong try_module_get(): 0 means success.
  * Otherwise an error is returned due to ongoing or failed
@@ -1201,6 +1246,9 @@ static void free_module(struct module *mod)
 	module_bug_cleanup(mod);
 	/* Wait for RCU-sched synchronizing before releasing mod->list and buglist. */
 	synchronize_rcu();
+	if (try_add_tainted_module(mod))
+		pr_err("%s: adding tainted module to the unloaded tainted modules list failed.\n",
+		       mod->name);
 	mutex_unlock(&module_mutex);
 
 	/* Clean up CFI for the module. */
@@ -3126,6 +3174,9 @@ struct module *__module_text_address(unsigned long addr)
 void print_modules(void)
 {
 	struct module *mod;
+#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING
+	struct mod_unload_taint *mod_taint;
+#endif
 	char buf[MODULE_FLAGS_BUF_SIZE];
 
 	printk(KERN_DEFAULT "Modules linked in:");
@@ -3136,6 +3187,20 @@ void print_modules(void)
 			continue;
 		pr_cont(" %s%s", mod->name, module_flags(mod, buf));
 	}
+#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING
+	if (!list_empty(&unloaded_tainted_modules)) {
+		printk(KERN_DEFAULT "Unloaded tainted modules:");
+		list_for_each_entry_rcu(mod_taint, &unloaded_tainted_modules,
+					list) {
+			size_t l;
+
+			l = module_flags_taint(mod_taint->taints, buf);
+			buf[l++] = '\0';
+			pr_cont(" %s(%s):%llu", mod_taint->name, buf,
+				mod_taint->count);
+		}
+#endif
+	}
 	preempt_enable();
 	if (last_unloaded_module[0])
 		pr_cont(" [last unloaded: %s]", last_unloaded_module);
-- 
2.34.1

Re: [PATCH v3 2/2] module: Introduce module unload taint tracking

[kernel test robot]

Hi Aaron,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on eeaec7801c421e17edda6e45a32d4a5596b633da]

url:    https://github.com/intel-lab-lkp/linux/commits/Aaron-Tomlin/module-Introduce-module-unload-taint-tracking/20220420-195459
base:   eeaec7801c421e17edda6e45a32d4a5596b633da
config: hexagon-randconfig-r045-20220420 (https://download.01.org/0day-ci/archive/20220421/202204210819.DQQP836u-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project bac6cd5bf85669e3376610cfc4c4f9ca015e7b9b)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/523ff1f52de42049b9cc5a4db0495a5f21a7ee7c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Aaron-Tomlin/module-Introduce-module-unload-taint-tracking/20220420-195459
        git checkout 523ff1f52de42049b9cc5a4db0495a5f21a7ee7c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> kernel/module/main.c:3204:2: error: meaningless 'volatile' on asm outside function
           preempt_enable();
           ^
   include/linux/preempt.h:277:28: note: expanded from macro 'preempt_enable'
   #define preempt_enable()                        barrier()
                                                   ^
   include/linux/compiler.h:86:28: note: expanded from macro 'barrier'
   # define barrier() __asm__ __volatile__("": : :"memory")
                              ^
   kernel/module/main.c:3204:2: error: expected ')'
   include/linux/preempt.h:277:28: note: expanded from macro 'preempt_enable'
   #define preempt_enable()                        barrier()
                                                   ^
   include/linux/compiler.h:86:43: note: expanded from macro 'barrier'
   # define barrier() __asm__ __volatile__("": : :"memory")
                                             ^
   kernel/module/main.c:3204:2: note: to match this '('
   include/linux/preempt.h:277:28: note: expanded from macro 'preempt_enable'
   #define preempt_enable()                        barrier()
                                                   ^
   include/linux/compiler.h:86:40: note: expanded from macro 'barrier'
   # define barrier() __asm__ __volatile__("": : :"memory")
                                          ^
   kernel/module/main.c:3205:2: error: expected identifier or '('
           if (last_unloaded_module[0])
           ^
   include/linux/compiler.h:56:23: note: expanded from macro 'if'
   #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                         ^
   kernel/module/main.c:3207:2: error: expected identifier or '('
           pr_cont("\n");
           ^
   include/linux/printk.h:531:2: note: expanded from macro 'pr_cont'
           printk(KERN_CONT fmt, ##__VA_ARGS__)
           ^
   include/linux/printk.h:446:26: note: expanded from macro 'printk'
   #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
                            ^
   include/linux/printk.h:416:3: note: expanded from macro 'printk_index_wrap'
           ({                                                              \
            ^
   kernel/module/main.c:3207:2: error: expected ')'
   include/linux/printk.h:531:2: note: expanded from macro 'pr_cont'
           printk(KERN_CONT fmt, ##__VA_ARGS__)
           ^
   include/linux/printk.h:446:26: note: expanded from macro 'printk'
   #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
                            ^
   include/linux/printk.h:416:3: note: expanded from macro 'printk_index_wrap'
           ({                                                              \
            ^
   kernel/module/main.c:3207:2: note: to match this '('
   include/linux/printk.h:531:2: note: expanded from macro 'pr_cont'
           printk(KERN_CONT fmt, ##__VA_ARGS__)
           ^
   include/linux/printk.h:446:26: note: expanded from macro 'printk'
   #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
                            ^
   include/linux/printk.h:416:2: note: expanded from macro 'printk_index_wrap'
           ({                                                              \
           ^
   kernel/module/main.c:3208:1: error: extraneous closing brace ('}')
   }
   ^
   6 errors generated.


vim +/volatile +3204 kernel/module/main.c

e610499e2656e61 kernel/module.c      Rusty Russell  2009-03-31  3172  
^1da177e4c3f415 kernel/module.c      Linus Torvalds 2005-04-16  3173  /* Don't grab lock, we're oopsing. */
^1da177e4c3f415 kernel/module.c      Linus Torvalds 2005-04-16  3174  void print_modules(void)
^1da177e4c3f415 kernel/module.c      Linus Torvalds 2005-04-16  3175  {
^1da177e4c3f415 kernel/module.c      Linus Torvalds 2005-04-16  3176  	struct module *mod;
523ff1f52de4204 kernel/module/main.c Aaron Tomlin   2022-04-20  3177  #ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING
523ff1f52de4204 kernel/module/main.c Aaron Tomlin   2022-04-20  3178  	struct mod_unload_taint *mod_taint;
523ff1f52de4204 kernel/module/main.c Aaron Tomlin   2022-04-20  3179  #endif
7fd8329ba502ef7 kernel/module.c      Petr Mladek    2016-09-21  3180  	char buf[MODULE_FLAGS_BUF_SIZE];
^1da177e4c3f415 kernel/module.c      Linus Torvalds 2005-04-16  3181  
b231125af7811a2 kernel/module.c      Linus Torvalds 2009-06-16  3182  	printk(KERN_DEFAULT "Modules linked in:");
d72b37513cdfbd3 kernel/module.c      Andi Kleen     2008-08-30  3183  	/* Most callers should already have preempt disabled, but make sure */
d72b37513cdfbd3 kernel/module.c      Andi Kleen     2008-08-30  3184  	preempt_disable();
0d21b0e3477395e kernel/module.c      Rusty Russell  2013-01-12  3185  	list_for_each_entry_rcu(mod, &modules, list) {
0d21b0e3477395e kernel/module.c      Rusty Russell  2013-01-12  3186  		if (mod->state == MODULE_STATE_UNFORMED)
0d21b0e3477395e kernel/module.c      Rusty Russell  2013-01-12  3187  			continue;
27bba4d6bb3779a kernel/module.c      Jiri Slaby     2014-02-03  3188  		pr_cont(" %s%s", mod->name, module_flags(mod, buf));
0d21b0e3477395e kernel/module.c      Rusty Russell  2013-01-12  3189  	}
523ff1f52de4204 kernel/module/main.c Aaron Tomlin   2022-04-20  3190  #ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING
523ff1f52de4204 kernel/module/main.c Aaron Tomlin   2022-04-20  3191  	if (!list_empty(&unloaded_tainted_modules)) {
523ff1f52de4204 kernel/module/main.c Aaron Tomlin   2022-04-20  3192  		printk(KERN_DEFAULT "Unloaded tainted modules:");
523ff1f52de4204 kernel/module/main.c Aaron Tomlin   2022-04-20  3193  		list_for_each_entry_rcu(mod_taint, &unloaded_tainted_modules,
523ff1f52de4204 kernel/module/main.c Aaron Tomlin   2022-04-20  3194  					list) {
523ff1f52de4204 kernel/module/main.c Aaron Tomlin   2022-04-20  3195  			size_t l;
523ff1f52de4204 kernel/module/main.c Aaron Tomlin   2022-04-20  3196  
523ff1f52de4204 kernel/module/main.c Aaron Tomlin   2022-04-20  3197  			l = module_flags_taint(mod_taint->taints, buf);
523ff1f52de4204 kernel/module/main.c Aaron Tomlin   2022-04-20  3198  			buf[l++] = '\0';
523ff1f52de4204 kernel/module/main.c Aaron Tomlin   2022-04-20  3199  			pr_cont(" %s(%s):%llu", mod_taint->name, buf,
523ff1f52de4204 kernel/module/main.c Aaron Tomlin   2022-04-20  3200  				mod_taint->count);
523ff1f52de4204 kernel/module/main.c Aaron Tomlin   2022-04-20  3201  		}
523ff1f52de4204 kernel/module/main.c Aaron Tomlin   2022-04-20  3202  #endif
523ff1f52de4204 kernel/module/main.c Aaron Tomlin   2022-04-20  3203  	}
d72b37513cdfbd3 kernel/module.c      Andi Kleen     2008-08-30 @3204  	preempt_enable();

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v3 2/2] module: Introduce module unload taint tracking

[Oleksandr Natalenko]

Hello.

Thanks for this submission. Please see one comment inline.

On středa 20. dubna 2022 13:52:57 CEST Aaron Tomlin wrote:
> Currently, only the initial module that tainted the kernel is
> recorded e.g. when an out-of-tree module is loaded.
> 
> The purpose of this patch is to allow the kernel to maintain a record of
> each unloaded module that taints the kernel. So, in addition to
> displaying a list of linked modules (see print_modules()) e.g. in the
> event of a detected bad page, unloaded modules that carried a taint/or
> taints are displayed too. A tainted module unload count is maintained.
> 
> The number of tracked modules is not fixed. This feature is disabled by
> default.
> 
> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> ---
>  init/Kconfig         | 11 ++++++++
>  kernel/module/main.c | 65 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 76 insertions(+)
> 
> diff --git a/init/Kconfig b/init/Kconfig
> index ddcbefe535e9..6b30210f787d 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -2118,6 +2118,17 @@ config MODULE_FORCE_UNLOAD
>  	  rmmod).  This is mainly for kernel developers and desperate users.
>  	  If unsure, say N.
>  
> +config MODULE_UNLOAD_TAINT_TRACKING
> +	bool "Tainted module unload tracking"
> +	depends on MODULE_UNLOAD
> +	default n
> +	help
> +	  This option allows you to maintain a record of each unloaded
> +	  module that tainted the kernel. In addition to displaying a
> +	  list of linked (or loaded) modules e.g. on detection of a bad
> +	  page (see bad_page()), the aforementioned details are also
> +	  shown. If unsure, say N.
> +
>  config MODVERSIONS
>  	bool "Module versioning support"
>  	help
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index ea78cec316dd..d7cc64172dd1 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -68,6 +68,16 @@
>   */
>  DEFINE_MUTEX(module_mutex);
>  LIST_HEAD(modules);
> +#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING
> +static LIST_HEAD(unloaded_tainted_modules);
> +
> +struct mod_unload_taint {
> +	struct list_head list;
> +	char name[MODULE_NAME_LEN];
> +	unsigned long taints;
> +	u64 count;
> +};
> +#endif
>  
>  /* Work queue for freeing init sections in success case */
>  static void do_free_init(struct work_struct *w);
> @@ -150,6 +160,41 @@ int unregister_module_notifier(struct notifier_block *nb)
>  }
>  EXPORT_SYMBOL(unregister_module_notifier);
>  
> +#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING
> +static int try_add_tainted_module(struct module *mod)
> +{
> +	struct mod_unload_taint *mod_taint;
> +
> +	module_assert_mutex_or_preempt();
> +
> +	list_for_each_entry_rcu(mod_taint, &unloaded_tainted_modules, list,
> +				lockdep_is_held(&module_mutex)) {
> +		size_t len = strlen(mod_taint->name);
> +
> +		if (len == strlen(mod->name) && !memcmp(mod_taint->name, mod->name, len) &&

Here, two strings are compared, so I'd expect to see `strncmp()` instead of `memcmp()`.

> +		    mod_taint->taints & mod->taints) {
> +			mod_taint->count++;
> +			goto out;
> +		}
> +	}
> +
> +	mod_taint = kmalloc(sizeof(*mod_taint), GFP_KERNEL);
> +	if (unlikely(!mod_taint))
> +		return -ENOMEM;
> +	strscpy(mod_taint->name, mod->name, MODULE_NAME_LEN);
> +	mod_taint->taints = mod->taints;
> +	list_add_rcu(&mod_taint->list, &unloaded_tainted_modules);
> +	mod_taint->count = 1;
> +out:
> +	return 0;
> +}
> +#else /* MODULE_UNLOAD_TAINT_TRACKING */
> +static int try_add_tainted_module(struct module *mod)
> +{
> +	return 0;
> +}
> +#endif
> +
>  /*
>   * We require a truly strong try_module_get(): 0 means success.
>   * Otherwise an error is returned due to ongoing or failed
> @@ -1201,6 +1246,9 @@ static void free_module(struct module *mod)
>  	module_bug_cleanup(mod);
>  	/* Wait for RCU-sched synchronizing before releasing mod->list and buglist. */
>  	synchronize_rcu();
> +	if (try_add_tainted_module(mod))
> +		pr_err("%s: adding tainted module to the unloaded tainted modules list failed.\n",
> +		       mod->name);
>  	mutex_unlock(&module_mutex);
>  
>  	/* Clean up CFI for the module. */
> @@ -3126,6 +3174,9 @@ struct module *__module_text_address(unsigned long addr)
>  void print_modules(void)
>  {
>  	struct module *mod;
> +#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING
> +	struct mod_unload_taint *mod_taint;
> +#endif
>  	char buf[MODULE_FLAGS_BUF_SIZE];
>  
>  	printk(KERN_DEFAULT "Modules linked in:");
> @@ -3136,6 +3187,20 @@ void print_modules(void)
>  			continue;
>  		pr_cont(" %s%s", mod->name, module_flags(mod, buf));
>  	}
> +#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING
> +	if (!list_empty(&unloaded_tainted_modules)) {
> +		printk(KERN_DEFAULT "Unloaded tainted modules:");
> +		list_for_each_entry_rcu(mod_taint, &unloaded_tainted_modules,
> +					list) {
> +			size_t l;
> +
> +			l = module_flags_taint(mod_taint->taints, buf);
> +			buf[l++] = '\0';
> +			pr_cont(" %s(%s):%llu", mod_taint->name, buf,
> +				mod_taint->count);
> +		}
> +#endif
> +	}
>  	preempt_enable();
>  	if (last_unloaded_module[0])
>  		pr_cont(" [last unloaded: %s]", last_unloaded_module);
> 


-- 
Oleksandr Natalenko (post-factum)

Re: [PATCH v3 2/2] module: Introduce module unload taint tracking

[Aaron Tomlin]

On Thu 2022-04-21 16:28 +0200, Oleksandr Natalenko wrote:
> Hello.

Hi Oleksandr,

> Thanks for this submission. Please see one comment inline.

Thanks for your feedback!

> > @@ -150,6 +160,41 @@ int unregister_module_notifier(struct notifier_block *nb)
> >  }
> >  EXPORT_SYMBOL(unregister_module_notifier);
> >  
> > +#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING
> > +static int try_add_tainted_module(struct module *mod)
> > +{
> > +	struct mod_unload_taint *mod_taint;
> > +
> > +	module_assert_mutex_or_preempt();
> > +
> > +	list_for_each_entry_rcu(mod_taint, &unloaded_tainted_modules, list,
> > +				lockdep_is_held(&module_mutex)) {
> > +		size_t len = strlen(mod_taint->name);
> > +
> > +		if (len == strlen(mod->name) && !memcmp(mod_taint->name, mod->name, len) &&
> 
> Here, two strings are compared, so I'd expect to see `strncmp()` instead of `memcmp()`.

Good point. There are other examples of this throughout
kernel/module/main.c; albeit, I will use strncmp() here.


Kind regards,

-- 
Aaron Tomlin

Re: [PATCH v3 2/2] module: Introduce module unload taint tracking

[Christoph Lameter]

On Thu, 21 Apr 2022, Aaron Tomlin wrote:

> > > +		if (len == strlen(mod->name) && !memcmp(mod_taint->name, mod->name, len) &&
> >
> > Here, two strings are compared, so I'd expect to see `strncmp()` instead of `memcmp()`.
>
> Good point. There are other examples of this throughout
> kernel/module/main.c; albeit, I will use strncmp() here.

Comparing the length first may be an attempt to avoid the expensive
memcmp. But here we need to first execute strlen() to obtain the string
length. This is already accessing all characters so this
check is wasteful and a straight str[n]cmp is better.

Re: [PATCH v3 2/2] module: Introduce module unload taint tracking

[Aaron Tomlin]

On Fri 2022-04-22 10:11 +0200, Christoph Lameter wrote:
> On Thu, 21 Apr 2022, Aaron Tomlin wrote:
> 
> > > > +		if (len == strlen(mod->name) && !memcmp(mod_taint->name, mod->name, len) &&
> > >
> > > Here, two strings are compared, so I'd expect to see `strncmp()` instead of `memcmp()`.
> >
> > Good point. There are other examples of this throughout
> > kernel/module/main.c; albeit, I will use strncmp() here.
> 
> Comparing the length first may be an attempt to avoid the expensive
> memcmp. But here we need to first execute strlen() to obtain the string
> length. This is already accessing all characters so this
> check is wasteful and a straight str[n]cmp is better.

Hi Christoph,

Agreed - we can skip the extra strlen().

Thanks,

-- 
Aaron Tomlin