zfs_zaccess_trivial() causes deadlock for xattr attributes

zfs_zaccess_trivial() calls the generic_permission() to read
xattr attributes. This may cause deadlock if xattr and dent
locks were already held. This commit add support to prevent
deadlock if dent_lock() is held from generic_permission()
context.

Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>

include/os/linux/spl/sys/vnode.h

@@ -100,6 +100,7 @@ typedef struct vattr {
 	long		va_fsid;	/* fs id */
 	long		va_nodeid;	/* node # */
 	uint32_t	va_nlink;	/* # links */
+	uint32_t	va_acl_status;	/* hold permission for ACE_ADD_FILE */
 	uint64_t	va_size;	/* file size */
 	inode_timespec_t va_atime;	/* last acc */
 	inode_timespec_t va_mtime;	/* last mod */

include/os/linux/zfs/sys/zfs_dir.h

@@ -68,7 +68,8 @@ extern void zfs_unlinked_drain(zfsvfs_t *zfsvfs);
 extern void zfs_unlinked_drain_stop_wait(zfsvfs_t *zfsvfs);
 extern int zfs_sticky_remove_access(znode_t *, znode_t *, cred_t *cr);
 extern int zfs_get_xattrdir(znode_t *, znode_t **, cred_t *, int);
-extern int zfs_make_xattrdir(znode_t *, vattr_t *, znode_t **, cred_t *);
+extern int zfs_make_xattrdir(znode_t *, vattr_t *, znode_t **, cred_t *,
+    boolean_t skip_acl);
 
 #ifdef	__cplusplus
 }

module/os/linux/zfs/zfs_dir.c

@@ -1098,7 +1098,8 @@ zfs_dirempty(znode_t *dzp)
 }
 
 int
-zfs_make_xattrdir(znode_t *zp, vattr_t *vap, znode_t **xzpp, cred_t *cr)
+zfs_make_xattrdir(znode_t *zp, vattr_t *vap, znode_t **xzpp, cred_t *cr,
+    boolean_t skip_acl)
 {
 	zfsvfs_t *zfsvfs = ZTOZSB(zp);
 	znode_t *xzp;
@@ -1112,7 +1113,7 @@ zfs_make_xattrdir(znode_t *zp, vattr_t *vap, znode_t **xzpp, cred_t *cr)
 
 	*xzpp = NULL;
 
-	if ((error = zfs_zaccess(zp, ACE_WRITE_NAMED_ATTRS, 0, B_FALSE, cr,
+	if ((error = zfs_zaccess(zp, ACE_WRITE_NAMED_ATTRS, 0, skip_acl, cr,
 	    kcred->user_ns)))
 		return (error);
 
@@ -1185,6 +1186,7 @@ zfs_get_xattrdir(znode_t *zp, znode_t **xzpp, cred_t *cr, int flags)
 	zfs_dirlock_t	*dl;
 	vattr_t		va;
 	int		error;
+	boolean_t	skip_acl = (flags & ATTR_NOACLCHECK) ? B_TRUE : B_FALSE;
 top:
 	error = zfs_dirent_lock(&dl, zp, "", &xzp, ZXATTR, NULL, NULL);
 	if (error)
@@ -1221,7 +1223,7 @@ zfs_get_xattrdir(znode_t *zp, znode_t **xzpp, cred_t *cr, int flags)
 	zfs_fuid_map_ids(zp, cr, &va.va_uid, &va.va_gid);
 
 	va.va_dentry = NULL;
-	error = zfs_make_xattrdir(zp, &va, xzpp, cr);
+	error = zfs_make_xattrdir(zp, &va, xzpp, cr, skip_acl);
 	zfs_dirent_unlock(dl);
 
 	if (error == ERESTART) {

module/os/linux/zfs/zfs_vnops_os.c

@@ -555,6 +555,7 @@ zfs_create(znode_t *dzp, char *name, vattr_t *vap, int excl,
 	boolean_t	fuid_dirtied;
 	boolean_t	have_acl = B_FALSE;
 	boolean_t	waited = B_FALSE;
+	boolean_t	skipaclchk = (flag & ATTR_NOACLCHECK) ?B_TRUE :B_FALSE;
 
 	/*
 	 * If we have an ephemeral id, ACL, or XVATTR then
@@ -623,11 +624,18 @@ zfs_create(znode_t *dzp, char *name, vattr_t *vap, int excl,
 		uint64_t txtype;
 		uint64_t projid = ZFS_DEFAULT_PROJID;
 
+		if (skipaclchk == B_TRUE && vap->va_acl_status) {
+			error = vap->va_acl_status;
+			if (have_acl)
+				zfs_acl_ids_free(&acl_ids);
+			goto out;
+		}
+
 		/*
 		 * Create a new file object and update the directory
 		 * to reference it.
 		 */
-		if ((error = zfs_zaccess(dzp, ACE_ADD_FILE, 0, B_FALSE, cr,
+		if ((error = zfs_zaccess(dzp, ACE_ADD_FILE, 0, skipaclchk, cr,
 		    mnt_ns))) {
 			if (have_acl)
 				zfs_acl_ids_free(&acl_ids);

module/os/linux/zfs/zpl_xattr.c

@@ -433,14 +433,25 @@ zpl_xattr_get(struct inode *ip, const char *name, void *value, size_t size)
 	cred_t *cr = CRED();
 	fstrans_cookie_t cookie;
 	int error;
+	boolean_t locked_xattr = B_FALSE;
 
 	crhold(cr);
 	cookie = spl_fstrans_mark();
 	if ((error = zpl_enter_verify_zp(zfsvfs, zp, FTAG)) != 0)
 		goto out;
-	rw_enter(&zp->z_xattr_lock, RW_READER);
+
+	/*
+	 * generic_permission() may end up calling this function
+	 * with z_xattr_lock lock held. We skip reacquiring the
+	 * same lock in order to avoid the deadlock.
+	 */
+	if (!RW_WRITE_HELD(&zp->z_xattr_lock)) {
+		rw_enter(&zp->z_xattr_lock, RW_READER);
+		locked_xattr = B_TRUE;
+	}
 	error = __zpl_xattr_get(ip, name, value, size, cr);
-	rw_exit(&zp->z_xattr_lock);
+	if (locked_xattr == B_TRUE)
+		rw_exit(&zp->z_xattr_lock);
 	zpl_exit(zfsvfs, FTAG);
 out:
 	spl_fstrans_unmark(cookie);
@@ -470,8 +481,13 @@ zpl_xattr_set_dir(struct inode *ip, const char *name, const void *value,
 	if (value != NULL)
 		lookup_flags |= CREATE_XATTR_DIR;
 
-	error = -zfs_lookup(ITOZ(ip), NULL, &dxzp, lookup_flags,
-	    cr, NULL, NULL);
+	error = zfs_zaccess(ITOZ(ip), ACE_WRITE_NAMED_ATTRS, 0, B_FALSE, cr,
+	    kcred->user_ns);
+	if (error)
+		goto out;
+
+	error = -zfs_lookup(ITOZ(ip), NULL, &dxzp, lookup_flags |
+	    ATTR_NOACLCHECK, cr, NULL, NULL);
 	if (error)
 		goto out;
 
@@ -498,8 +514,10 @@ zpl_xattr_set_dir(struct inode *ip, const char *name, const void *value,
 		vap->va_uid = crgetuid(cr);
 		vap->va_gid = crgetgid(cr);
 
+		vap->va_acl_status = zfs_zaccess(dxzp, ACE_ADD_FILE, 0,
+		    B_FALSE, cr, kcred->user_ns);
 		error = -zfs_create(dxzp, (char *)name, vap, 0, 0644, &xzp,
-		    cr, 0, NULL, kcred->user_ns);
+		    cr, ATTR_NOACLCHECK, NULL, kcred->user_ns);
 		if (error)
 			goto out;
 	}

module/zfs/zfs_replay.c

@@ -570,7 +570,8 @@ zfs_replay_create(void *arg1, void *arg2, boolean_t byteswap)
 
 		break;
 	case TX_MKXATTR:
-		error = zfs_make_xattrdir(dzp, &xva.xva_vattr, &zp, kcred);
+		error = zfs_make_xattrdir(dzp, &xva.xva_vattr, &zp, kcred,
+		    B_FALSE);
 		break;
 	case TX_SYMLINK:
 		name = (char *)(lr + 1);

tests/zfs-tests/tests/functional/acl/posix/posix_004_pos.ksh

@@ -44,6 +44,7 @@ log_must setfacl -d -m u:$ZFS_ACL_STAFF1:rwx $TESTDIR
 log_must setfacl -b $TESTDIR
 
 log_must chown $ZFS_ACL_STAFF1 $TESTDIR
+log_must setfacl -d -m u:$ZFS_ACL_STAFF1:rwx $TESTDIR
 log_must chown 0 $TESTDIR
 
 log_pass "chown works with POSIX ACLs"