linux-kernel - Re: [patch 3/6] vfs: mountinfo stable peer group id

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <E1Jc1Ln-0003jm-Bt@pomaz-ex.szeredi.hu>
Date:	Wed, 19 Mar 2008 17:41:15 +0100
From:	Miklos Szeredi <miklos@...redi.hu>
To:	viro@...IV.linux.org.uk
CC:	miklos@...redi.hu, akpm@...ux-foundation.org, linuxram@...ibm.com,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [patch 3/6] vfs: mountinfo stable peer group id

> > From: Miklos Szeredi <mszeredi@...e.cz>
> > 
> > Add a stable identifier for shared mounts.
> 
> > +static DEFINE_SPINLOCK(mnt_pgid_lock);
> 
> Um?  Do you ever need to take it outside of vfsmount_lock?
> 

Tried to think this through:

It's always called with namespace_sem, which is enough, no need for a
new lock.  The bigger problem, is that it _is_ called with
vfsmount_lock in one case, which is bad, since the allocation may
sleep.

That is in do_change_type().  But do we really need to hold
vfsmount_lock in that case?  I think not, the propagation tree has no
relevance outside namespace_sem, so that one should be sufficient.

This patch should fix it (untested, just for review).

I have a half mind to throw out the IDR allocation altogether, and
just go with a 64bit counter, some poeple would much prefer that...

Thanks,
Miklos

---
 fs/namespace.c |    2 --
 fs/pnode.c     |   20 ++++++++------------
 2 files changed, 8 insertions(+), 14 deletions(-)

Index: linux/fs/pnode.c
===================================================================
--- linux.orig/fs/pnode.c	2008-03-19 16:39:28.000000000 +0100
+++ linux/fs/pnode.c	2008-03-19 17:26:44.000000000 +0100
@@ -12,7 +12,6 @@
 #include <linux/idr.h>
 #include "pnode.h"
 
-static DEFINE_SPINLOCK(mnt_pgid_lock);
 static DEFINE_IDA(mnt_pgid_ida);
 
 /* return the next shared peer mount of @p */
@@ -37,19 +36,19 @@ static void __set_mnt_shared(struct vfsm
 	mnt->mnt_flags |= MNT_SHARED;
 }
 
+/*
+ * - must hold namespace_sem to protect the mnt_pgid_ida
+ * - must not hold vfsmount_lock, because this function might sleep
+ */
 void set_mnt_shared(struct vfsmount *mnt)
 {
 	int res;
 
- retry:
-	spin_lock(&mnt_pgid_lock);
-	if (IS_MNT_SHARED(mnt)) {
-		spin_unlock(&mnt_pgid_lock);
+	might_sleep();
+	if (IS_MNT_SHARED(mnt))
 		return;
-	}
-
+ retry:
 	res = ida_get_new(&mnt_pgid_ida, &mnt->mnt_pgid);
-	spin_unlock(&mnt_pgid_lock);
 	if (res == -EAGAIN) {
 		if (ida_pre_get(&mnt_pgid_ida, GFP_KERNEL))
 			goto retry;
@@ -159,11 +158,8 @@ static int do_make_slave(struct vfsmount
 			peer_mnt = NULL;
 	}
 
-	if (IS_MNT_SHARED(mnt) && list_empty(&mnt->mnt_share)) {
-		spin_lock(&mnt_pgid_lock);
+	if (IS_MNT_SHARED(mnt) && list_empty(&mnt->mnt_share))
 		ida_remove(&mnt_pgid_ida, mnt->mnt_pgid);
-		spin_unlock(&mnt_pgid_lock);
-	}
 	list_del_init(&mnt->mnt_share);
 
 	if (peer_mnt)
Index: linux/fs/namespace.c
===================================================================
--- linux.orig/fs/namespace.c	2008-03-19 16:39:28.000000000 +0100
+++ linux/fs/namespace.c	2008-03-19 17:23:06.000000000 +0100
@@ -1351,10 +1351,8 @@ static noinline int do_change_type(struc
 		return -EINVAL;
 
 	down_write(&namespace_sem);
-	spin_lock(&vfsmount_lock);
 	for (m = mnt; m; m = (recurse ? next_mnt(m, mnt) : NULL))
 		change_mnt_propagation(m, type);
-	spin_unlock(&vfsmount_lock);
 	up_write(&namespace_sem);
 	return 0;
 }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/