[PATCH] Rework patch for bug 7909

[Jeremy Allison]

On Thu, Sep 07, 2017 at 12:05:34PM +0200, Ralph Böhme wrote:
> Hi!
> 
> Back in the days a fix for bug 7909 resulted in the common VFS NFS4 ACL
> framework to set the SEC_STD_SYNCHRONIZE bit. This was deemed necessary just to
> make ZFS happy: the ZFS guys didn't want the SEC_STD_SYNCHRONIZE bit to appear
> in the mask in the filesystem and thus are filtering it out of ACEs when setting
> ACLs.
> 
> Now when fetching ACLs it turned out to be necessary to stick
> SEC_STD_SYNCHRONIZE back into the access mask, otherwise rename operation would
> fail (client side behaviour).
> 
> Other consumers of the framework like vfs_gpfs do not need this behaviour, if
> ZFS needs this, it must be put into vfs_zfsacl.
> 
> Having divergent behaviour wrt to SEC_STD_SYNCHRONIZE in all modules that use
> the NFS4 ACL framework also means that torture tests like "raw.acls.generic"
> fail and can't be used to test the modules which severly limits test coverage.
> 
> Please review & push if ok. Thanks!

LGTM - pushed !


> From a2e4e966622db0aa7f8e7be893c8c3d2b2c56741 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Wed, 6 Sep 2017 16:28:10 +0200
> Subject: [PATCH] vfs/nfs4_acls: move special handling of SMB_ACE4_SYNCHRONIZE
>  to vfs_zfsacl
> 
> Commit 99a74ff5e6a9f87ad7a650cb44e0f925f834b3a1 added special handling
> of SMB_ACE4_SYNCHRONIZE, always setting it in the access_mask when
> fabricating an ACL. While at the same time removing it from the
> access_mask when setting an ACL, but this is done direclty in
> vfs_zfsacl, not it the common code.
> 
> Forcing SMB_ACE4_SYNCHRONIZE to be always set is only needed on ZFS, the
> other VFS modules using the common NFSv4 infrastructure should not be
> made victims of the special ZFS behaviour.
> 
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=7909
> 
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
>  source3/modules/nfs4_acls.c  | 7 -------
>  source3/modules/vfs_zfsacl.c | 9 +++++++++
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/source3/modules/nfs4_acls.c b/source3/modules/nfs4_acls.c
> index 2007917821b..c715b83390d 100644
> --- a/source3/modules/nfs4_acls.c
> +++ b/source3/modules/nfs4_acls.c
> @@ -385,13 +385,6 @@ static bool smbacl4_nfs42win(TALLOC_CTX *mem_ctx,
>  		      ace->aceFlags, win_ace_flags));
>  
>  		mask = ace->aceMask;
> -		/* Windows clients expect SYNC on acls to
> -		   correctly allow rename. See bug #7909. */
> -		/* But not on DENY ace entries. See
> -		   bug #8442. */
> -		if(ace->aceType == SMB_ACE4_ACCESS_ALLOWED_ACE_TYPE) {
> -			mask = ace->aceMask | SMB_ACE4_SYNCHRONIZE;
> -		}
>  
>  		/* Mapping of owner@ and group@ to creator owner and
>  		   creator group. Keep old behavior in mode special. */
> diff --git a/source3/modules/vfs_zfsacl.c b/source3/modules/vfs_zfsacl.c
> index 76cf5281af7..4cb1b98f01b 100644
> --- a/source3/modules/vfs_zfsacl.c
> +++ b/source3/modules/vfs_zfsacl.c
> @@ -84,6 +84,15 @@ static NTSTATUS zfs_get_nt_acl_common(TALLOC_CTX *mem_ctx,
>  		aceprop.aceMask  = (uint32_t) acebuf[i].a_access_mask;
>  		aceprop.who.id   = (uint32_t) acebuf[i].a_who;
>  
> +		/*
> +		 * Windows clients expect SYNC on acls to correctly allow
> +		 * rename, cf bug #7909. But not on DENY ace entries, cf bug
> +		 * #8442.
> +		 */
> +		if (aceprop.aceType == SMB_ACE4_ACCESS_ALLOWED_ACE_TYPE) {
> +			aceprop.aceMask |= SMB_ACE4_SYNCHRONIZE;
> +		}
> +
>  		if(aceprop.aceFlags & ACE_OWNER) {
>  			aceprop.flags = SMB_ACE4_ID_SPECIAL;
>  			aceprop.who.special_id = SMB_ACE4_WHO_OWNER;
> -- 
> 2.13.5
>