8311 ZFS_READONLY is a little too strict

Reviewed by: Sanjay Nadkarni <sanjay.nadkarni@nexenta.com>
Reviewed by: Yuri Pankov <yuri.pankov@nexenta.com>
Reviewed by: Andrew Stormont <andyjstormont@gmail.com>
Reviewed by: Matt Ahrens <mahrens@delphix.com>
Reviewed by: John Kennedy <john.kennedy@delphix.com>
Approved by: Prakash Surya <prakash.surya@delphix.com>

11 files changed, 360 insertions, 19 deletions

diff --git a/usr/src/pkg/manifests/system-test-zfstest.mf b/usr/src/pkg/manifests/system-test-zfstest.mf
index 7d2637a592..62cc59b17a 100644
--- a/usr/src/pkg/manifests/system-test-zfstest.mf
+++ b/usr/src/pkg/manifests/system-test-zfstest.mf
@@ -154,6 +154,7 @@ file path=opt/zfs-tests/README mode=0444
 file path=opt/zfs-tests/bin/chg_usr_exec mode=0555
 file path=opt/zfs-tests/bin/devname2devid mode=0555
 file path=opt/zfs-tests/bin/dir_rd_update mode=0555
+file path=opt/zfs-tests/bin/dos_ro mode=0555
 file path=opt/zfs-tests/bin/file_check mode=0555
 file path=opt/zfs-tests/bin/file_trunc mode=0555
 file path=opt/zfs-tests/bin/file_write mode=0555
@@ -186,6 +187,7 @@ file path=opt/zfs-tests/tests/functional/acl/cifs/cifs.kshlib mode=0444
 file path=opt/zfs-tests/tests/functional/acl/cifs/cifs_attr_001_pos mode=0555
 file path=opt/zfs-tests/tests/functional/acl/cifs/cifs_attr_002_pos mode=0555
 file path=opt/zfs-tests/tests/functional/acl/cifs/cifs_attr_003_pos mode=0555
+file path=opt/zfs-tests/tests/functional/acl/cifs/cifs_attr_004_pos mode=0555
 file path=opt/zfs-tests/tests/functional/acl/cifs/cleanup mode=0555
 file path=opt/zfs-tests/tests/functional/acl/cifs/setup mode=0555
 file path=opt/zfs-tests/tests/functional/acl/nontrivial/cleanup mode=0555
diff --git a/usr/src/test/zfs-tests/cmd/dos_ro/Makefile b/usr/src/test/zfs-tests/cmd/dos_ro/Makefile
new file mode 100644
index 0000000000..642de01e59
--- /dev/null
+++ b/usr/src/test/zfs-tests/cmd/dos_ro/Makefile
@@ -0,0 +1,22 @@
+#
+# This file and its contents are supplied under the terms of the
+# Common Development and Distribution License ("CDDL"), version 1.0.
+# You may only use this file in accordance with the terms of version
+# 1.0 of the CDDL.
+#
+# A full copy of the text of the CDDL should have accompanied this
+# source.  A copy of the CDDL is also available via the Internet at
+# http://www.illumos.org/license/CDDL.
+#
+
+#
+# Copyright 2017 Nexenta Systems, Inc.  All rights reserved.
+#
+
+PROG = dos_ro
+
+include $(SRC)/cmd/Makefile.cmd
+
+LDLIBS += -lnvpair
+
+include ../Makefile.subdirs
diff --git a/usr/src/test/zfs-tests/cmd/dos_ro/dos_ro.c b/usr/src/test/zfs-tests/cmd/dos_ro/dos_ro.c
new file mode 100644
index 0000000000..4f0510f72b
--- /dev/null
+++ b/usr/src/test/zfs-tests/cmd/dos_ro/dos_ro.c
@@ -0,0 +1,143 @@
+/*
+ * This file and its contents are supplied under the terms of the
+ * Common Development and Distribution License ("CDDL"), version 1.0.
+ * You may only use this file in accordance with the terms of version
+ * 1.0 of the CDDL.
+ *
+ * A full copy of the text of the CDDL should have accompanied this
+ * source.  A copy of the CDDL is also available via the Internet at
+ * http://www.illumos.org/license/CDDL.
+ */
+
+/*
+ * Copyright 2017 Nexenta Systems, Inc.  All rights reserved.
+ */
+
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include <attr.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <libnvpair.h>
+
+extern const char *__progname;
+
+int vflag = 0;
+
+static int
+dosattr_set_ro(int fildes, const char *fname)
+{
+	nvlist_t	*nvl = NULL;
+	int		err;
+
+	err = nvlist_alloc(&nvl, NV_UNIQUE_NAME, 0);
+	if (err != 0)
+		return (err);
+
+	(void) nvlist_add_boolean_value(nvl, A_READONLY, 1);
+
+	if (fname == NULL) {
+		err = fsetattr(fildes, XATTR_VIEW_READWRITE, nvl);
+	} else {
+		err = setattrat(fildes, XATTR_VIEW_READWRITE, fname, nvl);
+	}
+	if (err < 0) {
+		err = errno;
+		if (vflag > 1) {
+			(void) fprintf(stderr,
+			    "dosattr_set: setattrat (%s), err %d\n",
+			    fname, err);
+		}
+	}
+
+	nvlist_free(nvl);
+
+	return (err);
+}
+
+void
+usage(void)
+{
+	(void) fprintf(stderr, "usage: %s [-v] file\n",
+	    __progname);
+	exit(1);
+}
+
+int
+main(int argc, char **argv)
+{
+	char *fname;
+	int c, fd, n;
+
+	while ((c = getopt(argc, argv, "v")) != -1) {
+		switch (c) {
+		case 'v':
+			vflag++;
+			break;
+		case '?':
+		default:
+			usage();
+			break;
+		}
+	}
+
+	if (optind + 1 != argc)
+		usage();
+	fname = argv[optind];
+
+	fd = open(fname, O_CREAT | O_RDWR, 0644);
+	if (fd < 0) {
+		perror(fname);
+		exit(1);
+	}
+
+	if (vflag)
+		(void) fprintf(stderr, "Write 1 (mode 644)\n");
+	n = write(fd, "mode 644 OK\n", 12);
+	if (n != 12) {
+		(void) fprintf(stderr, "write mode 644, err=%d\n", errno);
+		exit(1);
+	}
+
+	if (vflag)
+		(void) fprintf(stderr, "Chmod 444\n");
+	n = fchmod(fd, 0444);
+	if (n < 0) {
+		(void) fprintf(stderr, "chmod 444, err=%d\n", errno);
+		exit(1);
+	}
+
+	if (vflag)
+		(void) fprintf(stderr, "Write 2 (mode 444)\n");
+	n = write(fd, "mode 444 OK\n", 12);
+	if (n != 12) {
+		(void) fprintf(stderr, "write mode 444, err=%d\n", errno);
+		exit(1);
+	}
+
+	if (vflag)
+		(void) fprintf(stderr, "Set DOS R/O\n");
+	n = dosattr_set_ro(fd, NULL /* fname? */);
+	if (n != 0) {
+		(void) fprintf(stderr, "Set R/O, err=%d\n", n);
+		exit(1);
+	}
+
+	/*
+	 * This fails, but write on an already open handle should succeed
+	 * the same as when we've set the mode to 444 after open.
+	 */
+	if (vflag)
+		(void) fprintf(stderr, "Write 3 (DOS R/O)\n");
+	n = write(fd, "Write DOS RO?\n", 14);
+	if (n != 14) {
+		(void) fprintf(stderr, "write (DOS R/O), err=%d\n", errno);
+		exit(1);
+	}
+
+	return (0);
+}
diff --git a/usr/src/test/zfs-tests/include/commands.cfg b/usr/src/test/zfs-tests/include/commands.cfg
index afc3153e6e..462f0e55e3 100644
--- a/usr/src/test/zfs-tests/include/commands.cfg
+++ b/usr/src/test/zfs-tests/include/commands.cfg
@@ -172,6 +172,7 @@ export SBIN_FILES='fdisk
 export ZFSTEST_FILES='chg_usr_exec
     devname2devid
     dir_rd_update
+    dos_ro
     file_check
     file_trunc
     file_write
diff --git a/usr/src/test/zfs-tests/runfiles/delphix.run b/usr/src/test/zfs-tests/runfiles/delphix.run
index df1b6369b3..aaeb65de38 100644
--- a/usr/src/test/zfs-tests/runfiles/delphix.run
+++ b/usr/src/test/zfs-tests/runfiles/delphix.run
@@ -25,7 +25,8 @@ post = cleanup
 outputdir = /var/tmp/test_results
 
 [/opt/zfs-tests/tests/functional/acl/cifs]
-tests = ['cifs_attr_001_pos', 'cifs_attr_002_pos', 'cifs_attr_003_pos']
+tests = ['cifs_attr_001_pos', 'cifs_attr_002_pos', 'cifs_attr_003_pos',
+    'cifs_attr_004_pos']
 
 [/opt/zfs-tests/tests/functional/acl/nontrivial]
 tests = ['zfs_acl_chmod_001_neg', 'zfs_acl_chmod_002_pos',
diff --git a/usr/src/test/zfs-tests/runfiles/omnios.run b/usr/src/test/zfs-tests/runfiles/omnios.run
index 952ab1503d..df7dbb2d0e 100644
--- a/usr/src/test/zfs-tests/runfiles/omnios.run
+++ b/usr/src/test/zfs-tests/runfiles/omnios.run
@@ -25,7 +25,8 @@ post = cleanup
 outputdir = /var/tmp/test_results
 
 [/opt/zfs-tests/tests/functional/acl/cifs]
-tests = ['cifs_attr_001_pos', 'cifs_attr_002_pos', 'cifs_attr_003_pos']
+tests = ['cifs_attr_001_pos', 'cifs_attr_002_pos', 'cifs_attr_003_pos',
+    'cifs_attr_004_pos']
 
 [/opt/zfs-tests/tests/functional/acl/nontrivial]
 tests = ['zfs_acl_chmod_001_neg', 'zfs_acl_chmod_002_pos',
diff --git a/usr/src/test/zfs-tests/runfiles/openindiana.run b/usr/src/test/zfs-tests/runfiles/openindiana.run
index 407af74e57..81672aac69 100644
--- a/usr/src/test/zfs-tests/runfiles/openindiana.run
+++ b/usr/src/test/zfs-tests/runfiles/openindiana.run
@@ -25,7 +25,8 @@ post = cleanup
 outputdir = /var/tmp/test_results
 
 [/opt/zfs-tests/tests/functional/acl/cifs]
-tests = ['cifs_attr_001_pos', 'cifs_attr_002_pos', 'cifs_attr_003_pos']
+tests = ['cifs_attr_001_pos', 'cifs_attr_002_pos', 'cifs_attr_003_pos',
+    'cifs_attr_004_pos']
 
 [/opt/zfs-tests/tests/functional/acl/nontrivial]
 tests = ['zfs_acl_chmod_001_neg', 'zfs_acl_chmod_002_pos',
diff --git a/usr/src/test/zfs-tests/tests/functional/acl/cifs/cifs_attr_003_pos.ksh b/usr/src/test/zfs-tests/tests/functional/acl/cifs/cifs_attr_003_pos.ksh
index a47ad0277b..09ddc4281a 100644
--- a/usr/src/test/zfs-tests/tests/functional/acl/cifs/cifs_attr_003_pos.ksh
+++ b/usr/src/test/zfs-tests/tests/functional/acl/cifs/cifs_attr_003_pos.ksh
@@ -376,6 +376,7 @@ function unit_writeacl
 function test_readonly
 {
 	typeset object=$1
+	typeset exp
 
 	if [[ -z $object ]]; then
 		log_fail "Object($object) not defined."
@@ -394,14 +395,21 @@ function test_readonly
 
 		log_must set_attribute $object "R"
 
-		unit_writefile $object $user 1
+		# As with mode bits, root can bypass.
+		if [[ "$user" == "root" ]]; then
+			exp=0
+		else
+			exp=1
+		fi
+
+		unit_writefile $object $user $exp
 		unit_writedir $object $user
-		unit_appenddata $object $user 1
+		unit_appenddata $object $user $exp
 
 		if [[ -d $object ]]; then
 			unit_writexattr $object $user
 		else
-			unit_writexattr $object $user 1
+			unit_writexattr $object $user $exp
 		fi
 
 		unit_accesstime $object $user
diff --git a/usr/src/test/zfs-tests/tests/functional/acl/cifs/cifs_attr_004_pos.ksh b/usr/src/test/zfs-tests/tests/functional/acl/cifs/cifs_attr_004_pos.ksh
new file mode 100644
index 0000000000..b5055544b9
--- /dev/null
+++ b/usr/src/test/zfs-tests/tests/functional/acl/cifs/cifs_attr_004_pos.ksh
@@ -0,0 +1,141 @@
+#!/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) 2012 by Delphix. All rights reserved.
+# Copyright 2017 Nexenta Systems, Inc.  All rights reserved.
+#
+
+. $STF_SUITE/tests/functional/acl/acl_common.kshlib
+. $STF_SUITE/tests/functional/acl/cifs/cifs.kshlib
+
+#
+# DESCRIPTION:
+#	Verify the ability to continue writing to a file
+#	after opening the file read/write, and setting
+#	the DOS Readonly flag on that file.
+#
+# STRATEGY:
+#	Run the special program "dos_ro"
+
+verify_runnable "both"
+
+function cleanup
+{
+	if [[ -n $gobject ]]; then
+		destroy_object $gobject
+	fi
+
+	for fs in $TESTPOOL/$TESTFS $TESTPOOL ; do
+		mtpt=$(get_prop mountpoint $fs)
+		log_must rm -rf $mtpt/file.* $mtpt/dir.*
+	done
+
+	[[ -f $TESTFILE ]] && rm $TESTFILE
+}
+
+#
+# Set the special attribute to the given node
+#
+# $1: The given node (file/dir)
+# $2: The special attribute to be set
+#
+function set_attribute
+{
+	typeset object=$1
+	typeset attr=$2
+
+	if [[ -z $attr ]]; then
+		attr="AHRSadimu"
+		if [[ -f $object ]]; then
+			attr="${attr}q"
+		fi
+	fi
+	chmod S+c${attr} $object
+	return $?
+}
+
+#
+# Clear the special attribute to the given node
+#
+# $1: The given node (file/dir)
+# $2: The special attribute to be cleared
+#
+function clear_attribute
+{
+	typeset object=$1
+	typeset attr=$2
+
+	if [[ -z $attr ]]; then
+		if is_global_zone ; then
+			attr="AHRSadimu"
+			if [[ -f $object ]]; then
+				attr="${attr}q"
+			fi
+		else
+			attr="AHRS"
+		fi
+	fi
+
+	chmod S-c${attr} $object
+	return $?
+}
+
+FILES="file.0 file.1"
+FS="$TESTPOOL $TESTPOOL/$TESTFS"
+ATTRS="R"
+
+TESTFILE=/tmp/tfile
+TESTDIR=tdir
+TESTATTR=tattr
+TESTACL=user:$ZFS_ACL_OTHER1:write_data:allow
+TESTMODE=777
+TESTSTR="ZFS test suites"
+
+log_assert "Verify writable open handle still works after " \
+    "setting the DOS Readonly flag on a file."
+log_onexit cleanup
+
+echo "$TESTSTR" > $TESTFILE
+
+typeset gobject
+typeset gattr
+for fs in $FS ; do
+	mtpt=$(get_prop mountpoint $fs)
+	chmod 777 $mtpt
+	for user in root $ZFS_ACL_STAFF1; do
+		log_must set_cur_usr $user
+		for file in $FILES ; do
+			gobject=$mtpt/$file
+			create_object "file" $gobject $ZFS_ACL_CUR_USER
+			log_must dos_ro $gobject
+			destroy_object $gobject
+		done
+	done
+done
+
+log_pass "Writable handle OK after setting DOS R/O flag."
diff --git a/usr/src/uts/common/fs/zfs/zfs_acl.c b/usr/src/uts/common/fs/zfs/zfs_acl.c
index f2ef7e5134..519b4355d5 100644
--- a/usr/src/uts/common/fs/zfs/zfs_acl.c
+++ b/usr/src/uts/common/fs/zfs/zfs_acl.c
@@ -21,7 +21,7 @@
 /*
  * Copyright (c) 2005, 2010, Oracle and/or its affiliates. All rights reserved.
  * Copyright (c) 2013 by Delphix. All rights reserved.
- * Copyright 2014 Nexenta Systems, Inc.  All rights reserved.
+ * Copyright 2017 Nexenta Systems, Inc.  All rights reserved.
  */
 
 #include <sys/types.h>
@@ -2035,13 +2035,11 @@ zfs_zaccess_dataset_check(znode_t *zp, uint32_t v4_mode)
 	}
 
 	/*
-	 * Only check for READONLY on non-directories.
+	 * Intentionally allow ZFS_READONLY through here.
+	 * See zfs_zaccess_common().
 	 */
 	if ((v4_mode & WRITE_MASK_DATA) &&
-	    (((ZTOV(zp)->v_type != VDIR) &&
-	    (zp->z_pflags & (ZFS_READONLY | ZFS_IMMUTABLE))) ||
-	    (ZTOV(zp)->v_type == VDIR &&
-	    (zp->z_pflags & ZFS_IMMUTABLE)))) {
+	    (zp->z_pflags & ZFS_IMMUTABLE)) {
 		return (SET_ERROR(EPERM));
 	}
 
@@ -2253,6 +2251,24 @@ zfs_zaccess_common(znode_t *zp, uint32_t v4_mode, uint32_t *working_mode,
 		return (0);
 	}
 
+	/*
+	 * Note: ZFS_READONLY represents the "DOS R/O" attribute.
+	 * When that flag is set, we should behave as if write access
+	 * were not granted by anything in the ACL.  In particular:
+	 * We _must_ allow writes after opening the file r/w, then
+	 * setting the DOS R/O attribute, and writing some more.
+	 * (Similar to how you can write after fchmod(fd, 0444).)
+	 *
+	 * Therefore ZFS_READONLY is ignored in the dataset check
+	 * above, and checked here as if part of the ACL check.
+	 * Also note: DOS R/O is ignored for directories.
+	 */
+	if ((v4_mode & WRITE_MASK_DATA) &&
+	    (ZTOV(zp)->v_type != VDIR) &&
+	    (zp->z_pflags & ZFS_READONLY)) {
+		return (SET_ERROR(EPERM));
+	}
+
 	return (zfs_zaccess_aces_check(zp, working_mode, B_FALSE, cr));
 }
 
diff --git a/usr/src/uts/common/fs/zfs/zfs_vnops.c b/usr/src/uts/common/fs/zfs/zfs_vnops.c
index 250ed05f1d..b2a5cf51b5 100644
--- a/usr/src/uts/common/fs/zfs/zfs_vnops.c
+++ b/usr/src/uts/common/fs/zfs/zfs_vnops.c
@@ -707,9 +707,11 @@ zfs_write(vnode_t *vp, uio_t *uio, int ioflag, cred_t *cr, caller_context_t *ct)
 	}
 
 	/*
-	 * If immutable or not appending then return EPERM
+	 * If immutable or not appending then return EPERM.
+	 * Intentionally allow ZFS_READONLY through here.
+	 * See zfs_zaccess_common()
 	 */
-	if ((zp->z_pflags & (ZFS_IMMUTABLE | ZFS_READONLY)) ||
+	if ((zp->z_pflags & ZFS_IMMUTABLE) ||
 	    ((zp->z_pflags & ZFS_APPENDONLY) && !(ioflag & FAPPEND) &&
 	    (uio->uio_loffset < zp->z_size))) {
 		ZFS_EXIT(zfsvfs);
@@ -2791,10 +2793,9 @@ zfs_setattr(vnode_t *vp, vattr_t *vap, int flags, cred_t *cr,
 		return (SET_ERROR(EPERM));
 	}
 
-	if ((mask & AT_SIZE) && (zp->z_pflags & ZFS_READONLY)) {
-		ZFS_EXIT(zfsvfs);
-		return (SET_ERROR(EPERM));
-	}
+	/*
+	 * Note: ZFS_READONLY is handled in zfs_zaccess_common.
+	 */
 
 	/*
 	 * Verify timestamps doesn't overflow 32 bits.
@@ -4698,8 +4699,12 @@ zfs_map(vnode_t *vp, offset_t off, struct as *as, caddr_t *addrp,
 	ZFS_ENTER(zfsvfs);
 	ZFS_VERIFY_ZP(zp);
 
+	/*
+	 * Note: ZFS_READONLY is handled in zfs_zaccess_common.
+	 */
+
 	if ((prot & PROT_WRITE) && (zp->z_pflags &
-	    (ZFS_IMMUTABLE | ZFS_READONLY | ZFS_APPENDONLY))) {
+	    (ZFS_IMMUTABLE | ZFS_APPENDONLY))) {
 		ZFS_EXIT(zfsvfs);
 		return (SET_ERROR(EPERM));
 	}