[PATCH] Rework patch for bug 7909

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Rework patch for bug 7909

Samba - samba-technical mailing list
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!

Cheerio!
-slow

bug7909-master.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Rework patch for bug 7909

Samba - samba-technical mailing list
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 <[hidden email]>
> 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 <[hidden email]>
> ---
>  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
>