vfsmount_lock / mnt_parent

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: [email protected]
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.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

# 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 [email protected] 1.1522
# namei.c:
# - protect references to vfsmount->mnt_parent with vfsmount_lock
# --------------------------------------------
#

diff -Nru a/fs/namei.c b/fs/namei.c
@@ -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;

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: [email protected]
Phone: 91-80-5044999 Fax: 91-80-5268553
T/L : 9243696

Sorry I mised this thread but ...

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