[PATCH] Rework patch for bug 6133
Jeremy Allison
jra at samba.org
Fri Sep 8 23:05:42 UTC 2017
On Thu, Sep 07, 2017 at 12:27:53PM +0200, Ralph Böhme wrote:
> Hi!
>
> Attached is a patch that moves ACE4_ADD_FILE -> ACE4_DELETE_CHILD mapping from
> the NFSv4 framework to vfs_zfsacl.
>
> This was added in e6a5f11865a55e9644292ae92e4a4b5ec0662ccd to adopt the NFSv4
> framework to follow ZFS permission rules. But this is the wrong place, other
> filesystems like GPFS do not allow deletion when the user has SEC_DIR_ADD_FILE,
> so setting ACE4_DELETE_CHILD when the access_mask has ACE4_ADD_FILE is wrong:
>
> # su -s /bin/bash FOO\\aduser1
> bash-4.2$ mmgetacl .
> #NFSv4 ACL
> #owner:FOO\aduser1
> #group:FOO\domain users
> special:owner@:rwxc:allow:FileInherit:DirInherit:InheritOnly
> (X)READ/LIST (X)WRITE/CREATE (X)APPEND/MKDIR (X)SYNCHRONIZE (X)READ_ACL (X)READ_ATTR (X)READ_NAMED
> (X)DELETE (X)DELETE_CHILD (X)CHOWN (X)EXEC/SEARCH (X)WRITE_ACL (X)WRITE_ATTR (X)WRITE_NAMED
>
> user:FOO\aduser1:rwx-:allow:FileInherit:DirInherit
> (X)READ/LIST (X)WRITE/CREATE (X)APPEND/MKDIR (X)SYNCHRONIZE (X)READ_ACL (X)READ_ATTR (X)READ_NAMED
> (-)DELETE (-)DELETE_CHILD (-)CHOWN (X)EXEC/SEARCH (-)WRITE_ACL (-)WRITE_ATTR (-)WRITE_NAMED
>
> The ACL has an explicit ACE for the user that grants CREATE, but not
> DELETE_CHILD, so the user can't delete the file:
>
> bash-4.2$ rm file
> rm: cannot remove ‘file’: Permission denied
> bash-4.2$
>
> Adding DELETE_CHILD allows the user to delete the file:
> bash-4.2$ rm file
> bash-4.2$
>
> This patch therefor moves the change from the NFS4 framework into the ZFS
> module.
>
> Please review & push if ok. Thanks!
LGTM. Pushed !
> From 9aebdfcc39b48d06e122570186554c5c302006be Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Wed, 6 Sep 2017 16:44:12 +0200
> Subject: [PATCH 1/3] vfs_zfsacl: pass smb_fname to zfs_get_nt_acl_common
>
> This is in preperation of moving SMB_ACE4_ADD_FILE /
> SMB_ACE4_DELETE_CHILD mapping from the common NFSv4 framework into this
> module excusively.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=6133
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> source3/modules/vfs_zfsacl.c | 20 ++++++++------------
> 1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/source3/modules/vfs_zfsacl.c b/source3/modules/vfs_zfsacl.c
> index 4cb1b98f01b..97fe2028307 100644
> --- a/source3/modules/vfs_zfsacl.c
> +++ b/source3/modules/vfs_zfsacl.c
> @@ -41,7 +41,7 @@
> * using the NFSv4 format conversion
> */
> static NTSTATUS zfs_get_nt_acl_common(TALLOC_CTX *mem_ctx,
> - const char *name,
> + const struct smb_filename *smb_fname,
> struct SMB4ACL_T **ppacl)
> {
> int naces, i;
> @@ -49,13 +49,13 @@ static NTSTATUS zfs_get_nt_acl_common(TALLOC_CTX *mem_ctx,
> struct SMB4ACL_T *pacl;
>
> /* read the number of file aces */
> - if((naces = acl(name, ACE_GETACLCNT, 0, NULL)) == -1) {
> + if((naces = acl(smb_fname->base_name, ACE_GETACLCNT, 0, NULL)) == -1) {
> if(errno == ENOSYS) {
> DEBUG(9, ("acl(ACE_GETACLCNT, %s): Operation is not "
> "supported on the filesystem where the file "
> - "reside\n", name));
> + "reside\n", smb_fname->base_name));
> } else {
> - DEBUG(9, ("acl(ACE_GETACLCNT, %s): %s ", name,
> + DEBUG(9, ("acl(ACE_GETACLCNT, %s): %s ", smb_fname->base_name,
> strerror(errno)));
> }
> return map_nt_error_from_unix(errno);
> @@ -67,8 +67,8 @@ static NTSTATUS zfs_get_nt_acl_common(TALLOC_CTX *mem_ctx,
> return NT_STATUS_NO_MEMORY;
> }
> /* read the aces into the field */
> - if(acl(name, ACE_GETACL, naces, acebuf) < 0) {
> - DEBUG(9, ("acl(ACE_GETACL, %s): %s ", name,
> + if(acl(smb_fname->base_name, ACE_GETACL, naces, acebuf) < 0) {
> + DEBUG(9, ("acl(ACE_GETACL, %s): %s ", smb_fname->base_name,
> strerror(errno)));
> return map_nt_error_from_unix(errno);
> }
> @@ -210,9 +210,7 @@ static NTSTATUS zfsacl_fget_nt_acl(struct vfs_handle_struct *handle,
> NTSTATUS status;
> TALLOC_CTX *frame = talloc_stackframe();
>
> - status = zfs_get_nt_acl_common(frame,
> - fsp->fsp_name->base_name,
> - &pacl);
> + status = zfs_get_nt_acl_common(frame, fsp->fsp_name, &pacl);
> if (!NT_STATUS_IS_OK(status)) {
> TALLOC_FREE(frame);
> return status;
> @@ -234,9 +232,7 @@ static NTSTATUS zfsacl_get_nt_acl(struct vfs_handle_struct *handle,
> NTSTATUS status;
> TALLOC_CTX *frame = talloc_stackframe();
>
> - status = zfs_get_nt_acl_common(frame,
> - smb_fname->base_name,
> - &pacl);
> + status = zfs_get_nt_acl_common(frame, smb_fname, &pacl);
> if (!NT_STATUS_IS_OK(status)) {
> TALLOC_FREE(frame);
> return status;
> --
> 2.13.5
>
>
> From 8d6be0aa8ff81ed7645839ce7a37fd74201abdd0 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Wed, 6 Sep 2017 16:53:23 +0200
> Subject: [PATCH 2/3] vfs_zfsacl: ensure zfs_get_nt_acl_common() has access to
> stat info
>
> We'll need this in the next commit.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=6133
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> source3/modules/vfs_zfsacl.c | 25 ++++++++++++++++++++++---
> 1 file changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/source3/modules/vfs_zfsacl.c b/source3/modules/vfs_zfsacl.c
> index 97fe2028307..da13c4c4908 100644
> --- a/source3/modules/vfs_zfsacl.c
> +++ b/source3/modules/vfs_zfsacl.c
> @@ -40,13 +40,31 @@
> * read the local file's acls and return it in NT form
> * using the NFSv4 format conversion
> */
> -static NTSTATUS zfs_get_nt_acl_common(TALLOC_CTX *mem_ctx,
> +static NTSTATUS zfs_get_nt_acl_common(struct connection_struct *conn,
> + TALLOC_CTX *mem_ctx,
> const struct smb_filename *smb_fname,
> struct SMB4ACL_T **ppacl)
> {
> int naces, i;
> ace_t *acebuf;
> struct SMB4ACL_T *pacl;
> + SMB_STRUCT_STAT sbuf;
> + const SMB_STRUCT_STAT *psbuf = NULL;
> + int ret;
> +
> + if (VALID_STAT(smb_fname->st)) {
> + psbuf = &smb_fname->st;
> + }
> +
> + if (psbuf == NULL) {
> + ret = vfs_stat_smb_basename(conn, smb_fname, &sbuf);
> + if (ret != 0) {
> + DBG_INFO("stat [%s]failed: %s\n",
> + smb_fname_str_dbg(smb_fname), strerror(errno));
> + return map_nt_error_from_unix(errno);
> + }
> + psbuf = &sbuf;
> + }
>
> /* read the number of file aces */
> if((naces = acl(smb_fname->base_name, ACE_GETACLCNT, 0, NULL)) == -1) {
> @@ -210,7 +228,8 @@ static NTSTATUS zfsacl_fget_nt_acl(struct vfs_handle_struct *handle,
> NTSTATUS status;
> TALLOC_CTX *frame = talloc_stackframe();
>
> - status = zfs_get_nt_acl_common(frame, fsp->fsp_name, &pacl);
> + status = zfs_get_nt_acl_common(handle->conn, frame,
> + fsp->fsp_name, &pacl);
> if (!NT_STATUS_IS_OK(status)) {
> TALLOC_FREE(frame);
> return status;
> @@ -232,7 +251,7 @@ static NTSTATUS zfsacl_get_nt_acl(struct vfs_handle_struct *handle,
> NTSTATUS status;
> TALLOC_CTX *frame = talloc_stackframe();
>
> - status = zfs_get_nt_acl_common(frame, smb_fname, &pacl);
> + status = zfs_get_nt_acl_common(handle->conn, frame, smb_fname, &pacl);
> if (!NT_STATUS_IS_OK(status)) {
> TALLOC_FREE(frame);
> return status;
> --
> 2.13.5
>
>
> From 216abbcf5ea028a5091722d5d9024fe6728f5794 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Wed, 6 Sep 2017 16:56:47 +0200
> Subject: [PATCH 3/3] s3/vfs: move ACE4_ADD_FILE/ACE4_DELETE_CHILD mapping from
> NFSv4 framework to vfs_zfsacl
>
> This was added in e6a5f11865a55e9644292ae92e4a4b5ec0662ccd to adopt the
> NFSv4 framework to follow ZFS permission rules. But this is the wrong
> place, other filesystems like GPFS do not allow deletion when the user
> has SEC_DIR_ADD_FILE.
>
> This patch therefor moves the change from the NFS4 framework into the
> ZFS module.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=6133
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> source3/modules/nfs4_acls.c | 4 ----
> source3/modules/vfs_zfsacl.c | 4 ++++
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/source3/modules/nfs4_acls.c b/source3/modules/nfs4_acls.c
> index c715b83390d..19f0fefdb98 100644
> --- a/source3/modules/nfs4_acls.c
> +++ b/source3/modules/nfs4_acls.c
> @@ -351,10 +351,6 @@ static bool smbacl4_nfs42win(TALLOC_CTX *mem_ctx,
> DEBUG(10, ("mapped %d to %s\n", ace->who.id,
> sid_string_dbg(&sid)));
>
> - if (is_directory && (ace->aceMask & SMB_ACE4_ADD_FILE)) {
> - ace->aceMask |= SMB_ACE4_DELETE_CHILD;
> - }
> -
> if (!is_directory && params->map_full_control) {
> /*
> * Do we have all access except DELETE_CHILD
> diff --git a/source3/modules/vfs_zfsacl.c b/source3/modules/vfs_zfsacl.c
> index da13c4c4908..dd0f343b8c6 100644
> --- a/source3/modules/vfs_zfsacl.c
> +++ b/source3/modules/vfs_zfsacl.c
> @@ -66,6 +66,10 @@ static NTSTATUS zfs_get_nt_acl_common(struct connection_struct *conn,
> psbuf = &sbuf;
> }
>
> + if (S_ISDIR(psbuf->st_ex_mode) && (ace->aceMask & SMB_ACE4_ADD_FILE)) {
> + ace->aceMask |= SMB_ACE4_DELETE_CHILD;
> + }
> +
> /* read the number of file aces */
> if((naces = acl(smb_fname->base_name, ACE_GETACLCNT, 0, NULL)) == -1) {
> if(errno == ENOSYS) {
> --
> 2.13.5
>
More information about the samba-technical
mailing list