From c39754e003c30ec7c7c953a88e450e42c69935de Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Mon, 7 Aug 2023 11:03:41 +0200 Subject: [PATCH 1/6] s4:torture/smb2: let torture_smb2_con_sopt() use smb2_connect() There's no need for smb2_connect_ext(). BUG: https://bugzilla.samba.org/show_bug.cgi?id=15346 Signed-off-by: Stefan Metzmacher Reviewed-by: Andreas Schneider (cherry picked from commit ade663ee6ca1a2813b203ea667d933f4dab9e7b7) --- source4/torture/smb2/util.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/source4/torture/smb2/util.c b/source4/torture/smb2/util.c index ae6c14ebcbd2..01b1b2f0793a 100644 --- a/source4/torture/smb2/util.c +++ b/source4/torture/smb2/util.c @@ -475,19 +475,18 @@ bool torture_smb2_con_sopt(struct torture_context *tctx, return false; } - status = smb2_connect_ext(tctx, - host, - lpcfg_smb_ports(tctx->lp_ctx), - share, - lpcfg_resolve_context(tctx->lp_ctx), - samba_cmdline_get_creds(), - 0, - tree, - tctx->ev, - &options, - lpcfg_socket_options(tctx->lp_ctx), - lpcfg_gensec_settings(tctx, tctx->lp_ctx) - ); + status = smb2_connect(tctx, + host, + lpcfg_smb_ports(tctx->lp_ctx), + share, + lpcfg_resolve_context(tctx->lp_ctx), + samba_cmdline_get_creds(), + tree, + tctx->ev, + &options, + lpcfg_socket_options(tctx->lp_ctx), + lpcfg_gensec_settings(tctx, tctx->lp_ctx) + ); if (!NT_STATUS_IS_OK(status)) { torture_comment(tctx, "Failed to connect to SMB2 share \\\\%s\\%s - %s\n", host, share, nt_errstr(status)); -- 2.34.1 From 82b2ba15e2fc2979c90ef65e8cf24d6e47e8ef69 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Mon, 7 Aug 2023 11:03:41 +0200 Subject: [PATCH 2/6] s4:torture/smb2: let us have a common torture_smb2_con_share() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15346 Signed-off-by: Stefan Metzmacher Reviewed-by: Andreas Schneider (cherry picked from commit dc5a500f0a76720b2a5cb5b1142cf4c35cb6bdea) --- source4/torture/smb2/acls.c | 34 --------------------------------- source4/torture/smb2/util.c | 29 +++++++++++++++++++--------- source4/torture/vfs/acl_xattr.c | 34 --------------------------------- 3 files changed, 20 insertions(+), 77 deletions(-) diff --git a/source4/torture/smb2/acls.c b/source4/torture/smb2/acls.c index a5df9da3b454..a27d4e079e67 100644 --- a/source4/torture/smb2/acls.c +++ b/source4/torture/smb2/acls.c @@ -2139,40 +2139,6 @@ done: } #endif -/** - * SMB2 connect with explicit share - **/ -static bool torture_smb2_con_share(struct torture_context *tctx, - const char *share, - struct smb2_tree **tree) -{ - struct smbcli_options options; - NTSTATUS status; - const char *host = torture_setting_string(tctx, "host", NULL); - - lpcfg_smbcli_options(tctx->lp_ctx, &options); - - status = smb2_connect_ext(tctx, - host, - lpcfg_smb_ports(tctx->lp_ctx), - share, - lpcfg_resolve_context(tctx->lp_ctx), - samba_cmdline_get_creds(), - 0, - tree, - tctx->ev, - &options, - lpcfg_socket_options(tctx->lp_ctx), - lpcfg_gensec_settings(tctx, tctx->lp_ctx) - ); - if (!NT_STATUS_IS_OK(status)) { - torture_comment(tctx, "Failed to connect to SMB2 share \\\\%s\\%s - %s\n", - host, share, nt_errstr(status)); - return false; - } - return true; -} - static bool test_access_based(struct torture_context *tctx, struct smb2_tree *tree) { diff --git a/source4/torture/smb2/util.c b/source4/torture/smb2/util.c index 01b1b2f0793a..ecf80d9105b1 100644 --- a/source4/torture/smb2/util.c +++ b/source4/torture/smb2/util.c @@ -459,22 +459,16 @@ bool torture_smb2_connection(struct torture_context *tctx, struct smb2_tree **tr /** * SMB2 connect with share from soption **/ -bool torture_smb2_con_sopt(struct torture_context *tctx, - const char *soption, - struct smb2_tree **tree) +bool torture_smb2_con_share(struct torture_context *tctx, + const char *share, + struct smb2_tree **tree) { struct smbcli_options options; NTSTATUS status; const char *host = torture_setting_string(tctx, "host", NULL); - const char *share = torture_setting_string(tctx, soption, NULL); lpcfg_smbcli_options(tctx->lp_ctx, &options); - if (share == NULL) { - torture_comment(tctx, "No share for option %s\n", soption); - return false; - } - status = smb2_connect(tctx, host, lpcfg_smb_ports(tctx->lp_ctx), @@ -495,6 +489,23 @@ bool torture_smb2_con_sopt(struct torture_context *tctx, return true; } +/** + * SMB2 connect with share from soption + **/ +bool torture_smb2_con_sopt(struct torture_context *tctx, + const char *soption, + struct smb2_tree **tree) +{ + const char *share = torture_setting_string(tctx, soption, NULL); + + if (share == NULL) { + torture_comment(tctx, "No share for option %s\n", soption); + return false; + } + + return torture_smb2_con_share(tctx, share, tree); +} + /* create and return a handle to a test file with a specific access mask diff --git a/source4/torture/vfs/acl_xattr.c b/source4/torture/vfs/acl_xattr.c index 46d8ead6cb03..1deb2b3b9984 100644 --- a/source4/torture/vfs/acl_xattr.c +++ b/source4/torture/vfs/acl_xattr.c @@ -47,40 +47,6 @@ } \ } while (0) -/** - * SMB2 connect with explicit share - **/ -static bool torture_smb2_con_share(struct torture_context *tctx, - const char *share, - struct smb2_tree **tree) -{ - struct smbcli_options options; - NTSTATUS status; - const char *host = torture_setting_string(tctx, "host", NULL); - - lpcfg_smbcli_options(tctx->lp_ctx, &options); - - status = smb2_connect_ext(tctx, - host, - lpcfg_smb_ports(tctx->lp_ctx), - share, - lpcfg_resolve_context(tctx->lp_ctx), - samba_cmdline_get_creds(), - 0, - tree, - tctx->ev, - &options, - lpcfg_socket_options(tctx->lp_ctx), - lpcfg_gensec_settings(tctx, tctx->lp_ctx) - ); - if (!NT_STATUS_IS_OK(status)) { - printf("Failed to connect to SMB2 share \\\\%s\\%s - %s\n", - host, share, nt_errstr(status)); - return false; - } - return true; -} - static bool test_default_acl_posix(struct torture_context *tctx, struct smb2_tree *tree_unused) { -- 2.34.1 From 3f069855df2d09f5a03a8e4259afca4c1ea47a1e Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Mon, 7 Aug 2023 12:22:43 +0200 Subject: [PATCH 3/6] s4:torture/smb2: make it possible to pass existing_conn to smb2_connect_ext() BUG: https://bugzilla.samba.org/show_bug.cgi?id=15346 Signed-off-by: Stefan Metzmacher Reviewed-by: Andreas Schneider (cherry picked from commit 2b93058be3f6e5eaee239ad3b0e707c62089d18e) --- source4/libcli/smb2/connect.c | 4 +++- source4/torture/smb2/util.c | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/source4/libcli/smb2/connect.c b/source4/libcli/smb2/connect.c index 1f68d90538b2..64b678654468 100644 --- a/source4/libcli/smb2/connect.c +++ b/source4/libcli/smb2/connect.c @@ -405,6 +405,7 @@ NTSTATUS smb2_connect_ext(TALLOC_CTX *mem_ctx, const char *share, struct resolve_context *resolve_ctx, struct cli_credentials *credentials, + struct smbXcli_conn **existing_conn, uint64_t previous_session_id, struct smb2_tree **tree, struct tevent_context *ev, @@ -429,7 +430,7 @@ NTSTATUS smb2_connect_ext(TALLOC_CTX *mem_ctx, resolve_ctx, credentials, false, /* fallback_to_anonymous */ - NULL, /* existing_conn */ + existing_conn, previous_session_id, options, socket_options, @@ -473,6 +474,7 @@ NTSTATUS smb2_connect(TALLOC_CTX *mem_ctx, status = smb2_connect_ext(mem_ctx, host, ports, share, resolve_ctx, credentials, + NULL, /* existing_conn */ 0, /* previous_session_id */ tree, ev, options, socket_options, gensec_settings); diff --git a/source4/torture/smb2/util.c b/source4/torture/smb2/util.c index ecf80d9105b1..233f589c73f6 100644 --- a/source4/torture/smb2/util.c +++ b/source4/torture/smb2/util.c @@ -426,6 +426,7 @@ bool torture_smb2_connection_ext(struct torture_context *tctx, share, lpcfg_resolve_context(tctx->lp_ctx), samba_cmdline_get_creds(), + NULL, /* existing_conn */ previous_session_id, tree, tctx->ev, -- 2.34.1 From cb87d5a9668975907d584cf2a0a1dee0df61238e Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 4 Aug 2023 17:16:14 +0200 Subject: [PATCH 4/6] s4:torture/smb2: add smb2.multichannel.bugs.bug_15346 This demonstrates the race quite easily against Samba and works fine against Windows Server 2022. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15346 Signed-off-by: Stefan Metzmacher Reviewed-by: Andreas Schneider (cherry picked from commit 4028d6582907cf582730ceec56872d8584ad02e6) --- selftest/knownfail.d/samba3.smb2.multichannel | 1 + source4/torture/smb2/multichannel.c | 315 ++++++++++++++++++ 2 files changed, 316 insertions(+) create mode 100644 selftest/knownfail.d/samba3.smb2.multichannel diff --git a/selftest/knownfail.d/samba3.smb2.multichannel b/selftest/knownfail.d/samba3.smb2.multichannel new file mode 100644 index 000000000000..26b80a39d5be --- /dev/null +++ b/selftest/knownfail.d/samba3.smb2.multichannel @@ -0,0 +1 @@ +^samba3.smb2.multichannel.bugs.bug_15346 diff --git a/source4/torture/smb2/multichannel.c b/source4/torture/smb2/multichannel.c index 801c1ec801f4..cae28ff1bed6 100644 --- a/source4/torture/smb2/multichannel.c +++ b/source4/torture/smb2/multichannel.c @@ -31,6 +31,7 @@ #include "lib/cmdline/cmdline.h" #include "libcli/security/security.h" #include "libcli/resolve/resolve.h" +#include "lib/socket/socket.h" #include "lib/param/param.h" #include "lib/events/events.h" #include "oplock_break_handler.h" @@ -2343,6 +2344,315 @@ done: return ret; } +/* + * Test channel merging race + * This is a regression test for + * https://bugzilla.samba.org/show_bug.cgi?id=15346 + */ +struct test_multichannel_bug_15346_conn; + +struct test_multichannel_bug_15346_state { + struct torture_context *tctx; + struct test_multichannel_bug_15346_conn *conns; + size_t num_conns; + size_t num_ready; + bool asserted; + bool looping; +}; + +struct test_multichannel_bug_15346_conn { + struct test_multichannel_bug_15346_state *state; + size_t idx; + struct smbXcli_conn *smbXcli; + struct tevent_req *nreq; + struct tevent_req *ereq; +}; + +static void test_multichannel_bug_15346_ndone(struct tevent_req *subreq); +static void test_multichannel_bug_15346_edone(struct tevent_req *subreq); + +static void test_multichannel_bug_15346_ndone(struct tevent_req *subreq) +{ + struct test_multichannel_bug_15346_conn *conn = + (struct test_multichannel_bug_15346_conn *) + tevent_req_callback_data_void(subreq); + struct test_multichannel_bug_15346_state *state = conn->state; + struct torture_context *tctx = state->tctx; + NTSTATUS status; + bool ok = false; + + SMB_ASSERT(conn->nreq == subreq); + conn->nreq = NULL; + + status = smbXcli_negprot_recv(subreq, NULL, NULL); + TALLOC_FREE(subreq); + torture_assert_ntstatus_ok_goto(tctx, status, ok, asserted, + "smbXcli_negprot_recv failed"); + + torture_comment(tctx, "conn[%zu]: negprot done\n", conn->idx); + + conn->ereq = smb2cli_echo_send(conn->smbXcli, + tctx->ev, + conn->smbXcli, + state->num_conns * 2 * 1000); + torture_assert_goto(tctx, conn->ereq != NULL, ok, asserted, + "smb2cli_echo_send"); + tevent_req_set_callback(conn->ereq, + test_multichannel_bug_15346_edone, + conn); + + return; + +asserted: + SMB_ASSERT(!ok); + state->asserted = true; + state->looping = false; + return; +} + +static void test_multichannel_bug_15346_edone(struct tevent_req *subreq) +{ + struct test_multichannel_bug_15346_conn *conn = + (struct test_multichannel_bug_15346_conn *) + tevent_req_callback_data_void(subreq); + struct test_multichannel_bug_15346_state *state = conn->state; + struct torture_context *tctx = state->tctx; + NTSTATUS status; + bool ok = false; + + SMB_ASSERT(conn->ereq == subreq); + conn->ereq = NULL; + + status = smb2cli_echo_recv(subreq); + TALLOC_FREE(subreq); + torture_assert_ntstatus_ok_goto(tctx, status, ok, asserted, + "smb2cli_echo_recv failed"); + + torture_comment(tctx, "conn[%zu]: echo done\n", conn->idx); + + state->num_ready += 1; + if (state->num_ready < state->num_conns) { + return; + } + + state->looping = false; + return; + +asserted: + SMB_ASSERT(!ok); + state->asserted = true; + state->looping = false; + return; +} + +static bool test_multichannel_bug_15346(struct torture_context *tctx, + struct smb2_tree *tree1) +{ + const char *host = torture_setting_string(tctx, "host", NULL); + const char *share = torture_setting_string(tctx, "share", NULL); + struct resolve_context *resolve_ctx = lpcfg_resolve_context(tctx->lp_ctx); + const char *socket_options = lpcfg_socket_options(tctx->lp_ctx); + struct gensec_settings *gsettings = NULL; + bool ret = true; + NTSTATUS status; + struct smb2_transport *transport1 = tree1->session->transport; + struct test_multichannel_bug_15346_state *state = NULL; + uint32_t server_capabilities; + struct smb2_handle root_handle = {{0}}; + size_t i; + + if (smbXcli_conn_protocol(transport1->conn) < PROTOCOL_SMB3_00) { + torture_fail(tctx, + "SMB 3.X Dialect family required for Multichannel" + " tests\n"); + } + + server_capabilities = smb2cli_conn_server_capabilities( + tree1->session->transport->conn); + if (!(server_capabilities & SMB2_CAP_MULTI_CHANNEL)) { + torture_fail(tctx, + "Server does not support multichannel."); + } + + torture_comment(tctx, "Testing for BUG 15346\n"); + + state = talloc_zero(tctx, struct test_multichannel_bug_15346_state); + torture_assert_goto(tctx, state != NULL, ret, done, + "talloc_zero"); + state->tctx = tctx; + + gsettings = lpcfg_gensec_settings(state, tctx->lp_ctx); + torture_assert_goto(tctx, gsettings != NULL, ret, done, + "lpcfg_gensec_settings"); + + /* + * 32 is the W2K12R2 and W2K16 limit + * add 31 additional connections + */ + state->num_conns = 31; + state->conns = talloc_zero_array(state, + struct test_multichannel_bug_15346_conn, + state->num_conns); + torture_assert_goto(tctx, state->conns != NULL, ret, done, + "talloc_zero_array"); + + /* + * First we open additional the tcp connections + */ + + for (i = 0; i < state->num_conns; i++) { + struct test_multichannel_bug_15346_conn *conn = &state->conns[i]; + struct socket_context *sock = NULL; + uint16_t port = 445; + struct smbcli_options options = transport1->options; + + conn->state = state; + conn->idx = i; + + status = socket_connect_multi(state->conns, + host, + 1, &port, + resolve_ctx, + tctx->ev, + &sock, + &port); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "socket_connect_multi failed"); + + conn->smbXcli = smbXcli_conn_create(state->conns, + sock->fd, + host, + SMB_SIGNING_OFF, + 0, + &options.client_guid, + options.smb2_capabilities, + &options.smb3_capabilities); + torture_assert_goto(tctx, conn->smbXcli != NULL, ret, done, + "smbXcli_conn_create failed"); + sock->fd = -1; + TALLOC_FREE(sock); + } + + /* + * Now prepare the async SMB2 Negotiate requests + */ + for (i = 0; i < state->num_conns; i++) { + struct test_multichannel_bug_15346_conn *conn = &state->conns[i]; + + conn->nreq = smbXcli_negprot_send(conn->smbXcli, + tctx->ev, + conn->smbXcli, + state->num_conns * 2 * 1000, + smbXcli_conn_protocol(transport1->conn), + smbXcli_conn_protocol(transport1->conn), + 33, /* max_credits */ + NULL); + torture_assert_goto(tctx, conn->nreq != NULL, ret, done, "smbXcli_negprot_send"); + tevent_req_set_callback(conn->nreq, + test_multichannel_bug_15346_ndone, + conn); + } + + /* + * now we loop until all negprot and the first round + * of echos are done. + */ + state->looping = true; + while (state->looping) { + torture_assert_goto(tctx, tevent_loop_once(tctx->ev) == 0, + ret, done, "tevent_loop_once"); + } + + if (state->asserted) { + ret = false; + goto done; + } + + /* + * No we check that the connections are still usable + */ + for (i = 0; i < state->num_conns; i++) { + struct test_multichannel_bug_15346_conn *conn = &state->conns[i]; + + torture_comment(tctx, "conn[%zu]: checking echo again1\n", conn->idx); + + status = smb2cli_echo(conn->smbXcli, 1000); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2cli_echo failed"); + } + + status = smb2_util_roothandle(tree1, &root_handle); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_util_roothandle failed"); + + /* + * No we check that the connections are still usable + */ + for (i = 0; i < state->num_conns; i++) { + struct test_multichannel_bug_15346_conn *conn = &state->conns[i]; + struct smbcli_options options = transport1->options; + struct smb2_session *session = NULL; + struct smb2_tree *tree = NULL; + union smb_fileinfo io; + + torture_comment(tctx, "conn[%zu]: checking session bind\n", conn->idx); + + /* + * Prepare smb2_{tree,session,transport} structures + * for the existing connection. + */ + options.only_negprot = true; + status = smb2_connect_ext(state->conns, + host, + NULL, /* ports */ + share, + resolve_ctx, + samba_cmdline_get_creds(), + &conn->smbXcli, + 0, /* previous_session_id */ + &tree, + tctx->ev, + &options, + socket_options, + gsettings); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_connect_ext failed"); + conn->smbXcli = tree->session->transport->conn; + + session = smb2_session_channel(tree->session->transport, + lpcfg_gensec_settings(tree, tctx->lp_ctx), + tree, + tree1->session); + torture_assert_goto(tctx, session != NULL, ret, done, + "smb2_session_channel failed"); + + status = smb2_session_setup_spnego(session, + samba_cmdline_get_creds(), + 0 /* previous_session_id */); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_session_setup_spnego failed"); + + /* + * Fix up the bound smb2_tree + */ + tree->session = session; + tree->smbXcli = tree1->smbXcli; + + ZERO_STRUCT(io); + io.generic.level = RAW_FILEINFO_SMB2_ALL_INFORMATION; + io.generic.in.file.handle = root_handle; + + status = smb2_getinfo_file(tree, tree, &io); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_getinfo_file failed"); + } + + done: + talloc_free(state); + + return ret; +} + struct torture_suite *torture_smb2_multichannel_init(TALLOC_CTX *ctx) { struct torture_suite *suite = torture_suite_create(ctx, "multichannel"); @@ -2352,10 +2662,13 @@ struct torture_suite *torture_smb2_multichannel_init(TALLOC_CTX *ctx) "oplocks"); struct torture_suite *suite_leases = torture_suite_create(ctx, "leases"); + struct torture_suite *suite_bugs = torture_suite_create(ctx, + "bugs"); torture_suite_add_suite(suite, suite_generic); torture_suite_add_suite(suite, suite_oplocks); torture_suite_add_suite(suite, suite_leases); + torture_suite_add_suite(suite, suite_bugs); torture_suite_add_1smb2_test(suite_generic, "interface_info", test_multichannel_interface_info); @@ -2377,6 +2690,8 @@ struct torture_suite *torture_smb2_multichannel_init(TALLOC_CTX *ctx) test_multichannel_lease_break_test3); torture_suite_add_1smb2_test(suite_leases, "test4", test_multichannel_lease_break_test4); + torture_suite_add_1smb2_test(suite_bugs, "bug_15346", + test_multichannel_bug_15346); suite->description = talloc_strdup(suite, "SMB2 Multichannel tests"); -- 2.34.1 From 2e33ed66b9617ba8fe43709d296509498cf0e777 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 3 Aug 2023 15:34:29 +0200 Subject: [PATCH 5/6] s3:smbd: always clear filter_subreq in smb2srv_client_mc_negprot_next() Commit 5d66d5b84f87267243dcd5223210906ce589af91 introduced a 'verify_again:' target, if we ever hit that, we would leak the existing filter_subreq. Moving it just above a possible messaging_filtered_read_send() will allow us to only clear it if we actually create a new request. That will help us in the next commits. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15346 Signed-off-by: Stefan Metzmacher Reviewed-by: Andreas Schneider (cherry picked from commit 50d61e5300250922bf36bb699306f82dff6a00b9) --- source3/smbd/smbXsrv_client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source3/smbd/smbXsrv_client.c b/source3/smbd/smbXsrv_client.c index f5b296f65d2f..577131fc16f2 100644 --- a/source3/smbd/smbXsrv_client.c +++ b/source3/smbd/smbXsrv_client.c @@ -555,7 +555,6 @@ static void smb2srv_client_mc_negprot_next(struct tevent_req *req) uint32_t seqnum = 0; struct server_id last_server_id = { .pid = 0, }; - TALLOC_FREE(state->filter_subreq); SMB_ASSERT(state->db_rec == NULL); state->db_rec = smbXsrv_client_global_fetch_locked(table->global.db_ctx, &client_guid, @@ -636,6 +635,7 @@ verify_again: SMB_ASSERT(last_server_id.pid == 0); last_server_id = global->server_id; + TALLOC_FREE(state->filter_subreq); if (procid_is_local(&global->server_id)) { subreq = messaging_filtered_read_send(state, state->ev, -- 2.34.1 From 04407a824cc95d00ec17d71da476300614a00a5f Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Thu, 3 Aug 2023 15:45:45 +0200 Subject: [PATCH 6/6] s3:smbd: fix multichannel connection passing race If a client opens multiple connection with the same client guid in parallel, our connection passing is likely to hit a race. Assume we have 3 processes: smbdA: This process already handles all connections for a given client guid smbdB: This just received a new connection with an SMB2 neprot for the same client guid smbdC: This also received a new connection with an SMB2 neprot for the same client guid Now both smbdB and smbdC send a MSG_SMBXSRV_CONNECTION_PASS message to smbdA. These messages contain the socket fd for each connection. While waiting for a MSG_SMBXSRV_CONNECTION_PASSED message from smbdA, both smbdB and smbdC watch the smbXcli_client.tdb record for changes (that also verifies smbdA stays alive). Once one of them say smbdB received the MSG_SMBXSRV_CONNECTION_PASSED message, the dbwrap_watch logic will wakeup smbdC in order to let it recheck the smbXcli_client.tdb record in order to handle the case where smbdA died or deleted its record. Now smbdC rechecks the smbXcli_client.tdb record, but it was not woken because of a problem with smbdA. It meant that smbdC sends a MSG_SMBXSRV_CONNECTION_PASS message including the socket fd again. As a result smbdA got the socket fd from smbdC twice (or even more), and creates two (or more) smbXsrv_connection structures for the same low level tcp connection. And it also sends more than one SMB2 negprot response. Depending on the tevent logic, it will use different smbXsrv_connection structures to process incoming requests. And this will almost immediately result in errors. The typicall error is: smb2_validate_sequence_number: smb2_validate_sequence_number: bad message_id 2 (sequence id 2) (granted = 1, low = 1, range = 1) But other errors would also be possible. The detail that leads to the long delays on the client side is that our smbd_server_connection_terminate_ex() code will close only the fd of a single smbXsrv_connection, but the refcount on the socket fd in the kernel is still not 0, so the tcp connection is still alive... Now we remember the server_id of the process that we send the MSG_SMBXSRV_CONNECTION_PASS message to. And just keep watching the smbXcli_client.tdb record if the server_id don't change. As we just need more patience to wait for the MSG_SMBXSRV_CONNECTION_PASSED message. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15346 Signed-off-by: Stefan Metzmacher Reviewed-by: Andreas Schneider Autobuild-User(master): Andreas Schneider Autobuild-Date(master): Tue Aug 8 13:59:58 UTC 2023 on atb-devel-224 (cherry picked from commit f348b84fbcf203ab1ba92840cf7aecd55dbf9aa0) --- selftest/knownfail.d/samba3.smb2.multichannel | 1 - source3/smbd/smbXsrv_client.c | 31 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) delete mode 100644 selftest/knownfail.d/samba3.smb2.multichannel diff --git a/selftest/knownfail.d/samba3.smb2.multichannel b/selftest/knownfail.d/samba3.smb2.multichannel deleted file mode 100644 index 26b80a39d5be..000000000000 --- a/selftest/knownfail.d/samba3.smb2.multichannel +++ /dev/null @@ -1 +0,0 @@ -^samba3.smb2.multichannel.bugs.bug_15346 diff --git a/source3/smbd/smbXsrv_client.c b/source3/smbd/smbXsrv_client.c index 577131fc16f2..928a9d72caa5 100644 --- a/source3/smbd/smbXsrv_client.c +++ b/source3/smbd/smbXsrv_client.c @@ -488,6 +488,7 @@ struct smb2srv_client_mc_negprot_state { struct tevent_context *ev; struct smbd_smb2_request *smb2req; struct db_record *db_rec; + struct server_id sent_server_id; uint64_t watch_instance; uint32_t last_seqnum; struct tevent_req *filter_subreq; @@ -530,6 +531,8 @@ struct tevent_req *smb2srv_client_mc_negprot_send(TALLOC_CTX *mem_ctx, tevent_req_set_cleanup_fn(req, smb2srv_client_mc_negprot_cleanup); + server_id_set_disconnected(&state->sent_server_id); + smb2srv_client_mc_negprot_next(req); if (!tevent_req_is_in_progress(req)) { @@ -625,6 +628,30 @@ verify_again: return; } + if (server_id_equal(&state->sent_server_id, &global->server_id)) { + /* + * We hit a race with other concurrent connections, + * which have woken us. + * + * We already sent the pass or drop message to + * the process, so we need to wait for a + * response and not pass the connection + * again! Otherwise the process would + * receive the same tcp connection via + * more than one file descriptor and + * create more than one smbXsrv_connection + * structure for the same tcp connection, + * which means the client would see more + * than one SMB2 negprot response to its + * single SMB2 netprot request and we + * as server get the session keys and + * message id validation wrong + */ + goto watch_again; + } + + server_id_set_disconnected(&state->sent_server_id); + /* * If last_server_id is set, we expect * smbXsrv_client_global_verify_record() @@ -660,6 +687,7 @@ verify_again: */ goto verify_again; } + state->sent_server_id = global->server_id; if (tevent_req_nterror(req, status)) { return; } @@ -674,11 +702,14 @@ verify_again: */ goto verify_again; } + state->sent_server_id = global->server_id; if (tevent_req_nterror(req, status)) { return; } } +watch_again: + /* * If the record changed, but we are not happy with the change yet, * we better remove ourself from the waiter list -- 2.34.1