[PATCH 2/2] vfsmount_lock / mnt_parent

[PATCH 2/2] vfsmount_lock / mnt_parent

[Mike Waychison]

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

The attached patch ensures that we grab vfsmount_lock when grabbing a 
reference to mnt_parent in follow_up and follow_dotdot.

We also don't need to access ->mnt_parent in follow_mount and 
__follow_down to mntput because we already the parent pointer on the stack.


-- 
Mike Waychison
Sun Microsystems, Inc.
1 (650) 352-5299 voice
1 (416) 202-8336 voice
mailto: Michael.Waychison@Sun.COM
http://www.sun.com

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE:  The opinions expressed in this email are held by me,
and may not represent the views of Sun Microsystems, Inc.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[-- Attachment #2: follow_friends_vfsmount_lock.patch --]
[-- Type: text/x-patch, Size: 2026 bytes --]

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1521  -> 1.1522 
#	          fs/namei.c	1.87    -> 1.88   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 04/01/26	michael.waychison@sun.com	1.1522
# namei.c:
#   - protect references to vfsmount->mnt_parent with vfsmount_lock
# --------------------------------------------
#
diff -Nru a/fs/namei.c b/fs/namei.c
--- a/fs/namei.c	Mon Jan 26 21:39:45 2004
+++ b/fs/namei.c	Mon Jan 26 21:39:45 2004
@@ -420,15 +420,15 @@
 {
 	struct vfsmount *parent;
 	struct dentry *mountpoint;
-	spin_lock(&dcache_lock);
+	spin_lock(&vfsmount_lock);
 	parent=(*mnt)->mnt_parent;
 	if (parent == *mnt) {
-		spin_unlock(&dcache_lock);
+		spin_unlock(&vfsmount_lock);
 		return 0;
 	}
 	mntget(parent);
 	mountpoint=dget((*mnt)->mnt_mountpoint);
-	spin_unlock(&dcache_lock);
+	spin_unlock(&vfsmount_lock);
 	dput(*dentry);
 	*dentry = mountpoint;
 	mntput(*mnt);
@@ -446,9 +446,9 @@
 		struct vfsmount *mounted = lookup_mnt(*mnt, *dentry);
 		if (!mounted)
 			break;
+		mntput(*mnt);
 		*mnt = mounted;
 		dput(*dentry);
-		mntput(mounted->mnt_parent);
 		*dentry = dget(mounted->mnt_root);
 		res = 1;
 	}
@@ -464,9 +464,9 @@
 
 	mounted = lookup_mnt(*mnt, *dentry);
 	if (mounted) {
+		mntput(*mnt);
 		*mnt = mounted;
 		dput(*dentry);
-		mntput(mounted->mnt_parent);
 		*dentry = dget(mounted->mnt_root);
 		return 1;
 	}
@@ -498,14 +498,16 @@
 			dput(old);
 			break;
 		}
+		spin_unlock(&dcache_lock);
+		spin_lock(&vfsmount_lock);
 		parent = (*mnt)->mnt_parent;
 		if (parent == *mnt) {
-			spin_unlock(&dcache_lock);
+			spin_unlock(&vfsmount_lock);
 			break;
 		}
 		mntget(parent);
 		*dentry = dget((*mnt)->mnt_mountpoint);
-		spin_unlock(&dcache_lock);
+		spin_unlock(&vfsmount_lock);
 		dput(old);
 		mntput(*mnt);
 		*mnt = parent;

Re: [PATCH 2/2] vfsmount_lock / mnt_parent

[Maneesh Soni]

On Mon, Jan 26, 2004 at 11:11:31PM +0000, Mike Waychison wrote:
> The attached patch ensures that we grab vfsmount_lock when grabbing a 
> reference to mnt_parent in follow_up and follow_dotdot.
> 
> We also don't need to access ->mnt_parent in follow_mount and 
> __follow_down to mntput because we already the parent pointer on the stack.
> 
> 

As pointed by Viro on IRC, there are other places where we access/use 
mnt_parent without any protection. IIUC this needs either vfsmount_lock or the
namespace sem for protection. I did audit such places and hope not missed
anything else.

One such place is in autofs4's is_vfsmnt_tree_busy() routine. I hope Ian still 
has the expire patch which corrects it. Didn't know why this patch never hit
lkml.

IMO do_kern_mount() probably don't need any protection for mnt_parent as it is 
still initializing the vfsmount struct.

The other remaining place is m68k/atari/stram.c:swap_init() where it is
initializing a static vfsmount struct, so again IMO no lock required here.

Thanks,
Maneesh

-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-5044999 Fax: 91-80-5268553
T/L : 9243696

Re: [PATCH 2/2] vfsmount_lock / mnt_parent

[raven]

Sorry I mised this thread but ...

On Tue, 27 Jan 2004, Maneesh Soni wrote:

> On Mon, Jan 26, 2004 at 11:11:31PM +0000, Mike Waychison wrote:
> > The attached patch ensures that we grab vfsmount_lock when grabbing a 
> > reference to mnt_parent in follow_up and follow_dotdot.
> > 
> > We also don't need to access ->mnt_parent in follow_mount and 
> > __follow_down to mntput because we already the parent pointer on the stack.
> > 
> > 
> 
> As pointed by Viro on IRC, there are other places where we access/use 
> mnt_parent without any protection. IIUC this needs either vfsmount_lock or the
> namespace sem for protection. I did audit such places and hope not missed
> anything else.
> 
> One such place is in autofs4's is_vfsmnt_tree_busy() routine. I hope Ian still 
> has the expire patch which corrects it. Didn't know why this patch never hit
> lkml.

The patch has never been posted seperately. It is part of a patch set for 
autofs4, to support the autofs 4.1.0+ daemon, that I sent to Andrew 
Morton. I was hoping that Jeremy would review them and they would make 
their way to Al but perhaps neither has had time to follow up.

I must add that Mike has pointed out that the vfsmount_lock, used by the 
patch is not exported and the patch does not change that.

Ian