[RFC] Creation of files in directories with delete-on-close set
Jeremy Allison
jra at samba.org
Tue Jan 16 19:20:17 UTC 2018
On Mon, Jan 15, 2018 at 04:46:57PM +0100, Ralph Böhme wrote:
> Hi folks,
>
> I just came across the fact that smbd does not enforce set delete-on-close on
> parent directory on file (or directory) creation.
>
> Windows server do this check and the s4 ntvfs server does as well.
>
> Adding such a check is trivial (cf attached patch), but this oviously has
> performance implications as it requires an additional record fetch from
> locking.tdb per create.
>
> For protocol completeness, we could add the check but guard it by a default off
> option.
>
> Thoughts?
I'm happy with this. It's an optional correctness parameter
that improves our compatibility with Windows which can be
turned on for applications/environments that need it, but is
off by default so it doesn't harm our default create performance.
LGTM.
> --
> Ralph Boehme, Samba Team https://samba.org/
> Samba Developer, SerNet GmbH https://sernet.de/en/samba/
> From c3e12b1846e4fdea27e3a0419f2ea3b94c8bdc2a Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Fri, 12 Jan 2018 17:32:44 +0100
> Subject: [PATCH 1/2] selftest: run deltest20 against s3 as well
>
> This marks the test as knownfail, the next commit fixes it.
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> selftest/knownfail.d/samba3.base.delete | 2 ++
> source4/torture/basic/delete.c | 4 ----
> 2 files changed, 2 insertions(+), 4 deletions(-)
> create mode 100644 selftest/knownfail.d/samba3.base.delete
>
> diff --git a/selftest/knownfail.d/samba3.base.delete b/selftest/knownfail.d/samba3.base.delete
> new file mode 100644
> index 00000000000..6129ca7a66c
> --- /dev/null
> +++ b/selftest/knownfail.d/samba3.base.delete
> @@ -0,0 +1,2 @@
> +samba3.base.delete.deltest20\(nt4_dc\)
> +samba3.base.delete.deltest20\(ad_dc\)
> diff --git a/source4/torture/basic/delete.c b/source4/torture/basic/delete.c
> index d74063c310f..54815b95512 100644
> --- a/source4/torture/basic/delete.c
> +++ b/source4/torture/basic/delete.c
> @@ -1766,10 +1766,6 @@ static bool deltest20(struct torture_context *tctx, struct smbcli_state *cli1, s
>
> /* Test 20 -- non-empty directory hardest to get right... */
>
> - if (torture_setting_bool(tctx, "samba3", false)) {
> - return true;
> - }
> -
> smbcli_deltree(cli1->tree, dname);
>
> dnum1 = smbcli_nt_create_full(cli1->tree, dname, 0,
> --
> 2.13.6
>
>
> From f40028b6de0cb3c35906b4b2971ad10113b492cf Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Thu, 11 Jan 2018 17:52:06 +0100
> Subject: [PATCH 2/2] s3/smbd: fix handling of delete-on-close on directories
>
> This implements a check to test the delete-on-close flag of a directory
> for requests to create files in this directory.
>
> Windows server implement this check, Samba doesn't as it has performance
> implications.
>
> This commit implements the check and a new option to control it. By
> default the check is skipped, setting "check parent directory delete on
> close = yes" enables it.
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> .../tuning/checkparentdirectorydeleteonclose.xml | 13 +++++++
> lib/param/loadparm.c | 2 +
> selftest/knownfail.d/samba3.base.delete | 2 -
> selftest/target/Samba3.pm | 1 +
> selftest/target/Samba4.pm | 1 +
> source3/param/loadparm.c | 1 +
> source3/smbd/open.c | 45 +++++++++++++++++++++-
> 7 files changed, 62 insertions(+), 3 deletions(-)
> create mode 100644 docs-xml/smbdotconf/tuning/checkparentdirectorydeleteonclose.xml
> delete mode 100644 selftest/knownfail.d/samba3.base.delete
>
> diff --git a/docs-xml/smbdotconf/tuning/checkparentdirectorydeleteonclose.xml b/docs-xml/smbdotconf/tuning/checkparentdirectorydeleteonclose.xml
> new file mode 100644
> index 00000000000..1de06091dd7
> --- /dev/null
> +++ b/docs-xml/smbdotconf/tuning/checkparentdirectorydeleteonclose.xml
> @@ -0,0 +1,13 @@
> +<samba:parameter name="check parent directory delete on close"
> + context="S"
> + type="boolean"
> + xmlns:samba="http://www.samba.org/samba/DTD/samba-doc">
> +<description>
> + <para>A Windows SMB server prevents the client from creating files in a
> + directory that has the delete-on-close flag set. By default Samba doesn't
> + perform this check as this check is a quite expensive operation in Samba.
> + </para>
> +</description>
> +
> +<value type="default">no</value>
> +</samba:parameter>
> diff --git a/lib/param/loadparm.c b/lib/param/loadparm.c
> index 7854f57a158..efad4a10f03 100644
> --- a/lib/param/loadparm.c
> +++ b/lib/param/loadparm.c
> @@ -2998,6 +2998,8 @@ struct loadparm_context *loadparm_init(TALLOC_CTX *mem_ctx)
>
> lpcfg_do_global_parameter(lp_ctx, "prefork children", "1");
>
> + lpcfg_do_global_parameter(lp_ctx, "check parent directory delete on close", "no");
> +
> for (i = 0; parm_table[i].label; i++) {
> if (!(lp_ctx->flags[i] & FLAG_CMDLINE)) {
> lp_ctx->flags[i] |= FLAG_DEFAULT;
> diff --git a/selftest/knownfail.d/samba3.base.delete b/selftest/knownfail.d/samba3.base.delete
> deleted file mode 100644
> index 6129ca7a66c..00000000000
> --- a/selftest/knownfail.d/samba3.base.delete
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -samba3.base.delete.deltest20\(nt4_dc\)
> -samba3.base.delete.deltest20\(ad_dc\)
> diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
> index f4ae0f3c02f..43dbfe88856 100755
> --- a/selftest/target/Samba3.pm
> +++ b/selftest/target/Samba3.pm
> @@ -231,6 +231,7 @@ sub setup_nt4_dc($$)
> rpc_daemon:lsasd = fork
> rpc_daemon:fssd = fork
> fss: sequence timeout = 1
> + check parent directory delete on close = yes
> ";
>
> my $vars = $self->provision($path, "SAMBA-TEST",
> diff --git a/selftest/target/Samba4.pm b/selftest/target/Samba4.pm
> index c161ee082a0..123bf6c3a91 100755
> --- a/selftest/target/Samba4.pm
> +++ b/selftest/target/Samba4.pm
> @@ -1815,6 +1815,7 @@ sub provision_ad_dc($$$$$$)
> smbd:writetimeupdatedelay = 500000
> create mask = 755
> dos filemode = yes
> + check parent directory delete on close = yes
>
> dcerpc endpoint servers = -winreg -srvsvc
>
> diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
> index 096c23f4fb3..b1b502c117e 100644
> --- a/source3/param/loadparm.c
> +++ b/source3/param/loadparm.c
> @@ -244,6 +244,7 @@ static const struct loadparm_service _sDefault =
> .smb_encrypt = SMB_SIGNING_DEFAULT,
> .kernel_share_modes = true,
> .durable_handles = true,
> + .check_parent_directory_delete_on_close = false,
> .param_opt = NULL,
> .dummy = ""
> };
> diff --git a/source3/smbd/open.c b/source3/smbd/open.c
> index e55c3941092..5817bdbe9ae 100644
> --- a/source3/smbd/open.c
> +++ b/source3/smbd/open.c
> @@ -253,6 +253,11 @@ NTSTATUS check_parent_access(struct connection_struct *conn,
> struct security_descriptor *parent_sd = NULL;
> uint32_t access_granted = 0;
> struct smb_filename *parent_smb_fname = NULL;
> + struct share_mode_lock *lck = NULL;
> + struct file_id id = {0};
> + uint32_t name_hash;
> + bool delete_on_close_set;
> + int ret;
>
> if (!parent_dirname(talloc_tos(),
> smb_fname->base_name,
> @@ -320,7 +325,45 @@ NTSTATUS check_parent_access(struct connection_struct *conn,
> return status;
> }
>
> - return NT_STATUS_OK;
> + if (!(access_mask & (SEC_DIR_ADD_FILE | SEC_DIR_ADD_SUBDIR))) {
> + return NT_STATUS_OK;
> + }
> + if (!lp_check_parent_directory_delete_on_close(SNUM(conn))) {
> + return NT_STATUS_OK;
> + }
> +
> + /* Check if the directory has delete-on-close set */
> + ret = SMB_VFS_STAT(conn, parent_smb_fname);
> + if (ret != 0) {
> + status = map_nt_error_from_unix(errno);
> + goto out;
> + }
> +
> + id = SMB_VFS_FILE_ID_CREATE(conn, &parent_smb_fname->st);
> +
> + status = file_name_hash(conn, parent_smb_fname->base_name, &name_hash);
> + if (!NT_STATUS_IS_OK(status)) {
> + goto out;
> + }
> +
> + lck = get_existing_share_mode_lock(talloc_tos(), id);
> + if (lck == NULL) {
> + status = NT_STATUS_OK;
> + goto out;
> + }
> +
> + delete_on_close_set = is_delete_on_close_set(lck, name_hash);
> + if (delete_on_close_set) {
> + status = NT_STATUS_DELETE_PENDING;
> + goto out;
> + }
> +
> + status = NT_STATUS_OK;
> +
> +out:
> + TALLOC_FREE(lck);
> + TALLOC_FREE(parent_smb_fname);
> + return status;
> }
>
> /****************************************************************************
> --
> 2.13.6
>
More information about the samba-technical
mailing list