6748275 Panic in ufs_acl_access due to a race between clearing an ACL and ufs_iaccess()

6 files changed, 111 insertions, 106 deletions

diff --git a/usr/src/uts/common/fs/ufs/ufs_acl.c b/usr/src/uts/common/fs/ufs/ufs_acl.c
index d63d75af1a..94760724f7 100644
--- a/usr/src/uts/common/fs/ufs/ufs_acl.c
+++ b/usr/src/uts/common/fs/ufs/ufs_acl.c
@@ -19,11 +19,9 @@
  * CDDL HEADER END
  */
 /*
- * Copyright 2007 Sun Microsystems, Inc.  All rights reserved.
+ * Copyright 2008 Sun Microsystems, Inc.  All rights reserved.
  * Use is subject to license terms.
  */
-#pragma ident	"%Z%%M%	%I%	%E% SMI"
-
 
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -646,6 +644,7 @@ ufs_acl_access(struct inode *ip, int mode, cred_t *cr)
 	uid_t owner;
 
 	ASSERT(ip->i_ufs_acl != NULL);
+	ASSERT(RW_LOCK_HELD(&ip->i_contents));
 
 	sp = ip->i_ufs_acl;
 
diff --git a/usr/src/uts/common/fs/ufs/ufs_dir.c b/usr/src/uts/common/fs/ufs/ufs_dir.c
index cbf1b4259b..cb4a673c3a 100644
--- a/usr/src/uts/common/fs/ufs/ufs_dir.c
+++ b/usr/src/uts/common/fs/ufs/ufs_dir.c
@@ -19,7 +19,7 @@
  * CDDL HEADER END
  */
 /*
- * Copyright 2007 Sun Microsystems, Inc.  All rights reserved.
+ * Copyright 2008 Sun Microsystems, Inc.  All rights reserved.
  * Use is subject to license terms.
  */
 
@@ -36,9 +36,6 @@
  * contributors.
  */
 
-
-#pragma ident	"%Z%%M%	%I%	%E% SMI"
-
 /*
  * Directory manipulation routines.
  *
@@ -153,8 +150,28 @@ static int dirbadname();
 static int dirmangled();
 
 /*
+ * Check accessibility of directory against inquired mode and type.
+ * Execute access is required to search the directory.
+ * Access for write is interpreted as allowing
+ * deletion of files in the directory.
+ * Note, the reader i_contents lock will be acquired in
+ * ufs_iaccess().
+ */
+int
+ufs_diraccess(struct inode *ip, int mode, struct cred *cr)
+{
+	if (((ip->i_mode & IFMT) != IFDIR) &&
+	    ((ip->i_mode & IFMT) != IFATTRDIR))
+		return (ENOTDIR);
+
+	return (ufs_iaccess(ip, mode, cr, 1));
+}
+
+/*
  * Look for a given name in a directory.  On successful return, *ipp
  * will point to the VN_HELD inode.
+ * The caller is responsible for checking accessibility upfront
+ * via ufs_diraccess().
  */
 int
 ufs_dirlook(
@@ -191,15 +208,6 @@ ufs_dirlook(
 
 	if (dp->i_ufsvfs)
 		ulp = &dp->i_ufsvfs->vfs_ulockfs;
-	/*
-	 * Check accessibility of directory.
-	 */
-	if (((dp->i_mode & IFMT) != IFDIR) &&
-	    ((dp->i_mode & IFMT) != IFATTRDIR))
-		return (ENOTDIR);
-
-	if (err = ufs_iaccess(dp, IEXEC, cr))
-		return (err);
 
 	/*
 	 * Check the directory name lookup cache, first for individual files
@@ -665,6 +673,12 @@ ufs_direnter_cm(
 	ASSERT(namlen);
 
 	/*
+	 * Check accessibility of target directory.
+	 */
+	if (err = ufs_diraccess(tdp, IEXEC, cr))
+		return (err);
+
+	/*
 	 * If name is "." or ".." then if this is a create look it up
 	 * and return EEXIST.
 	 */
@@ -708,21 +722,6 @@ ufs_direnter_cm(
 	}
 
 	/*
-	 * Check accessibility of directory.
-	 */
-	if (((tdp->i_mode & IFMT) != IFDIR) &&
-	    ((tdp->i_mode & IFMT) != IFATTRDIR)) {
-		return (ENOTDIR);
-	}
-
-	/*
-	 * Execute access is required to search the directory.
-	 */
-	if (err = ufs_iaccess(tdp, IEXEC, cr)) {
-		return (err);
-	}
-
-	/*
 	 * Search for the entry. Return VN_HELD tip if found.
 	 */
 	tip = NULL;
@@ -742,7 +741,7 @@ ufs_direnter_cm(
 		 * The entry does not exist. Check write permission in
 		 * directory to see if entry can be created.
 		 */
-		if (err = ufs_iaccess(tdp, IWRITE, cr))
+		if (err = ufs_iaccess(tdp, IWRITE, cr, 0))
 			goto out;
 		/*
 		 * Make new inode and directory entry.
@@ -907,20 +906,12 @@ ufs_direnter_lr(
 		err = ENOENT;
 		goto out2;
 	}
+
 	/*
-	 * Check accessibility of directory.
-	 */
-	if (((tdp->i_mode & IFMT) != IFDIR) &&
-	    (tdp->i_mode & IFMT) != IFATTRDIR) {
-		err = ENOTDIR;
-		goto out2;
-	}
-	/*
-	 * Execute access is required to search the directory.
+	 * Check accessibility of target directory.
 	 */
-	if (err = ufs_iaccess(tdp, IEXEC, cr)) {
+	if (err = ufs_diraccess(tdp, IEXEC, cr))
 		goto out2;
-	}
 
 	/*
 	 * Search for the entry. Return VN_HELD tip if found.
@@ -956,7 +947,7 @@ ufs_direnter_lr(
 		 * The entry does not exist. Check write permission in
 		 * directory to see if entry can be created.
 		 */
-		if (err = ufs_iaccess(tdp, IWRITE, cr))
+		if (err = ufs_iaccess(tdp, IWRITE, cr, 0))
 			goto out;
 		err = ufs_diraddentry(tdp, namep, op, namlen, &slot, sip, sdp,
 		    cr);
@@ -1527,7 +1518,7 @@ retry:
 	 * Must have write permission to rewrite target entry.
 	 * Perform additional checks for sticky directories.
 	 */
-	if ((err = ufs_iaccess(tdp, IWRITE, cr)) != 0 ||
+	if ((err = ufs_iaccess(tdp, IWRITE, cr, 0)) != 0 ||
 	    (err = ufs_sticky_remove_access(tdp, tip, cr)) != 0)
 		goto out;
 
@@ -2468,29 +2459,20 @@ ufs_dirremove(
 	}
 
 	ASSERT(RW_WRITE_HELD(&dp->i_rwlock));
-	/*
-	 * Check accessibility of directory.
-	 */
-retry:
-	if (((dp->i_mode & IFMT) != IFDIR) &&
-	    ((dp->i_mode & IFMT) != IFATTRDIR)) {
-		return (ENOTDIR);
-	}
 
+retry:
 	/*
-	 * Execute access is required to search the directory.
-	 * Access for write is interpreted as allowing
-	 * deletion of files in the directory.
+	 * Check accessibility of directory.
 	 */
-	if (err = ufs_iaccess(dp, IEXEC|IWRITE, cr)) {
+	if (err = ufs_diraccess(dp, IEXEC|IWRITE, cr))
 		return (err);
-	}
 
 	ip = NULL;
 	slot.fbp = NULL;
 	slot.status = FOUND;	/* don't need to look for empty slot */
 	rw_enter(&dp->i_ufsvfs->vfs_dqrwlock, RW_READER);
 	rw_enter(&dp->i_contents, RW_WRITER);
+
 	err = ufs_dircheckforname(dp, namep, namlen, &slot, &ip, cr, 0);
 	if (err)
 		goto out_novfs;
@@ -3219,7 +3201,7 @@ ufs_xattrmkdir(
 	 * Validate permission to create attribute directory
 	 */
 
-	if ((err = ufs_iaccess(tdp, IWRITE, cr)) != 0) {
+	if ((err = ufs_iaccess(tdp, IWRITE, cr, 1)) != 0) {
 		return (err);
 	}
 
diff --git a/usr/src/uts/common/fs/ufs/ufs_inode.c b/usr/src/uts/common/fs/ufs/ufs_inode.c
index 9565079c80..760043a2d9 100644
--- a/usr/src/uts/common/fs/ufs/ufs_inode.c
+++ b/usr/src/uts/common/fs/ufs/ufs_inode.c
@@ -36,9 +36,6 @@
  * contributors.
  */
 
-
-#pragma ident	"%Z%%M%	%I%	%E% SMI"
-
 #include <sys/types.h>
 #include <sys/t_lock.h>
 #include <sys/param.h>
@@ -1520,12 +1517,20 @@ done:
  * is checked.  Depending on the calling user, the appropriate
  * mode bits are selected; privileges to override missing permission
  * bits are checked through secpolicy_vnode_access().
+ * The i_contens lock must be held as reader here to prevent racing with
+ * the acl subsystem removing/setting/changing acls on this inode.
+ * The caller is responsible for indicating whether or not the i_contents
+ * lock needs to be acquired here or if already held.
  */
 int
-ufs_iaccess(void *vip, int mode, struct cred *cr)
+ufs_iaccess(struct inode  *ip, int mode, struct cred *cr, int dolock)
 {
-	struct inode *ip = vip;
 	int shift = 0;
+	int ret = 0;
+
+	if (dolock)
+		rw_enter(&ip->i_contents, RW_READER);
+	ASSERT(RW_LOCK_HELD(&ip->i_contents));
 
 	if (mode & IWRITE) {
 		/*
@@ -1537,24 +1542,23 @@ ufs_iaccess(void *vip, int mode, struct cred *cr)
 			if ((ip->i_mode & IFMT) != IFCHR &&
 			    (ip->i_mode & IFMT) != IFBLK &&
 			    (ip->i_mode & IFMT) != IFIFO) {
-				return (EROFS);
+				ret = EROFS;
+				goto out;
 			}
 		}
 	}
 	/*
-	 * If there is a shadow inode check for the presence of an acl,
-	 * if the acl is there use the ufs_acl_access routine to check
-	 * the acl
+	 * If there is an acl, check the acl and return.
 	 */
-	if (ip->i_ufs_acl && ip->i_ufs_acl->aowner)
-		return (ufs_acl_access(ip, mode, cr));
+	if (ip->i_ufs_acl && ip->i_ufs_acl->aowner) {
+		ret = ufs_acl_access(ip, mode, cr);
+		goto out;
+	}
 
 	/*
-	 * Access check is based on only
-	 * one of owner, group, public.
+	 * Access check is based on only one of owner, group, public.
 	 * If not owner, then check group.
-	 * If not a member of the group, then
-	 * check public access.
+	 * If not a member of the group, then check public access.
 	 */
 	if (crgetuid(cr) != ip->i_uid) {
 		shift += 3;
@@ -1565,10 +1569,14 @@ ufs_iaccess(void *vip, int mode, struct cred *cr)
 	mode &= ~(ip->i_mode << shift);
 
 	if (mode == 0)
-		return (0);
+		goto out;
 
 	/* test missing privilege bits */
-	return (secpolicy_vnode_access(cr, ITOV(ip), ip->i_uid, mode));
+	ret = secpolicy_vnode_access(cr, ITOV(ip), ip->i_uid, mode);
+out:
+	if (dolock)
+		rw_exit(&ip->i_contents);
+	return (ret);
 }
 
 /*
diff --git a/usr/src/uts/common/fs/ufs/ufs_subr.c b/usr/src/uts/common/fs/ufs/ufs_subr.c
index e2d1f6577b..d8d4090ea8 100644
--- a/usr/src/uts/common/fs/ufs/ufs_subr.c
+++ b/usr/src/uts/common/fs/ufs/ufs_subr.c
@@ -19,7 +19,7 @@
  * CDDL HEADER END
  */
 /*
- * Copyright 2007 Sun Microsystems, Inc.  All rights reserved.
+ * Copyright 2008 Sun Microsystems, Inc.  All rights reserved.
  * Use is subject to license terms.
  */
 
@@ -36,9 +36,6 @@
  * contributors.
  */
 
-
-#pragma ident	"%Z%%M%	%I%	%E% SMI"
-
 #include <sys/types.h>
 #include <sys/t_lock.h>
 #include <sys/param.h>
@@ -1444,16 +1441,21 @@ ufs_putsummaryinfo(dev_t dev, struct ufsvfs *ufsvfsp, struct fs *fs)
  * if you are privileged, if you own the entry or if the entry is
  * a plain file and you have write access to that file.
  * Function returns 0 if remove access is granted.
+ * Note, the caller is responsible for holding the i_contents lock
+ * at least as reader on the inquired inode 'ip'.
  */
 int
 ufs_sticky_remove_access(struct inode *dp, struct inode *ip, struct cred *cr)
 {
 	uid_t uid;
+
+	ASSERT(RW_LOCK_HELD(&ip->i_contents));
+
 	if ((dp->i_mode & ISVTX) &&
 	    (uid = crgetuid(cr)) != dp->i_uid &&
 	    uid != ip->i_uid &&
 	    ((ip->i_mode & IFMT) != IFREG ||
-	    ufs_iaccess(ip, IWRITE, cr) != 0))
+	    ufs_iaccess(ip, IWRITE, cr, 0) != 0))
 		return (secpolicy_vnode_remove(cr));
 
 	return (0);
diff --git a/usr/src/uts/common/fs/ufs/ufs_vnops.c b/usr/src/uts/common/fs/ufs/ufs_vnops.c
index d9d894fc0a..b4fd67b528 100644
--- a/usr/src/uts/common/fs/ufs/ufs_vnops.c
+++ b/usr/src/uts/common/fs/ufs/ufs_vnops.c
@@ -179,6 +179,7 @@ static	int ufs_getsecattr(struct vnode *, vsecattr_t *, int, struct cred *,
 		caller_context_t *);
 static	int ufs_setsecattr(struct vnode *, vsecattr_t *, int, struct cred *,
 		caller_context_t *);
+static	int ufs_priv_access(void *, int, struct cred *);
 extern int as_map_locked(struct as *, caddr_t, size_t, int ((*)()), void *);
 
 /*
@@ -2071,6 +2072,19 @@ out:
 	return (err);
 }
 
+/*
+ * Special wrapper to provide a callback for secpolicy_vnode_setattr().
+ * The i_contents lock is already held by the caller and we need to
+ * declare the inode as 'void *' argument.
+ */
+static int
+ufs_priv_access(void *vip, int mode, struct cred *cr)
+{
+	struct inode *ip = vip;
+
+	return (ufs_iaccess(ip, mode, cr, 0));
+}
+
 /*ARGSUSED4*/
 static int
 ufs_setattr(
@@ -2156,7 +2170,7 @@ again:
 			error = EISDIR;
 			goto update_inode;
 		}
-		if (error = ufs_iaccess(ip, IWRITE, cr))
+		if (error = ufs_iaccess(ip, IWRITE, cr, 0))
 			goto update_inode;
 
 		rw_exit(&ip->i_contents);
@@ -2200,12 +2214,9 @@ again:
 	oldva.va_gid = ip->i_gid;
 
 	vap->va_mask &= ~AT_SIZE;
-	/*
-	 * ufs_iaccess is "close enough"; that's because it doesn't
-	 * map the defines.
-	 */
+
 	error = secpolicy_vnode_setattr(cr, vp, vap, &oldva, flags,
-	    ufs_iaccess, ip);
+	    ufs_priv_access, ip);
 	if (error)
 		goto update_inode;
 
@@ -2413,13 +2424,10 @@ ufs_access(struct vnode *vp, int mode, int flags, struct cred *cr,
 	caller_context_t *ct)
 {
 	struct inode *ip = VTOI(vp);
-	int error;
 
 	if (ip->i_ufsvfs == NULL)
 		return (EIO);
 
-	rw_enter(&ip->i_contents, RW_READER);
-
 	/*
 	 * The ufs_iaccess function wants to be called with
 	 * mode bits expressed as "ufs specific" bits.
@@ -2431,11 +2439,7 @@ ufs_access(struct vnode *vp, int mode, int flags, struct cred *cr,
 #if IWRITE != VWRITE || IREAD != VREAD || IEXEC != VEXEC
 #error "ufs_access needs to map Vmodes to Imodes"
 #endif
-	error = ufs_iaccess(ip, mode, cr);
-
-	rw_exit(&ip->i_contents);
-
-	return (error);
+	return (ufs_iaccess(ip, mode, cr, 1));
 }
 
 /* ARGSUSED */
@@ -2754,7 +2758,7 @@ ufs_lookup(struct vnode *dvp, char *nm, struct vnode **vpp,
 			error = ENOENT;
 			goto out;
 		}
-		if ((error = ufs_iaccess(VTOI(vp), IEXEC, cr)) != 0) {
+		if ((error = ufs_iaccess(VTOI(vp), IEXEC, cr, 1)) != 0) {
 			VN_RELE(vp);
 			goto out;
 		}
@@ -2787,7 +2791,7 @@ ufs_lookup(struct vnode *dvp, char *nm, struct vnode **vpp,
 		 * of the directory. We only need the lock if
 		 * we are going to return it.
 		 */
-		if ((error = ufs_iaccess(ip, IEXEC, cr)) == 0) {
+		if ((error = ufs_iaccess(ip, IEXEC, cr, 1)) == 0) {
 			VN_HOLD(dvp);
 			*vpp = dvp;
 		}
@@ -2801,7 +2805,7 @@ ufs_lookup(struct vnode *dvp, char *nm, struct vnode **vpp,
 		/*
 		 * Check accessibility of directory.
 		 */
-		if ((error = ufs_iaccess(ip, IEXEC, cr)) != 0) {
+		if ((error = ufs_iaccess(ip, IEXEC, cr, 1)) != 0) {
 			VN_RELE(vp);
 			goto out;
 		}
@@ -2828,6 +2832,12 @@ ufs_lookup(struct vnode *dvp, char *nm, struct vnode **vpp,
 		}
 
 retry_lookup:
+	/*
+	 * Check accessibility of directory.
+	 */
+	if (error = ufs_diraccess(ip, IEXEC, cr))
+		goto out;
+
 	ufsvfsp = ip->i_ufsvfs;
 	error = ufs_lockfs_begin(ufsvfsp, &ulp, ULOCKFS_LOOKUP_MASK);
 	if (error)
@@ -2949,7 +2959,7 @@ again:
 		}
 		if (xvp) {
 			rw_exit(&ip->i_rwlock);
-			if (error = ufs_iaccess(ip, IEXEC, cr)) {
+			if (error = ufs_iaccess(ip, IEXEC, cr, 1)) {
 				VN_RELE(xvp);
 			} else {
 				error = EEXIST;
@@ -2992,7 +3002,7 @@ again:
 			    (mode & IWRITE))
 				error = EISDIR;
 			else if (mode)
-				error = ufs_iaccess(ip, mode, cr);
+				error = ufs_iaccess(ip, mode, cr, 0);
 			else
 				error = 0;
 		}
@@ -3372,7 +3382,6 @@ retry_rename:
 
 	tdp = VTOI(tdvp);
 
-
 	/*
 	 * We only allow renaming of attributes from ATTRDIR to ATTRDIR.
 	 */
@@ -3382,6 +3391,12 @@ retry_rename:
 	}
 
 	/*
+	 * Check accessibility of directory.
+	 */
+	if (error = ufs_diraccess(sdp, IEXEC, cr))
+		goto unlock;
+
+	/*
 	 * Look up inode of file we're supposed to rename.
 	 */
 	gethrestime(&now);
@@ -3495,7 +3510,7 @@ retry_firstlock:
 	 */
 	rw_enter(&sdp->i_contents, RW_READER);
 	rw_enter(&sip->i_contents, RW_READER);
-	if ((error = ufs_iaccess(sdp, IWRITE, cr)) != 0 ||
+	if ((error = ufs_iaccess(sdp, IWRITE, cr, 0)) != 0 ||
 	    (error = ufs_sticky_remove_access(sdp, sip, cr)) != 0) {
 		rw_exit(&sip->i_contents);
 		rw_exit(&sdp->i_contents);
@@ -3515,7 +3530,7 @@ retry_firstlock:
 	    ((sip->i_mode & IFMT) == IFATTRDIR)) && sdp != tdp) {
 		ino_t	inum;
 
-		if ((error = ufs_iaccess(sip, IWRITE, cr))) {
+		if (error = ufs_iaccess(sip, IWRITE, cr, 0)) {
 			rw_exit(&sip->i_contents);
 			rw_exit(&sdp->i_contents);
 			goto errout;
diff --git a/usr/src/uts/common/sys/fs/ufs_inode.h b/usr/src/uts/common/sys/fs/ufs_inode.h
index 8d43801345..05c2d4e508 100644
--- a/usr/src/uts/common/sys/fs/ufs_inode.h
+++ b/usr/src/uts/common/sys/fs/ufs_inode.h
@@ -39,8 +39,6 @@
 #ifndef	_SYS_FS_UFS_INODE_H
 #define	_SYS_FS_UFS_INODE_H
 
-#pragma ident	"%Z%%M%	%I%	%E% SMI"
-
 #include <sys/isa_defs.h>
 #include <sys/fbuf.h>
 #include <sys/fdbuffer.h>
@@ -838,13 +836,14 @@ extern	void	ufs_iinactive(struct inode *);
 extern	void	ufs_iupdat(struct inode *, int);
 extern	int	ufs_rmidle(struct inode *);
 extern	int	ufs_itrunc(struct inode *, u_offset_t, int, cred_t *);
-extern	int	ufs_iaccess(void *, int, cred_t *);
+extern	int	ufs_iaccess(struct inode *, int, cred_t *, int);
 extern  int	rdip(struct inode *, struct uio *, int, struct cred *);
 extern  int	wrip(struct inode *, struct uio *, int, struct cred *);
 
 extern void	ufs_imark(struct inode *);
 extern void	ufs_itimes_nolock(struct inode *);
 
+extern	int	ufs_diraccess(struct inode *, int, struct cred *);
 extern	int	ufs_dirlook(struct inode *, char *, struct inode **,
     cred_t *, int);
 extern	int	ufs_direnter_cm(struct inode *, char *, enum de_op,