Re: [RFC PATCH] module: Introduce module unload taint tracking

On Thu 2021-12-09 15:42:08, Luis Chamberlain wrote:

On Thu, Dec 09, 2021 at 04:49:17PM +0000, Aaron Tomlin wrote:

quoted

On Wed 2021-12-08 12:47 -0800, Luis Chamberlain wrote:

quoted

Loading and unloading modules... to keep track of *which ones are
tainted*. I'd find it extremely hard to believe this is such a common
thing and hot path that we need this.

In any case, since a linked list is used, I'm curious why did you
decide to bound this to an arbitrary limit of say 20? If this
feature is enabled why not make this boundless?

It can be, once set to 0. Indeed, the limit specified above is arbitrary.
Personally, I prefer to have some limit that can be controlled by the user.
In fact, if agreed, I can incorporate the limit [when specified] into the
output generated via print_modules().

If someone enables this feature I can't think of a reason why they
would want to limit this to some arbitrary number. So my preference
is to remove that limitation completely. I see no point to it.

I agree with Luis here. We could always add the limit later when
people report some real life problems with too long list. It is
always good to know that someone did some heavy lifting in
the system.

It might be even interesting to remember timestamp of the removal
to match it with another events reported in the system log.

quoted

quoted

quoted

@@ -3703,6 +3778,16 @@ static noinline int do_init_module(struct module *mod)
 	mod->state = MODULE_STATE_LIVE;
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_LIVE, mod);
+#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING
+	mutex_lock(&module_mutex);
+	old = find_mod_unload_taint(mod->name, strlen(mod->name),
+				mod->taints);
+	if (old) {
+		list_del_rcu(&old->list);
+		synchronize_rcu();
+	}
+	mutex_unlock(&module_mutex);

But here we seem to delete an old instance of the module taint
history if it is loaded again and has the same taint properties.
Why?

At first glance, in this particular case, I believe this makes sense to
avoid duplication

If you just bump the count then its not duplication, it just adds
more information that the same module name with the same taint flag
has been unloaded now more than once.

Please, do not remove records that a module was removed. IMHO, it
might be useful to track all removed module, including the non-tainted
ones. Module removal is always tricky and not much tested. The tain
flags might be just shown as extra information in the output.

Best Regards,
Petr