[Bug 3.4.5] reiserfs: mutex_destroy called with locked mutex

[Bug 3.4.5] reiserfs: mutex_destroy called with locked mutex

[Knut Petersen]

I hit the following problem during a heavy compile job on kernel 3.4.5:

Jul 17 20:29:59 golem kernel: [27408.140718] ------------[ cut here ]------------
Jul 17 20:29:59 golem kernel: [27408.140739] WARNING: at kernel/mutex-debug.c:106 mutex_destroy+0x35/0x3f()
Jul 17 20:29:59 golem kernel: [27408.140748] Hardware name:
Jul 17 20:29:59 golem kernel: [27408.140753] Modules linked in: ipt_MASQUERADE xt_pkttype xt_TCPMSS xt_tcpudp xt_LOG xt_limit iptable_nat nf_nat pppoe pppox af_packet ppp_generic s
lhc ir_kbd_i2c snd_pcm_oss snd_mixer_oss snd_seq snd_seq_device ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_raw xt_NOTRACK ipt_REJECT iptable_raw iptable_filter ip6table_
mangle nf_conntrack_netbios_ns nf_conntrack_broadcast nf_conntrack_ipv4 nf_defrag_ipv4 ip_tables xt_conntrack nf_conntrack ip6table_filter ip6_tables x_tables ipv6 isl6421 ir_lirc_
codec cx24116 lirc_dev ir_mce_kbd_decoder ir_sanyo_decoder cx88_dvb ir_sony_decoder ir_jvc_decoder ir_rc6_decoder videobuf_dvb snd_hda_codec_realtek dvb_core ir_rc5_decoder ir_nec_
decoder snd_hda_intel rc_hauppauge snd_hda_codec tuner snd_rme96 snd_hwdep snd_pcm cx8800 cx8802 cx88xx snd_timer snd rc_core tveeprom v4l2_common soundcore videodev btcx_risc vide
obuf_dma_sg videobuf_core snd_page_alloc
Jul 17 20:29:59 golem kernel: [27408.140933] Pid: 17424, comm: mkdir Not tainted 3.4.5-main #49
Jul 17 20:29:59 golem kernel: [27408.140940] Call Trace:
Jul 17 20:29:59 golem kernel: [27408.140950]  [<c01218bb>] ? console_unlock+0x1bc/0x1e2
Jul 17 20:29:59 golem kernel: [27408.140960]  [<c0120dfc>] warn_slowpath_common+0x68/0x7d
Jul 17 20:29:59 golem kernel: [27408.140969]  [<c01505a5>] ? mutex_destroy+0x35/0x3f
Jul 17 20:29:59 golem kernel: [27408.140978]  [<c0120e25>] warn_slowpath_null+0x14/0x18
Jul 17 20:29:59 golem kernel: [27408.140987]  [<c01505a5>] mutex_destroy+0x35/0x3f
Jul 17 20:29:59 golem kernel: [27408.140998]  [<c01cad96>] lockdep_annotate_inode_mutex_key+0x3a/0x69
Jul 17 20:29:59 golem kernel: [27408.141008]  [<c01cadd6>] unlock_new_inode+0x11/0x55
Jul 17 20:29:59 golem kernel: [27408.141018]  [<c020914c>] reiserfs_mkdir+0x274/0x29b
Jul 17 20:29:59 golem kernel: [27408.141030]  [<c01c153c>] vfs_mkdir+0x8f/0xdd
Jul 17 20:29:59 golem kernel: [27408.141039]  [<c029a9a2>] ? apparmor_path_mkdir+0x1b/0x1d
Jul 17 20:29:59 golem kernel: [27408.141049]  [<c01c388b>] sys_mkdirat+0x76/0xa9
Jul 17 20:29:59 golem kernel: [27408.141058]  [<c01c38d5>] sys_mkdir+0x17/0x19
Jul 17 20:29:59 golem kernel: [27408.141069]  [<c049f1dd>] syscall_call+0x7/0xb
Jul 17 20:29:59 golem kernel: [27408.141077] ---[ end trace ea73962172137cb3 ]---

cu,
  Knut

Re: [Bug 3.4.5] reiserfs: mutex_destroy called with locked mutex

[Linus Torvalds]

On Tue, Jul 17, 2012 at 10:26 PM, Knut Petersen
<Knut_Petersen@t-online.de> wrote:
> I hit the following problem during a heavy compile job on kernel 3.4.5:

I think it's triggered by the fact that reiserfs does

        d_instantiate(dentry, inode);
        unlock_new_inode(inode);

ie it does the "unlock_new_inode()" *after* it has already made the
inode visible to others in the filesystem. So you can now have a
concurrent lookup of that dentry that finds the (locked) inode before
"unlock_new_inode()" has had the chance to set the lockdep class.
Also, since it's been instantiated, the only *valid* inode pointer is
in the dentry, so the code really really shouldn't use the "inode"
pointer any more because the refcount has been transferred to the
dentry. So maybe memory pressure could turn the dentry negative (and
free the inode) while the unlock_new_inode() code runs.

fs/ext[23]/namei.c has the same pattern, though, and I think it should
be harmless (the inode is marked I_NEW and we get the i_lock for
unlock_new_inode, so the freeing code should know to keep away from
it). So I don't think the freeing code could trigger, but a concurrent
lookup then trying to look up the new directory (and taking the new
directory i_semaphore lock) could happen, afaik.

So I think we should re-order the d_instantiate/unlock_new_inode calls. Al?

                   Linus

Re: [Bug 3.4.5] reiserfs: mutex_destroy called with locked mutex

[Al Viro]

On Wed, Jul 18, 2012 at 09:26:57AM -0700, Linus Torvalds wrote:
> On Tue, Jul 17, 2012 at 10:26 PM, Knut Petersen
> <Knut_Petersen@t-online.de> wrote:
> > I hit the following problem during a heavy compile job on kernel 3.4.5:
> 
> I think it's triggered by the fact that reiserfs does
> 
>         d_instantiate(dentry, inode);
>         unlock_new_inode(inode);
> 
> ie it does the "unlock_new_inode()" *after* it has already made the
> inode visible to others in the filesystem. So you can now have a
> concurrent lookup of that dentry that finds the (locked) inode before
> "unlock_new_inode()" has had the chance to set the lockdep class.
> Also, since it's been instantiated, the only *valid* inode pointer is
> in the dentry, so the code really really shouldn't use the "inode"
> pointer any more because the refcount has been transferred to the
> dentry. So maybe memory pressure could turn the dentry negative (and
> free the inode) while the unlock_new_inode() code runs.
> 
> fs/ext[23]/namei.c has the same pattern, though, and I think it should
> be harmless (the inode is marked I_NEW and we get the i_lock for
> unlock_new_inode, so the freeing code should know to keep away from
> it). So I don't think the freeing code could trigger, but a concurrent
> lookup then trying to look up the new directory (and taking the new
> directory i_semaphore lock) could happen, afaik.
> 
> So I think we should re-order the d_instantiate/unlock_new_inode calls. Al?

Umm...  The thing is, we'd get WARN_ON() in iput_final() triggering in
that scenario before lockdep could complain.

Note that dentry is pinned by reference we are holding, so no matter who
comes and finds it in dcache, no amount of memory pressure is going to
evict it or its inode.  Busy dentries do _not_ become negative under
us.   So no, I don't believe that this is what the original poster had been seeing.

OTOH, you are right and it's better to reorder these pairs on the general principle;
it's just that we are seeing something else here.

Below is the obvious patch reordering such guys; it's a lot more than just
ext[23] and reiserfs, BTW.  I don't believe that this should go in this
cycle, though, and it won't change the situation for the original poster.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index da52cdb..ffa2be5 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -269,8 +269,8 @@ ecryptfs_create(struct inode *directory_inode, struct dentry *ecryptfs_dentry,
 		iput(ecryptfs_inode);
 		goto out;
 	}
-	d_instantiate(ecryptfs_dentry, ecryptfs_inode);
 	unlock_new_inode(ecryptfs_inode);
+	d_instantiate(ecryptfs_dentry, ecryptfs_inode);
 out:
 	return rc;
 }
diff --git a/fs/ext2/namei.c b/fs/ext2/namei.c
index 9ba7de0..73b0d95 100644
--- a/fs/ext2/namei.c
+++ b/fs/ext2/namei.c
@@ -41,8 +41,8 @@ static inline int ext2_add_nondir(struct dentry *dentry, struct inode *inode)
 {
 	int err = ext2_add_link(dentry, inode);
 	if (!err) {
-		d_instantiate(dentry, inode);
 		unlock_new_inode(inode);
+		d_instantiate(dentry, inode);
 		return 0;
 	}
 	inode_dec_link_count(inode);
@@ -242,8 +242,8 @@ static int ext2_mkdir(struct inode * dir, struct dentry * dentry, umode_t mode)
 	if (err)
 		goto out_fail;
 
-	d_instantiate(dentry, inode);
 	unlock_new_inode(inode);
+	d_instantiate(dentry, inode);
 out:
 	return err;
 
diff --git a/fs/ext3/namei.c b/fs/ext3/namei.c
index 85286db..8f4fdda 100644
--- a/fs/ext3/namei.c
+++ b/fs/ext3/namei.c
@@ -1671,8 +1671,8 @@ static int ext3_add_nondir(handle_t *handle,
 	int err = ext3_add_entry(handle, dentry, inode);
 	if (!err) {
 		ext3_mark_inode_dirty(handle, inode);
-		d_instantiate(dentry, inode);
 		unlock_new_inode(inode);
+		d_instantiate(dentry, inode);
 		return 0;
 	}
 	drop_nlink(inode);
@@ -1836,8 +1836,8 @@ out_clear_inode:
 	if (err)
 		goto out_clear_inode;
 
-	d_instantiate(dentry, inode);
 	unlock_new_inode(inode);
+	d_instantiate(dentry, inode);
 out_stop:
 	brelse(dir_block);
 	ext3_journal_stop(handle);
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index eca3e48..d0d3f0e 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -2072,8 +2072,8 @@ static int ext4_add_nondir(handle_t *handle,
 	int err = ext4_add_entry(handle, dentry, inode);
 	if (!err) {
 		ext4_mark_inode_dirty(handle, inode);
-		d_instantiate(dentry, inode);
 		unlock_new_inode(inode);
+		d_instantiate(dentry, inode);
 		return 0;
 	}
 	drop_nlink(inode);
@@ -2249,8 +2249,8 @@ out_clear_inode:
 	err = ext4_mark_inode_dirty(handle, dir);
 	if (err)
 		goto out_clear_inode;
-	d_instantiate(dentry, inode);
 	unlock_new_inode(inode);
+	d_instantiate(dentry, inode);
 out_stop:
 	brelse(dir_block);
 	ext4_journal_stop(handle);
diff --git a/fs/jffs2/dir.c b/fs/jffs2/dir.c
index 2324519..ad7774d 100644
--- a/fs/jffs2/dir.c
+++ b/fs/jffs2/dir.c
@@ -226,8 +226,8 @@ static int jffs2_create(struct inode *dir_i, struct dentry *dentry,
 		  __func__, inode->i_ino, inode->i_mode, inode->i_nlink,
 		  f->inocache->pino_nlink, inode->i_mapping->nrpages);
 
-	d_instantiate(dentry, inode);
 	unlock_new_inode(inode);
+	d_instantiate(dentry, inode);
 	return 0;
 
  fail:
@@ -446,8 +446,8 @@ static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char
 	mutex_unlock(&dir_f->sem);
 	jffs2_complete_reservation(c);
 
-	d_instantiate(dentry, inode);
 	unlock_new_inode(inode);
+	d_instantiate(dentry, inode);
 	return 0;
 
  fail:
@@ -591,8 +591,8 @@ static int jffs2_mkdir (struct inode *dir_i, struct dentry *dentry, umode_t mode
 	mutex_unlock(&dir_f->sem);
 	jffs2_complete_reservation(c);
 
-	d_instantiate(dentry, inode);
 	unlock_new_inode(inode);
+	d_instantiate(dentry, inode);
 	return 0;
 
  fail:
@@ -766,8 +766,8 @@ static int jffs2_mknod (struct inode *dir_i, struct dentry *dentry, umode_t mode
 	mutex_unlock(&dir_f->sem);
 	jffs2_complete_reservation(c);
 
-	d_instantiate(dentry, inode);
 	unlock_new_inode(inode);
+	d_instantiate(dentry, inode);
 	return 0;
 
  fail:
diff --git a/fs/jfs/namei.c b/fs/jfs/namei.c
index c426293..3b91a7a 100644
--- a/fs/jfs/namei.c
+++ b/fs/jfs/namei.c
@@ -176,8 +176,8 @@ static int jfs_create(struct inode *dip, struct dentry *dentry, umode_t mode,
 		unlock_new_inode(ip);
 		iput(ip);
 	} else {
-		d_instantiate(dentry, ip);
 		unlock_new_inode(ip);
+		d_instantiate(dentry, ip);
 	}
 
       out2:
@@ -309,8 +309,8 @@ static int jfs_mkdir(struct inode *dip, struct dentry *dentry, umode_t mode)
 		unlock_new_inode(ip);
 		iput(ip);
 	} else {
-		d_instantiate(dentry, ip);
 		unlock_new_inode(ip);
+		d_instantiate(dentry, ip);
 	}
 
       out2:
@@ -1043,8 +1043,8 @@ static int jfs_symlink(struct inode *dip, struct dentry *dentry,
 		unlock_new_inode(ip);
 		iput(ip);
 	} else {
-		d_instantiate(dentry, ip);
 		unlock_new_inode(ip);
+		d_instantiate(dentry, ip);
 	}
 
       out2:
@@ -1424,8 +1424,8 @@ static int jfs_mknod(struct inode *dir, struct dentry *dentry,
 		unlock_new_inode(ip);
 		iput(ip);
 	} else {
-		d_instantiate(dentry, ip);
 		unlock_new_inode(ip);
+		d_instantiate(dentry, ip);
 	}
 
       out1:
diff --git a/fs/reiserfs/namei.c b/fs/reiserfs/namei.c
index 3916be1..8567fb8 100644
--- a/fs/reiserfs/namei.c
+++ b/fs/reiserfs/namei.c
@@ -634,8 +634,8 @@ static int reiserfs_create(struct inode *dir, struct dentry *dentry, umode_t mod
 	reiserfs_update_inode_transaction(inode);
 	reiserfs_update_inode_transaction(dir);
 
-	d_instantiate(dentry, inode);
 	unlock_new_inode(inode);
+	d_instantiate(dentry, inode);
 	retval = journal_end(&th, dir->i_sb, jbegin_count);
 
       out_failed:
@@ -712,8 +712,8 @@ static int reiserfs_mknod(struct inode *dir, struct dentry *dentry, umode_t mode
 		goto out_failed;
 	}
 
-	d_instantiate(dentry, inode);
 	unlock_new_inode(inode);
+	d_instantiate(dentry, inode);
 	retval = journal_end(&th, dir->i_sb, jbegin_count);
 
       out_failed:
@@ -800,8 +800,8 @@ static int reiserfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode
 	// the above add_entry did not update dir's stat data
 	reiserfs_update_sd(&th, dir);
 
-	d_instantiate(dentry, inode);
 	unlock_new_inode(inode);
+	d_instantiate(dentry, inode);
 	retval = journal_end(&th, dir->i_sb, jbegin_count);
 out_failed:
 	reiserfs_write_unlock_once(dir->i_sb, lock_depth);
@@ -1096,8 +1096,8 @@ static int reiserfs_symlink(struct inode *parent_dir,
 		goto out_failed;
 	}
 
-	d_instantiate(dentry, inode);
 	unlock_new_inode(inode);
+	d_instantiate(dentry, inode);
 	retval = journal_end(&th, parent_dir->i_sb, jbegin_count);
       out_failed:
 	reiserfs_write_unlock(parent_dir->i_sb);

Re: [Bug 3.4.5] reiserfs: mutex_destroy called with locked mutex

[Linus Torvalds]

On Wed, Jul 18, 2012 at 2:20 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Jul 18, 2012 at 09:26:57AM -0700, Linus Torvalds wrote:
>>
>> So I don't think the freeing code could trigger, but a concurrent
>> lookup then trying to look up the new directory (and taking the new
>> directory i_semaphore lock) could happen, afaik.
>
> Umm...  The thing is, we'd get WARN_ON() in iput_final() triggering in
> that scenario before lockdep could complain.

Not for the "look up directory in the dcache, and then lock that
inode" case, afaik. You'd get the lock before iput_final(), no?

So then "unlock_new_inode()" would run with the inode mutex held,
which could explain the lockdep warning, no?

             Linus

Re: [Bug 3.4.5] reiserfs: mutex_destroy called with locked mutex

[Al Viro]

On Wed, Jul 18, 2012 at 02:25:02PM -0700, Linus Torvalds wrote:
> On Wed, Jul 18, 2012 at 2:20 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Wed, Jul 18, 2012 at 09:26:57AM -0700, Linus Torvalds wrote:
> >>
> >> So I don't think the freeing code could trigger, but a concurrent
> >> lookup then trying to look up the new directory (and taking the new
> >> directory i_semaphore lock) could happen, afaik.
> >
> > Umm...  The thing is, we'd get WARN_ON() in iput_final() triggering in
> > that scenario before lockdep could complain.
> 
> Not for the "look up directory in the dcache, and then lock that
> inode" case, afaik. You'd get the lock before iput_final(), no?
> 
> So then "unlock_new_inode()" would run with the inode mutex held,
> which could explain the lockdep warning, no?

Umm..  Right you are, I was thinking about the "can the freeing code
actually trigger".  OK; I'm still not sure this should go in before
-final, but it could be the reason behind those (false positive)
warnings from lockdep.  Could probably step into something nasty
around e.g. writeback or quota, so maybe it's worth doing just in
case...

It's definitely the right thing to do wrt not giving the rest of the
VFS/VM nasty surprises - everything might work correctly with IO
coming on such still-I_NEW-and-locked inode, but that's not a case
that will be often considered when modifying code.  The only questions
are "is this the WARN_ON() Knut had stepped on" (and I agree with your
scenario now) and "is it critical enough to shove it into the tree
less than a week before -final".  Up to you...

Re: [Bug 3.4.5] reiserfs: mutex_destroy called with locked mutex

[Linus Torvalds]

On Wed, Jul 18, 2012 at 2:33 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
>  The only questions
> are "is this the WARN_ON() Knut had stepped on" (and I agree with your
> scenario now) and "is it critical enough to shove it into the tree
> less than a week before -final".  Up to you...


I agree that it isn't critical. Even the lockdep thing is just a
warning, and is apparently quite hard to trigger.

Will you hold on to the patch in your VFS tree, and we can just merge
it for 3.6?

                    Linus

Re: [Bug 3.4.5] reiserfs: mutex_destroy called with locked mutex

[Al Viro]

On Wed, Jul 18, 2012 at 03:37:52PM -0700, Linus Torvalds wrote:
> On Wed, Jul 18, 2012 at 2:33 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> >  The only questions
> > are "is this the WARN_ON() Knut had stepped on" (and I agree with your
> > scenario now) and "is it critical enough to shove it into the tree
> > less than a week before -final".  Up to you...
> 
> 
> I agree that it isn't critical. Even the lockdep thing is just a
> warning, and is apparently quite hard to trigger.
> 
> Will you hold on to the patch in your VFS tree, and we can just merge
> it for 3.6?

Sure, no problem...