skip permission checks for extended attributes #14220

ixhamza · 2022-11-23T20:18:05Z

zfs_zaccess_trivial() calls the generic_permission() to read xattr attributes. This causes deadlock if called from zpl_xattr_set_dir() context as xattr and the dent locks are already held in this scenario. This commit skips the permissions checks for extended attributes since the Linux VFS stack already checks it before passing us the control.

How Has This Been Tested?

Without this patch, setfacl would be deadlocked:

USER_NON_ROOT=$USER
truncate -s 1G /tmp/f1
zpool create tank /tmp/f1 -f
zfs create -o xattr=dir -o acltype=posix -o mountpoint=/data tank/data
echo "TEMP" > /data/file1
chown $USER_NON_ROOT:$USER_NON_ROOT /data/file1
setfacl -m u:$USER_NON_ROOT:rwx /data/file1

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

ixhamza · 2022-11-23T20:19:36Z

@amotin, @freqlabs, @anodos325.

youzhongyang

This is nice! I noticed the same issue one week ago but then was distracted by something else.

The fix looks good to me, I think a test case is worth to be added.

ixhamza · 2022-11-23T22:08:15Z

Thanks, @youzhongyang. @amotin suggested some modifications internally, I am testing those and will also add the test case.

ryao · 2022-11-25T00:45:30Z

module/os/linux/zfs/zpl_xattr.c

@@ -433,14 +433,26 @@ zpl_xattr_get(struct inode *ip, const char *name, void *value, size_t size)
 	cred_t *cr = CRED();
 	fstrans_cookie_t cookie;
 	int error;
+	int xattr_lock = 0;


This could be a boolean_t rather than an int.

behlendorf · 2022-11-28T20:29:40Z

module/os/linux/zfs/zfs_acl.c

@@ -2465,6 +2465,8 @@ zfs_zaccess_trivial(znode_t *zp, uint32_t *working_mode, cred_t *cr,
 		return (unmapped ? SET_ERROR(EPERM) : 0);
 	}

+	rw_enter(&zp->z_gperm_lock, RW_READER);


This lock is only ever taken as a reader and doesn't provide any protection. Should this have been a RW_WRITER? Was the intent here to serial calls to generic_permission or was it to safely set/clear zp->z_lockedxattr . If the latter, couldn't we instead us the zp->z_lock.

Agreed. @amotin also pointed it out and I am working on an alternative solution. The lock was supposed to protect the generic_permission() API as zp->z_lockedxattr is global to each znode. So, the write locks on every permission check do not seem to be a good idea.

ghost · 2022-12-06T15:30:36Z

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;


skip_acl for consistency, spacing around ternary operator

Suggested change

boolean_t skipaclchk = (flag & ATTR_NOACLCHECK) ?B_TRUE :B_FALSE;

boolean_t skip_acl = (flag & ATTR_NOACLCHECK) ? B_TRUE : B_FALSE;

I did it to keep it in one line but will change.

amotin · 2022-12-06T15:45:56Z

module/os/freebsd/zfs/zfs_dir.c

@@ -911,7 +912,7 @@ zfs_get_xattrdir(znode_t *zp, znode_t **xzpp, cred_t *cr, int flags)
 	va.va_mode = S_IFDIR | S_ISVTX | 0777;
 	zfs_fuid_map_ids(zp, cr, &va.va_uid, &va.va_gid);

-	error = zfs_make_xattrdir(zp, &va, xzpp, cr);
+	error = zfs_make_xattrdir(zp, &va, xzpp, cr, B_FALSE);



While FreeBSD does not have this deadlock, could we make the code symmetric? If we do not check additional ACLs on Linux, why should we do it on FreeBSD?

Thanks, I will update this for consistency. The boolean is not being used for FreeBSD but was added to make the build compatible.

Hmm. I see Linux calls zfs_zaccess(), while FreeBSD doesn't at all, so there is nothing really to skip. So you may ignore that comment of mine. But now I wonder why is this there on Linux?

I checked and it was added during the initial commit while porting zfs on Linux 34dc7c2f2#diff-5c59a2f8d51fe5cd3ff56b5d01f01a9c7cd1a28b32a61a8bbc136bbc91e18da9R821. Since Linux kernel checks the permissions within VFS stack already, I am also not very sure if it's something we really need here or maybe it covers some edge case that I am not aware of.
@behlendorf.

@behlendorf Do you think if zfs_zaccess() should be kept in zfs_make_xattrdir() for Linux. FreeBSD does not have it and I am not sure if it's really required for Linux also.

@ixhamza it seems to me this shouldn't be required for Linux either, and the VFS will be performing the needed access checks. If you can do some manual testing to make sure there's nothing obvious we're overlooking I think we can drop this. It's been so long I don't recall why it was originally added with the original port.

@behlendorf Thank you. I did some testing by setting/retrieving extended attributes and manipulating ACLs for different users and VFS already seems to be checking all the required permissions. I have also updated the PR.

module/os/freebsd/zfs/zfs_dir.c

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

zfs_zaccess_trivial() calls the generic_permission() to read xattr attributes. This causes deadlock if called from zpl_xattr_set_dir() context as xattr and the dent locks are already held in this scenario. This commit skips the permissions checks for extended attributes since the Linux VFS stack already checks it before passing us the control. Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>

ghost

That worked out nicely.

zfs_zaccess_trivial() calls the generic_permission() to read xattr attributes. This causes deadlock if called from zpl_xattr_set_dir() context as xattr and the dent locks are already held in this scenario. This commit skips the permissions checks for extended attributes since the Linux VFS stack already checks it before passing us the control. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Youzhong Yang <yyang@mathworks.com> Signed-off-by: Ameer Hamza <ahamza@ixsystems.com> Closes openzfs#14220

youzhongyang suggested changes Nov 23, 2022

View reviewed changes

ryao reviewed Nov 25, 2022

View reviewed changes

ixhamza force-pushed the NAS-118611 branch from 0d0b457 to 555c0cf Compare November 25, 2022 20:25

ixhamza marked this pull request as draft November 25, 2022 21:23

ixhamza force-pushed the NAS-118611 branch from 555c0cf to 739af2d Compare November 27, 2022 17:45

ixhamza marked this pull request as ready for review November 28, 2022 11:50

ghost added the Status: Code Review Needed Ready for review and testing label Nov 28, 2022

ghost approved these changes Nov 28, 2022

View reviewed changes

ixhamza mentioned this pull request Nov 28, 2022

Skip permission checks for POSIX ACL type truenas/zfs#106

Merged

13 tasks

behlendorf reviewed Nov 28, 2022

View reviewed changes

ixhamza marked this pull request as draft November 28, 2022 20:52

ixhamza force-pushed the NAS-118611 branch 3 times, most recently from b27fd4f to 11d64be Compare December 4, 2022 22:02

ixhamza marked this pull request as ready for review December 5, 2022 14:06

ixhamza force-pushed the NAS-118611 branch from 11d64be to 9664631 Compare December 5, 2022 22:16

ghost reviewed Dec 6, 2022

View reviewed changes

amotin reviewed Dec 6, 2022

View reviewed changes

ixhamza force-pushed the NAS-118611 branch from 9664631 to 3d6bd4d Compare December 6, 2022 18:53

ghost reviewed Dec 6, 2022

View reviewed changes

module/os/freebsd/zfs/zfs_dir.c Outdated Show resolved Hide resolved

ixhamza force-pushed the NAS-118611 branch from 3d6bd4d to 4fd4697 Compare December 6, 2022 20:10

ghost approved these changes Dec 7, 2022

View reviewed changes

tests/zfs-tests/tests/functional/acl/posix/posix_004_pos.ksh Show resolved Hide resolved

ixhamza force-pushed the NAS-118611 branch 3 times, most recently from 85357d9 to 2280f07 Compare December 9, 2022 15:19

ixhamza changed the title ~~zfs_zaccess_trivial() causes deadlock for xattr attributes~~ skip permission checks for extended attributes Dec 9, 2022

ixhamza force-pushed the NAS-118611 branch from 2280f07 to 9d00ccd Compare December 9, 2022 22:34

ixhamza force-pushed the NAS-118611 branch from 9d00ccd to 4de1699 Compare December 9, 2022 22:42

behlendorf approved these changes Dec 9, 2022

View reviewed changes

amotin approved these changes Dec 12, 2022

View reviewed changes

ghost approved these changes Dec 12, 2022

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 12, 2022

behlendorf merged commit e378571 into openzfs:master Dec 12, 2022

This was referenced Dec 28, 2022

[2.1.8] skip permission checks for extended attributes #14336

Merged

[truenas/zfs-2.1-release] skip permission checks for extended attributes truenas/zfs#112

Merged

ixhamza deleted the NAS-118611 branch August 2, 2023 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

skip permission checks for extended attributes #14220

skip permission checks for extended attributes #14220

ixhamza commented Nov 23, 2022 •

edited

Loading

ixhamza commented Nov 23, 2022

youzhongyang left a comment

ixhamza commented Nov 23, 2022

ryao Nov 25, 2022

behlendorf Nov 28, 2022

ixhamza Nov 28, 2022

ghost Dec 6, 2022

ixhamza Dec 6, 2022

amotin Dec 6, 2022

ixhamza Dec 6, 2022

amotin Dec 6, 2022

ixhamza Dec 6, 2022 •

edited

Loading

ixhamza Dec 9, 2022

behlendorf Dec 9, 2022

ixhamza Dec 9, 2022

ghost left a comment

	boolean_t skipaclchk = (flag & ATTR_NOACLCHECK) ?B_TRUE :B_FALSE;
	boolean_t skip_acl = (flag & ATTR_NOACLCHECK) ? B_TRUE : B_FALSE;

skip permission checks for extended attributes #14220

skip permission checks for extended attributes #14220

Conversation

ixhamza commented Nov 23, 2022 • edited Loading

How Has This Been Tested?

Types of changes

Checklist:

ixhamza commented Nov 23, 2022

youzhongyang left a comment

Choose a reason for hiding this comment

ixhamza commented Nov 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ixhamza Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

ixhamza commented Nov 23, 2022 •

edited

Loading

ixhamza Dec 6, 2022 •

edited

Loading