Date:   Fri, 10 Dec 2021 15:48:04 +0000
From:   Aaron Tomlin <atomlin@redhat.com>
To:     Luis Chamberlain <mcgrof@kernel.org>
Cc:     Christoph Lameter <cl@linux.com>, Petr Mladek <pmladek@suse.com>,
        Miroslav Benes <mbenes@suse.cz>,
        Andrew Morton <akpm@linux-foundation.org>, jeyu@kernel.org,
        linux-kernel@vger.kernel.org, linux-modules@vger.kernel.org,
        atomlin@atomlin.com, ghalat@redhat.com
Subject: Re: [RFC PATCH] module: Introduce module unload taint tracking
Message-ID: <20211210154804.zq4dyycwd4jlb3bg@ava.usersys.com>
References: <20211124173327.3878177-1-atomlin@redhat.com>
 <YbEZ4HgSYQEPuRmS@bombadil.infradead.org>
 <20211209153131.a54fdfbci4qnyy6h@ava.usersys.com>
 <YbKUUJUtjBk/n913@bombadil.infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <YbKUUJUtjBk/n913@bombadil.infradead.org>
Precedence: bulk

On Thu 2021-12-09 15:42 -0800, Luis Chamberlain wrote:
> > 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.

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

> > > > @@ -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.

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

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