[RFC] vfs_nfs4acl_xattr NFS4.1 support, support for XDR encoding and more...

[Jeremy Allison]

On Tue, Oct 24, 2017 at 10:58:13AM +0200, Ralph Böhme wrote:
> Hi!
> 
> I've been recently working on adding support for NFS4.1 ACLs and XDR storage
> format for our vfs_nfs4acl_xattr module.
> 
> I'm mostly done, WIP patch attached. In fact the only WIP patch without a
> signed-off is the new manpage.
> 
> What I'm trying to achieve is the following:
> 
> - modularize the exisiting module so choosing between existing IDL/NDR and a new
>   XRD based encoding is feasible (done)
> 
> - add support for RFC 5661 NFS 4.1 based ACLs in both backends (done)
> 
> - rework how ACL inheritance is achieved and how files/directories without an
>   ACL xattr get a default ACL, this now just works much the same as in
>   vfs_acl_xattr (done)
> 
> - add options to choose between NFS 4.0 and 4.1 ACLs, the storage format, the
>   xattr name and the default ACLA (done)
> 
> - add tests for both encodings and both NFS4 ACL levels (done)
> 
> - fix various issues along the way (done)
> 
> I've left the default behaviour as is, ie NDR marshalled blob in an xattr
> "system.nfs4acl", NFS 4.0 ACLs, "everyone everything" default ACL but would like
> to take the opportunity to discuss changing some of those defaults.
> 
> Is this module actually used anywhere besides for testing in autobuild? As the
> module uses the system xattr namespace, the module will only work when stacked
> with xattr_tdb. Any attempt to use it directly on a Linux filesystem that
> supports xattrs will result in ENOTSUP from the xattr library functions, even as
> root. So I doubt this is used anywhere in production, but who knows.

Hmmm. Good point.

> For the new XDR based backend I've set the default to "security.nfs4acl", so
> with this backend the module is fully NT ACL compatible out-of-the-box and could
> be used as a direct replacement for vfs_acl_xattr (if someone wishes).

Yeah, that sounds like a good idea.

> Note that the latest NFS4 ACL patches by Andreas Gruenbacher already had a ACL
> flags member in the ACL struct and ACL flags defines.
> 
> Question 1: do we want to change the default for the existing NDR backend to
> "security.nfs4acl" as well?
> 
> Question 2: do we want to raise the default level to 4.1?
> 
> WIP patch attached, including the generated manpage for your convenience.
> 
> Thoughts?

I really like this. A couple of comments on the patch.

1). Shouldn't patches #2 and #3 be merged. In #2 you're
modifying a functions signature and then modifying the
caller in #3.

2). [PATCH 19/25] vfs_nfs4acl_xattr: do xattr ops as root
You need to save off the errno if SMB_VFS_NEXT_FGETXATTR()
fails before calling unbecome_root(), as this may corrupt
errno. Refer to saved_errno in the while() loop of course.

3). [PATCH 20/25] vfs_nfs4acl_xattr: add POSIX mode check and reset
Same errno issue.

FYI. I notice we don't do this in other parts of the code,
but looking at the internals of unbecome_root(), it can
call a bunch of system calls that may modify errno, so
moving forward, anytime we modify code calling unbecome_root()
after a system call, we should take care of this.

4). [PATCH 22/25] vfs_nfs4acl_xattr: add XDR backend
In nfs4acl_get_xdrblob_size() - integer wrap checks
please on this:
size += nacl->na41_aces.na41_aces_len * sizeof(struct nfsace4);

Haven't gone through all patch #22 carefully, but I'd
recommend belt-and-braces integer wrap checks.

Cheers,

	Jeremy.