linux-kernel - Re: [RFC PATCH 1/1] vmalloc: add test driver to analyse vmalloc allocator

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20181115083957.GE23831@dhcp22.suse.cz>
Date:   Thu, 15 Nov 2018 09:39:57 +0100
From:   Michal Hocko <mhocko@...nel.org>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     "Uladzislau Rezki (Sony)" <urezki@...il.com>,
        Kees Cook <keescook@...omium.org>,
        Shuah Khan <shuah@...nel.org>, linux-mm@...ck.org,
        LKML <linux-kernel@...r.kernel.org>,
        Matthew Wilcox <willy@...radead.org>,
        Oleksiy Avramchenko <oleksiy.avramchenko@...ymobile.com>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [RFC PATCH 1/1] vmalloc: add test driver to analyse vmalloc
 allocator

On Wed 14-11-18 15:00:53, Andrew Morton wrote:
> On Wed, 14 Nov 2018 16:17:37 +0100 Michal Hocko <mhocko@...nel.org> wrote:
> 
> > On Tue 13-11-18 14:10:46, Andrew Morton wrote:
> > [...]
> > > > +static int vmalloc_test_init(void)
> > > > +{
> > > > +	__my_vmalloc_node_range =
> > > > +		(void *) kallsyms_lookup_name("__vmalloc_node_range");
> > > > +
> > > > +	if (__my_vmalloc_node_range)
> > > > +		do_concurrent_test();
> > > > +
> > > > +	return -EAGAIN; /* Fail will directly unload the module */
> > > > +}
> > > 
> > > It's unclear why this module needs access to the internal
> > > __vmalloc_node_range().  Please fully explain this in the changelog.
> > > 
> > > Then, let's just export the thing.  (I expect this module needs a
> > > Kconfig dependency on CONFIG_KALLSYMS, btw).  A suitable way of doing
> > > that would be
> > > 
> > > /* Exported for lib/test_vmalloc.c.  Please do not use elsewhere */
> > > EXPORT_SYMBOL_GPL(__vmalloc_node_range);
> > 
> > There was a previous discussion that testing for internal infrastructure
> > is useful quite often and such a testing module needs an access to such
> > an internal infrastructure. Exporting those symbols via standard
> > EXPORT_SYMBOL_GPL is far from optimal because we can be pretty much sure
> > an abuse will arise sooner than later. I was proposing
> > EXPORT_SYMBOL_SELFTEST that would link only against testing modules.
> 
> That's rather overdoing things, I think.  If someone uses a
> dont-use-this symbol then they get to own both pieces when it breaks.

I do not think this has ever worked out. People are abusing internal
stuff and then we have to live with that. That is my experience at
least.

> We could simply do
> 
> #define EXPORT_SYMBOL_SELFTEST EXPORT_SYMBOL_GPL
>
> then write a script which checks the tree for usages of the
> thus-tagged symbols outside tools/testing and lib/ (?)

and then yell at people? We can try it out of course. The namespace
would be quite clear and we could document the supported usage pattern.
We also want to make EXPORT_SYMBOL_SELFTEST conditional. EXPORTs are not
free and we do not want to add them if the whole testing infrastructure
is disabled (assuming there is a global one for that).

> > If that is not viable for some reason then kallsyms_lookup_name is a
> > dirty-but-usable workaround.
> 
> Well yes.  It adds a dependency on CONFIG_KALLSYMS and will cause
> silent breakage if __vmalloc_node_range gets renamed, has its arguments
> changed, etc.

Yeah, I've said dirty ;)

-- 
Michal Hocko
SUSE Labs