Fix file attributes

This branch contains the following fixes/improvements.

* Fix setting i_flags
* Fix wrong operator in xvattr.h
* Fix fchange macro in zpl_ioctl_setflags()
* Added configure check to use inode_set_flags()
* Added a test case for chattr for better test coverage

Reviewed-by: Tim Chase <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Chunwei Chen <[email protected]>
Closes openzfs#5486 
Closes openzfs#5470 
Closes openzfs#5469

config/kernel-inode-set-flags.m4

@@ -0,0 +1,18 @@
+dnl #
+dnl # 3.15 API change
+dnl # inode_set_flags introduced to set i_flags
+dnl #
+AC_DEFUN([ZFS_AC_KERNEL_INODE_SET_FLAGS], [
+	AC_MSG_CHECKING([whether inode_set_flags() exists])
+	ZFS_LINUX_TRY_COMPILE([
+		#include <linux/fs.h>
+	],[
+		struct inode inode;
+		inode_set_flags(&inode, S_IMMUTABLE, S_IMMUTABLE);
+	],[
+		AC_MSG_RESULT(yes)
+		AC_DEFINE(HAVE_INODE_SET_FLAGS, 1, [inode_set_flags() exists])
+	],[
+		AC_MSG_RESULT(no)
+	])
+])

config/kernel.m4

@@ -56,6 +56,7 @@ AC_DEFUN([ZFS_AC_CONFIG_KERNEL], [
 	ZFS_AC_KERNEL_INODE_OPERATIONS_CHECK_ACL_WITH_FLAGS
 	ZFS_AC_KERNEL_INODE_OPERATIONS_GET_ACL
 	ZFS_AC_KERNEL_INODE_OPERATIONS_SET_ACL
+	ZFS_AC_KERNEL_INODE_SET_FLAGS
 	ZFS_AC_KERNEL_GET_ACL_HANDLE_CACHE
 	ZFS_AC_KERNEL_SHOW_OPTIONS
 	ZFS_AC_KERNEL_FILE_INODE

configure.ac

@@ -180,6 +180,7 @@ AC_CONFIG_FILES([
 	tests/zfs-tests/tests/functional/cachefile/Makefile
 	tests/zfs-tests/tests/functional/casenorm/Makefile
 	tests/zfs-tests/tests/functional/checksum/Makefile
+	tests/zfs-tests/tests/functional/chattr/Makefile
 	tests/zfs-tests/tests/functional/clean_mirror/Makefile
 	tests/zfs-tests/tests/functional/cli_root/Makefile
 	tests/zfs-tests/tests/functional/cli_root/zdb/Makefile

include/sys/xvattr.h

@@ -225,15 +225,15 @@ typedef struct xvattr {
  * of requested attributes (xva_reqattrmap[]).
  */
 #define	XVA_SET_REQ(xvap, attr)					\
-	ASSERT((xvap)->xva_vattr.va_mask | AT_XVATTR);		\
+	ASSERT((xvap)->xva_vattr.va_mask & AT_XVATTR);		\
 	ASSERT((xvap)->xva_magic == XVA_MAGIC);			\
 	(xvap)->xva_reqattrmap[XVA_INDEX(attr)] |= XVA_ATTRBIT(attr)
 /*
  * XVA_CLR_REQ() clears an attribute bit in the proper element in the bitmap
  * of requested attributes (xva_reqattrmap[]).
  */
 #define	XVA_CLR_REQ(xvap, attr)					\
-	ASSERT((xvap)->xva_vattr.va_mask | AT_XVATTR);		\
+	ASSERT((xvap)->xva_vattr.va_mask & AT_XVATTR);		\
 	ASSERT((xvap)->xva_magic == XVA_MAGIC);			\
 	(xvap)->xva_reqattrmap[XVA_INDEX(attr)] &= ~XVA_ATTRBIT(attr)
 
@@ -242,7 +242,7 @@ typedef struct xvattr {
  * of returned attributes (xva_rtnattrmap[]).
  */
 #define	XVA_SET_RTN(xvap, attr)					\
-	ASSERT((xvap)->xva_vattr.va_mask | AT_XVATTR);		\
+	ASSERT((xvap)->xva_vattr.va_mask & AT_XVATTR);		\
 	ASSERT((xvap)->xva_magic == XVA_MAGIC);			\
 	(XVA_RTNATTRMAP(xvap))[XVA_INDEX(attr)] |= XVA_ATTRBIT(attr)
 
@@ -251,7 +251,7 @@ typedef struct xvattr {
  * to see of the corresponding attribute bit is set.  If so, returns non-zero.
  */
 #define	XVA_ISSET_REQ(xvap, attr)					\
-	((((xvap)->xva_vattr.va_mask | AT_XVATTR) &&			\
+	((((xvap)->xva_vattr.va_mask & AT_XVATTR) &&			\
 		((xvap)->xva_magic == XVA_MAGIC) &&			\
 		((xvap)->xva_mapsize > XVA_INDEX(attr))) ?		\
 	((xvap)->xva_reqattrmap[XVA_INDEX(attr)] & XVA_ATTRBIT(attr)) :	0)
@@ -261,7 +261,7 @@ typedef struct xvattr {
  * to see of the corresponding attribute bit is set.  If so, returns non-zero.
  */
 #define	XVA_ISSET_RTN(xvap, attr)					\
-	((((xvap)->xva_vattr.va_mask | AT_XVATTR) &&			\
+	((((xvap)->xva_vattr.va_mask & AT_XVATTR) &&			\
 		((xvap)->xva_magic == XVA_MAGIC) &&			\
 		((xvap)->xva_mapsize > XVA_INDEX(attr))) ?		\
 	((XVA_RTNATTRMAP(xvap))[XVA_INDEX(attr)] & XVA_ATTRBIT(attr)) : 0)

module/zfs/zfs_znode.c

@@ -479,6 +479,34 @@ zfs_inode_set_ops(zfs_sb_t *zsb, struct inode *ip)
 	}
 }
 
+void
+zfs_set_inode_flags(znode_t *zp, struct inode *ip)
+{
+	/*
+	 * Linux and Solaris have different sets of file attributes, so we
+	 * restrict this conversion to the intersection of the two.
+	 */
+#ifdef HAVE_INODE_SET_FLAGS
+	unsigned int flags = 0;
+	if (zp->z_pflags & ZFS_IMMUTABLE)
+		flags |= S_IMMUTABLE;
+	if (zp->z_pflags & ZFS_APPENDONLY)
+		flags |= S_APPEND;
+
+	inode_set_flags(ip, flags, S_IMMUTABLE|S_APPEND);
+#else
+	if (zp->z_pflags & ZFS_IMMUTABLE)
+		ip->i_flags |= S_IMMUTABLE;
+	else
+		ip->i_flags &= ~S_IMMUTABLE;
+
+	if (zp->z_pflags & ZFS_APPENDONLY)
+		ip->i_flags |= S_APPEND;
+	else
+		ip->i_flags &= ~S_APPEND;
+#endif
+}
+
 /*
  * Update the embedded inode given the znode.  We should work toward
  * eliminating this function as soon as possible by removing values
@@ -588,6 +616,7 @@ zfs_znode_alloc(zfs_sb_t *zsb, dmu_buf_t *db, int blksz,
 	set_nlink(ip, (uint32_t)links);
 	zfs_uid_write(ip, z_uid);
 	zfs_gid_write(ip, z_gid);
+	zfs_set_inode_flags(zp, ip);
 
 	/* Cache the xattr parent id */
 	if (zp->z_pflags & ZFS_XATTR)
@@ -918,6 +947,7 @@ void
 zfs_xvattr_set(znode_t *zp, xvattr_t *xvap, dmu_tx_t *tx)
 {
 	xoptattr_t *xoap;
+	boolean_t update_inode = B_FALSE;
 
 	xoap = xva_getxoptattr(xvap);
 	ASSERT(xoap);
@@ -929,7 +959,6 @@ zfs_xvattr_set(znode_t *zp, xvattr_t *xvap, dmu_tx_t *tx)
 		    &times, sizeof (times), tx);
 		XVA_SET_RTN(xvap, XAT_CREATETIME);
 	}
-
 	if (XVA_ISSET_REQ(xvap, XAT_READONLY)) {
 		ZFS_ATTR_SET(zp, ZFS_READONLY, xoap->xoa_readonly,
 		    zp->z_pflags, tx);
@@ -955,11 +984,8 @@ zfs_xvattr_set(znode_t *zp, xvattr_t *xvap, dmu_tx_t *tx)
 		    zp->z_pflags, tx);
 		XVA_SET_RTN(xvap, XAT_IMMUTABLE);
 
-		ZTOI(zp)->i_flags |= S_IMMUTABLE;
-	} else {
-		ZTOI(zp)->i_flags &= ~S_IMMUTABLE;
+		update_inode = B_TRUE;
 	}
-
 	if (XVA_ISSET_REQ(xvap, XAT_NOUNLINK)) {
 		ZFS_ATTR_SET(zp, ZFS_NOUNLINK, xoap->xoa_nounlink,
 		    zp->z_pflags, tx);
@@ -970,12 +996,8 @@ zfs_xvattr_set(znode_t *zp, xvattr_t *xvap, dmu_tx_t *tx)
 		    zp->z_pflags, tx);
 		XVA_SET_RTN(xvap, XAT_APPENDONLY);
 
-		ZTOI(zp)->i_flags |= S_APPEND;
-	} else {
-
-		ZTOI(zp)->i_flags &= ~S_APPEND;
+		update_inode = B_TRUE;
 	}
-
 	if (XVA_ISSET_REQ(xvap, XAT_NODUMP)) {
 		ZFS_ATTR_SET(zp, ZFS_NODUMP, xoap->xoa_nodump,
 		    zp->z_pflags, tx);
@@ -1015,6 +1037,9 @@ zfs_xvattr_set(znode_t *zp, xvattr_t *xvap, dmu_tx_t *tx)
 		    zp->z_pflags, tx);
 		XVA_SET_RTN(xvap, XAT_SPARSE);
 	}
+
+	if (update_inode)
+		zfs_set_inode_flags(zp, ZTOI(zp));
 }
 
 int
@@ -1220,12 +1245,12 @@ zfs_rezget(znode_t *zp)
 
 	zp->z_unlinked = (ZTOI(zp)->i_nlink == 0);
 	set_nlink(ZTOI(zp), (uint32_t)links);
+	zfs_set_inode_flags(zp, ZTOI(zp));
 
 	zp->z_blksz = doi.doi_data_block_size;
 	zp->z_atime_dirty = 0;
 	zfs_inode_update(zp);
 
-
 	zfs_znode_hold_exit(zsb, zh);
 
 	return (0);

module/zfs/zpl_file.c

@@ -737,8 +737,7 @@ zpl_ioctl_getflags(struct file *filp, void __user *arg)
  * is outside of our jurisdiction.
  */
 
-#define	fchange(f0, f1, b0, b1) ((((f0) & (b0)) == (b0)) != \
-	(((b1) & (f1)) == (f1)))
+#define	fchange(f0, f1, b0, b1) (!((f0) & (b0)) != !((f1) & (b1)))
 
 static int
 zpl_ioctl_setflags(struct file *filp, void __user *arg)

tests/runfiles/linux.run

@@ -60,6 +60,9 @@ tests = ['cache_002_pos', 'cache_003_pos', 'cache_004_neg',
 [tests/functional/casenorm]
 tests = ['case_all_values', 'norm_all_values']
 
+[tests/functional/chattr]
+tests = ['chattr_001_pos', 'chattr_002_neg']
+
 [tests/functional/checksum]
 tests = ['run_edonr_test', 'run_sha2_test', 'run_skein_test', 'filetest_001_pos']
 

tests/zfs-tests/tests/functional/Makefile.am

@@ -5,6 +5,7 @@ SUBDIRS = \
 	cache \
 	cachefile \
 	casenorm \
+	chattr \
 	checksum \
 	clean_mirror \
 	cli_root \

tests/zfs-tests/tests/functional/chattr/Makefile.am

@@ -0,0 +1,6 @@
+pkgdatadir = $(datadir)/@PACKAGE@/zfs-tests/tests/functional/chattr
+dist_pkgdata_SCRIPTS = \
+	setup.ksh \
+	cleanup.ksh \
+	chattr_001_pos.ksh \
+	chattr_002_neg.ksh

tests/zfs-tests/tests/functional/chattr/chattr_001_pos.ksh

@@ -0,0 +1,75 @@
+#!/bin/ksh -p
+#
+# CDDL HEADER START
+#
+# The contents of this file are subject to the terms of the
+# Common Development and Distribution License (the "License").
+# You may not use this file except in compliance with the License.
+#
+# You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE
+# or http://www.opensolaris.org/os/licensing.
+# See the License for the specific language governing permissions
+# and limitations under the License.
+#
+# When distributing Covered Code, include this CDDL HEADER in each
+# file and include the License file at usr/src/OPENSOLARIS.LICENSE.
+# If applicable, add the following below this CDDL HEADER, with the
+# fields enclosed by brackets "[]" replaced with your own identifying
+# information: Portions Copyright [yyyy] [name of copyright owner]
+#
+# CDDL HEADER END
+#
+
+#
+# Copyright 2009 Sun Microsystems, Inc.  All rights reserved.
+# Use is subject to license terms.
+#
+
+#
+# Copyright (c) 2013 by Delphix. All rights reserved.
+#
+. $STF_SUITE/include/libtest.shlib
+. $STF_SUITE/tests/functional/userquota/userquota_common.kshlib
+
+#
+#
+# DESCRIPTION:
+#       Check whether chattr works as expected
+#
+#
+# STRATEGY:
+#       1. Create 3 files
+#       2. Use chattr to make them writable, immutable and appendonly
+#       3. Try to write and append to each file
+#
+
+set -A files writable immutable append
+
+function cleanup
+{
+	for i in ${files[*]}; do
+		log_must chattr -ia $TESTDIR/$i
+		log_must rm -f $TESTDIR/$i
+	done
+}
+
+log_onexit cleanup
+
+log_assert "Check whether chattr works as expected"
+
+log_must touch $TESTDIR/writable
+log_must touch $TESTDIR/immutable
+log_must touch $TESTDIR/append
+
+log_must chattr -i $TESTDIR/writable
+log_must chattr +i $TESTDIR/immutable
+log_must chattr +a $TESTDIR/append
+
+log_must echo test > $TESTDIR/writable
+log_must echo test >> $TESTDIR/writable
+log_mustnot echo test > $TESTDIR/immutable
+log_mustnot echo test >> $TESTDIR/immutable
+log_mustnot echo test > $TESTDIR/append
+log_must echo test >> $TESTDIR/append
+
+log_pass "chattr works as expected"

tests/zfs-tests/tests/functional/chattr/chattr_002_neg.ksh

@@ -0,0 +1,81 @@
+#!/bin/ksh -p
+#
+# CDDL HEADER START
+#
+# The contents of this file are subject to the terms of the
+# Common Development and Distribution License (the "License").
+# You may not use this file except in compliance with the License.
+#
+# You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE
+# or http://www.opensolaris.org/os/licensing.
+# See the License for the specific language governing permissions
+# and limitations under the License.
+#
+# When distributing Covered Code, include this CDDL HEADER in each
+# file and include the License file at usr/src/OPENSOLARIS.LICENSE.
+# If applicable, add the following below this CDDL HEADER, with the
+# fields enclosed by brackets "[]" replaced with your own identifying
+# information: Portions Copyright [yyyy] [name of copyright owner]
+#
+# CDDL HEADER END
+#
+
+#
+# Copyright 2009 Sun Microsystems, Inc.  All rights reserved.
+# Use is subject to license terms.
+#
+
+#
+# Copyright (c) 2013 by Delphix. All rights reserved.
+#
+. $STF_SUITE/include/libtest.shlib
+. $STF_SUITE/tests/functional/userquota/userquota_common.kshlib
+
+#
+#
+# DESCRIPTION:
+#       Check whether unprivileged user can chattr
+#
+#
+# STRATEGY:
+#       1. Create 3 files
+#       2. Use chattr to make them writable, immutable and appendonly
+#       3. Try to chattr with unprivileged user
+#
+
+set -A files writable immutable append
+
+function cleanup
+{
+	for i in ${files[*]}; do
+		log_must chattr -ia $TESTDIR/$i
+		log_must rm -f $TESTDIR/$i
+	done
+	log_must $CHMOD 0755 $TESTDIR
+}
+
+log_onexit cleanup
+
+log_assert "Check whether unprivileged user can chattr"
+
+log_must $CHMOD 0777 $TESTDIR
+
+log_must user_run $QUSER1 touch $TESTDIR/writable
+log_must user_run $QUSER1 touch $TESTDIR/immutable
+log_must user_run $QUSER1 touch $TESTDIR/append
+
+log_must chattr -i $TESTDIR/writable
+log_must chattr +i $TESTDIR/immutable
+log_must chattr +a $TESTDIR/append
+
+log_must user_run $QUSER1 chattr -i $TESTDIR/writable
+log_must user_run $QUSER1 chattr -a $TESTDIR/writable
+log_must user_run $QUSER1 chattr +i $TESTDIR/immutable
+log_must user_run $QUSER1 chattr +a $TESTDIR/append
+
+log_mustnot user_run $QUSER1 chattr +i $TESTDIR/writable
+log_mustnot user_run $QUSER1 chattr +a $TESTDIR/writable
+log_mustnot user_run $QUSER1 chattr -i $TESTDIR/immutable
+log_mustnot user_run $QUSER1 chattr -a $TESTDIR/append
+
+log_pass "Unprivileged user cannot chattr as expected"