[PATCH] block2mtd lockdep_init_map warning

[PATCH] block2mtd lockdep_init_map warning

[Erez Zadok]

Hi David,

I've reported before a lockdep warning when block2mtd is modloaded, and a
device is initialized, as in

	modprobe block2mtd block2mtd=/dev/loop0

A typical warning looks like this:

BUG: key f88565c0 not in .data!
WARNING: at kernel/lockdep.c:2331 lockdep_init_map()
Pid: 1823, comm: modprobe Not tainted 2.6.24-rc6-unionfs2 #135
 [<c02038c0>] show_trace_log_lvl+0x1a/0x2f
 [<c02042bb>] show_trace+0x12/0x14
 [<c0204a01>] dump_stack+0x6c/0x72
 [<c022edf0>] lockdep_init_map+0x94/0x374
 [<c022e79d>] debug_mutex_init+0x2c/0x3c
 [<c0229bb0>] __mutex_init+0x38/0x40
 [<f885520d>] 0xf885520d
 [<c0226816>] parse_args+0x121/0x1fb
 [<c0237aaf>] sys_init_module+0x10e8/0x1576
 [<c0202836>] sysenter_past_esp+0x5f/0xa5
 =======================

This is a long-standing problem I've seen in several of the latest stable
kernels.  Once lockdep turns itself off, there's no easy way to turn it back
on short of a reboot.

I looked more closely at the mtd code.  I believe the problem is that
lockdep doesn't like a mutex_init to be called from a module_init code path,
possibly because the module's symbols aren't all initialized yet.  (This
could arguably be considered a limitation of lockdep.)

So I tried to defer the call to module_init until it's absolutely needed.  I
couldn't find a clean way to do that via the struct mtd_info ops (there's no
suitable ->init op), so instead I used an int to mark whether the mutex is
initialized or not.  Below is a patch.  It works, but it's not as clean as
it should be: a better way would be to probably add an mtd_info ->init op or
so.

At least with this patch, lockdep doesn't complain any longer, so I can run
a clean set of regression tests w/ unionfs on top of jffs2 and other file
systems.

In lieu of a better fix, is this patch acceptable?

Thanks,
Erez.

------------------------------------------------------------------------------

block2mtd: defer mutex initialization to avoid a lockdep warning

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>

diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
index be4b994..2c6d3e7 100644
--- a/drivers/mtd/devices/block2mtd.c
+++ b/drivers/mtd/devices/block2mtd.c
@@ -32,6 +32,7 @@ struct block2mtd_dev {
 	struct list_head list;
 	struct block_device *blkdev;
 	struct mtd_info mtd;
+	int mutex_initialized;
 	struct mutex write_mutex;
 };
 
@@ -85,6 +86,11 @@ static int block2mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
 	size_t len = instr->len;
 	int err;
 
+	if (!dev->mutex_initialized) {
+		mutex_init(&dev->write_mutex);
+		dev->mutex_initialized = 1;
+	}
+
 	instr->state = MTD_ERASING;
 	mutex_lock(&dev->write_mutex);
 	err = _block2mtd_erase(dev, from, len);
@@ -194,6 +200,11 @@ static int block2mtd_write(struct mtd_info *mtd, loff_t to, size_t len,
 	struct block2mtd_dev *dev = mtd->priv;
 	int err;
 
+	if (!dev->mutex_initialized) {
+		mutex_init(&dev->write_mutex);
+		dev->mutex_initialized = 1;
+	}
+
 	if (!len)
 		return 0;
 	if (to >= mtd->size)
@@ -275,8 +286,6 @@ static struct block2mtd_dev *add_device(char *devname, int erase_size)
 		goto devinit_err;
 	}
 
-	mutex_init(&dev->write_mutex);
-
 	/* Setup the MTD structure */
 	/* make the name contain the block device in */
 	dev->mtd.name = kmalloc(sizeof("block2mtd: ") + strlen(devname),

Re: [PATCH] block2mtd lockdep_init_map warning

[Jörn Engel]

Maybe it is not obvious that I maintain this driver and would like to be
kept on Cc:.  Will send a patch to fix that shortly.

On Sun, 6 January 2008 02:17:32 -0500, Erez Zadok wrote:
> 
> Hi David,
> 
> I've reported before a lockdep warning when block2mtd is modloaded, and a
> device is initialized, as in
> 
> 	modprobe block2mtd block2mtd=/dev/loop0
> 
> A typical warning looks like this:
> 
> BUG: key f88565c0 not in .data!
> WARNING: at kernel/lockdep.c:2331 lockdep_init_map()
> Pid: 1823, comm: modprobe Not tainted 2.6.24-rc6-unionfs2 #135
>  [<c02038c0>] show_trace_log_lvl+0x1a/0x2f
>  [<c02042bb>] show_trace+0x12/0x14
>  [<c0204a01>] dump_stack+0x6c/0x72
>  [<c022edf0>] lockdep_init_map+0x94/0x374
>  [<c022e79d>] debug_mutex_init+0x2c/0x3c
>  [<c0229bb0>] __mutex_init+0x38/0x40
>  [<f885520d>] 0xf885520d
>  [<c0226816>] parse_args+0x121/0x1fb
>  [<c0237aaf>] sys_init_module+0x10e8/0x1576
>  [<c0202836>] sysenter_past_esp+0x5f/0xa5
>  =======================
> 
> This is a long-standing problem I've seen in several of the latest stable
> kernels.  Once lockdep turns itself off, there's no easy way to turn it back
> on short of a reboot.
> 
> I looked more closely at the mtd code.  I believe the problem is that
> lockdep doesn't like a mutex_init to be called from a module_init code path,
> possibly because the module's symbols aren't all initialized yet.  (This
> could arguably be considered a limitation of lockdep.)
> 
> So I tried to defer the call to module_init until it's absolutely needed.  I
> couldn't find a clean way to do that via the struct mtd_info ops (there's no
> suitable ->init op), so instead I used an int to mark whether the mutex is
> initialized or not.  Below is a patch.  It works, but it's not as clean as
> it should be: a better way would be to probably add an mtd_info ->init op or
> so.
> 
> At least with this patch, lockdep doesn't complain any longer, so I can run
> a clean set of regression tests w/ unionfs on top of jffs2 and other file
> systems.
> 
> In lieu of a better fix, is this patch acceptable?

Not for me.  I don't mind if you keep such a hack until a proper
solution if found in your private tree.  But it is a horrible solution
to a problem introduced elsewhere.

Ingo, Peter, does either of you actually care about this problem?  In
the last round when I debugged this problem there was a notable lack of
reaction from either of you.

> block2mtd: defer mutex initialization to avoid a lockdep warning
> 
> Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
> 
> diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
> index be4b994..2c6d3e7 100644
> --- a/drivers/mtd/devices/block2mtd.c
> +++ b/drivers/mtd/devices/block2mtd.c
> @@ -32,6 +32,7 @@ struct block2mtd_dev {
>  	struct list_head list;
>  	struct block_device *blkdev;
>  	struct mtd_info mtd;
> +	int mutex_initialized;
>  	struct mutex write_mutex;
>  };
>  
> @@ -85,6 +86,11 @@ static int block2mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
>  	size_t len = instr->len;
>  	int err;
>  
> +	if (!dev->mutex_initialized) {
> +		mutex_init(&dev->write_mutex);
> +		dev->mutex_initialized = 1;
> +	}
> +
>  	instr->state = MTD_ERASING;
>  	mutex_lock(&dev->write_mutex);
>  	err = _block2mtd_erase(dev, from, len);
> @@ -194,6 +200,11 @@ static int block2mtd_write(struct mtd_info *mtd, loff_t to, size_t len,
>  	struct block2mtd_dev *dev = mtd->priv;
>  	int err;
>  
> +	if (!dev->mutex_initialized) {
> +		mutex_init(&dev->write_mutex);
> +		dev->mutex_initialized = 1;
> +	}
> +
>  	if (!len)
>  		return 0;
>  	if (to >= mtd->size)
> @@ -275,8 +286,6 @@ static struct block2mtd_dev *add_device(char *devname, int erase_size)
>  		goto devinit_err;
>  	}
>  
> -	mutex_init(&dev->write_mutex);
> -
>  	/* Setup the MTD structure */
>  	/* make the name contain the block device in */
>  	dev->mtd.name = kmalloc(sizeof("block2mtd: ") + strlen(devname),
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Jörn

-- 
Joern's library part 13:
http://www.chip-architect.com/

Re: [PATCH] block2mtd lockdep_init_map warning

[Peter Zijlstra]

On Sun, 2008-01-06 at 14:13 +0100, Jörn Engel wrote:

> Ingo, Peter, does either of you actually care about this problem?  In
> the last round when I debugged this problem there was a notable lack of
> reaction from either of you.

Yeah I do, I just know very little about the module stuff and haven't
come around to looking into it.

I agree that Erez's patch is quite horrible.

Re: [PATCH] block2mtd lockdep_init_map warning

[Erez Zadok]

In message <20080106131343.GA3120@lazybastard.org>, =?utf-8?B?SsO2cm4=?= Engel writes:
> Maybe it is not obvious that I maintain this driver and would like to be
> kept on Cc:.  Will send a patch to fix that shortly.

I was looking in MAINTAINERS, and you weren't listed on the main MTD section
as a maintainer.  Perhaps an update?

If you get a patch, I'd love to test it.

> On Sun, 6 January 2008 02:17:32 -0500, Erez Zadok wrote:
[...]
> > In lieu of a better fix, is this patch acceptable?
> 
> Not for me.  I don't mind if you keep such a hack until a proper
> solution if found in your private tree.  But it is a horrible solution
> to a problem introduced elsewhere.

Yup, that's what I figured.  I'll keep this ugly hack but I won't push it to
my unionfs tree on kernel.org.

> Ingo, Peter, does either of you actually care about this problem?  In
> the last round when I debugged this problem there was a notable lack of
> reaction from either of you.

The problem appears to be an interaction of two components--module loading
and lockdep--that's perhaps why it wasn't given enough attention.

I run a lot of regression tests w/ unionfs on top of many different file
systems, and monitoring for all sorts of problems, including lockdep
warnings.  So it's frustrating that each time I get to test w/ jffs2, I get
this lockdep warning, and lockdep shuts down until a reboot.  That kills my
automated testing and masks out possible other lockdep problems (that's why
I wanted to at least have some workaround).  Other lockdep problems I
reported with other subsystems had been fixed within days; this one,
unfortunately, has lingered for a while.  So if a fix can be made in a
reasonable time-frame, I'd really appreciate it.

Thanks,
Erez.

Re: [PATCH] block2mtd lockdep_init_map warning

[Jörn Engel]

On Sun, 6 January 2008 14:11:47 -0500, Erez Zadok wrote:
> 
> The problem appears to be an interaction of two components--module loading
> and lockdep--that's perhaps why it wasn't given enough attention.

Correct.  For modules lockdep depends on initializations done after
module_init has finished.  However block2mtd is an odd sod that can call
into lockdep code during module_init, causing the bug you noticed.

Several solutions are possible.  Modules could get two initcalls, one to
decide whether module load should get aborted, the other run later,
after the remaining module initializations are done.  Or the module
loader could always do the initializations and revoke them later, if
module_init failed.

But I personally am too unfamiliar with the module code to trust my
judgement and have yet to receive feedback.  Even you seem to ignore my
mails and not even Cc: me later on.  I must have done something really
horrible in my last life, it seems.

Jörn

-- 
A quarrel is quickly settled when deserted by one party; there is
no battle unless there be two.
-- Seneca

Re: [PATCH] block2mtd lockdep_init_map warning

[Peter Zijlstra]

On Sun, 2008-01-06 at 14:11 -0500, Erez Zadok wrote:

> > Ingo, Peter, does either of you actually care about this problem?  In
> > the last round when I debugged this problem there was a notable lack of
> > reaction from either of you.
> 
> The problem appears to be an interaction of two components--module loading
> and lockdep--that's perhaps why it wasn't given enough attention.

Would something like this work for people?

Not-Yet-Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
Index: linux-2.6/include/linux/sched.h
===================================================================
--- linux-2.6.orig/include/linux/sched.h
+++ linux-2.6/include/linux/sched.h
@@ -1160,6 +1160,7 @@ struct task_struct {
 	int lockdep_depth;
 	struct held_lock held_locks[MAX_LOCK_DEPTH];
 	unsigned int lockdep_recursion;
+	struct module *loading_module;
 #endif
 
 /* journalling filesystem info */
Index: linux-2.6/kernel/module.c
===================================================================
--- linux-2.6.orig/kernel/module.c
+++ linux-2.6/kernel/module.c
@@ -2023,6 +2023,9 @@ static struct module *load_module(void _
 		printk(KERN_WARNING "%s: Ignoring obsolete parameters\n",
 		       mod->name);
 
+#ifdef CONFIG_LOCKDEP
+	current->loading_module = mod;
+#endif
 	/* Size of section 0 is 0, so this works well if no params */
 	err = parse_args(mod->name, mod->args,
 			 (struct kernel_param *)
@@ -2030,6 +2033,9 @@ static struct module *load_module(void _
 			 sechdrs[setupindex].sh_size
 			 / sizeof(struct kernel_param),
 			 NULL);
+#ifdef CONFIG_LOCKDEP
+	current->loading_module = NULL
+#endif
 	if (err < 0)
 		goto arch_cleanup;
 
@@ -2454,6 +2460,17 @@ int is_module_address(unsigned long addr
 		}
 	}
 
+#ifdef CONFIG_LOCKDEP
+	if (current->loading_module) {
+		mod = current->loading_module;
+		if (within(addr, mod->module_init, mod->init_text_size)
+		    || within(addr, mod->module_core, mod->core_text_size)) {
+			preempt_enable();
+			return 1;
+		}
+	}
+#endif
+
 	preempt_enable();
 
 	return 0;

Re: [PATCH] block2mtd lockdep_init_map warning

[Jörn Engel]

On Mon, 7 January 2008 11:05:26 +0100, Peter Zijlstra wrote:
> 
> Would something like this work for people?

Looks a lot better than what I thought of.  However, does the #ifdef
within is_module_address() make sense when afaict lockdep is the only
caller of that function?  Looks as if the whole function should be made
conditional or none of it.

> Not-Yet-Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> ---
> Index: linux-2.6/include/linux/sched.h
> ===================================================================
> --- linux-2.6.orig/include/linux/sched.h
> +++ linux-2.6/include/linux/sched.h
> @@ -1160,6 +1160,7 @@ struct task_struct {
>  	int lockdep_depth;
>  	struct held_lock held_locks[MAX_LOCK_DEPTH];
>  	unsigned int lockdep_recursion;
> +	struct module *loading_module;
>  #endif
>  
>  /* journalling filesystem info */
> Index: linux-2.6/kernel/module.c
> ===================================================================
> --- linux-2.6.orig/kernel/module.c
> +++ linux-2.6/kernel/module.c
> @@ -2023,6 +2023,9 @@ static struct module *load_module(void _
>  		printk(KERN_WARNING "%s: Ignoring obsolete parameters\n",
>  		       mod->name);
>  
> +#ifdef CONFIG_LOCKDEP
> +	current->loading_module = mod;
> +#endif
>  	/* Size of section 0 is 0, so this works well if no params */
>  	err = parse_args(mod->name, mod->args,
>  			 (struct kernel_param *)
> @@ -2030,6 +2033,9 @@ static struct module *load_module(void _
>  			 sechdrs[setupindex].sh_size
>  			 / sizeof(struct kernel_param),
>  			 NULL);
> +#ifdef CONFIG_LOCKDEP
> +	current->loading_module = NULL
> +#endif
>  	if (err < 0)
>  		goto arch_cleanup;
>  
> @@ -2454,6 +2460,17 @@ int is_module_address(unsigned long addr
>  		}
>  	}
>  
> +#ifdef CONFIG_LOCKDEP
> +	if (current->loading_module) {
> +		mod = current->loading_module;
> +		if (within(addr, mod->module_init, mod->init_text_size)
> +		    || within(addr, mod->module_core, mod->core_text_size)) {
> +			preempt_enable();
> +			return 1;
> +		}
> +	}
> +#endif
> +
>  	preempt_enable();
>  
>  	return 0;
> 
> 
> 

Jörn

-- 
I don't understand it. Nobody does.
-- Richard P. Feynman

Re: [PATCH] block2mtd lockdep_init_map warning

[Peter Zijlstra]

On Mon, 2008-01-07 at 11:20 +0100, Jörn Engel wrote:
> On Mon, 7 January 2008 11:05:26 +0100, Peter Zijlstra wrote:
> > 
> > Would something like this work for people?
> 
> Looks a lot better than what I thought of.  However, does the #ifdef
> within is_module_address() make sense when afaict lockdep is the only
> caller of that function?  Looks as if the whole function should be made
> conditional or none of it.

Ah, I hadn't bothered to check if there were more users. /me does a (not
so quick) git grep and finds lockdep is indeed the only caller. Sure, we
can move the whole function into the ifdef.

Re: [PATCH] block2mtd lockdep_init_map warning

[Rusty Russell]

On Monday 07 January 2008 21:05:26 Peter Zijlstra wrote:
> On Sun, 2008-01-06 at 14:11 -0500, Erez Zadok wrote:
> > > Ingo, Peter, does either of you actually care about this problem?  In
> > > the last round when I debugged this problem there was a notable lack of
> > > reaction from either of you.
> >
> > The problem appears to be an interaction of two components--module
> > loading and lockdep--that's perhaps why it wasn't given enough attention.
>
> Would something like this work for people?

Hi Peter,

    There's nothing wrong with this patch, but I think it papers over a more
general problem: we enter the module (to parse args) while it's not in the
module list.  This also means we won't get a nice oops if it crashes.

    This is untested, but does it solve it for you?

Thanks,
Rusty.

diff -r 68fd1b22db89 kernel/module.c
--- a/kernel/module.c	Mon Jan 07 18:59:50 2008 +1100
+++ b/kernel/module.c	Tue Jan 08 11:46:11 2008 +1100
@@ -2043,6 +2043,11 @@ static struct module *load_module(void _
 		printk(KERN_WARNING "%s: Ignoring obsolete parameters\n",
 		       mod->name);
 
+	/* Now sew it into the lists so we can get lockdep and oops
+         * info during argument parsing.  Noone should access us, since
+         * strong_try_module_get() will fail. */
+	stop_machine_run(__link_module, mod, NR_CPUS);
+
 	/* Size of section 0 is 0, so this works well if no params */
 	err = parse_args(mod->name, mod->args,
 			 (struct kernel_param *)
@@ -2051,7 +2056,7 @@ static struct module *load_module(void _
 			 / sizeof(struct kernel_param),
 			 NULL);
 	if (err < 0)
-		goto arch_cleanup;
+		goto unlink;
 
 	err = mod_sysfs_setup(mod,
 			      (struct kernel_param *)
@@ -2059,7 +2064,7 @@ static struct module *load_module(void _
 			      sechdrs[setupindex].sh_size
 			      / sizeof(struct kernel_param));
 	if (err < 0)
-		goto arch_cleanup;
+		goto unlink;
 	add_sect_attrs(mod, hdr->e_shnum, secstrings, sechdrs);
 	add_notes_attrs(mod, hdr->e_shnum, secstrings, sechdrs);
 
@@ -2074,7 +2079,8 @@ static struct module *load_module(void _
 	/* Done! */
 	return mod;
 
- arch_cleanup:
+ unlink:
+	stop_machine_run(__unlink_module, mod, NR_CPUS);
 	module_arch_cleanup(mod);
  cleanup:
 	module_unload_free(mod);
@@ -2130,10 +2136,6 @@ sys_init_module(void __user *umod,
 		mutex_unlock(&module_mutex);
 		return PTR_ERR(mod);
 	}
-
-	/* Now sew it into the lists.  They won't access us, since
-           strong_try_module_get() will fail. */
-	stop_machine_run(__link_module, mod, NR_CPUS);
 
 	/* Drop lock so they can recurse */
 	mutex_unlock(&module_mutex);

Re: [PATCH] block2mtd lockdep_init_map warning

[Peter Zijlstra]

On Tue, 2008-01-08 at 11:47 +1100, Rusty Russell wrote:
> On Monday 07 January 2008 21:05:26 Peter Zijlstra wrote:
> > On Sun, 2008-01-06 at 14:11 -0500, Erez Zadok wrote:
> > > > Ingo, Peter, does either of you actually care about this problem?  In
> > > > the last round when I debugged this problem there was a notable lack of
> > > > reaction from either of you.
> > >
> > > The problem appears to be an interaction of two components--module
> > > loading and lockdep--that's perhaps why it wasn't given enough attention.
> >
> > Would something like this work for people?
> 
> Hi Peter,
> 
>     There's nothing wrong with this patch, but I think it papers over a more
> general problem: we enter the module (to parse args) while it's not in the
> module list.  This also means we won't get a nice oops if it crashes.
> 
>     This is untested, but does it solve it for you?

I think it should, Erez care to give it a spin?

> diff -r 68fd1b22db89 kernel/module.c
> --- a/kernel/module.c	Mon Jan 07 18:59:50 2008 +1100
> +++ b/kernel/module.c	Tue Jan 08 11:46:11 2008 +1100
> @@ -2043,6 +2043,11 @@ static struct module *load_module(void _
>  		printk(KERN_WARNING "%s: Ignoring obsolete parameters\n",
>  		       mod->name);
>  
> +	/* Now sew it into the lists so we can get lockdep and oops
> +         * info during argument parsing.  Noone should access us, since
> +         * strong_try_module_get() will fail. */
> +	stop_machine_run(__link_module, mod, NR_CPUS);
> +
>  	/* Size of section 0 is 0, so this works well if no params */
>  	err = parse_args(mod->name, mod->args,
>  			 (struct kernel_param *)
> @@ -2051,7 +2056,7 @@ static struct module *load_module(void _
>  			 / sizeof(struct kernel_param),
>  			 NULL);
>  	if (err < 0)
> -		goto arch_cleanup;
> +		goto unlink;
>  
>  	err = mod_sysfs_setup(mod,
>  			      (struct kernel_param *)
> @@ -2059,7 +2064,7 @@ static struct module *load_module(void _
>  			      sechdrs[setupindex].sh_size
>  			      / sizeof(struct kernel_param));
>  	if (err < 0)
> -		goto arch_cleanup;
> +		goto unlink;
>  	add_sect_attrs(mod, hdr->e_shnum, secstrings, sechdrs);
>  	add_notes_attrs(mod, hdr->e_shnum, secstrings, sechdrs);
>  
> @@ -2074,7 +2079,8 @@ static struct module *load_module(void _
>  	/* Done! */
>  	return mod;
>  
> - arch_cleanup:
> + unlink:
> +	stop_machine_run(__unlink_module, mod, NR_CPUS);
>  	module_arch_cleanup(mod);
>   cleanup:
>  	module_unload_free(mod);
> @@ -2130,10 +2136,6 @@ sys_init_module(void __user *umod,
>  		mutex_unlock(&module_mutex);
>  		return PTR_ERR(mod);
>  	}
> -
> -	/* Now sew it into the lists.  They won't access us, since
> -           strong_try_module_get() will fail. */
> -	stop_machine_run(__link_module, mod, NR_CPUS);
>  
>  	/* Drop lock so they can recurse */
>  	mutex_unlock(&module_mutex);

Re: [PATCH] block2mtd lockdep_init_map warning

[Jörn Engel]

On Tue, 8 January 2008 11:47:00 +1100, Rusty Russell wrote:
> 
>     There's nothing wrong with this patch, but I think it papers over a more
> general problem: we enter the module (to parse args) while it's not in the
> module list.  This also means we won't get a nice oops if it crashes.
> 
>     This is untested, but does it solve it for you?

Not sure about lockdep, but it makes all the difference for an oops,
forced by explicit BUG() in block2mtd.

Before:
kernel BUG at drivers/mtd/devices/block2mtd.c:382!
invalid opcode: 0000 [#1]
Modules linked in:
CPU:    0
EIP:    0060:[<e0818003>]    Not tainted VLI
EFLAGS: 00000246   (2.6.22 #38)
EIP is at 0xe0818003
eax: dfe8c7da   ebx: dfe8c7d0   ecx: e08185a8   edx: e08185a8
esi: e0818b1c   edi: dfe8c7da   ebp: df95de78   esp: df95de78
ds: 007b   es: 007b   fs: 0000  gs: 0033  ss: 0068
Process insmod (pid: 1076, ti=df95c000 task=df903080 task.ti=df95c000)
Stack: df95dec4 c0125576 00000000 df95de90 fffffff8 e0815932 00000006 00000000 
       e08185a8 e0818b4c 00000283 00000000 0000000b e0818b1c 00000000 dfe8c7dc 
       00000014 e0815b48 00000014 df95dfb0 c01314e4 00000001 00000000 df95df24 
Call Trace:
 [<c0103248>] show_trace_log_lvl+0x1a/0x2f
 [<c01032f8>] show_stack_log_lvl+0x9b/0xa3
 [<c01034be>] show_registers+0x1be/0x290
 [<c010366f>] die+0xdf/0x1a1
 [<c01037ba>] do_trap+0x89/0xa2
 [<c0103b25>] do_invalid_op+0x88/0x92
 [<c0384e82>] error_code+0x6a/0x70
 [<c0125576>] parse_args+0x120/0x1f6
 [<c01314e4>] sys_init_module+0xf76/0x1297
 [<c01023ee>] syscall_call+0x7/0xb
 =======================
Code: <0f> 0b eb fe 55 85 c0 89 e5 53 89 c3 74 31 8b 40 28 e8 ee 3e 93 df 
EIP: [<e0818003>] 0xe0818003 SS:ESP 0068:df95de78

After:
kernel BUG at drivers/mtd/devices/block2mtd.c:382!
invalid opcode: 0000 [#1]
Modules linked in: block2mtd
CPU:    0
EIP:    0060:[<f8820003>]    Not tainted VLI
EFLAGS: 00000246   (2.6.22 #39)
EIP is at block2mtd_setup+0x3/0x7 [block2mtd]
eax: f7da387a   ebx: f7da3870   ecx: f88205a8   edx: f88205a8
esi: f8820b1c   edi: f7da387a   ebp: f788be78   esp: f788be78
ds: 007b   es: 007b   fs: 0000  gs: 0033  ss: 0068
Process insmod (pid: 1072, ti=f788a000 task=c1b22b00 task.ti=f788a000)
Stack: f788bec4 c0125576 00000000 f788be90 fffffff8 f881d932 00000006 00000000 
       f88205a8 f8820b4c 00000283 00000000 0000000b f8820b1c 00000000 f7da387c 
       00000014 f881db48 00000014 f788bfb0 c01314f1 00000001 00000000 f788bf24 
Call Trace:
 [<c0103248>] show_trace_log_lvl+0x1a/0x2f
 [<c01032f8>] show_stack_log_lvl+0x9b/0xa3
 [<c01034be>] show_registers+0x1be/0x290
 [<c010366f>] die+0xdf/0x1a1
 [<c01037ba>] do_trap+0x89/0xa2
 [<c0103b25>] do_invalid_op+0x88/0x92
 [<c0384e92>] error_code+0x6a/0x70
 [<c0125576>] parse_args+0x120/0x1f6
 [<c01314f1>] sys_init_module+0xf83/0x12a4
 [<c01023ee>] syscall_call+0x7/0xb
 =======================
Code: <0f> 0b eb fe 55 85 c0 89 e5 53 89 c3 74 31 8b 40 28 e8 fa be 92 c7 
EIP: [<f8820003>] block2mtd_setup+0x3/0x7 [block2mtd] SS:ESP 0068:f788be78


Patch didn't compile due to function ordering.  Here is an updated version.

Acked-and-tested-by: Joern Engel <joern@lazybastard.org>

diff --git a/kernel/module.c b/kernel/module.c
index c2e3e2e..0397b1e 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1645,6 +1645,17 @@ static inline void add_kallsyms(struct module *mod,
 }
 #endif /* CONFIG_KALLSYMS */
 
+/*
+ * link the module with the whole machine is stopped with interrupts off
+ * - this defends against kallsyms not taking locks
+ */
+static int __link_module(void *_mod)
+{
+	struct module *mod = _mod;
+	list_add(&mod->list, &modules);
+	return 0;
+}
+
 /* Allocate and load the module: note that size of section 0 is always
    zero, and we rely on this for optional sections. */
 static struct module *load_module(void __user *umod,
@@ -2023,6 +2034,11 @@ static struct module *load_module(void __user *umod,
 		printk(KERN_WARNING "%s: Ignoring obsolete parameters\n",
 		       mod->name);
 
+	/* Now sew it into the lists so we can get lockdep and oops
+         * info during argument parsing.  Noone should access us, since
+         * strong_try_module_get() will fail. */
+	stop_machine_run(__link_module, mod, NR_CPUS);
+
 	/* Size of section 0 is 0, so this works well if no params */
 	err = parse_args(mod->name, mod->args,
 			 (struct kernel_param *)
@@ -2031,7 +2047,7 @@ static struct module *load_module(void __user *umod,
 			 / sizeof(struct kernel_param),
 			 NULL);
 	if (err < 0)
-		goto arch_cleanup;
+		goto unlink;
 
 	err = mod_sysfs_setup(mod,
 			      (struct kernel_param *)
@@ -2039,7 +2055,7 @@ static struct module *load_module(void __user *umod,
 			      sechdrs[setupindex].sh_size
 			      / sizeof(struct kernel_param));
 	if (err < 0)
-		goto arch_cleanup;
+		goto unlink;
 	add_sect_attrs(mod, hdr->e_shnum, secstrings, sechdrs);
 	add_notes_attrs(mod, hdr->e_shnum, secstrings, sechdrs);
 
@@ -2054,7 +2070,8 @@ static struct module *load_module(void __user *umod,
 	/* Done! */
 	return mod;
 
- arch_cleanup:
+ unlink:
+	stop_machine_run(__unlink_module, mod, NR_CPUS);
 	module_arch_cleanup(mod);
  cleanup:
 	module_unload_free(mod);
@@ -2076,17 +2093,6 @@ static struct module *load_module(void __user *umod,
 	goto free_hdr;
 }
 
-/*
- * link the module with the whole machine is stopped with interrupts off
- * - this defends against kallsyms not taking locks
- */
-static int __link_module(void *_mod)
-{
-	struct module *mod = _mod;
-	list_add(&mod->list, &modules);
-	return 0;
-}
-
 /* This is where the real work happens */
 asmlinkage long
 sys_init_module(void __user *umod,
@@ -2111,10 +2117,6 @@ sys_init_module(void __user *umod,
 		return PTR_ERR(mod);
 	}
 
-	/* Now sew it into the lists.  They won't access us, since
-           strong_try_module_get() will fail. */
-	stop_machine_run(__link_module, mod, NR_CPUS);
-
 	/* Drop lock so they can recurse */
 	mutex_unlock(&module_mutex);
 

Re: [PATCH] block2mtd lockdep_init_map warning

[Erez Zadok]

In message <20080116212054.GA891@lazybastard.org>, =?utf-8?B?SsO2cm4=?= Engel writes:
[...]
> Patch didn't compile due to function ordering.  Here is an updated version.

Joern/Peter, I've tested this updated patch with v2.6.24-rc8-74-ga7da60f.
It worked fine for me.

Thanks,
Erez.

> Acked-and-tested-by: Joern Engel <joern@lazybastard.org>
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index c2e3e2e..0397b1e 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1645,6 +1645,17 @@ static inline void add_kallsyms(struct module *mod,
>  }
>  #endif /* CONFIG_KALLSYMS */
>  
> +/*
> + * link the module with the whole machine is stopped with interrupts off
> + * - this defends against kallsyms not taking locks
> + */
> +static int __link_module(void *_mod)
> +{
> +	struct module *mod = _mod;
> +	list_add(&mod->list, &modules);
> +	return 0;
> +}
> +
>  /* Allocate and load the module: note that size of section 0 is always
>     zero, and we rely on this for optional sections. */
>  static struct module *load_module(void __user *umod,
> @@ -2023,6 +2034,11 @@ static struct module *load_module(void __user *umod,
>  		printk(KERN_WARNING "%s: Ignoring obsolete parameters\n",
>  		       mod->name);
>  
> +	/* Now sew it into the lists so we can get lockdep and oops
> +         * info during argument parsing.  Noone should access us, since
> +         * strong_try_module_get() will fail. */
> +	stop_machine_run(__link_module, mod, NR_CPUS);
> +
>  	/* Size of section 0 is 0, so this works well if no params */
>  	err = parse_args(mod->name, mod->args,
>  			 (struct kernel_param *)
> @@ -2031,7 +2047,7 @@ static struct module *load_module(void __user *umod,
>  			 / sizeof(struct kernel_param),
>  			 NULL);
>  	if (err < 0)
> -		goto arch_cleanup;
> +		goto unlink;
>  
>  	err = mod_sysfs_setup(mod,
>  			      (struct kernel_param *)
> @@ -2039,7 +2055,7 @@ static struct module *load_module(void __user *umod,
>  			      sechdrs[setupindex].sh_size
>  			      / sizeof(struct kernel_param));
>  	if (err < 0)
> -		goto arch_cleanup;
> +		goto unlink;
>  	add_sect_attrs(mod, hdr->e_shnum, secstrings, sechdrs);
>  	add_notes_attrs(mod, hdr->e_shnum, secstrings, sechdrs);
>  
> @@ -2054,7 +2070,8 @@ static struct module *load_module(void __user *umod,
>  	/* Done! */
>  	return mod;
>  
> - arch_cleanup:
> + unlink:
> +	stop_machine_run(__unlink_module, mod, NR_CPUS);
>  	module_arch_cleanup(mod);
>   cleanup:
>  	module_unload_free(mod);
> @@ -2076,17 +2093,6 @@ static struct module *load_module(void __user *umod,
>  	goto free_hdr;
>  }
>  
> -/*
> - * link the module with the whole machine is stopped with interrupts off
> - * - this defends against kallsyms not taking locks
> - */
> -static int __link_module(void *_mod)
> -{
> -	struct module *mod = _mod;
> -	list_add(&mod->list, &modules);
> -	return 0;
> -}
> -
>  /* This is where the real work happens */
>  asmlinkage long
>  sys_init_module(void __user *umod,
> @@ -2111,10 +2117,6 @@ sys_init_module(void __user *umod,
>  		return PTR_ERR(mod);
>  	}
>  
> -	/* Now sew it into the lists.  They won't access us, since
> -           strong_try_module_get() will fail. */
> -	stop_machine_run(__link_module, mod, NR_CPUS);
> -
>  	/* Drop lock so they can recurse */
>  	mutex_unlock(&module_mutex);