[SCM] Samba Shared Repository - branch master updated
Andreas Schneider
asn at samba.org
Tue Aug 8 14:00:01 UTC 2023
The branch, master has been updated
via f348b84fbcf s3:smbd: fix multichannel connection passing race
via 50d61e53002 s3:smbd: always clear filter_subreq in smb2srv_client_mc_negprot_next()
via 4028d658290 s4:torture/smb2: add smb2.multichannel.bugs.bug_15346
via 2b93058be3f s4:torture/smb2: make it possible to pass existing_conn to smb2_connect_ext()
via dc5a500f0a7 s4:torture/smb2: let us have a common torture_smb2_con_share()
via ade663ee6ca s4:torture/smb2: let torture_smb2_con_sopt() use smb2_connect()
from 9ec22e68024 dcerpc.idl: fix definitions for DCERPC_PKT_CO_CANCEL and DCERPC_PKT_ORPHANED payload
https://git.samba.org/?p=samba.git;a=shortlog;h=master
- Log -----------------------------------------------------------------
commit f348b84fbcf203ab1ba92840cf7aecd55dbf9aa0
Author: Stefan Metzmacher <metze at samba.org>
Date: Thu Aug 3 15:45:45 2023 +0200
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 <metze at samba.org>
Reviewed-by: Andreas Schneider <asn at samba.org>
Autobuild-User(master): Andreas Schneider <asn at cryptomilk.org>
Autobuild-Date(master): Tue Aug 8 13:59:58 UTC 2023 on atb-devel-224
commit 50d61e5300250922bf36bb699306f82dff6a00b9
Author: Stefan Metzmacher <metze at samba.org>
Date: Thu Aug 3 15:34:29 2023 +0200
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 <metze at samba.org>
Reviewed-by: Andreas Schneider <asn at samba.org>
commit 4028d6582907cf582730ceec56872d8584ad02e6
Author: Stefan Metzmacher <metze at samba.org>
Date: Fri Aug 4 17:16:14 2023 +0200
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 <metze at samba.org>
Reviewed-by: Andreas Schneider <asn at samba.org>
commit 2b93058be3f6e5eaee239ad3b0e707c62089d18e
Author: Stefan Metzmacher <metze at samba.org>
Date: Mon Aug 7 12:22:43 2023 +0200
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 <metze at samba.org>
Reviewed-by: Andreas Schneider <asn at samba.org>
commit dc5a500f0a76720b2a5cb5b1142cf4c35cb6bdea
Author: Stefan Metzmacher <metze at samba.org>
Date: Mon Aug 7 11:03:41 2023 +0200
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 <metze at samba.org>
Reviewed-by: Andreas Schneider <asn at samba.org>
commit ade663ee6ca1a2813b203ea667d933f4dab9e7b7
Author: Stefan Metzmacher <metze at samba.org>
Date: Mon Aug 7 11:03:41 2023 +0200
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 <metze at samba.org>
Reviewed-by: Andreas Schneider <asn at samba.org>
-----------------------------------------------------------------------
Summary of changes:
source3/smbd/smbXsrv_client.c | 33 +++-
source4/libcli/smb2/connect.c | 4 +-
source4/torture/smb2/acls.c | 34 ----
source4/torture/smb2/multichannel.c | 315 ++++++++++++++++++++++++++++++++++++
source4/torture/smb2/util.c | 55 ++++---
source4/torture/vfs/acl_xattr.c | 34 ----
6 files changed, 383 insertions(+), 92 deletions(-)
Changeset truncated at 500 lines:
diff --git a/source3/smbd/smbXsrv_client.c b/source3/smbd/smbXsrv_client.c
index f5b296f65d2..928a9d72caa 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)) {
@@ -555,7 +558,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,
@@ -626,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()
@@ -636,6 +662,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,
@@ -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
diff --git a/source4/libcli/smb2/connect.c b/source4/libcli/smb2/connect.c
index 1f68d90538b..64b67865446 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/acls.c b/source4/torture/smb2/acls.c
index 2f15ad27e48..7e3d8b5ee7d 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/multichannel.c b/source4/torture/smb2/multichannel.c
index 801c1ec801f..cae28ff1bed 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");
--
Samba Shared Repository
More information about the samba-cvs
mailing list