Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/1] radix-tree: do not export radix_tree_preloads as GPL
@ 2020-07-17 10:18 Alberto Milone
  2020-07-17 10:43 ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 9+ messages in thread
From: Alberto Milone @ 2020-07-17 10:18 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: mingo, bigeasy

Commit cfa6705d89b6 ("radix-tree: Use local_lock
for protection") replaced a DEFINE_PER_CPU() with an
EXPORT_PER_CPU_SYMBOL_GPL(), which made the
radix_tree_preloads symbol GPL only. All the other
symbols in the file are exported with EXPORT_SYMBOL().

The change breaks the NVIDIA 390 legacy driver.

This commit uses EXPORT_PER_CPU_SYMBOL() for
radix_tree_preloads.

Signed-off-by: Alberto Milone <alberto.milone@canonical.com>
---
 lib/radix-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/radix-tree.c b/lib/radix-tree.c
index 34e406fe561f..5f3ec9be6e37 100644
--- a/lib/radix-tree.c
+++ b/lib/radix-tree.c
@@ -61,7 +61,7 @@ struct kmem_cache *radix_tree_node_cachep;
 DEFINE_PER_CPU(struct radix_tree_preload, radix_tree_preloads) = {
 	.lock = INIT_LOCAL_LOCK(lock),
 };
-EXPORT_PER_CPU_SYMBOL_GPL(radix_tree_preloads);
+EXPORT_PER_CPU_SYMBOL(radix_tree_preloads);
 
 static inline struct radix_tree_node *entry_to_node(void *ptr)
 {
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] radix-tree: do not export radix_tree_preloads as GPL
  2020-07-17 10:18 [PATCH 1/1] radix-tree: do not export radix_tree_preloads as GPL Alberto Milone
@ 2020-07-17 10:43 ` Sebastian Andrzej Siewior
       [not found]   ` <ba5d59f6-2e40-d13a-ecc8-d8430a1b6a14@canonical.com>
  0 siblings, 1 reply; 9+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-07-17 10:43 UTC (permalink / raw)
  To: Alberto Milone; +Cc: linux-fsdevel, mingo

On 2020-07-17 12:18:48 [+0200], Alberto Milone wrote:
> Commit cfa6705d89b6 ("radix-tree: Use local_lock
> for protection") replaced a DEFINE_PER_CPU() with an
> EXPORT_PER_CPU_SYMBOL_GPL(), which made the
> radix_tree_preloads symbol GPL only. All the other
> symbols in the file are exported with EXPORT_SYMBOL().

Is this a problem if you disable CONFIG_DEBUG_LOCK_ALLOC ?

> The change breaks the NVIDIA 390 legacy driver.
> 
> This commit uses EXPORT_PER_CPU_SYMBOL() for
> radix_tree_preloads.
> 

Sebastian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Fwd: [PATCH 1/1] radix-tree: do not export radix_tree_preloads as GPL
       [not found]   ` <ba5d59f6-2e40-d13a-ecc8-d8430a1b6a14@canonical.com>
@ 2020-07-17 12:35     ` Alberto Milone
  2020-07-17 13:21     ` Sebastian Andrzej Siewior
  1 sibling, 0 replies; 9+ messages in thread
From: Alberto Milone @ 2020-07-17 12:35 UTC (permalink / raw)
  To: linux-fsdevel




-------- Forwarded Message --------
Subject: 	Re: [PATCH 1/1] radix-tree: do not export radix_tree_preloads
as GPL
Date: 	Fri, 17 Jul 2020 14:33:31 +0200
From: 	Alberto Milone <alberto.milone@canonical.com>
To: 	Sebastian Andrzej Siewior <bigeasy@linutronix.de>
CC: 	linux-fsdevel@vger.kernel.org, mingo@kernel.org




On 17/07/2020 12:43, Sebastian Andrzej Siewior wrote:
> On 2020-07-17 12:18:48 [+0200], Alberto Milone wrote:
>> Commit cfa6705d89b6 ("radix-tree: Use local_lock
>> for protection") replaced a DEFINE_PER_CPU() with an
>> EXPORT_PER_CPU_SYMBOL_GPL(), which made the
>> radix_tree_preloads symbol GPL only. All the other
>> symbols in the file are exported with EXPORT_SYMBOL().
> Is this a problem if you disable CONFIG_DEBUG_LOCK_ALLOC ?

I checked and CONFIG_DEBUG_LOCK_ALLOC is not enabled in our kernels.



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] radix-tree: do not export radix_tree_preloads as GPL
       [not found]   ` <ba5d59f6-2e40-d13a-ecc8-d8430a1b6a14@canonical.com>
  2020-07-17 12:35     ` Fwd: " Alberto Milone
@ 2020-07-17 13:21     ` Sebastian Andrzej Siewior
  2020-07-17 13:45       ` Alberto Milone
  1 sibling, 1 reply; 9+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-07-17 13:21 UTC (permalink / raw)
  To: Alberto Milone; +Cc: linux-fsdevel, mingo

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] radix-tree: do not export radix_tree_preloads as GPL
  2020-07-17 13:21     ` Sebastian Andrzej Siewior
@ 2020-07-17 13:45       ` Alberto Milone
  2020-07-17 14:28         ` Seth Forshee
  0 siblings, 1 reply; 9+ messages in thread
From: Alberto Milone @ 2020-07-17 13:45 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior; +Cc: linux-fsdevel, mingo, Seth Forshee

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] radix-tree: do not export radix_tree_preloads as GPL
  2020-07-17 13:45       ` Alberto Milone
@ 2020-07-17 14:28         ` Seth Forshee
  2020-07-17 14:31           ` Sebastian Andrzej Siewior
  2020-07-17 14:34           ` Seth Forshee
  0 siblings, 2 replies; 9+ messages in thread
From: Seth Forshee @ 2020-07-17 14:28 UTC (permalink / raw)
  To: Alberto Milone; +Cc: Sebastian Andrzej Siewior, linux-fsdevel, mingo

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.

Thanks,
Seth

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] radix-tree: do not export radix_tree_preloads as GPL
  2020-07-17 14:28         ` Seth Forshee
@ 2020-07-17 14:31           ` Sebastian Andrzej Siewior
  2020-07-17 14:34           ` Seth Forshee
  1 sibling, 0 replies; 9+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-07-17 14:31 UTC (permalink / raw)
  To: Seth Forshee; +Cc: Alberto Milone, linux-fsdevel, mingo

On 2020-07-17 09:28:48 [-0500], Seth Forshee wrote:
> 
> 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.

static inline void radix_tree_preload_end(void)
 {
         local_unlock(&radix_tree_preloads.lock);
 }

=> 
 #define local_unlock(lock)              __local_unlock(lock)

=>
 #define __local_unlock(lock)                                    \
         do {                                                    \
                 local_lock_release(this_cpu_ptr(lock));         \
                 preempt_enable();                               \
         } while (0)

=>
 static inline void local_lock_release(local_lock_t *l) { }

> Thanks,
> Seth

Sebastian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] radix-tree: do not export radix_tree_preloads as GPL
  2020-07-17 14:28         ` Seth Forshee
  2020-07-17 14:31           ` Sebastian Andrzej Siewior
@ 2020-07-17 14:34           ` Seth Forshee
       [not found]             ` <20200717155519.GM3644@ubuntu-x1>
  1 sibling, 1 reply; 9+ messages in thread
From: Seth Forshee @ 2020-07-17 14:34 UTC (permalink / raw)
  To: Alberto Milone; +Cc: Sebastian Andrzej Siewior, linux-fsdevel, mingo

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.

Seth

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] radix-tree: do not export radix_tree_preloads as GPL
       [not found]             ` <20200717155519.GM3644@ubuntu-x1>
@ 2020-07-20 14:49               ` Seth Forshee
  0 siblings, 0 replies; 9+ messages in thread
From: Seth Forshee @ 2020-07-20 14:49 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior; +Cc: Alberto Milone, linux-fsdevel, mingo

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-07-20 14:49 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-17 10:18 [PATCH 1/1] radix-tree: do not export radix_tree_preloads as GPL Alberto Milone
2020-07-17 10:43 ` Sebastian Andrzej Siewior
     [not found]   ` <ba5d59f6-2e40-d13a-ecc8-d8430a1b6a14@canonical.com>
2020-07-17 12:35     ` Fwd: " Alberto Milone
2020-07-17 13:21     ` Sebastian Andrzej Siewior
2020-07-17 13:45       ` Alberto Milone
2020-07-17 14:28         ` Seth Forshee
2020-07-17 14:31           ` Sebastian Andrzej Siewior
2020-07-17 14:34           ` Seth Forshee
     [not found]             ` <20200717155519.GM3644@ubuntu-x1>
2020-07-20 14:49               ` Seth Forshee

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).