linux-next: build failure after merge of the modules tree

linux-next: build failure after merge of the modules tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 2107 bytes --]

Hi Rusty,

After merging the modules tree, today's linux-next build (powerpc
allnoconfig) failed like this:

kernel/params.c:628:6: error: redefinition of 'kernel_param_lock'
 void kernel_param_lock(struct module *mod)
      ^
In file included from include/linux/module.h:18:0,
                 from kernel/params.c:21:
include/linux/moduleparam.h:246:20: note: previous definition of 'kernel_param_lock' was here
 static inline void kernel_param_lock(struct module *mod)
                    ^
kernel/params.c:633:6: error: redefinition of 'kernel_param_unlock'
 void kernel_param_unlock(struct module *mod)
      ^
In file included from include/linux/module.h:18:0,
                 from kernel/params.c:21:
include/linux/moduleparam.h:249:20: note: previous definition of 'kernel_param_unlock' was here
 static inline void kernel_param_unlock(struct module *mod)
                    ^

Caused by commit b51d23e4e9fe ("module: add per-module param_lock").
This had not been tested with CONFIG_SYSFS not set.

I have added the following patch for today (which may not be complete,
but builds).

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Thu, 25 Jun 2015 16:38:51 +1000
Subject: [PATCH] modules: clean up add per-module param_lock

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 include/linux/moduleparam.h | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index c12f2147c350..428d7c25c383 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -239,17 +239,8 @@ __check_old_set_param(int (*oldset)(const char *, struct kernel_param *))
 	return 0;
 }
 
-#ifdef CONFIG_SYSFS
 extern void kernel_param_lock(struct module *mod);
 extern void kernel_param_unlock(struct module *mod);
-#else
-static inline void kernel_param_lock(struct module *mod)
-{
-}
-static inline void kernel_param_unlock(struct module *mod)
-{
-}
-#endif
 
 #ifndef MODULE
 /**
-- 
2.1.4

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: linux-next: build failure after merge of the modules tree

[Dan Streetman]

On Thu, Jun 25, 2015 at 2:54 AM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> Hi Rusty,
>
> After merging the modules tree, today's linux-next build (powerpc
> allnoconfig) failed like this:

I promise I'll build with all[no|mod|yes]config next time! ;-)

Thanks for catching this one too.

>
> kernel/params.c:628:6: error: redefinition of 'kernel_param_lock'
>  void kernel_param_lock(struct module *mod)
>       ^
> In file included from include/linux/module.h:18:0,
>                  from kernel/params.c:21:
> include/linux/moduleparam.h:246:20: note: previous definition of 'kernel_param_lock' was here
>  static inline void kernel_param_lock(struct module *mod)
>                     ^
> kernel/params.c:633:6: error: redefinition of 'kernel_param_unlock'
>  void kernel_param_unlock(struct module *mod)
>       ^
> In file included from include/linux/module.h:18:0,
>                  from kernel/params.c:21:
> include/linux/moduleparam.h:249:20: note: previous definition of 'kernel_param_unlock' was here
>  static inline void kernel_param_unlock(struct module *mod)
>                     ^
>
> Caused by commit b51d23e4e9fe ("module: add per-module param_lock").
> This had not been tested with CONFIG_SYSFS not set.
>
> I have added the following patch for today (which may not be complete,
> but builds).

this is one way to handle it, but it will cause all modules to use the
built-in param_lock,
which (I don't think) is needed if there's no sysfs; another way would
be to elide the
param_lock entirely if !CONFIG_SYSFS.  I'll send a patch for that.

Rusty, is param locking needed if there is no sysfs?

>
> From: Stephen Rothwell <sfr@canb.auug.org.au>
> Date: Thu, 25 Jun 2015 16:38:51 +1000
> Subject: [PATCH] modules: clean up add per-module param_lock
>
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> ---
>  include/linux/moduleparam.h | 9 ---------
>  1 file changed, 9 deletions(-)
>
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index c12f2147c350..428d7c25c383 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -239,17 +239,8 @@ __check_old_set_param(int (*oldset)(const char *, struct kernel_param *))
>         return 0;
>  }
>
> -#ifdef CONFIG_SYSFS
>  extern void kernel_param_lock(struct module *mod);
>  extern void kernel_param_unlock(struct module *mod);
> -#else
> -static inline void kernel_param_lock(struct module *mod)
> -{
> -}
> -static inline void kernel_param_unlock(struct module *mod)
> -{
> -}
> -#endif
>
>  #ifndef MODULE
>  /**
> --
> 2.1.4
>
> --
> Cheers,
> Stephen Rothwell                    sfr@canb.auug.org.au

[PATCH] modules: elide param_lock if !CONFIG_SYSFS

[Dan Streetman]

Only include the built-in and per-module param_lock, and corresponding
lock/unlock functions, if sysfs is enabled.  If there is no sysfs there
is no need for locking kernel params.

This fixes a build break when CONFIG_SYSFS is not enabled, introduced
by commit b51d23e.

Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Dan Streetman <ddstreet@ieee.org>
---
 include/linux/module.h |  2 ++
 kernel/params.c        | 21 ++++++++++++++++-----
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 6ba0e87..46efa1c 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -240,7 +240,9 @@ struct module {
 	unsigned int num_syms;
 
 	/* Kernel parameters. */
+#ifdef CONFIG_SYSFS
 	struct mutex param_lock;
+#endif
 	struct kernel_param *kp;
 	unsigned int num_kp;
 
diff --git a/kernel/params.c b/kernel/params.c
index 9c955cd..d2c6a2a 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -25,6 +25,7 @@
 #include <linux/slab.h>
 #include <linux/ctype.h>
 
+#ifdef CONFIG_SYSFS
 /* Protects all built-in parameters, modules use their own param_lock */
 static DEFINE_MUTEX(param_lock);
 
@@ -38,6 +39,17 @@ static inline struct mutex *__param_lock(struct module *mod)
 #endif
 }
 
+static inline void __check_param_lock(struct module *mod)
+{
+	BUG_ON(!mutex_is_locked(__param_lock(mod)));
+}
+#else /* !CONFIG_SYSFS */
+/* we don't need any param lock if there's no sysfs */
+static inline void __check_param_lock(struct module *mod)
+{
+}
+#endif
+
 /* This just allows us to keep track of which parameters are kmalloced. */
 struct kmalloced_param {
 	struct list_head list;
@@ -465,7 +477,7 @@ static int param_array(struct module *mod,
 		/* nul-terminate and parse */
 		save = val[len];
 		((char *)val)[len] = '\0';
-		BUG_ON(!mutex_is_locked(__param_lock(mod)));
+		__check_param_lock(mod);
 		ret = set(val, &kp);
 
 		if (ret != 0)
@@ -502,7 +514,7 @@ static int param_array_get(char *buffer, const struct kernel_param *kp)
 		if (i)
 			buffer[off++] = ',';
 		p.arg = arr->elem + arr->elemsize * i;
-		BUG_ON(!mutex_is_locked(__param_lock(p.mod)));
+		__check_param_lock(p.mod);
 		ret = arr->ops->get(buffer + off, &p);
 		if (ret < 0)
 			return ret;
@@ -622,18 +634,17 @@ static ssize_t param_attr_store(struct module_attribute *mattr,
 #define __modinit __init
 #endif
 
+#ifdef CONFIG_SYSFS
 void kernel_param_lock(struct module *mod)
 {
 	mutex_lock(__param_lock(mod));
 }
+EXPORT_SYMBOL(kernel_param_lock);
 
 void kernel_param_unlock(struct module *mod)
 {
 	mutex_unlock(__param_lock(mod));
 }
-
-#ifdef CONFIG_SYSFS
-EXPORT_SYMBOL(kernel_param_lock);
 EXPORT_SYMBOL(kernel_param_unlock);
 
 /*
-- 
2.1.0

Re: [PATCH] modules: elide param_lock if !CONFIG_SYSFS

[Rusty Russell]

Dan Streetman <ddstreet@ieee.org> writes:
> Only include the built-in and per-module param_lock, and corresponding
> lock/unlock functions, if sysfs is enabled.  If there is no sysfs there
> is no need for locking kernel params.
>
> This fixes a build break when CONFIG_SYSFS is not enabled, introduced
> by commit b51d23e.

This doesn't even come close to applying to my tree?

I've fixed it like so, and tested it compiles both with and without
SYSFS.

Subject: param: fix module param locks when !CONFIG_SYSFS.

As Dan Streetman points out, the entire point of locking for is to
stop sysfs accesses, so they're elided entirely in the !SYSFS case.

Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>

diff --git a/include/linux/module.h b/include/linux/module.h
index abaecf1a63df..237bce382e32 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -240,7 +240,9 @@ struct module {
 	unsigned int num_syms;
 
 	/* Kernel parameters. */
+#ifdef CONFIG_SYSFS
 	struct mutex param_lock;
+#endif
 	struct kernel_param *kp;
 	unsigned int num_kp;
 
diff --git a/kernel/module.c b/kernel/module.c
index 1dcf09f214fa..c19b2c50b317 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1820,6 +1820,10 @@ static void mod_sysfs_fini(struct module *mod)
 	mod_kobject_put(mod);
 }
 
+static void init_param_lock(struct module *mod)
+{
+	mutex_init(&mod->param_lock);
+}
 #else /* !CONFIG_SYSFS */
 
 static int mod_sysfs_setup(struct module *mod,
@@ -1842,6 +1846,9 @@ static void del_usage_links(struct module *mod)
 {
 }
 
+static void init_param_lock(struct module *mod)
+{
+}
 #endif /* CONFIG_SYSFS */
 
 static void mod_sysfs_teardown(struct module *mod)
@@ -3442,7 +3449,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	if (err)
 		goto unlink_mod;
 
-	mutex_init(&mod->param_lock);
+	init_param_lock(mod);
 
 	/* Now we've got everything in the final locations, we can
 	 * find optional sections. */
diff --git a/kernel/params.c b/kernel/params.c
index 8890d0b8dffc..faa461c16f12 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -25,12 +25,22 @@
 #include <linux/slab.h>
 #include <linux/ctype.h>
 
+#ifdef CONFIG_SYSFS
 /* Protects all built-in parameters, modules use their own param_lock */
 static DEFINE_MUTEX(param_lock);
 
 /* Use the module's mutex, or if built-in use the built-in mutex */
 #define KPARAM_MUTEX(mod)	((mod) ? &(mod)->param_lock : &param_lock)
-#define KPARAM_IS_LOCKED(mod)	mutex_is_locked(KPARAM_MUTEX(mod))
+
+static inline void check_kparam_locked(struct module *mod)
+{
+	BUG_ON(!mutex_is_locked(KPARAM_MUTEX(mod)));
+}
+#else
+static inline void check_kparam_locked(struct module *mod)
+{
+}
+#endif /* !CONFIG_SYSFS */
 
 /* This just allows us to keep track of which parameters are kmalloced. */
 struct kmalloced_param {
@@ -459,7 +469,7 @@ static int param_array(struct module *mod,
 		/* nul-terminate and parse */
 		save = val[len];
 		((char *)val)[len] = '\0';
-		BUG_ON(!KPARAM_IS_LOCKED(mod));
+		check_kparam_locked(mod);
 		ret = set(val, &kp);
 
 		if (ret != 0)
@@ -496,7 +506,7 @@ static int param_array_get(char *buffer, const struct kernel_param *kp)
 		if (i)
 			buffer[off++] = ',';
 		p.arg = arr->elem + arr->elemsize * i;
-		BUG_ON(!KPARAM_IS_LOCKED(p.mod));
+		check_kparam_locked(p.mod);
 		ret = arr->ops->get(buffer + off, &p);
 		if (ret < 0)
 			return ret;
@@ -616,6 +626,7 @@ static ssize_t param_attr_store(struct module_attribute *mattr,
 #define __modinit __init
 #endif
 
+#ifdef CONFIG_SYSFS
 void kernel_param_lock(struct module *mod)
 {
 	mutex_lock(KPARAM_MUTEX(mod));
@@ -626,7 +637,6 @@ void kernel_param_unlock(struct module *mod)
 	mutex_unlock(KPARAM_MUTEX(mod));
 }
 
-#ifdef CONFIG_SYSFS
 EXPORT_SYMBOL(kernel_param_lock);
 EXPORT_SYMBOL(kernel_param_unlock);
 

Re: [PATCH] modules: elide param_lock if !CONFIG_SYSFS

[Dan Streetman]

On Thu, Jun 25, 2015 at 5:18 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Dan Streetman <ddstreet@ieee.org> writes:
>> Only include the built-in and per-module param_lock, and corresponding
>> lock/unlock functions, if sysfs is enabled.  If there is no sysfs there
>> is no need for locking kernel params.
>>
>> This fixes a build break when CONFIG_SYSFS is not enabled, introduced
>> by commit b51d23e.
>
> This doesn't even come close to applying to my tree?

sorry, I had the !CONFIG_MODULES patch in my tree also, so this was on
top of that one:
https://lkml.org/lkml/2015/6/24/550

I can resend that one, on top of this one, or you can fix it up.

Sorry for not getting it right the first time ;-)

>
> I've fixed it like so, and tested it compiles both with and without
> SYSFS.
>
> Subject: param: fix module param locks when !CONFIG_SYSFS.
>
> As Dan Streetman points out, the entire point of locking for is to
> stop sysfs accesses, so they're elided entirely in the !SYSFS case.
>
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index abaecf1a63df..237bce382e32 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -240,7 +240,9 @@ struct module {
>         unsigned int num_syms;
>
>         /* Kernel parameters. */
> +#ifdef CONFIG_SYSFS
>         struct mutex param_lock;
> +#endif
>         struct kernel_param *kp;
>         unsigned int num_kp;
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 1dcf09f214fa..c19b2c50b317 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1820,6 +1820,10 @@ static void mod_sysfs_fini(struct module *mod)
>         mod_kobject_put(mod);
>  }
>
> +static void init_param_lock(struct module *mod)
> +{
> +       mutex_init(&mod->param_lock);
> +}
>  #else /* !CONFIG_SYSFS */
>
>  static int mod_sysfs_setup(struct module *mod,
> @@ -1842,6 +1846,9 @@ static void del_usage_links(struct module *mod)
>  {
>  }
>
> +static void init_param_lock(struct module *mod)
> +{
> +}
>  #endif /* CONFIG_SYSFS */
>
>  static void mod_sysfs_teardown(struct module *mod)
> @@ -3442,7 +3449,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>         if (err)
>                 goto unlink_mod;
>
> -       mutex_init(&mod->param_lock);
> +       init_param_lock(mod);

oops, i missed fixing this up, when I tested with allnoconfig this
wasn't built due to CONFIG_MODULES=n.  I should have built only with
syfs=n :(

>
>         /* Now we've got everything in the final locations, we can
>          * find optional sections. */
> diff --git a/kernel/params.c b/kernel/params.c
> index 8890d0b8dffc..faa461c16f12 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -25,12 +25,22 @@
>  #include <linux/slab.h>
>  #include <linux/ctype.h>
>
> +#ifdef CONFIG_SYSFS
>  /* Protects all built-in parameters, modules use their own param_lock */
>  static DEFINE_MUTEX(param_lock);
>
>  /* Use the module's mutex, or if built-in use the built-in mutex */
>  #define KPARAM_MUTEX(mod)      ((mod) ? &(mod)->param_lock : &param_lock)
> -#define KPARAM_IS_LOCKED(mod)  mutex_is_locked(KPARAM_MUTEX(mod))
> +
> +static inline void check_kparam_locked(struct module *mod)
> +{
> +       BUG_ON(!mutex_is_locked(KPARAM_MUTEX(mod)));
> +}
> +#else
> +static inline void check_kparam_locked(struct module *mod)
> +{
> +}
> +#endif /* !CONFIG_SYSFS */
>
>  /* This just allows us to keep track of which parameters are kmalloced. */
>  struct kmalloced_param {
> @@ -459,7 +469,7 @@ static int param_array(struct module *mod,
>                 /* nul-terminate and parse */
>                 save = val[len];
>                 ((char *)val)[len] = '\0';
> -               BUG_ON(!KPARAM_IS_LOCKED(mod));
> +               check_kparam_locked(mod);
>                 ret = set(val, &kp);
>
>                 if (ret != 0)
> @@ -496,7 +506,7 @@ static int param_array_get(char *buffer, const struct kernel_param *kp)
>                 if (i)
>                         buffer[off++] = ',';
>                 p.arg = arr->elem + arr->elemsize * i;
> -               BUG_ON(!KPARAM_IS_LOCKED(p.mod));
> +               check_kparam_locked(p.mod);
>                 ret = arr->ops->get(buffer + off, &p);
>                 if (ret < 0)
>                         return ret;
> @@ -616,6 +626,7 @@ static ssize_t param_attr_store(struct module_attribute *mattr,
>  #define __modinit __init
>  #endif
>
> +#ifdef CONFIG_SYSFS
>  void kernel_param_lock(struct module *mod)
>  {
>         mutex_lock(KPARAM_MUTEX(mod));
> @@ -626,7 +637,6 @@ void kernel_param_unlock(struct module *mod)
>         mutex_unlock(KPARAM_MUTEX(mod));
>  }
>
> -#ifdef CONFIG_SYSFS
>  EXPORT_SYMBOL(kernel_param_lock);
>  EXPORT_SYMBOL(kernel_param_unlock);
>

Re: [PATCH] modules: elide param_lock if !CONFIG_SYSFS

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1776 bytes --]

Hi Rusty,

On Thu, 25 Jun 2015 17:34:31 -0400 Dan Streetman <ddstreet@ieee.org> wrote:
>
> On Thu, Jun 25, 2015 at 5:18 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> > Dan Streetman <ddstreet@ieee.org> writes:
> >> Only include the built-in and per-module param_lock, and corresponding
> >> lock/unlock functions, if sysfs is enabled.  If there is no sysfs there
> >> is no need for locking kernel params.
> >>
> >> This fixes a build break when CONFIG_SYSFS is not enabled, introduced
> >> by commit b51d23e.
> >
> > This doesn't even come close to applying to my tree?
> 
> sorry, I had the !CONFIG_MODULES patch in my tree also, so this was on
> top of that one:
> https://lkml.org/lkml/2015/6/24/550
> 
> I can resend that one, on top of this one, or you can fix it up.
> 
> Sorry for not getting it right the first time ;-)

This is what I ended up applying to yesterday's linux-next:

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Fri, 26 Jun 2015 13:19:19 +1000
Subject: [PATCH] modules: only use mod->param_lock if CONFIG_MODULES

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 kernel/params.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/params.c b/kernel/params.c
index faa461c16f12..adc0bbc06cc5 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -30,7 +30,11 @@
 static DEFINE_MUTEX(param_lock);
 
 /* Use the module's mutex, or if built-in use the built-in mutex */
+#ifdef CONFIG_MODULES
 #define KPARAM_MUTEX(mod)	((mod) ? &(mod)->param_lock : &param_lock)
+#else
+#define KPARAM_MUTEX(mod)	(&param_lock)
+#endif
 
 static inline void check_kparam_locked(struct module *mod)
 {
-- 
2.1.4


-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] modules: elide param_lock if !CONFIG_SYSFS

[Rusty Russell]

Stephen Rothwell <sfr@canb.auug.org.au> writes:
> Hi Rusty,
>
> On Thu, 25 Jun 2015 17:34:31 -0400 Dan Streetman <ddstreet@ieee.org> wrote:
>>
>> On Thu, Jun 25, 2015 at 5:18 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> > Dan Streetman <ddstreet@ieee.org> writes:
>> >> Only include the built-in and per-module param_lock, and corresponding
>> >> lock/unlock functions, if sysfs is enabled.  If there is no sysfs there
>> >> is no need for locking kernel params.
>> >>
>> >> This fixes a build break when CONFIG_SYSFS is not enabled, introduced
>> >> by commit b51d23e.
>> >
>> > This doesn't even come close to applying to my tree?
>> 
>> sorry, I had the !CONFIG_MODULES patch in my tree also, so this was on
>> top of that one:
>> https://lkml.org/lkml/2015/6/24/550
>> 
>> I can resend that one, on top of this one, or you can fix it up.
>> 
>> Sorry for not getting it right the first time ;-)
>
> This is what I ended up applying to yesterday's linux-next:

Thanks, added correct description to previous patch and applied this
on top.

Sorry for the hassle,
Rusty.

Re: [PATCH] modules: elide param_lock if !CONFIG_SYSFS

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1113 bytes --]

Hi Rusty,

On Fri, 26 Jun 2015 06:48:37 +0930 Rusty Russell <rusty@rustcorp.com.au> wrote:
>
> Dan Streetman <ddstreet@ieee.org> writes:
> > Only include the built-in and per-module param_lock, and corresponding
> > lock/unlock functions, if sysfs is enabled.  If there is no sysfs there
> > is no need for locking kernel params.
> >
> > This fixes a build break when CONFIG_SYSFS is not enabled, introduced
> > by commit b51d23e.
> 
> This doesn't even come close to applying to my tree?

That's because (as Dan noted) there was another fix patch in the tree
he was fixing ...

> I've fixed it like so, and tested it compiles both with and without
> SYSFS.
> 
> Subject: param: fix module param locks when !CONFIG_SYSFS.
> 
> As Dan Streetman points out, the entire point of locking for is to
> stop sysfs accesses, so they're elided entirely in the !SYSFS case.
> 
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>

Except the commit you added to the modules tree has no signed off by or
commentary :-(

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]