Date:   Mon, 13 Dec 2021 16:16:36 +0100
From:   Petr Mladek <pmladek@suse.com>
To:     Aaron Tomlin <atomlin@redhat.com>
Cc:     Luis Chamberlain <mcgrof@kernel.org>,
        Christoph Lameter <cl@linux.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: <Ybdj1EDnMSl2NLab@alley>
References: <20211124173327.3878177-1-atomlin@redhat.com>
 <YbEZ4HgSYQEPuRmS@bombadil.infradead.org>
 <20211209153131.a54fdfbci4qnyy6h@ava.usersys.com>
 <YbKUUJUtjBk/n913@bombadil.infradead.org>
 <YbMlVFwBiRujKdEX@alley>
 <20211210160931.ftvxpulno73a2l7c@ava.usersys.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211210160931.ftvxpulno73a2l7c@ava.usersys.com>
Precedence: bulk

On Fri 2021-12-10 16:09:31, Aaron Tomlin wrote:
> On Fri 2021-12-10 11:00 +0100, Petr Mladek wrote:
> > > 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.
> 
> Fair enough.
> 
> > It might be even interesting to remember timestamp of the removal
> > to match it with another events reported in the system log.
> 
> I'm not so sure about this. We could gather such details already via Ftrace
> (e.g. see load_module()). Personally, I'd prefer to maintain a simple list.

Fair enough. It was just an idea. Simple list is a good start. We
could always add more details if people find it useful.


> > > 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.
> 
> This is an interesting suggestion. Albeit, as per the subject, I prefer to
> just keep track of any module that tainted the kernel. That being said,
> Petr, if you'd prefer to track each module unload/or deletion event, then I
> would suggest for instance to remove a module once it has been reintroduced
> or maintain an unload count as suggested by Luis.

I just have fresh in mind the patchset
https://lore.kernel.org/r/20211129034509.2646872-1-ming.lei@redhat.com
It is about that removing sysfs interface is tricky and might lead to
use after free problems. I could imagine many other similar problems
that might happen with any module.

But I agree that the information about modules that tainted the kernel is
more important. I do not want to block the feature by requiring more
requirements.


Also we should keep in mind that the default panic() message should
be reasonably short. Only the last lines might be visible on screen.
Serial consoles might be really slow.

It is perfectly fine to add few lines, like the existing list of
loaded modules. Any potentially extensive output should be optional.
There already is support for optional info, see panic_print_sys_info().

Best Regards,
Petr