[PATCH] zfsacl: Fix ACL behavior issues with vfs_zfsacl

[Andrew Walker]

> - commit message is all in one line
>
> - this is a hot code path and parmetric options are expensive, can you
> please use a
>   configuration object, the existing parametric option denymissingspecial
> should
>   be converted to the config object option while at it. Cf other VFS
> modules on how
>   to do it, eg gpfs or fruit
>
> - please check mixed space/tabs indentation
>
> - please check trailing space
>
> - pleaseuse common struct initializer:
>
>   SMB_ACE4PROP_T hidden_ace;
>   if (S_ISDIR(fsp->fsp_name->st.st_ex_mode)) {
>           hidden_ace.flags = SMB_ACE4_ID_SPECIAL;
>           hidden_ace.who.id = SMB_ACE4_WHO_EVERYONE;
>           hidden_ace.aceType = SMB_ACE4_ACCESS_ALLOWED_ACE_TYPE;
>           hidden_ace.aceFlags =
> (SMB_ACE4_FILE_INHERIT_ACE|SMB_ACE4_DIRECTORY_INHERIT_ACE);
>           hidden_ace.aceMask = 0;
>           DBG_DEBUG("zfsacl: setting empty everyone@ ace on dir  %s \n",
> fsp->fsp_name->base_name);
>   } else {
>           hidden_ace.flags = SMB_ACE4_ID_SPECIAL;
>           hidden_ace.who.id = SMB_ACE4_WHO_EVERYONE;
>           hidden_ace.aceType = SMB_ACE4_ACCESS_ALLOWED_ACE_TYPE;
>           hidden_ace.aceFlags = 0;
>           hidden_ace.aceMask = 0;
>           DBG_DEBUG("zfsacl: setting empty everyone@ ace on file  %s \n",
> fsp->fsp_name->base_name);
>   }
>
>   rewrite as:
>
>   SMB_ACE4PROP_T hidden_ace = (SMB_ACE4PROP_T) {
>           hidden_ace.flags = SMB_ACE4_ID_SPECIAL;
>           hidden_ace.who.id = SMB_ACE4_WHO_EVERYONE;
>           hidden_ace.aceType = SMB_ACE4_ACCESS_ALLOWED_ACE_TYPE;
>   };
>
>   if (S_ISDIR(fsp->fsp_name->st.st_ex_mode)) {
>           hidden_ace.aceFlags =
> (SMB_ACE4_FILE_INHERIT_ACE|SMB_ACE4_DIRECTORY_INHERIT_ACE);
>   }
>   DBG_DEBUG("zfsacl: setting empty everyone@ ace on file  %s \n",
> fsp->fsp_name->base_name);
>
> - please split the patch in two patches, each just adding one option
>
> - please add the new options to the manpage
>
> - I don't understand the mechanics of setting the SEC_DESC_DACL_PROTECTED
> flag
>   when no inheritable ACEs are present. If ZFS does support the flag, why
> not
>   just map the flag from the wire like gpfs? I might be missing
> something...
>
> - the option name block_zfs_acl_chmod seems a bit misleading. Can we come
> up
>   with something else that better describes the intended semantics? Eg
>   "ignore_empty_mode".
>
> Thanks!
> -slow
>
> --
> Ralph Boehme, Samba Team                https://samba.org/
> Samba Developer, SerNet GmbH   https://sernet.de/en/samba/
> GPG-Fingerprint   FAE2C6088A24252051C559E4AA1E9B7126399E46
>
Hi Ralph,

Thanks for the feedback and suggestions. I'll try to get this done this
week or next week. You are correct that ZFS has the  NFSv4.1 ACL flags, but
FreeBSD does not currently implement NFSv4.1 ACL inheritance. The
suggestion of just mapping what we receive over the wire is a good one. I
could probably do this for the case of Solaris and Illumos.

One possible alternative is that I could move this logic/lies to libsunacl
(the library that maps ZFS ACLs to FreeBSD ACLs) so that there won't be a
FreeBSD-specific parameter for vfs_zfsacl. In this case the only thing I
would need to add to fix disabling inheritance in samba is mapping the
NFSv4.1 ACL flags to control flags like gpfs does.

Let me know if you prefer the second approach.

Andrew