Cross-platform acltype #10520

ghost · 2020-06-30T22:52:29Z

Motivation and Context

The acltype property is currently hidden on FreeBSD and does not
reflect the NFSv4 style ZFS ACLs used on the platform. This makes it
difficult to observe that a pool imported from FreeBSD on Linux has a
different type of ACL that is being ignored, and vice versa.

Description

Add an nfsv4 acltype and expose the property on FreeBSD.

Setting acltype to an unhandled style is treated the same as setting
it to off. The ACLs will not be removed, but they will be ignored.

How Has This Been Tested?

Internal CI with a suite of NFSv4 ACL tests pass. Porting a tests suite to ZTS is still a work in progress.

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)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the ZFS on Linux 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.

ghost · 2020-06-30T23:00:16Z

Setting acltype to an unhandled style is treated the same as setting it to off. The ACLs will not be removed, but they will be ignored.

This is my intent, but I am surely missing this mark at least on FreeBSD. On Linux it was easy to set z_acl_type = OFF for NFSV4, but since there was no such property on FreeBSD yet, I had no such mechanism in place to do the same there. The VFS code is very similar in some places between FreeBSD and Linux, but very different in others, so it is not yet clear to me where to place checks for z_acl_type on FreeBSD.

ghost · 2020-06-30T23:02:42Z

I should mention this is based on the work in #9709

ghost · 2020-06-30T23:08:52Z

Looking at how the mount options for ACLs are displayed on Linux, I’m not sure if they should match what is documented for ext4, as in acl|noacl since posix|off might be confusing without the context to hint at it being an ACL option, or just leave it as posixacl|noacl.
It’s not clear to me as primarily a FreeBSD developer whether changing these affects anything other than how it is displayed (the function is __zpl_show_options).

I have left the mount options as posixacl|noacl, but if that should change just let me know :)

man/man8/zfsprops.8

module/zcommon/zfs_prop.c

behlendorf · 2020-06-30T23:19:09Z

I have left the mount options as posixacl|noacl, but if that should change just let me know

We should leave them unchanged to avoid any user/kernel compatibility issues. Plus this really isn't user visible unless they're already bypassing the zfs mount command.

codecov · 2020-07-01T05:32:46Z

Codecov Report

Merging #10520 into master will decrease coverage by 14.88%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master   #10520       +/-   ##
===========================================
- Coverage   79.65%   64.76%   -14.89%     
===========================================
  Files         396      312       -84     
  Lines      125475   107453    -18022     
===========================================
- Hits        99949    69596    -30353     
- Misses      25526    37857    +12331

Flag	Coverage Δ
#kernel	`?`
#user	`64.76% <100.00%> (-0.39%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
module/zcommon/zfs_prop.c	`95.75% <100.00%> (-3.66%)`	⬇️
module/zfs/objlist.c	`0.00% <0.00%> (-100.00%)`	⬇️
module/zfs/pathname.c	`0.00% <0.00%> (-100.00%)`	⬇️
include/sys/dmu_redact.h	`0.00% <0.00%> (-100.00%)`	⬇️
include/sys/dmu_traverse.h	`0.00% <0.00%> (-100.00%)`	⬇️
module/lua/ltablib.c	`2.34% <0.00%> (-95.32%)`	⬇️
module/zfs/bqueue.c	`0.00% <0.00%> (-94.45%)`	⬇️
module/zfs/zfs_rlock.c	`0.00% <0.00%> (-94.21%)`	⬇️
module/zcommon/zfs_deleg.c	`0.00% <0.00%> (-92.46%)`	⬇️
module/zfs/dmu_diff.c	`0.00% <0.00%> (-87.88%)`	⬇️
... and 261 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36482bf...9952570. Read the comment docs.

man/man8/zfsprops.8

ghost · 2020-07-14T18:41:12Z

@trasz could you take a look at this? I need some help figuring out where to check acltype on FreeBSD / how to skip ACL checks if not nfsv4.

trasz · 2020-07-14T19:39:02Z

@trasz could you take a look at this? I need some help figuring out where to check acltype on FreeBSD / how to skip ACL checks if not nfsv4.

Assuming I understand the question correctly, and the code works the way it used to, there's nothing to skip: in FreeBSD it's the filesystem that checks permissions, not VFS, and zfs_zaccess() only looks at NFSv4 ACLs. Regarding ACL access (setting and retrieval), zfs_freebsd_setacl() and zfs_freebsd_getacl() already check the ACL type.

ghost · 2020-07-14T20:26:55Z

@trasz Thanks for helping me wrap my head around this.

To be clear, I'm referring to z_acl_type, which is new to FreeBSD in this PR. It can be set to one of off | nfsv4 | posix and we should treat either of off | posix as there being no ACL.

Assuming we're on the same page, I can see how we could ignore the property if there isn't an existing ACL, though it might be useful to force aclmode to discard. But if there was an NFSv4 style ACL and the property changes to off (or posix), how can we ignore the ACL then?

ghfields · 2020-07-23T15:03:12Z

I see this is still in "draft" stage, but should this be added to the OpenZFS 2.0 project list? https://github.com/openzfs/zfs/projects/25

anodos325 · 2020-08-05T17:58:27Z

Let's make sure that we're modifying results of zfs_pathconf() appropriately based on ACL branding. This move us closer to just having both ACL types supported on FreeBSD.

ghost · 2020-09-24T15:35:58Z

Rebased

ghost · 2020-09-25T22:54:51Z

@pbhenson would you mind reviewing this PR please? It incorporates parts of your work to bring NFSv4 ACLs to Linux, though the intent here is only to extend the acltype property for use on FreeBSD for now.

pbhenson · 2020-09-30T01:08:35Z

@pbhenson would you mind reviewing this PR please?

Sure; I can't say too much about the FreeBSD internals :), but the general changes look fine to me.

I can't believe it's already been like 4 months since I worked on this :(, my day job is at a university so it's been pretty crazy with the remote learning, and budget wise I've been a bit more focused on the paid side gigs . Once you get this merged I'll have to get back to it; it's all done in my head ;).

module/os/freebsd/zfs/zfs_vnops.c

ghost · 2020-10-08T18:09:55Z

Rebased
Addressed feedback

The acltype property is currently hidden on FreeBSD and does not reflect the NFSv4 style ZFS ACLs used on the platform. This makes it difficult to observe that a pool imported from FreeBSD on Linux has a different type of ACL that is being ignored, and vice versa. Add an nfsv4 acltype and expose the property on FreeBSD. Make the default acltype nfsv4 on FreeBSD. Setting acltype to an unhandled style is treated the same as setting it to off. The ACLs will not be removed, but they will be ignored. Signed-off-by: Ryan Moeller <[email protected]>

ghost · 2020-10-08T21:20:02Z

cstyle 🤦

The acltype property is currently hidden on FreeBSD and does not reflect the NFSv4 style ZFS ACLs used on the platform. This makes it difficult to observe that a pool imported from FreeBSD on Linux has a different type of ACL that is being ignored, and vice versa. Add an nfsv4 acltype and expose the property on FreeBSD. Make the default acltype nfsv4 on FreeBSD. Setting acltype to an unhanded style is treated the same as setting it to off. The ACLs will not be removed, but they will be ignored. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes #10520

The acltype property is currently hidden on FreeBSD and does not reflect the NFSv4 style ZFS ACLs used on the platform. This makes it difficult to observe that a pool imported from FreeBSD on Linux has a different type of ACL that is being ignored, and vice versa. Add an nfsv4 acltype and expose the property on FreeBSD. Make the default acltype nfsv4 on FreeBSD. Setting acltype to an unhanded style is treated the same as setting it to off. The ACLs will not be removed, but they will be ignored. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#10520

ghost added the Status: Design Review Needed Architecture or design is under discussion label Jun 30, 2020

ghost commented Jun 30, 2020

View reviewed changes

man/man8/zfsprops.8 Outdated Show resolved Hide resolved

ghost commented Jun 30, 2020

View reviewed changes

module/zcommon/zfs_prop.c Outdated Show resolved Hide resolved

ghost force-pushed the acltypes branch from 88f2065 to ced6ce6 Compare July 1, 2020 21:51

ghost commented Jul 1, 2020

View reviewed changes

man/man8/zfsprops.8 Outdated Show resolved Hide resolved

ghost force-pushed the acltypes branch from ced6ce6 to 75b0b1f Compare August 10, 2020 15:07

ghost force-pushed the acltypes branch from 75b0b1f to 1447e85 Compare September 1, 2020 15:30

ghost force-pushed the acltypes branch from 1447e85 to 2960feb Compare September 24, 2020 15:13

ghost marked this pull request as ready for review September 25, 2020 22:48

ghost added Status: Code Review Needed Ready for review and testing and removed Status: Design Review Needed Architecture or design is under discussion labels Sep 30, 2020

ghost requested a review from behlendorf October 8, 2020 16:51

behlendorf approved these changes Oct 8, 2020

View reviewed changes

module/os/freebsd/zfs/zfs_vnops.c Show resolved Hide resolved

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Oct 8, 2020

ghost force-pushed the acltypes branch from 2960feb to f096f27 Compare October 8, 2020 18:09

ghost force-pushed the acltypes branch from f096f27 to 9952570 Compare October 8, 2020 21:19

behlendorf merged commit 485b50b into openzfs:master Oct 14, 2020

ghost deleted the acltypes branch October 14, 2020 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cross-platform acltype #10520

Cross-platform acltype #10520

ghost commented Jun 30, 2020 •

edited by ghost

Loading

ghost commented Jun 30, 2020

ghost commented Jun 30, 2020

ghost commented Jun 30, 2020

behlendorf commented Jun 30, 2020

codecov bot commented Jul 1, 2020 •

edited

Loading

ghost commented Jul 14, 2020

trasz commented Jul 14, 2020

ghost commented Jul 14, 2020

ghfields commented Jul 23, 2020

anodos325 commented Aug 5, 2020

ghost commented Sep 24, 2020

ghost commented Sep 25, 2020

pbhenson commented Sep 30, 2020

ghost commented Oct 8, 2020

ghost commented Oct 8, 2020

Cross-platform acltype #10520

Cross-platform acltype #10520

Conversation

ghost commented Jun 30, 2020 • edited by ghost Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

ghost commented Jun 30, 2020

ghost commented Jun 30, 2020

ghost commented Jun 30, 2020

behlendorf commented Jun 30, 2020

codecov bot commented Jul 1, 2020 • edited Loading

Codecov Report

ghost commented Jul 14, 2020

trasz commented Jul 14, 2020

ghost commented Jul 14, 2020

ghfields commented Jul 23, 2020

anodos325 commented Aug 5, 2020

ghost commented Sep 24, 2020

ghost commented Sep 25, 2020

pbhenson commented Sep 30, 2020

ghost commented Oct 8, 2020

ghost commented Oct 8, 2020

ghost commented Jun 30, 2020 •

edited by ghost

Loading

codecov bot commented Jul 1, 2020 •

edited

Loading