NFSv4 ACL support - WIP, review requested

[pbhenson]

Description

Implements zfs native NFSv4 ACL support by providing an xattr handler for the system.nfs4_acl attribute. ACL's can be manipulated using the existing nfs4-acl-tools package.

Motivation and Context

No explanation is required as to why it would be nice to support the native zfs ACL; this will resolve #4966

How Has This Been Tested?

This was built and tested under Ubuntu 18.04 LTS; only a limited amount of testing to verify basic functionality has been performed at this point pending initial review of the code.

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:

This is an initial WIP that will certainly not be ready to commit, at this point I'm just looking for feedback on the code and potential unresolved issues, in particular (as demarked in the code via XXX comments):

posix acl support is controlled by the kernel config option CONFIG_FS_POSIX_ACL, there isn't really anything similar for NFS4 ACL's. The closest thing might be CONFIG_NFS_ACL_SUPPORT, but that doesn't really seem appropriate. Maybe the native zfs acl support shouldn't be conditional? Just always available?

In zfs_vfsops.c, turning on and off posix acl support adds/removes the relevant mount flag:
zfsvfs->z_sb->s_flags |= MS_NFS4ACL;
zfsvfs->z_sb->s_flags &= ~MS_NFS4ACL;
Does something similar need to be done for nfs4 acl support?

Similarly, in zpl_super.c, it adds posixacl and noacl to some list, I'm not sure if nfs4acl should be added to the same list.

There were a handful of standard functions from security/keys/user_defined.c that were marked GPL only, I reimplemented different local functions to do the same thing; I'm not sure what policy is on stuff like that.

There's a function which was user_key_payload in older kernels, but is now user_key_payload_rcu as of 4.11, it needs to be wrapped to do the right thing but I'm not sure my autoconf-fu is up to it, so would appreciate a hand with it.

The constants from zfs acl flags and nfs4-acl-tools flags don't match:

from zfs acl.h:

#define ACE_INHERITED_ACE               0x0080
#define ACE_OWNER                       0x1000
#define ACE_GROUP                       0x2000
#define ACE_EVERYONE                    0x4000

From nfs4-acl-tools:

#define NFS4_ACE_OWNER                        0x00000080
#define NFS4_ACE_GROUP                        0x00000100
#define NFS4_ACE_EVERYONE                     0x00000200

I don't see these defined in the NFSv4 RFC? Where did they originally come from? For now the implementation is mapping between them so it does the right thing.

[behlendorf]

Thanks for starting to work on this. It's appreciated!

I had a chance to look over the WIP PR and posted a first round of comments to answer some of your questions. In general your proposal looks like a reasonable way to handle this. It's a shame Linux doesn't provide better generic infrastructure for NFSv4 ACLs, but it is what it is.

One thing not mentioned inline but which will eventually need to be added to the PR are some functional tests. I'd suggest starting by re-adding, and updating for Linux as appropriate, the original ZFS ACL tests from OpenZFS. They were originally dropped until support was added for NFSv4 ACLs, you can find them here.

The LTP also had a good set of ACL tests which can be used to verify correctness.

[pbhenson]

Hey everybody, I'd appreciate any further review or comments on this that will help get it ready for possible merging :).

One nit I'm having is getting automake to properly deal with adding back the nontrivial and trivial subdirectories in tests/zfs-tests/tests/functional/acl, I update Makefile.am to:

SUBDIRS = nontrivial posix trivial

but there's no Makefile generated in the new subdirectories when I run autogen.sh and then make dies when it goes into them.

Beyond that, I could use some suggestions on what other kernel options to base the inclusion of ZFS_NFS4_ACL on, and a good review of the permissions checking integration, I'm sure I missed something.

Thanks much...

[behlendorf]

@pbhenson I hope to have a chance to take a proper look at this in the new year. Thanks for continuing to push it forward.

One nit I'm having is getting automake to properly deal with adding back the nontrivial and trivial

It looks like you need to add an entry for the new Makefile to configure.ac. This will ensure autogen.sh will generate the Makefile.in for ./configure.

[pbhenson]

It looks like you need to add an entry for the new Makefile to configure.ac. This will ensure
autogen.sh will generate the Makefile.in for ./configure.

Yep, that did it; sorry, my autoconf-fu isn't the best 8-/.

I'm running the tests now and will see what acl issues pop up with them, thanks.

I see from the buildbot results that there appears to have been some changes in the key API over time, only the newest stuff is compiling successfully. I'll need to take a look at what all is different and try to make it more backwards compatible with older kernels.

Thanks much...

[behlendorf]

@pbhenson thanks for pulling in those additional Illumos changes to this PR. To make this whole stack easier to review, and test, it would be helpful if you could slightly restructure this PR. Each of the Illumos commits should be formatted like this for example, we have some guidelines to make it easier to track these commits. Also please go ahead and squash all your additional changes in to a single commit.

[ryantrinkle]

This is awesome! Thank you so much for your work, @pbhenson!

I started testing it out, and nfs4_getfacl seems to work, but I wasn't able to get nfs4_setfacl to work. I boiled the test case down to this:

#include <sys/xattr.h>

int main() {
  char buffer[65536];
  int len = getxattr("/tank/asdf", "system.nfs4_acl", 0, 0);
  getxattr("/tank/asdf", "system.nfs4_acl", buffer, len);
  setxattr("/tank/asdf", "system.nfs4_acl", buffer, len, XATTR_REPLACE);
}

The setxattr fails with ENOSYS - I'd expect it to succeed (and make no change to the ACL). The full strace is here.

If it would be at all helpful for me to gather more info, please let me know!

[pbhenson]

it would be helpful if you could slightly restructure this PR

Ok, requested changes complete, I hope.

I've been looking at the nfs4 acl tests; the openzfs ones assume you're using an illumos compatible ls and chmod, which isn't really the case here 8-/. I'm not sure whether to try and tweak them to use the nfs4 acltools (which wouldn't indicate whether the acl is trivial or not, which is also an assumed capability) or try and port illumos ls/chmod to use the xattr interface for getting/setting ACLs. Neither approach seems ideal . The LTP ones you pointed me to a while ago seem to be for POSIX ACLs over NFSv4, not native NFS4 acls...

[pbhenson]

I started testing it out, and nfs4_getfacl seems to work, but I wasn't able to get nfs4_setfacl to work. I
boiled the test case down to this:

I ran into this once, but then when I added some printk's to try and narrow it down, it went away 8-/. And then when I took them out it didn't come back . I'm not really sure what's going on with that, presumably I screwed something up somewhere. Hopefully more expert eyes will notice something on review, or perhaps I'll have to get the actual kernel debugger going rather than relying on printk.

It looks like if I destroy my testpool, then recreate it, this problem reliably pops up. But then if I recompile the module with extra debugging, and reload it, it goes away. Maybe if I reboot between compiling with extra debugging output and reloading? I'll fiddle with it some more and see if I can track down.

Thanks for looking at it...

[behlendorf]

Ok, requested changes complete, I hope.

Thanks, yes that's exactly what I had in mind. I noticed this will also need to be rebased on to the latest master at some point as well.

edit: the only conflict was in the man page so this was a straight forward rebase. As expected this resolved the unrelated build failures the CI encountered.

the openzfs ones assume you're using an illumos compatible ls and chmod, which isn't really the case here

In order to adapt the test cases what you're going to need to do is create compatibility functions which can be used instead. Then they can be used in place of ls and chmod in the tests. This way we won't break the tests for other platforms. There's an existing is_linux helper function you can use, so something like this:

function check_acl
{
        if is_linux; then
                acltools
        else
                ls/chmod
        fi
}

[behlendorf]

A few quick comments, I can take a proper look at this next week.

[behlendorf]

It looks like we can an opportunity to simplify this logic. The check_acl() interface was first introduced in the 2.6.32 kernel, since this is the oldest supported kernel we can drop the HAVE_PERMISSION compatibility code entirely from the CONFIG_FS_POSIX_ACL case. Either get_acl or .check_acl must exist.

This can then simply become the following, and zpl_permission() function can rely on generic_permission(ip, mask) for the non-nfsacl case.

#if defined(ZFS_NFS4_ACL) && defined(HAVE_PERMISSION)
        .permission	= `,
#endif

The HAVE_PERMISSION_WITH_NAMEIDATA check is for an even older kernel and can be removed entirely. Let's keep the HAVE_PERMISSION check though since it's already written and is useful to verify the expected prototype.

[pbhenson]

Ok, I think I did what you wanted here, let me know if otherwise.

[pbhenson]

Thanks, yes that's exactly what I had in mind. I noticed this will also need to be rebased on to the
latest master at some point as well.

edit: the only conflict was in the man page so this was a straight forward rebase. As expected this resolved the unrelated build failures the CI encountered.

I rebased and fixed the man page conflict. I'll take another look at the CI builds once they run again, I might need some more tweaks to get the idmap stuff working on the even older kernels, I only tested CentOS 7 when I was working on it locally.

In order to adapt the test cases what you're going to need to do is create compatibility functions
which can be used instead.

I wondered what FreeBSD did about this, as my understanding is they support zfs ACLs. It looks like they just don't run the zfs tests? At least I couldn't find anything about running the standard zfs test suite under FreeBSD. Given they're going to switch to the ZoL source base for FreeBSD maybe it's worth trying to collaborate with them on making the ACL tests more OS agnostic? I see they decided to extend the POSIX getfacl and setfacl commands to handle NFS4/zfs ACLs rather than extend ls/chmod like illumos or add new separate acl commands like linux. Any chance the linux powers that be might be interested in incorporating the nfs4 acl tools into the posix acl tools like freebsd? That would be one less OS difference to deal with.

[pbhenson]

edit: the only conflict was in the man page so this was a straight forward rebase. As expected this
resolved the unrelated build failures the CI encountered.

It seems the TEST checks are failing for OS's that the BUILD checks are working, it's complaining that it can't find the new include file <acl/acl_common.h> I added? Did I do something wrong with that?

[codecov]

Codecov Report

Merging #7728 into master will decrease coverage by 0.17%.
The diff coverage is 28.92%.

@@            Coverage Diff             @@
##           master    #7728      +/-   ##
==========================================
- Coverage   78.56%   78.39%   -0.18%     
==========================================
  Files         379      379              
  Lines      114909   115128     +219     
==========================================
- Hits        90284    90258      -26     
- Misses      24625    24870     +245

Flag	Coverage Δ
#kernel	78.69% <28.92%> (-0.28%)	⬇️
#user	67.35% <100%> (-0.14%)	⬇️

---

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 06f3fc2...fc8218d. Read the comment docs.

[behlendorf]

can't find the new include file <acl/acl_common.h> I added?

This is caused by not adding the new directory and acl_common.h header to the Makefile.am, which results in them being omitted from the make dist tarball.

That said, I believe what we should do here is to not add acl/acl_common.h in the first place. It looks like zfs_acl.c is the only place the trivial_acl_t definition is needed, and we don't want to add prototypes for functions which aren't implemented.

You should be able to cherry-pick ec86bdc in to this patch stack to resolve the issue. It works for me on Fedora 29.

worth trying to collaborate with them on making the ACL tests more OS agnostic?

Absolutely, since they're looking at moving to using the ZoL code base we're going to need to make sure the tests are written in such a way they'll work in both environments. Starting with their ACL tests might be a good idea to minimize differences. Although they may not have been updated since from what I can see it's expecting to use ls and chmod.

Any chance the linux powers that be might be interested in incorporating the nfs4 acl tools into the posix acl tools like freebsd?

I don't know if that's ever been considered, but it would be up to the getfacl/setfacl maintainer. It's worth asking, but either way that would only be a solution going forward. We still need to somehow handle testing in environment where only nfs4-acl-tools is available.

[pbhenson]

That said, I believe what we should do here is to not add acl/acl_common.h in the first place. It
looks like zfs_acl.c is the only place the trivial_acl_t definition is needed, and we don't want to add > prototypes for functions which aren't implemented.

Eh; the concept of whether an ACL is trivial is exposed to userspace on illumos by ls, which uses the functionality provided by acl_common. At some point I'm going to want to have a way to determine from userspace whether or not the ACL on a given object is trivial (this will be required for the tests to pass), and I don't want to have to duplicate code in userspace that is already used inside zfs. I intentionally left the include separate with the thought of possibly at some point pulling in acl_common.c to expose some acl stuff.

You should be able to cherry-pick [ec86bdc](https://github.com/zfsonlinux/zfs/commit

Ok, I'll just do that for now and punt on userspace trivial acl details for later.

Absolutely, since they're looking at moving to using the ZoL code base we're going to need to make
sure the tests are written in such a way they'll work in both environments. Starting with their ACL
tests might be a good
idea to minimize differences. Although they may not have been updated since from what I can see i
it's expecting to use ls and chmod.

Ah, that's where they were hiding, I missed them. Yeah, I don't think they pass as is on freebsd 8-/. I don't see anywhere they redefined chmod/ls.

I don't know if that's ever been considered, but it would be up to the getfacl/setfacl maintainer. It's
worth asking, but either way that would only be a solution going forward. We still need to somehow
handle testing in environment where only nfs4-acl-tools is available.

Eh, I'll punt that too then for now.

[pbhenson]

I know I still need to work on the tests, but any more feedback on the actual implementation?

Thanks...

[pbhenson]

I've taken a few looks at the tests, but I think it's going to be a PITA to clean them up, and honestly I haven't been that motivated given the lack of feedback on the actual implementation. I don't really want to spend the time on the tests if this will probably never be integrated. Any chance I could get some review and reach a point where it's more or less "if the tests worked this code would be good to go"? Then I'd be less concerned that overhauling the tests would be a waste of effort.

Thanks...

[trasz]

On FreeBSD ACLs are being handled using setfacl(1)/getfacl(1) utilities, and that applies also to NFSv4 ACLs.

As for tests - you might want to take a look at https://github.com/freebsd/freebsd/blob/master/tests/sys/acl/tools-nfs4.test. This is an input to this tool: https://github.com/freebsd/freebsd/blob/master/tests/sys/acl/run.

[pbhenson]

I think using getfacl/setfacl for NFSv4 ACL's is a great idea :), I actually already mentioned it in this pull request. However, even if the maintainers of that were willing, it would take a while to percolate down to the distributions, and Brian says the tests would need to work with both the existing nfs4-acl-tools now and get/setfacl later.

[behlendorf]

@pbhenson thanks for sticking with this feature despite the lack of feedback. I definitely do think we should integrate some version of this functionality and the proposed approach looks reasonable. But regardless of the specific internal implementation details we're going to need to somehow add test coverage for it.

I'm not familiar with the available utilities, but perhaps for testing on Linux we could create setfacl(1)/getfacl(1) wrapper scripts which call nfs4-acl-tools. Or are the interfaces too different for that to be practical?

[pbhenson]

There's no output format difference that a bit of perl magic can't bridge ;).

The question is though, what is the goal for testing? Try to make the existing illumos tests (which rely on ACL support existing in ls/chmod) work (with some type of linux wrapper), or have a separate suite of tests only expected to work on linux/freebsd using get/setfacl? It appears at this point that freebsd went the route of ignoring the illumos tests and doing their own thing. Hmm, actually, on second look, the tests that were pointed out are generic nfsv4 acl tests, not zfs tests; zfs tests are more complicated as you need to factor in the various differences caused by aclmode, aclinherit, etc... I'm not sure, does the freebsd port do any zfs acl tests?

I guess the bottom line question is do we want to have one standard set of zfs tests that run on every platform (some perhaps skipped for things like the illumos acl tar/cpio tests) or should each platform do its own test thing?

[trasz]

The FreeBSD tests test the semantics, ie whether the inheritance works as expected, or what happens with ACL after chmod, and vice versa. They don't test for alternative behaviour from non-default aclmode or aclinherit, though.

Which basically means, they are ZFS tests - the NFSv4 spec doesn't really specify how to handle chmod, for example, nor does it specify what DELETE or DELETE_CHILD actually mean.

As for Illumos tests - when I wrote the FreeBSD tests there were no Illumos tests for that.

[behlendorf]

do we want to have one standard set of zfs tests that run on every platform

As nice as that would be, I don't think we need to go that far.

From my point of view, what we need are tests included in the ZoL repository which are regularly run as part of the CI to verify that NFSACLs are working properly. Right now that means they need to work on Linux, in the near future we're going to need them to work on FreeBSD and Linux.

[anodos325]

From a usability standpoint in samba, it would be much better to expose the ACLs as NFS41 style ACL_FLAGS (ACL_AUTO_INHERIT|ACL_PROTECTED|ACL_DEFAULTED) and ACE_INHERITED_ACE. Windows clients expect to be able to set SEC_DESC_DACL_PROTECTED when disabling permissions inheritance from File Explorer (and CLI tools).

The problem I see is that internally RFC5661 ACLs are stored in ZFS. They are also presented that way in FreeBSD and Solaris. If you only present only RFC3530 ACLs, then there is a real possibility that ACL information will be lost. I believe the Solaris kernel SMB server uses ACL_PROTECTED to present SEC_DESC_DACL_PROTECTED to SMB clients and both FreeBSD and Solaris present ACE_INHERITED ACE.

[mattmacy]

@pbhenson could you please rebase your branch to recent master? FreeBSD needs the ACLMODE prop and it would be nicer if it actually worked on Linux. Thanks.

[behlendorf]

@pbhenson thanks for your patience with this PR. With the FreeBSD integration work seriously underway we're in a much better position now to get you some reviewers for this work. If you can get this rebased on master we should be able to run all the tests on FreeBSD and Linux. Then work through any remaining issues to get it merged.

[pbhenson]

Sure, I'll get it rebased. Looks like all the time I spend accommodating old kernels will now be time spend ripping out the accommodations , as you guys dropped 2.6 support.

One of the things holding up integration in the past was the test suite, which sadly I must confess has made no progress :(. It's been pointed out FreeBSD had some ACL tests, perhaps with the merge in progress it will be easier to sort out the testing issue.

[mattmacy]

See also:
#9685

[pbhenson]

See also:
#9685

I'm confused? My PR already enables aclmode? Why is there a separate one?

Also, you added a new enum for the property rather than restoring the existing one? That won't be compatible with existing pools or cross platform imports.

I'm working on rebasing this to current master. Well, actually, I made a new branch and am reapplying just relevant stuff, as direct rebasing involved removing code I had earlier added due to the kernel support changes and it was turning into a mess 8-/. I'm hoping to have that done by next week.

[behlendorf]

My PR already enables aclmode? Why is there a separate one?

This was to lay the groundwork for testing it on the FreeBSD side. But after talking with @mattmacy we thought the best approach was to tackle this after the rest of the FreeBSD merge. This way we can break this in a couple of smaller chunks to get everything integrated and tested.

@pbhenson what do you think about the following plan to split up the needed work so we can get this feature integrated.

1. Merge FreeBSD platform code (This is getting close, and a FreeBSD CI bot will be added)

2. Merge aclmode initially as FreeBSD-only along with the passing test cases. That should help significantly shrink this PR so we can focus on the Linux aspects of it with known good tests.

3. a. Enable / merge NFSv4 ACLs for Linux once everything has been worked out and we're happy with how it works and the tests are passing.
   b. Unify the linux/zfs/zfs_acl.c and freebsd/zfs/zfs_acl.c source, the majority of this code should be the same and can be shared, if it ends up makes sense this could be part of 3a.

Also, you added a new enum for the property rather than restoring the existing one?

ZFS_PROP_PRIVATE was for a different retried property. We'll need to add ZFS_PROP_ACLMODE to the list at the end as mentioned in the comment above. This isn't its original placement but adding it to the end will preserve compatibility on the Linux side and for FreeBSD existing applications will already need to be recompiled for this switch so it not a major issue there.

I'm working on rebasing this to current master.

That sounds great, thank you. Sorry about dropping the compatibility code for older kernels, but it should help simplify things.

[pbhenson]

@pbhenson what do you think about the following plan to split up the needed work so we can get this feature integrated.

I think high-level strategic planning like that is above my pay grade in my role as a drive-by contributor ;). I'll try to work with whatever you guys think is best to get things merged cleanly and efficiently.

2. Merge `aclmode` initially as FreeBSD-only along with the passing test cases.  That should help significantly shrink this PR so we can focus on the Linux aspects of it with known good tests.

Having an existing test framework sounds great :), trying to sort out what to do about that is the main cause of my delayed progress from when I first started on it.

One potential caveat though is that on FreeBSD ZFS ACLs are a first-class citizen with direct kernel support, whereas under linux at least for now they are duct taped to the NFSv4 xattr ACL support, which only has a subset of the full functionality.

   b. Unify the `linux/zfs/zfs_acl.c` and `freebsd/zfs/zfs_acl.c` source, the majority of this code should be the same and can be shared, if it ends up makes sense this could be part of 3a.

I agree, the majority of the code should be shared, to avoid weird corner cases where ACLs behave slightly differently on each platform 8-/.

ZFS_PROP_PRIVATE was for a different retried property. We'll need to add ZFS_PROP_ACLMODE to the list at the end as mentioned in the comment above. This isn't its original placement but adding it to the end will preserve compatibility on the Linux side and for FreeBSD existing applications will already need to be recompiled for this switch so it not a major issue there.

Ah, I was under the impression that the enum values were stored on disk and the mapping needed to be maintained. After taking a look at that particular aspect of the on disk format, I see that the properties are stored as text, not numeric values, so any given implementation's mapping doesn't matter. I will update my code to add it at the end instead of back where it was originally.

That sounds great, thank you. Sorry about dropping the compatibility code for older kernels, but it should help simplify things.

Simpler in the long run, but more interesting in the short :).

[pbhenson]

I created a new pull request with an updated implementation on top of current master, let's move the party to:

#9709

Thanks...