[PATCH] Make Durable-Handle reconnect more robust
Ralph Böhme
slow at samba.org
Thu Aug 30 17:50:18 UTC 2018
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!
-slow
--
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
-------------- next part --------------
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