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

[Michal Hocko]

On Wed 14-11-18 15:00:53, Andrew Morton wrote:
> On Wed, 14 Nov 2018 16:17:37 +0100 Michal Hocko <mhocko@xxxxxxxxxx> 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