[2.6.38] Possible deadlock at pivot_root?

[2.6.38] Possible deadlock at pivot_root?

[Tetsuo Handa]

I got a freeze (without lockdep warning) with a small out-of-tree patch
applied on 2.6.38. It seems to me that the deadlock occurred when running
pivot_root(). I guess it is spinning at spin_lock() below.

Is below lock dependency safe?

d_path():

  write_seqlock(&rename_lock);
    br_read_lock(vfsmount_lock);
      spin_lock(&dentry->d_lock);

      spin_unlock(&dentry->d_lock);
    br_read_unlock(vfsmount_lock);
  write_sequnlock(&rename_lock);

pivot_root():

  down_write(&namespace_sem);
    mutex_lock(&old.dentry->d_inode->i_mutex);
      br_write_lock(vfsmount_lock);
        spin_lock(&dentry->d_lock);

        spin_unlock(&dentry->d_lock);
      br_write_unlock(vfsmount_lock);
    mutex_unlock(&old.dentry->d_inode->i_mutex);
  up_write(&namespace_sem);

[  384.711037] 8 locks held by make/6110:
[  384.711037]  #0:  (&ccs_ss){.+.+.+}, at: [<c11bc320>] ccs_path_perm+0x0/0x1c0
[  384.711037]  #1:  (rename_lock){+.+...}, at: [<c10e41c9>] __d_path+0x39/0x80
[  384.711037]  #2:  (vfsmount_lock){++++..}, at: [<c10e8500>] vfsmount_lock_local_lock+0x0/0x60
[  384.711037]  #3:  (&serio->lock){-.-...}, at: [<c12ddb68>] serio_interrupt+0x28/0x80
[  384.711037]  #4:  (&(&dev->event_lock)->rlock){-.-...}, at: [<c12e053f>] input_event+0x3f/0x70
[  384.711037]  #5:  (rcu_read_lock){.+.+..}, at: [<c12dfe30>] input_pass_event+0x0/0x1c0
[  384.711037]  #6:  (sysrq_key_table_lock){-.-...}, at: [<c124c348>] __handle_sysrq+0x18/0x110
[  384.711037]  #7:  (tasklist_lock){.?.?..}, at: [<c10701a6>] debug_show_all_locks+0x36/0x1d0
[  384.711037] 

(I think #3 to #7 are caused by pressing sysrq key.)
Complete log is at http://I-love.SAKURA.ne.jp/tmp/dmesg-2.6.38.txt

Regards.

[2.6.38] Deadlock between rename_lock and vfsmount_lock.

[Tetsuo Handa]

Tetsuo Handa wrote:
> I got a freeze (without lockdep warning) with a small out-of-tree patch
> applied on 2.6.38. It seems to me that the deadlock occurred when running
> pivot_root(). I guess it is spinning at spin_lock() below.

It seems to me that this is a deadlock between rename_lock and vfsmount_lock.

CPU 0: __d_path()                       CPU 1: pivot_root()
write_seqlock(&rename_lock);            br_write_lock(vfsmount_lock);
br_read_lock(vfsmount_lock);            seq = read_seqbegin(&rename_lock);

__d_path() calls prepend_path() with rename_lock lock held for write.

char *__d_path(const struct path *path, struct path *root,
               char *buf, int buflen)
{
        char *res = buf + buflen;
        int error;

        prepend(&res, &buflen, "\0", 1);
        write_seqlock(&rename_lock);
        error = prepend_path(path, root, &res, &buflen);
        write_sequnlock(&rename_lock);

        if (error)
                return ERR_PTR(error);
        return res;
}

prepend_path() tries to hold vfsmount_lock for read.

static int prepend_path(const struct path *path, struct path *root,
                        char **buffer, int *buflen)
{
        struct dentry *dentry = path->dentry;
        struct vfsmount *vfsmnt = path->mnt;
        bool slash = false;
        int error = 0;

        br_read_lock(vfsmount_lock);
        while (dentry != root->dentry || vfsmnt != root->mnt) {
                struct dentry * parent;

                if (dentry == vfsmnt->mnt_root || IS_ROOT(dentry)) {
                        /* Global root? */
                        if (vfsmnt->mnt_parent == vfsmnt) {
                                goto global_root;
                        }
                        dentry = vfsmnt->mnt_mountpoint;
                        vfsmnt = vfsmnt->mnt_parent;
                        continue;
                }
                parent = dentry->d_parent;
                prefetch(parent);
                spin_lock(&dentry->d_lock);
                error = prepend_name(buffer, buflen, &dentry->d_name);
                spin_unlock(&dentry->d_lock);
                if (!error)
                        error = prepend(buffer, buflen, "/", 1);
                if (error)
                        break;

                slash = true;
                dentry = parent;
        }

out:
        if (!error && !slash)
                error = prepend(buffer, buflen, "/", 1);

        br_read_unlock(vfsmount_lock);
        return error;

global_root:
        /*
         * Filesystems needing to implement special "root names"
         * should do so with ->d_dname()
         */
        if (IS_ROOT(dentry) &&
            (dentry->d_name.len != 1 || dentry->d_name.name[0] != '/')) {
                WARN(1, "Root dentry has weird name <%.*s>\n",
                     (int) dentry->d_name.len, dentry->d_name.name);
        }
        root->mnt = vfsmnt;
        root->dentry = dentry;
        goto out;
}

pivot_root() calls is_subdir() with vfsmount_lock lock held for write.

SYSCALL_DEFINE2(pivot_root, const char __user *, new_root,
                const char __user *, put_old)
{
(...snipped...)
        br_write_lock(vfsmount_lock);
        if (tmp != new.mnt) {
                for (;;) {
                        if (tmp->mnt_parent == tmp)
                                goto out3; /* already mounted on put_old */
                        if (tmp->mnt_parent == new.mnt)
                                break;
                        tmp = tmp->mnt_parent;
                }
                if (!is_subdir(tmp->mnt_mountpoint, new.dentry))
                        goto out3;
        } else if (!is_subdir(old.dentry, new.dentry))
                goto out3;
        detach_mnt(new.mnt, &parent_path);
        detach_mnt(root.mnt, &root_parent);
        /* mount old root on put_old */
        attach_mnt(root.mnt, &old);
        /* mount new_root on / */
        attach_mnt(new.mnt, &root_parent);
        touch_mnt_namespace(current->nsproxy->mnt_ns);
        br_write_unlock(vfsmount_lock);
(...snipped...)
}

is_subdir() tries to hold rename_lock for read.

int is_subdir(struct dentry *new_dentry, struct dentry *old_dentry)
{
        int result;
        unsigned seq;

        if (new_dentry == old_dentry)
                return 1;

        do {
                /* for restarting inner loop in case of seq retry */
                seq = read_seqbegin(&rename_lock);
                /*
                 * Need rcu_readlock to protect against the d_parent trashing
                 * due to d_move
                 */
                rcu_read_lock();
                if (d_ancestor(old_dentry, new_dentry))
                        result = 1;
                else
                        result = 0;
                rcu_read_unlock();
        } while (read_seqretry(&rename_lock, seq));

        return result;
}

is_subdir() tries to hold rename_lock lock, but it is held by __d_path().
prepend_path() tries to hold vfsmount_lock lock but is held by pivot_root().

Regards.

Re: [2.6.38] Deadlock between rename_lock and vfsmount_lock.

[Tetsuo Handa]

I established steps to reproduce.

On a 2.6.38 kernel built with CONFIG_SMP=y running on an SMP machine, run

  while :; do newns /sbin/pivot_root /proc/ /proc/sys/; done

on one terminal and run

  while :; do /bin/ls -l /proc/*/exe; done

on another terminal. (The "newns" is a program that unshares the mnt namespace
before execve() using CLONE_NEWNS.)

Below patch releases/reacquires vfsmount_lock when rescheduling is required.
---
 fs/dcache.c             |   15 ++++++++++++++-
 fs/namespace.c          |    6 ++++++
 include/linux/sched.h   |    2 ++
 include/linux/seqlock.h |    4 ++++
 4 files changed, 26 insertions(+), 1 deletion(-)

--- linux-2.6.38.orig/include/linux/sched.h
+++ linux-2.6.38/include/linux/sched.h
@@ -1528,6 +1528,8 @@ struct task_struct {
 		unsigned long memsw_bytes; /* uncharged mem+swap usage */
 	} memcg_batch;
 #endif
+	u8 vfsmntlock_held_for_write;
+	u8 retry_vfsmntlock;
 };
 
 /* Future-safe accessor for struct task_struct's cpus_allowed. */
--- linux-2.6.38.orig/include/linux/seqlock.h
+++ linux-2.6.38/include/linux/seqlock.h
@@ -82,6 +82,8 @@ static inline int write_tryseqlock(seqlo
 	return ret;
 }
 
+extern bool need_to_giveup(const seqlock_t *sl);
+
 /* Start of read calculation -- fetch last complete writer token */
 static __always_inline unsigned read_seqbegin(const seqlock_t *sl)
 {
@@ -92,6 +94,8 @@ repeat:
 	smp_rmb();
 	if (unlikely(ret & 1)) {
 		cpu_relax();
+		if (need_to_giveup(sl))
+			return ret;
 		goto repeat;
 	}
 
--- linux-2.6.38.orig/fs/dcache.c
+++ linux-2.6.38/fs/dcache.c
@@ -2861,6 +2861,7 @@ int is_subdir(struct dentry *new_dentry,
 	if (new_dentry == old_dentry)
 		return 1;
 
+	current->retry_vfsmntlock = 0;
 	do {
 		/* for restarting inner loop in case of seq retry */
 		seq = read_seqbegin(&rename_lock);
@@ -2874,11 +2875,23 @@ int is_subdir(struct dentry *new_dentry,
 		else
 			result = 0;
 		rcu_read_unlock();
-	} while (read_seqretry(&rename_lock, seq));
+	} while (read_seqretry(&rename_lock, seq) && !current->retry_vfsmntlock);
 
 	return result;
 }
 
+bool need_to_giveup(const seqlock_t *sl)
+{
+	if (sl == &rename_lock && current->vfsmntlock_held_for_write &&
+	    need_resched()) {
+		current->retry_vfsmntlock = true;
+		WARN(1, "Requesting for releasing vfsmount_lock");
+		return true;
+	}
+	return false;
+}
+EXPORT_SYMBOL(need_to_giveup);
+
 int path_is_under(struct path *path1, struct path *path2)
 {
 	struct vfsmount *mnt = path1->mnt;
--- linux-2.6.38.orig/fs/namespace.c
+++ linux-2.6.38/fs/namespace.c
@@ -2515,7 +2515,9 @@ SYSCALL_DEFINE2(pivot_root, const char _
 		goto out2; /* not attached */
 	/* make sure we can reach put_old from new_root */
 	tmp = old.mnt;
+retry_vfsmnt_lock:
 	br_write_lock(vfsmount_lock);
+	current->vfsmntlock_held_for_write = 1;
 	if (tmp != new.mnt) {
 		for (;;) {
 			if (tmp->mnt_parent == tmp)
@@ -2536,6 +2538,7 @@ SYSCALL_DEFINE2(pivot_root, const char _
 	attach_mnt(new.mnt, &root_parent);
 	touch_mnt_namespace(current->nsproxy->mnt_ns);
 	br_write_unlock(vfsmount_lock);
+	current->vfsmntlock_held_for_write = 0;
 	chroot_fs_refs(&root, &new);
 
 	error = 0;
@@ -2552,6 +2555,9 @@ out0:
 	return error;
 out3:
 	br_write_unlock(vfsmount_lock);
+	current->vfsmntlock_held_for_write = 0;
+	if (current->retry_vfsmntlock)
+		goto retry_vfsmnt_lock;
 	goto out2;
 }
 

You will see warning like below.

[  330.216989] ------------[ cut here ]------------
[  330.218601] WARNING: at fs/dcache.c:2888 need_to_giveup+0x52/0x60()
[  330.219509] Hardware name: VMware Virtual Platform
[  330.220644] Requesting for releasing vfsmount_lock
[  330.221426] Modules linked in: ipv6 video sbs sbshc battery ac floppy rtc_cmos button rtc_core serio_raw rtc_lib pcnet32 mii i2c_piix4 i2c_core mptspi mptscsih mptbase scsi_transport_spi ext3 jbd mbcache
[  330.230432] Pid: 8936, comm: pivot_root Tainted: G        W   2.6.38 #2
[  330.232003] Call Trace:
[  330.233153]  [<c04d2bb2>] ? need_to_giveup+0x52/0x60
[  330.234420]  [<c043f6bc>] ? warn_slowpath_common+0x7c/0xa0
[  330.235418]  [<c04d2bb2>] ? need_to_giveup+0x52/0x60
[  330.236417]  [<c043f75e>] ? warn_slowpath_fmt+0x2e/0x30
[  330.237459]  [<c04d2bb2>] ? need_to_giveup+0x52/0x60
[  330.238492]  [<c04d2d37>] ? is_subdir+0xb7/0xe0
[  330.239429]  [<c04d2cbd>] ? is_subdir+0x3d/0xe0
[  330.240420]  [<c04da8eb>] ? sys_pivot_root+0x22b/0x2a0
[  330.241775]  [<c06c519c>] ? restore_all_notrace+0x0/0x18
[  330.243005]  [<c04289a0>] ? do_page_fault+0x0/0x420
[  330.244414]  [<c0402c50>] ? sysenter_do_call+0x12/0x36
[  330.245416] ---[ end trace 5017bf44a4ddf1e2 ]---

3.6.37 seems to be OK because __d_path() was protected by dcache_lock.

Regards.

Re: [2.6.38] Deadlock between rename_lock and vfsmount_lock.

[Al Viro]

On Fri, Mar 18, 2011 at 07:59:08PM +0900, Tetsuo Handa wrote:
> I established steps to reproduce.
> 
> On a 2.6.38 kernel built with CONFIG_SMP=y running on an SMP machine, run
> 
>   while :; do newns /sbin/pivot_root /proc/ /proc/sys/; done
> 
> on one terminal and run
> 
>   while :; do /bin/ls -l /proc/*/exe; done
> 
> on another terminal. (The "newns" is a program that unshares the mnt namespace
> before execve() using CLONE_NEWNS.)
> 
> Below patch releases/reacquires vfsmount_lock when rescheduling is required.
> ---
>  fs/dcache.c             |   15 ++++++++++++++-
>  fs/namespace.c          |    6 ++++++
>  include/linux/sched.h   |    2 ++
>  include/linux/seqlock.h |    4 ++++
>  4 files changed, 26 insertions(+), 1 deletion(-)

That's incredibly ugly.  I agree that the deadlock exists and needs to be
dealt with, but not that way.  _Strongly_ NAKed.  I'll see what I can come
up with, but that variant is not an option.

Re: [2.6.38] Deadlock between rename_lock and vfsmount_lock.

[Tetsuo Handa]

Al Viro wrote:
> That's incredibly ugly.  I agree that the deadlock exists and needs to be
> dealt with, but not that way.  _Strongly_ NAKed.  I'll see what I can come
> up with, but that variant is not an option.

Of course I didn't intend the patch for commit.
I showed the patch for identifying the location.

Re: [2.6.38] Deadlock between rename_lock and vfsmount_lock.

[Al Viro]

On Fri, Mar 18, 2011 at 11:06:03AM +0000, Al Viro wrote:

> That's incredibly ugly.  I agree that the deadlock exists and needs to be
> dealt with, but not that way.  _Strongly_ NAKed.  I'll see what I can come
> up with, but that variant is not an option.

Actually, why do we hold vfsmount_lock over that loop at all?  We already
hold namespace_sem, so ->mnt_parent is protected...

FWIW, there's a thing I really don't like about that area - I would really
prefer to have namespace_sem nest _inside_ i_mutex and have no fs operations
whatsoever done under it.  Note that we already take care (mostly) to
keep actual fs shutdowns outside of it.

The real obstacle is follow_down() we do under namespace_sem on several
paths; otherwise we'd be able to grab i_mutex first and be done with that.
Moreover, we have extra ugliness in follow_down(); I really wonder
what is the only instance trying to do here.

Look: we pass mounting_here == true to autofs4_d_manage().  It sees the
flag, then checks if we have DCACHE_MOUNTED set and returns either -EISDIR
or 0.  -EISDIR leads to follow_down() returning 0 immediately.  0 leads
to trying to cross the mountpoint.  First of all, we could as well return
0 regardless - if DCACHE_MOUNTED is not set, follow_down() will see that
and return 0 since there's nothing left to do.

What's more, we have a funny situation here - we might as well replace
                if (managed & DCACHE_MANAGE_TRANSIT) {
with
                if (!mounting_here && managed & DCACHE_MANAGE_TRANSIT) {
in follow_down() and lose that argument of ->d_manage().

So let's do this:

lock_mount(path, follow)
retry:
	lock path->dentry->d_inode
	if unlikely(can't mount)
		unlock
		fail
	grab namespace_sem
	if !follow || likely(lookup_mnt() returns NULL)
		we are done
	drop namespace_sem
	unlock
	drop path
	replace it with result of lookup_mnt() (and its ->mnt_root)
	goto retry;

and use that (local to fs/namespace.c) in do_add_mount()/do_move_mount()/
do_loopback() (with follow = 1) and pivot_root() (follow = 0).  BTW,
the lack of following in do_loopback() looks like a bug...
Also in do_loopback(): take release_mounts() after unlocking everything.
graft_tree() doesn't grab i_mutex - callers take it.
follow_down() loses mounting_here argument and so does ->d_manage().
cant_mount() calls are all merged into one in lock_mount().

Comments?

Re: [2.6.38] Deadlock between rename_lock and vfsmount_lock.

[Al Viro]

On Fri, Mar 18, 2011 at 12:07:48PM +0000, Al Viro wrote:

> Actually, why do we hold vfsmount_lock over that loop at all?  We already
> hold namespace_sem, so ->mnt_parent is protected...

Argh...  No, it isn't.  We flip it to final (mnt->mnt_parent = mnt)
outside of namespace_sem in release_mounts().  HOWEVER, by that point
we have already cleared ->mnt_ns - under namespace_sem.

So what we need is check_mnt(new.mnt) in addition to test for root.mnt
we already have there.  That'll be enough.

Re: [2.6.38] Deadlock between rename_lock and vfsmount_lock.

[Al Viro]

On Fri, Mar 18, 2011 at 12:07:48PM +0000, Al Viro wrote:
> lock_mount(path, follow)
> retry:
> 	lock path->dentry->d_inode
> 	if unlikely(can't mount)
> 		unlock
> 		fail
> 	grab namespace_sem
> 	if !follow || likely(lookup_mnt() returns NULL)
> 		we are done
> 	drop namespace_sem
> 	unlock
> 	drop path
> 	replace it with result of lookup_mnt() (and its ->mnt_root)
> 	goto retry;
> 
> and use that (local to fs/namespace.c) in do_add_mount()/do_move_mount()/
> do_loopback() (with follow = 1) and pivot_root() (follow = 0).  BTW,
> the lack of following in do_loopback() looks like a bug...

So's the lack of following on old in pivot_root, actually.  IOW, follow
argument is always 1...

Re: [2.6.38] Deadlock between rename_lock and vfsmount_lock.

[Al Viro]

	See #untested in vfs-2.6.git.  I think it should do it, but
it *is* completely untested at the moment.

Re: [2.6.38] Deadlock between rename_lock and vfsmount_lock.

[Tetsuo Handa]

Al Viro wrote:
> 	See #untested in vfs-2.6.git.  I think it should do it, but
> it *is* completely untested at the moment.

As far as I can test, I did not find any problems. Thank you.

Re: [2.6.38] Deadlock between rename_lock and vfsmount_lock.

[Greg KH]

On Sat, Mar 19, 2011 at 11:39:19AM +0900, Tetsuo Handa wrote:
> Al Viro wrote:
> > 	See #untested in vfs-2.6.git.  I think it should do it, but
> > it *is* completely untested at the moment.
> 
> As far as I can test, I did not find any problems. Thank you.

Did this fix go to Linus?  If so, any pointers to the git commit id?
I'm guessing I will want this in the .38-stable tree?

thanks,

greg k-h

Re: [2.6.38] Deadlock between rename_lock and vfsmount_lock.

[Tetsuo Handa]

Greg KH wrote:
> On Sat, Mar 19, 2011 at 11:39:19AM +0900, Tetsuo Handa wrote:
> > Al Viro wrote:
> > > 	See #untested in vfs-2.6.git.  I think it should do it, but
> > > it *is* completely untested at the moment.
> > 
> > As far as I can test, I did not find any problems. Thank you.
> 
> Did this fix go to Linus?  If so, any pointers to the git commit id?

It is commit 27cb1572e3e6bb1f8cf6bb3d74c914a87b131792 "fix deadlock in pivot_root()"
in linux-2.6.git .

> I'm guessing I will want this in the .38-stable tree?

Yes. We are waiting for tests by Al.

Re: [2.6.38] Deadlock between rename_lock and vfsmount_lock.

[Greg KH]

On Thu, Mar 24, 2011 at 09:04:28AM +0900, Tetsuo Handa wrote:
> Greg KH wrote:
> > On Sat, Mar 19, 2011 at 11:39:19AM +0900, Tetsuo Handa wrote:
> > > Al Viro wrote:
> > > > 	See #untested in vfs-2.6.git.  I think it should do it, but
> > > > it *is* completely untested at the moment.
> > > 
> > > As far as I can test, I did not find any problems. Thank you.
> > 
> > Did this fix go to Linus?  If so, any pointers to the git commit id?
> 
> It is commit 27cb1572e3e6bb1f8cf6bb3d74c914a87b131792 "fix deadlock in pivot_root()"
> in linux-2.6.git .
> 
> > I'm guessing I will want this in the .38-stable tree?
> 
> Yes. We are waiting for tests by Al.

Great, thanks for letting me know, I'll watch out for the above patch to
hit Linus's tree.

greg k-h