Fix regression in POSIX mode behavior

Commit 235a85657 introduced a regression in evaluation of POSIX modes
that require group DENY entries in the internal ZFS ACL. An example
of such a POSX mode is 007. When write_implies_delete_child is set,
then ACE_WRITE_DATA is added to `wanted_dirperms` in prior to calling
zfs_zaccess_common(). This occurs is zfs_zaccess_delete().

Unfortunately, when zfs_zaccess_aces_check hits this particular DENY
ACE, zfs_groupmember() is checked to determine whether access should be
denied, and since zfs_groupmember() always returns B_TRUE on Linux and
so this check is failed, resulting ultimately in EPERM being returned.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Andrew Walker <[email protected]>
Closes #11760 

5 files changed, 154 insertions, 4 deletions

diff --git a/module/zfs/zfs_fuid.c b/module/zfs/zfs_fuid.c
index 015dde481..a90bf5fee 100644
--- a/module/zfs/zfs_fuid.c
+++ b/module/zfs/zfs_fuid.c
@@ -728,7 +728,6 @@ zfs_fuid_info_free(zfs_fuid_info_t *fuidp)
 boolean_t
 zfs_groupmember(zfsvfs_t *zfsvfs, uint64_t id, cred_t *cr)
 {
-#ifdef HAVE_KSID
 	uid_t		gid;
 
 #ifdef illumos
@@ -773,9 +772,6 @@ zfs_groupmember(zfsvfs_t *zfsvfs, uint64_t id, cred_t *cr)
 	 */
 	gid = zfs_fuid_map_id(zfsvfs, id, cr, ZFS_GROUP);
 	return (groupmember(gid, cr));
-#else
-	return (B_TRUE);
-#endif
 }
 
 void
diff --git a/tests/runfiles/common.run b/tests/runfiles/common.run
index 2c5ccaa99..cc2414550 100644
--- a/tests/runfiles/common.run
+++ b/tests/runfiles/common.run
@@ -28,6 +28,10 @@ failsafe = callbacks/zfs_failsafe
 outputdir = /var/tmp/test_results
 tags = ['functional']
 
+[tests/functional/acl/off]
+tests = ['posixmode']
+tags = ['functional', 'acl']
+
 [tests/functional/alloc_class]
 tests = ['alloc_class_001_pos', 'alloc_class_002_neg', 'alloc_class_003_pos',
     'alloc_class_004_pos', 'alloc_class_005_pos', 'alloc_class_006_pos',
diff --git a/tests/runfiles/sanity.run b/tests/runfiles/sanity.run
index 3200261d8..e32cf5f62 100644
--- a/tests/runfiles/sanity.run
+++ b/tests/runfiles/sanity.run
@@ -30,6 +30,10 @@ failsafe = callbacks/zfs_failsafe
 outputdir = /var/tmp/test_results
 tags = ['functional']
 
+[tests/functional/acl/off]
+tests = ['posixmode']
+tags = ['functional', 'acl']
+
 [tests/functional/alloc_class]
 tests = ['alloc_class_003_pos', 'alloc_class_004_pos', 'alloc_class_005_pos',
     'alloc_class_006_pos', 'alloc_class_008_pos', 'alloc_class_010_pos',
diff --git a/tests/zfs-tests/tests/functional/acl/off/Makefile.am b/tests/zfs-tests/tests/functional/acl/off/Makefile.am
index df8adcafe..36aa13dd0 100644
--- a/tests/zfs-tests/tests/functional/acl/off/Makefile.am
+++ b/tests/zfs-tests/tests/functional/acl/off/Makefile.am
@@ -4,6 +4,7 @@ pkgdatadir = $(datadir)/@PACKAGE@/zfs-tests/tests/functional/acl/off
 
 dist_pkgdata_SCRIPTS = \
 	dosmode.ksh \
+	posixmode.ksh \
 	cleanup.ksh \
 	setup.ksh
 
diff --git a/tests/zfs-tests/tests/functional/acl/off/posixmode.ksh b/tests/zfs-tests/tests/functional/acl/off/posixmode.ksh
new file mode 100755
index 000000000..63870caa3
--- /dev/null
+++ b/tests/zfs-tests/tests/functional/acl/off/posixmode.ksh
@@ -0,0 +1,145 @@
+#!/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
+#
+
+#
+# Portions Copyright 2021 iXsystems, Inc.
+#
+
+. $STF_SUITE/include/libtest.shlib
+. $STF_SUITE/tests/functional/acl/acl_common.kshlib
+
+#
+# DESCRIPTION:
+#	Verify that POSIX mode bits function correctly.
+#
+#	These tests are incomplete and will be added to over time.
+#
+#	NOTE: Creating directory entries behaves differently between platforms.
+#	The parent directory's group is used on FreeBSD, while the effective
+#	group is used on Linux.  We chown to the effective group when creating
+#	directories and files in these tests to achieve consistency across all
+#	platforms.
+#
+# STRATEGY:
+#	1. Sanity check the POSIX mode test on tmpfs
+#	2. Test POSIX mode bits on ZFS
+#
+
+verify_runnable "both"
+
+function cleanup
+{
+	umount -f $tmpdir
+	rm -rf $tmpdir $TESTDIR/dir
+}
+
+log_assert "Verify POSIX mode bits function correctly"
+log_onexit cleanup
+
+owner=$ZFS_ACL_STAFF1
+other=$ZFS_ACL_STAFF2
+group=$ZFS_ACL_STAFF_GROUP
+if is_linux; then
+	wheel=root
+else
+	wheel=wheel
+fi
+
+function test_posix_mode # base
+{
+	typeset base=$1
+	typeset dir=$base/dir
+	typeset file=$dir/file
+
+	# dir owned by root
+	log_must mkdir $dir
+	log_must chown :$wheel $dir
+	log_must chmod 007 $dir
+
+	# file owned by root
+	log_must touch $file
+	log_must chown :$wheel $file
+	log_must ls -la $dir
+	log_must rm $file
+
+	log_must touch $file
+	log_must chown :$wheel $file
+	log_must user_run $other rm $file
+
+	# file owned by user
+	log_must user_run $owner touch $file
+	log_must chown :$group $file
+	log_must ls -la $dir
+	log_must user_run $owner rm $file
+
+	log_must user_run $owner touch $file
+	log_must chown :$group $file
+	log_must user_run $other rm $file
+
+	log_must user_run $owner touch $file
+	log_must chown :$group $file
+	log_must rm $file
+
+	log_must rm -rf $dir
+
+	# dir owned by user
+	log_must user_run $owner mkdir $dir
+	log_must chown :$group $dir
+	log_must user_run $owner chmod 007 $dir
+
+	# file owned by root
+	log_must touch $file
+	log_must chown :$wheel $file
+	log_must ls -la $dir
+	log_must rm $file
+
+	log_must touch $file
+	log_must chown :$wheel $file
+	log_mustnot user_run $other rm $file
+	log_must rm $file
+
+	# file owned by user
+	log_mustnot user_run $owner touch $file
+	log_must touch $file
+	log_must chown $owner:$group $file
+	log_must ls -la $dir
+	log_mustnot user_run $owner rm $file
+	log_mustnot user_run $other rm $file
+	log_must rm $file
+
+	log_must rm -rf $dir
+}
+
+# Sanity check on tmpfs first
+tmpdir=$(TMPDIR=$TEST_BASE_DIR mktemp -d)
+log_must mount -t tmpfs tmp $tmpdir
+log_must chmod 777 $tmpdir
+
+test_posix_mode $tmpdir
+
+log_must umount $tmpdir
+log_must rmdir $tmpdir
+
+# Verify ZFS
+test_posix_mode $TESTDIR
+
+log_pass "POSIX mode bits function correctly"