Re: [PATCH 1/1] radix-tree: do not export radix_tree_preloads as GPL

[Seth Forshee <seth.forshee@canonical.com>]

On Fri, Jul 17, 2020 at 10:55:19AM -0500, Seth Forshee wrote:
> On Fri, Jul 17, 2020 at 09:34:39AM -0500, Seth Forshee wrote:
> > On Fri, Jul 17, 2020 at 09:28:48AM -0500, Seth Forshee wrote:
> > > On Fri, Jul 17, 2020 at 03:45:10PM +0200, Alberto Milone wrote:
> > > > On 17/07/2020 15:21, Sebastian Andrzej Siewior wrote:
> > > > > On 2020-07-17 14:33:31 [+0200], Alberto Milone wrote:
> > > > >> I checked and CONFIG_DEBUG_LOCK_ALLOC is not enabled in our kernels.
> > > > > The access to that variable is optimized away if not for debug. I made
> > > > > this:
> > > > > | #include <linux/module.h>
> > > > > | #include <linux/idr.h>
> > > > > | 
> > > > > | static int le_init(void)
> > > > > | {
> > > > > |         idr_preload_end();
> > > > > |         return 0;
> > > > > | }
> > > > > | module_init(le_init);
> > > > > | 
> > > > > | static void le_exit(void)
> > > > > | {
> > > > > | }
> > > > > | module_exit(le_exit);
> > > > > |    
> > > > > | MODULE_DESCRIPTION("driver");
> > > > > | MODULE_LICENSE("prop");
> > > > >
> > > > > and it produced a .ko. Here the "idr_preload_end()" was reduced to
> > > > > "preempt_enable()" as intended. No access to
> > > > > "&radix_tree_preloads.lock".
> > > > >
> > > > > Sebastian
> > > > * Subscribing Seth
> > > 
> > > Looks like the driver is not using idr_preload_end() though, it is
> > > calling radix_tree_preload_end() which uses radix_tree_preloads whether
> > > or not CONFIG_DEBUG_LOCK_ALLOC is enabled.
> > 
> > Sorry, I didn't dig deep enough. I see that radix_tree_preload_end() is
> > expected to opimize away that access too. I wonder if different
> > toolchains could be ending up with different reults.
> 
> Your example gives me the same error about using radix_tree_preloads. I
> also added:
> 
>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
>  #warn "CONFIG_DEBUG_LOCK_ALLOC enabled"
>  #endif
> 
> Nothing is printed, so the option really does appear to be off. I've
> been staring at it a while and I can't see why the module still ends up
> referencing radix_tree_preloads, but it is clearly happening.

Even ignoring what is happening with our kernels, isn't this a
regression with CONFIG_DEBUG_LOCK_ALLOC=y that should be fixed? There
have been similar cases in the past where gpl-only exports leaked out
into interfaces which were previously usable by non-gpl modules, and
they were fixed -- 31c5bda3a656089f01963d290a40ccda181f816e and
9e987b70ada27554c5d176421de1d167218c49b5 are a couple of examples I was
able to find with minimal effort. Why is this case any different?

Thanks,
Seth