[PATCH] Make Durable-Handle reconnect more robust
Jeremy Allison
jra at samba.org
Thu Aug 30 23:05:44 UTC 2018
On Thu, Aug 30, 2018 at 07:50:18PM +0200, Ralph Böhme via samba-technical wrote:
> On Thu, Aug 30, 2018 at 05:17:56PM +0200, Ralph Böhme via samba-technical wrote:
> > On Thu, Aug 30, 2018 at 04:37:06PM +0200, Ralph Böhme via samba-technical wrote:
> > > Please review&push if happy. Thanks!
> >
> > gimme a second, I'm trying to add a test via a new VFS module "delay_inject".
>
> here it is.
>
> Please review&push if happy. Thanks!
LGTM, Ralph. RB+ and pushed, thanks !
> --
> Ralph Boehme, Samba Team https://samba.org/
> Samba Developer, SerNet GmbH https://sernet.de/en/samba/
> GPG Key Fingerprint: FAE2 C608 8A24 2520 51C5
> 59E4 AA1E 9B71 2639 9E46
> From 79dfa7a284a039908c56e877e05310f9bb43667c Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Thu, 30 Aug 2018 17:27:08 +0200
> Subject: [PATCH 1/5] vfs_delay_inject: adding delay to VFS calls
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=13549
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> source3/modules/vfs_delay_inject.c | 58 ++++++++++++++++++++++++++++++++++++++
> source3/modules/wscript_build | 7 +++++
> source3/wscript | 1 +
> 3 files changed, 66 insertions(+)
> create mode 100644 source3/modules/vfs_delay_inject.c
>
> diff --git a/source3/modules/vfs_delay_inject.c b/source3/modules/vfs_delay_inject.c
> new file mode 100644
> index 00000000000..21fea9b10f4
> --- /dev/null
> +++ b/source3/modules/vfs_delay_inject.c
> @@ -0,0 +1,58 @@
> +/*
> + * Unix SMB/CIFS implementation.
> + * Samba VFS module for delay injection in VFS calls
> + * Copyright (C) Ralph Boehme 2018
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "includes.h"
> +#include "smbd/smbd.h"
> +
> +#undef DBGC_CLASS
> +#define DBGC_CLASS DBGC_VFS
> +
> +static void inject_delay(const char *vfs_func, vfs_handle_struct *handle)
> +{
> + int delay;
> +
> + delay = lp_parm_int(SNUM(handle->conn), "delay_inject", vfs_func, 0);
> + if (delay == 0) {
> + return;
> + }
> +
> + DBG_DEBUG("Injected delay for [%s] of [%d] ms\n", vfs_func, delay);
> +
> + smb_msleep(delay);
> +}
> +
> +static int vfs_delay_inject_ntimes(vfs_handle_struct *handle,
> + const struct smb_filename *smb_fname,
> + struct smb_file_time *ft)
> +{
> + inject_delay("ntimes", handle);
> +
> + return SMB_VFS_NEXT_NTIMES(handle, smb_fname, ft);
> +}
> +
> +static struct vfs_fn_pointers vfs_delay_inject_fns = {
> + .ntimes_fn = vfs_delay_inject_ntimes,
> +};
> +
> +static_decl_vfs;
> +NTSTATUS vfs_delay_inject_init(TALLOC_CTX *ctx)
> +{
> + return smb_register_vfs(SMB_VFS_INTERFACE_VERSION, "delay_inject",
> + &vfs_delay_inject_fns);
> +}
> diff --git a/source3/modules/wscript_build b/source3/modules/wscript_build
> index 9f08e306408..6677ced3db1 100644
> --- a/source3/modules/wscript_build
> +++ b/source3/modules/wscript_build
> @@ -571,3 +571,10 @@ bld.SAMBA3_MODULE('vfs_error_inject',
> init_function='',
> internal_module=bld.SAMBA3_IS_STATIC_MODULE('vfs_error_inject'),
> enabled=bld.SAMBA3_IS_ENABLED_MODULE('vfs_error_inject'))
> +
> +bld.SAMBA3_MODULE('vfs_delay_inject',
> + subsystem='vfs',
> + source='vfs_delay_inject.c',
> + init_function='',
> + internal_module=bld.SAMBA3_IS_STATIC_MODULE('vfs_delay_inject'),
> + enabled=bld.SAMBA3_IS_ENABLED_MODULE('vfs_delay_inject'))
> diff --git a/source3/wscript b/source3/wscript
> index c27aeb182e8..e1b24d05707 100644
> --- a/source3/wscript
> +++ b/source3/wscript
> @@ -1677,6 +1677,7 @@ main() {
> if Options.options.enable_selftest or Options.options.developer:
> default_shared_modules.extend(TO_LIST('vfs_fake_acls vfs_nfs4acl_xattr'))
> default_shared_modules.extend(TO_LIST('vfs_error_inject'))
> + default_shared_modules.extend(TO_LIST('vfs_delay_inject'))
>
> if conf.CONFIG_SET('AD_DC_BUILD_IS_ENABLED'):
> default_static_modules.extend(TO_LIST('pdb_samba_dsdb auth_samba4 vfs_dfs_samba4'))
> --
> 2.13.6
>
>
> From 3dea031e1b67536e81325e11cfcb8b529244cb2b Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Thu, 30 Aug 2018 19:15:19 +0200
> Subject: [PATCH 2/5] selftest: add a durable handle test with delayed
> disconnect
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=13549
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> selftest/knownfail.d/samba3.blackbox | 1 +
> selftest/target/Samba3.pm | 8 ++
> .../script/tests/test_durable_handle_reconnect.sh | 21 +++++
> source3/selftest/tests.py | 3 +
> source4/torture/smb2/durable_v2_open.c | 95 ++++++++++++++++++++++
> source4/torture/smb2/smb2.c | 2 +
> 6 files changed, 130 insertions(+)
> create mode 100644 selftest/knownfail.d/samba3.blackbox
> create mode 100755 source3/script/tests/test_durable_handle_reconnect.sh
>
> diff --git a/selftest/knownfail.d/samba3.blackbox b/selftest/knownfail.d/samba3.blackbox
> new file mode 100644
> index 00000000000..b03a1ac09e2
> --- /dev/null
> +++ b/selftest/knownfail.d/samba3.blackbox
> @@ -0,0 +1 @@
> +^samba3.blackbox.durable_v2_delay.durable_v2_delay\(simpleserver:local\)
> diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
> index d90c8f7e85b..bcbf0addf2b 100755
> --- a/selftest/target/Samba3.pm
> +++ b/selftest/target/Samba3.pm
> @@ -2218,6 +2218,14 @@ sub provision($$$$$$$$$)
> copy = tmp
> vfs objects = error_inject
> include = $libdir/error_inject.conf
> +
> +[delay_inject]
> + copy = tmp
> + vfs objects = delay_inject
> + kernel share modes = no
> + kernel oplocks = no
> + posix locking = no
> + include = $libdir/delay_inject.conf
> ";
> close(CONF);
>
> diff --git a/source3/script/tests/test_durable_handle_reconnect.sh b/source3/script/tests/test_durable_handle_reconnect.sh
> new file mode 100755
> index 00000000000..bca8e2def96
> --- /dev/null
> +++ b/source3/script/tests/test_durable_handle_reconnect.sh
> @@ -0,0 +1,21 @@
> +#!/bin/sh
> +#
> +# Test Durable Handle reconnect with injected delay in the disconnect.
> +#
> +# Copyright (C) 2018 Ralph Boehme
> +
> +. $(dirname $0)/../../../testprogs/blackbox/subunit.sh
> +failed=0
> +
> +delay_inject_conf=$(dirname $SMB_CONF_PATH)/delay_inject.conf
> +
> +echo 'delay_inject:ntimes = 5000' > $delay_inject_conf
> +
> +testit "durable_v2_delay" $VALGRIND \
> + $BINDIR/smbtorture //$SERVER_IP/delay_inject \
> + -U$USERNAME%$PASSWORD smb2.durable-v2-delay ||
> + failed=$(expr $failed + 1)
> +
> +rm $delay_inject_conf
> +
> +testok $0 $failed
> diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
> index 3b6735231de..39bfb508e87 100755
> --- a/source3/selftest/tests.py
> +++ b/source3/selftest/tests.py
> @@ -651,6 +651,9 @@ plantestsuite("samba3.blackbox.net_tdb", "simpleserver:local",
> plantestsuite("samba3.blackbox.smbd_error", "simpleserver:local",
> [os.path.join(samba3srcdir, "script/tests/test_smbd_error.sh")])
>
> +plantestsuite("samba3.blackbox.durable_v2_delay", "simpleserver:local",
> + [os.path.join(samba3srcdir, "script/tests/test_durable_handle_reconnect.sh")])
> +
> plantestsuite("samba3.blackbox.net_cache_samlogon", "ad_member:local",
> [os.path.join(samba3srcdir, "script/tests/test_net_cache_samlogon.sh"),
> '$SERVER', 'tmp', '$DC_USERNAME', '$DC_PASSWORD'])
> diff --git a/source4/torture/smb2/durable_v2_open.c b/source4/torture/smb2/durable_v2_open.c
> index 0a928ec8c26..25e6d27465a 100644
> --- a/source4/torture/smb2/durable_v2_open.c
> +++ b/source4/torture/smb2/durable_v2_open.c
> @@ -2035,3 +2035,98 @@ struct torture_suite *torture_smb2_durable_v2_open_init(TALLOC_CTX *ctx)
>
> return suite;
> }
> +
> +/**
> + * basic test for doing a durable open
> + * tcp disconnect, reconnect, do a durable reopen (succeeds)
> + */
> +static bool test_durable_v2_reconnect_delay(struct torture_context *tctx,
> + struct smb2_tree *tree)
> +{
> + NTSTATUS status;
> + TALLOC_CTX *mem_ctx = talloc_new(tctx);
> + char fname[256];
> + struct smb2_handle _h;
> + struct smb2_handle *h = NULL;
> + struct smb2_create io;
> + struct GUID create_guid = GUID_random();
> + struct smbcli_options options;
> + uint64_t previous_session_id;
> + uint8_t b = 0;
> + bool ret = true;
> +
> + options = tree->session->transport->options;
> + previous_session_id = smb2cli_session_current_id(tree->session->smbXcli);
> +
> + /* Choose a random name in case the state is left a little funky. */
> + snprintf(fname, 256, "durable_v2_reconnect_delay_%s.dat",
> + generate_random_str(tctx, 8));
> +
> + smb2_util_unlink(tree, fname);
> +
> + smb2_oplock_create_share(&io, fname,
> + smb2_util_share_access(""),
> + smb2_util_oplock_level("b"));
> + io.in.durable_open = false;
> + io.in.durable_open_v2 = true;
> + io.in.persistent_open = false;
> + io.in.create_guid = create_guid;
> + io.in.timeout = 0;
> +
> + status = smb2_create(tree, mem_ctx, &io);
> + CHECK_STATUS(status, NT_STATUS_OK);
> +
> + _h = io.out.file.handle;
> + h = &_h;
> + CHECK_VAL(io.out.oplock_level, smb2_util_oplock_level("b"));
> + CHECK_VAL(io.out.durable_open_v2, true);
> +
> + status = smb2_util_write(tree, *h, &b, 0, 1);
> + CHECK_STATUS(status, NT_STATUS_OK);
> +
> + /* disconnect, leaving the durable open */
> + TALLOC_FREE(tree);
> +
> + if (!torture_smb2_connection_ext(tctx, previous_session_id,
> + &options, &tree)) {
> + torture_warning(tctx, "couldn't reconnect, bailing\n");
> + ret = false;
> + goto done;
> + }
> +
> + ZERO_STRUCT(io);
> + io.in.fname = fname;
> + io.in.durable_open_v2 = false;
> + io.in.durable_handle_v2 = h;
> + io.in.create_guid = create_guid;
> + h = NULL;
> +
> + status = smb2_create(tree, mem_ctx, &io);
> + CHECK_STATUS(status, NT_STATUS_OK);
> + CHECK_VAL(io.out.oplock_level, smb2_util_oplock_level("b"));
> + _h = io.out.file.handle;
> + h = &_h;
> +
> +done:
> + if (h != NULL) {
> + smb2_util_close(tree, *h);
> + }
> +
> + smb2_util_unlink(tree, fname);
> +
> + talloc_free(tree);
> +
> + talloc_free(mem_ctx);
> +
> + return ret;
> +}
> +
> +struct torture_suite *torture_smb2_durable_v2_delay_init(TALLOC_CTX *ctx)
> +{
> + struct torture_suite *suite =
> + torture_suite_create(ctx, "durable-v2-delay");
> +
> + torture_suite_add_1smb2_test(suite, "durable_v2_reconnect_delay", test_durable_v2_reconnect_delay);
> +
> + return suite;
> +}
> diff --git a/source4/torture/smb2/smb2.c b/source4/torture/smb2/smb2.c
> index 344cf5a40a5..12f7edf8f86 100644
> --- a/source4/torture/smb2/smb2.c
> +++ b/source4/torture/smb2/smb2.c
> @@ -163,6 +163,8 @@ NTSTATUS torture_smb2_init(TALLOC_CTX *ctx)
> torture_smb2_durable_open_disconnect_init(suite));
> torture_suite_add_suite(suite,
> torture_smb2_durable_v2_open_init(suite));
> + torture_suite_add_suite(suite,
> + torture_smb2_durable_v2_delay_init(suite));
> torture_suite_add_suite(suite, torture_smb2_dir_init(suite));
> torture_suite_add_suite(suite, torture_smb2_lease_init(suite));
> torture_suite_add_suite(suite, torture_smb2_compound_init(suite));
> --
> 2.13.6
>
>
> From 3da871d4099cae3aae70d8c541d0e287613b3e85 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Thu, 30 Aug 2018 15:50:02 +0200
> Subject: [PATCH 3/5] s3:smbd: reorder tcon global record deletion and closing
> files of a tcon
>
> As such, this doesn't change overall behaviour, but in case we ever add
> semantics acting on tcon record changes via an API like
> dbwrap_watch_send(), this will make a difference as it enforces
> ordering.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=13549
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> source3/smbd/smbXsrv_tcon.c | 38 +++++++++++++++++++-------------------
> 1 file changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/source3/smbd/smbXsrv_tcon.c b/source3/smbd/smbXsrv_tcon.c
> index eb483937d63..ded4010f5da 100644
> --- a/source3/smbd/smbXsrv_tcon.c
> +++ b/source3/smbd/smbXsrv_tcon.c
> @@ -904,6 +904,25 @@ NTSTATUS smbXsrv_tcon_disconnect(struct smbXsrv_tcon *tcon, uint64_t vuid)
> table = tcon->table;
> tcon->table = NULL;
>
> + if (tcon->compat) {
> + bool ok;
> +
> + ok = chdir_current_service(tcon->compat);
> + if (!ok) {
> + status = NT_STATUS_INTERNAL_ERROR;
> + DEBUG(0, ("smbXsrv_tcon_disconnect(0x%08x, '%s'): "
> + "chdir_current_service() failed: %s\n",
> + tcon->global->tcon_global_id,
> + tcon->global->share_name,
> + nt_errstr(status)));
> + tcon->compat = NULL;
> + return status;
> + }
> +
> + close_cnum(tcon->compat, vuid);
> + tcon->compat = NULL;
> + }
> +
> tcon->status = NT_STATUS_NETWORK_NAME_DELETED;
>
> global_rec = tcon->global->db_rec;
> @@ -966,25 +985,6 @@ NTSTATUS smbXsrv_tcon_disconnect(struct smbXsrv_tcon *tcon, uint64_t vuid)
> }
> tcon->db_rec = NULL;
>
> - if (tcon->compat) {
> - bool ok;
> -
> - ok = chdir_current_service(tcon->compat);
> - if (!ok) {
> - status = NT_STATUS_INTERNAL_ERROR;
> - DEBUG(0, ("smbXsrv_tcon_disconnect(0x%08x, '%s'): "
> - "chdir_current_service() failed: %s\n",
> - tcon->global->tcon_global_id,
> - tcon->global->share_name,
> - nt_errstr(status)));
> - tcon->compat = NULL;
> - return status;
> - }
> -
> - close_cnum(tcon->compat, vuid);
> - tcon->compat = NULL;
> - }
> -
> return error;
> }
>
> --
> 2.13.6
>
>
> From f527a74e92a8cc75adfbdd36ca0a4e7aa22d9f93 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Wed, 29 Aug 2018 17:19:29 +0200
> Subject: [PATCH 4/5] s3:smbd: let session logoff close files and tcons before
> deleting the session
>
> This avoids a race in durable handle reconnects if the reconnect comes
> in while the old session is still in the tear-down phase.
>
> The new session is supposed to rendezvous with and wait for destruction
> of the old session, which is internally implemented with
> dbwrap_watch_send() on the old session record.
>
> If the old session deletes the session record before calling
> file_close_user() which marks all file handles as disconnected, the
> durable handle reconnect in the new session will fail as the records are
> not yet marked as disconnected which is a prerequisite.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=13549
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> selftest/knownfail.d/samba3.blackbox | 1 -
> source3/smbd/smbXsrv_session.c | 46 ++++++++++++++++++------------------
> 2 files changed, 23 insertions(+), 24 deletions(-)
> delete mode 100644 selftest/knownfail.d/samba3.blackbox
>
> diff --git a/selftest/knownfail.d/samba3.blackbox b/selftest/knownfail.d/samba3.blackbox
> deleted file mode 100644
> index b03a1ac09e2..00000000000
> --- a/selftest/knownfail.d/samba3.blackbox
> +++ /dev/null
> @@ -1 +0,0 @@
> -^samba3.blackbox.durable_v2_delay.durable_v2_delay\(simpleserver:local\)
> diff --git a/source3/smbd/smbXsrv_session.c b/source3/smbd/smbXsrv_session.c
> index af47f4aa8c3..1193b4a3bea 100644
> --- a/source3/smbd/smbXsrv_session.c
> +++ b/source3/smbd/smbXsrv_session.c
> @@ -1663,6 +1663,29 @@ NTSTATUS smbXsrv_session_logoff(struct smbXsrv_session *session)
> session->client = NULL;
> session->status = NT_STATUS_USER_SESSION_DELETED;
>
> + if (session->compat) {
> + file_close_user(sconn, session->compat->vuid);
> + }
> +
> + if (session->tcon_table != NULL) {
> + /*
> + * Note: We only have a tcon_table for SMB2.
> + */
> + status = smb2srv_tcon_disconnect_all(session);
> + if (!NT_STATUS_IS_OK(status)) {
> + DEBUG(0, ("smbXsrv_session_logoff(0x%08x): "
> + "smb2srv_tcon_disconnect_all() failed: %s\n",
> + session->global->session_global_id,
> + nt_errstr(status)));
> + error = status;
> + }
> + }
> +
> + if (session->compat) {
> + invalidate_vuid(sconn, session->compat->vuid);
> + session->compat = NULL;
> + }
> +
> global_rec = session->global->db_rec;
> session->global->db_rec = NULL;
> if (global_rec == NULL) {
> @@ -1722,29 +1745,6 @@ NTSTATUS smbXsrv_session_logoff(struct smbXsrv_session *session)
> }
> session->db_rec = NULL;
>
> - if (session->compat) {
> - file_close_user(sconn, session->compat->vuid);
> - }
> -
> - if (session->tcon_table != NULL) {
> - /*
> - * Note: We only have a tcon_table for SMB2.
> - */
> - status = smb2srv_tcon_disconnect_all(session);
> - if (!NT_STATUS_IS_OK(status)) {
> - DEBUG(0, ("smbXsrv_session_logoff(0x%08x): "
> - "smb2srv_tcon_disconnect_all() failed: %s\n",
> - session->global->session_global_id,
> - nt_errstr(status)));
> - error = status;
> - }
> - }
> -
> - if (session->compat) {
> - invalidate_vuid(sconn, session->compat->vuid);
> - session->compat = NULL;
> - }
> -
> return error;
> }
>
> --
> 2.13.6
>
>
> From 7c44564854d0f2e9d3af6e84516edbc8be880643 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Thu, 30 Aug 2018 15:57:33 +0200
> Subject: [PATCH 5/5] s3:smbd: add a comment stating that file_close_user() is
> redundant for SMB2
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=13549
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
> source3/smbd/smbXsrv_session.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/source3/smbd/smbXsrv_session.c b/source3/smbd/smbXsrv_session.c
> index 1193b4a3bea..19f4bc05600 100644
> --- a/source3/smbd/smbXsrv_session.c
> +++ b/source3/smbd/smbXsrv_session.c
> @@ -1664,6 +1664,12 @@ NTSTATUS smbXsrv_session_logoff(struct smbXsrv_session *session)
> session->status = NT_STATUS_USER_SESSION_DELETED;
>
> if (session->compat) {
> + /*
> + * For SMB2 this is a bit redundant as files are also close
> + * below via smb2srv_tcon_disconnect_all() -> ... ->
> + * smbXsrv_tcon_disconnect() -> close_cnum() ->
> + * file_close_conn().
> + */
> file_close_user(sconn, session->compat->vuid);
> }
>
> --
> 2.13.6
>
More information about the samba-technical
mailing list