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

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

quoted

Indeed - is this acceptable to you? I prefer this approach rather than
remove it from the aforementioned list solely based on the module name.

Sure, it makes sense to keep all the stupid ways we are harming the
kernel. Makes sense. The other point I made about count though would
be good, in case the taint was the same.

Agreed. So, just to confirm you'd prefer not remove any module that tainted
the kernel from the aforementioned list when the same module and taints
bitmask is reintroduced? If I understand correctly, we'd simply maintain a
list of modules that tainted the kernel during module deletion/or unload
and their respective unload count? If so then this was not my original
objective yet I'm happy with this approach too - I'll take on this
implementation in the next iteration.

quoted

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.

Fair enough. If necessary we could introduce the above later.

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?

Yes, this was my original approach. Once the same module [with the same
taints bitmask] is reintroduced it will be listed on the 'modules' list
thus no need to track it on the unloaded list anymore. That being said, as
per the above, let's now keep track of each removal and maintain an unload
count.

quoted

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.

Agreed.

quoted

All right. I will make the required changes. Thanks once again.

Sure, so hey just one more thing. Can you add a simple selftest
lib/test_taint.c which can be used to test tainting and you new
tracker ? You can add a new selftest on

tools/testing/selftests/module/

I had already written some module based testing on
tools/testing/selftests/kmod/kmod.sh so you can borrow stuff
from there if you find it useful. But I think we need to start
doing basic testing for module. I know Lucas has tons of test
on kmod, so we should also look at what is there and what needs
testing outside of that.

Then there is the question of what should be tested using kunit and
or selftests. From my experience, if you need a shell, use selftests.
Also, if you need parallelization, use selftests, given kunit by
default uses a uniprocessor architecture, user-mode-linux. I'll let
you figure out what is the best place to add the test for this. It
could just be its a better place to add these tests to kmod upstream
as there are tons of tests there already. But kunit test can't be
added there.

Live patching already has its own set of selftests.

Sure - will do. Thanks once again!

-- 
Aaron Tomlin