| Message ID | 20200717101848.1869465-1-alberto.milone@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] radix-tree: do not export radix_tree_preloads as GPL | expand |
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
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
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
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
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
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
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
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) {
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(-)