Attachment 17562 Details for Bug 15200 – fix for 4.16 (requires the patches from bug #15159)

[patch] fix for 4.16 (requires the patches from bug #15159)

fix-bug-15200-v4-16.patch (text/plain), 11.96 KB, created by Björn Baumbach on 2022-10-14 15:50:50 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Björn Baumbach

Created: 2022-10-14 15:50:50 UTC

Size: 11.96 KB

patch

obsolete

>From f2c45c39db49ffa72ef1c4fe163cbec55875a2c1 Mon Sep 17 00:00:00 2001
>From: Stefan Metzmacher <metze@samba.org>
>Date: Wed, 12 Oct 2022 13:30:32 +0200
>Subject: [PATCH 1/5] smbXsrv_client: ignore NAME_NOT_FOUND from
> smb2srv_client_connection_passed
>
>If we hit a race, when a client disconnects the connection after the initial
>SMB2 Negotiate request, before the connection is completely passed to
>process serving the given client guid, the temporary smbd which accepted the
>new connection may already detected the disconnect and exitted before
>the long term smbd servicing the client guid was able to send the
>MSG_SMBXSRV_CONNECTION_PASSED message.
>
>The result was a log message like this:
>
>  smbXsrv_client_connection_pass_loop: smb2srv_client_connection_passed() failed => NT_STATUS_OBJECT_NAME_NOT_FOUND
>
>and all connections belonging to the client guid were dropped,
>because we called exit_server_cleanly().
>
>Now we ignore NT_STATUS_OBJECT_NAME_NOT_FOUND from
>smb2srv_client_connection_passed() and let the normal
>event loop detect the broken connection, so that only
>that connection is terminated (not the whole smbd process).
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15200
>
>Signed-off-by: Stefan Metzmacher <metze@samba.org>
>Reviewed-by: Ralph Boehme <slow@samba.org>
>(cherry picked from commit 636ec45c93ad040ba70296aa543884c145b3e789)
>---
> source3/smbd/smbXsrv_client.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
>diff --git a/source3/smbd/smbXsrv_client.c b/source3/smbd/smbXsrv_client.c
>index 1ee4410c1cf..c948b50d75b 100644
>--- a/source3/smbd/smbXsrv_client.c
>+++ b/source3/smbd/smbXsrv_client.c
>@@ -1063,6 +1063,16 @@ static void smbXsrv_client_connection_pass_loop(struct tevent_req *subreq)
> 	}
> 
> 	status = smb2srv_client_connection_passed(client, pass_info0);
>+	if (NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND)) {
>+		/*
>+		 * We hit a race where, the client dropped the connection
>+		 * while the socket was passed to us and the origin
>+		 * process already existed.
>+		 */
>+		DBG_DEBUG("smb2srv_client_connection_passed() ignore %s\n",
>+			  nt_errstr(status));
>+		status = NT_STATUS_OK;
>+	}
> 	if (!NT_STATUS_IS_OK(status)) {
> 		const char *r = "smb2srv_client_connection_passed() failed";
> 		DBG_ERR("%s => %s\n", r, nt_errstr(status));
>-- 
>2.34.1
>
>
>From f53b3179d1ff563c0fb3f01acb5f7f2c4bffd6ec Mon Sep 17 00:00:00 2001
>From: Stefan Metzmacher <metze@samba.org>
>Date: Wed, 12 Oct 2022 13:40:26 +0200
>Subject: [PATCH 2/5] smbXsrv_client: fix a debug message in
> smbXsrv_client_global_verify_record()
>
>DBG_WARNING() already adds the function name as prefix.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15200
>
>Signed-off-by: Stefan Metzmacher <metze@samba.org>
>Reviewed-by: Ralph Boehme <slow@samba.org>
>(cherry picked from commit acb3d821deaf06faa16f6428682ecdb02babeb98)
>---
> source3/smbd/smbXsrv_client.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
>diff --git a/source3/smbd/smbXsrv_client.c b/source3/smbd/smbXsrv_client.c
>index c948b50d75b..648ca84e8c7 100644
>--- a/source3/smbd/smbXsrv_client.c
>+++ b/source3/smbd/smbXsrv_client.c
>@@ -227,8 +227,7 @@ static void smbXsrv_client_global_verify_record(struct db_record *db_rec,
> 			(ndr_pull_flags_fn_t)ndr_pull_smbXsrv_client_globalB);
> 	if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) {
> 		NTSTATUS status = ndr_map_error2ntstatus(ndr_err);
>-		DBG_WARNING("smbXsrv_client_global_verify_record: "
>-			    "key '%s' ndr_pull_struct_blob - %s\n",
>+		DBG_WARNING("key '%s' ndr_pull_struct_blob - %s\n",
> 			    hex_encode_talloc(frame, key.dptr, key.dsize),
> 			    nt_errstr(status));
> 		TALLOC_FREE(frame);
>-- 
>2.34.1
>
>
>From f8fc88df72bfd8b095fda116b9da1c294f1fbb8b Mon Sep 17 00:00:00 2001
>From: Stefan Metzmacher <metze@samba.org>
>Date: Wed, 12 Oct 2022 13:54:41 +0200
>Subject: [PATCH 3/5] smbXsrv_client: call
> smb2srv_client_connection_{pass,drop}() before dbwrap_watched_watch_send()
>
>dbwrap_watched_watch_send() should typically be the last thing to call
>before the db record is unlocked, as it's not that easy to undo.
>
>In future we want to recover from smb2srv_client_connection_{pass,drop}()
>returning NT_STATUS_OBJECT_NAME_NOT_FOUND and it would add complexity if
>would need to undo dbwrap_watched_watch_send() at that point.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15200
>
>Signed-off-by: Stefan Metzmacher <metze@samba.org>
>Reviewed-by: Ralph Boehme <slow@samba.org>
>(cherry picked from commit 56c597bc2b29dc3e555f737ba189f521d0e31e8c)
>---
> source3/smbd/smbXsrv_client.c | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
>diff --git a/source3/smbd/smbXsrv_client.c b/source3/smbd/smbXsrv_client.c
>index 648ca84e8c7..c671c99b671 100644
>--- a/source3/smbd/smbXsrv_client.c
>+++ b/source3/smbd/smbXsrv_client.c
>@@ -592,31 +592,30 @@ static void smb2srv_client_mc_negprot_next(struct tevent_req *req)
> 		tevent_req_set_callback(subreq, smb2srv_client_mc_negprot_done, req);
> 	}
> 
>-	subreq = dbwrap_watched_watch_send(state,
>-					   state->ev,
>-					   state->db_rec,
>-					   global->server_id);
>-	if (tevent_req_nomem(subreq, req)) {
>-		return;
>-	}
>-	tevent_req_set_callback(subreq, smb2srv_client_mc_negprot_watched, req);
>-
> 	if (procid_is_local(&global->server_id)) {
> 		status = smb2srv_client_connection_pass(state->smb2req,
> 							global);
>-		TALLOC_FREE(global);
> 		if (tevent_req_nterror(req, status)) {
> 			return;
> 		}
> 	} else {
> 		status = smb2srv_client_connection_drop(state->smb2req,
> 							global);
>-		TALLOC_FREE(global);
> 		if (tevent_req_nterror(req, status)) {
> 			return;
> 		}
> 	}
> 
>+	subreq = dbwrap_watched_watch_send(state,
>+					   state->ev,
>+					   state->db_rec,
>+					   global->server_id);
>+	if (tevent_req_nomem(subreq, req)) {
>+		return;
>+	}
>+	tevent_req_set_callback(subreq, smb2srv_client_mc_negprot_watched, req);
>+
>+	TALLOC_FREE(global);
> 	TALLOC_FREE(state->db_rec);
> 	return;
> }
>-- 
>2.34.1
>
>
>From 77024a9f490c4fee618fedfe9887c9a8d419040f Mon Sep 17 00:00:00 2001
>From: Stefan Metzmacher <metze@samba.org>
>Date: Wed, 12 Oct 2022 14:15:53 +0200
>Subject: [PATCH 4/5] smbXsrv_client: make sure we only wait for
> smb2srv_client_mc_negprot_filter once and only when needed
>
>This will simplify the following changes...
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15200
>
>Signed-off-by: Stefan Metzmacher <metze@samba.org>
>Reviewed-by: Ralph Boehme <slow@samba.org>
>(cherry picked from commit 8c8d8cf01e01c2726d03fa1c81e0ce9992ee736c)
>---
> source3/smbd/smbXsrv_client.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
>diff --git a/source3/smbd/smbXsrv_client.c b/source3/smbd/smbXsrv_client.c
>index c671c99b671..08dfd866856 100644
>--- a/source3/smbd/smbXsrv_client.c
>+++ b/source3/smbd/smbXsrv_client.c
>@@ -462,6 +462,7 @@ struct smb2srv_client_mc_negprot_state {
> 	struct tevent_context *ev;
> 	struct smbd_smb2_request *smb2req;
> 	struct db_record *db_rec;
>+	struct tevent_req *filter_subreq;
> };
> 
> static void smb2srv_client_mc_negprot_cleanup(struct tevent_req *req,
>@@ -519,6 +520,7 @@ static void smb2srv_client_mc_negprot_next(struct tevent_req *req)
> 	struct tevent_req *subreq = NULL;
> 	NTSTATUS status;
> 
>+	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,
>@@ -590,6 +592,7 @@ static void smb2srv_client_mc_negprot_next(struct tevent_req *req)
> 			return;
> 		}
> 		tevent_req_set_callback(subreq, smb2srv_client_mc_negprot_done, req);
>+		state->filter_subreq = subreq;
> 	}
> 
> 	if (procid_is_local(&global->server_id)) {
>@@ -650,6 +653,9 @@ static void smb2srv_client_mc_negprot_done(struct tevent_req *subreq)
> 	NTSTATUS status;
> 	int ret;
> 
>+	SMB_ASSERT(state->filter_subreq == subreq);
>+	state->filter_subreq = NULL;
>+
> 	ret = messaging_filtered_read_recv(subreq, state, &rec);
> 	TALLOC_FREE(subreq);
> 	if (ret != 0) {
>-- 
>2.34.1
>
>
>From 3b7be5efd52c727f532ed3558041b6aee6ec9421 Mon Sep 17 00:00:00 2001
>From: Stefan Metzmacher <metze@samba.org>
>Date: Wed, 12 Oct 2022 14:57:18 +0200
>Subject: [PATCH 5/5] smbXsrv_client: handle NAME_NOT_FOUND from
> smb2srv_client_connection_{pass,drop}()
>
>If we get NT_STATUS_OBJECT_NOT_FOUND from smb2srv_client_connection_{pass,drop}()
>we should just keep the connection and overwrite the stale record in
>smbXsrv_client_global.tdb. It's basically a race with serverid_exists()
>and a process that doesn't cleanly teardown.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15200
>
>Signed-off-by: Stefan Metzmacher <metze@samba.org>
>Reviewed-by: Ralph Boehme <slow@samba.org>
>(cherry picked from commit 5d66d5b84f87267243dcd5223210906ce589af91)
>---
> source3/smbd/smbXsrv_client.c | 49 +++++++++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
>
>diff --git a/source3/smbd/smbXsrv_client.c b/source3/smbd/smbXsrv_client.c
>index 08dfd866856..28e5035d21d 100644
>--- a/source3/smbd/smbXsrv_client.c
>+++ b/source3/smbd/smbXsrv_client.c
>@@ -189,6 +189,7 @@ static void smbXsrv_client_global_verify_record(struct db_record *db_rec,
> 					bool *is_free,
> 					bool *was_free,
> 					TALLOC_CTX *mem_ctx,
>+					const struct server_id *dead_server_id,
> 					struct smbXsrv_client_global0 **_g)
> {
> 	TDB_DATA key;
>@@ -197,6 +198,7 @@ static void smbXsrv_client_global_verify_record(struct db_record *db_rec,
> 	struct smbXsrv_client_globalB global_blob;
> 	enum ndr_err_code ndr_err;
> 	struct smbXsrv_client_global0 *global = NULL;
>+	bool dead = false;
> 	bool exists;
> 	TALLOC_CTX *frame = talloc_stackframe();
> 
>@@ -250,6 +252,22 @@ static void smbXsrv_client_global_verify_record(struct db_record *db_rec,
> 
> 	global = global_blob.info.info0;
> 
>+	dead = server_id_equal(dead_server_id, &global->server_id);
>+	if (dead) {
>+		struct server_id_buf tmp;
>+
>+		DBG_NOTICE("key '%s' server_id %s is already dead.\n",
>+			   hex_encode_talloc(frame, key.dptr, key.dsize),
>+			   server_id_str_buf(global->server_id, &tmp));
>+		if (DEBUGLVL(DBGLVL_NOTICE)) {
>+			NDR_PRINT_DEBUG(smbXsrv_client_globalB, &global_blob);
>+		}
>+		TALLOC_FREE(frame);
>+		dbwrap_record_delete(db_rec);
>+		*is_free = true;
>+		return;
>+	}
>+
> 	exists = serverid_exists(&global->server_id);
> 	if (!exists) {
> 		struct server_id_buf tmp;
>@@ -519,6 +537,7 @@ static void smb2srv_client_mc_negprot_next(struct tevent_req *req)
> 	bool is_free = false;
> 	struct tevent_req *subreq = NULL;
> 	NTSTATUS status;
>+	struct server_id last_server_id = { .pid = 0, };
> 
> 	TALLOC_FREE(state->filter_subreq);
> 	SMB_ASSERT(state->db_rec == NULL);
>@@ -530,10 +549,14 @@ static void smb2srv_client_mc_negprot_next(struct tevent_req *req)
> 		return;
> 	}
> 
>+verify_again:
>+	TALLOC_FREE(global);
>+
> 	smbXsrv_client_global_verify_record(state->db_rec,
> 					    &is_free,
> 					    NULL,
> 					    state,
>+					    &last_server_id,
> 					    &global);
> 	if (is_free) {
> 		/*
>@@ -582,6 +605,16 @@ static void smb2srv_client_mc_negprot_next(struct tevent_req *req)
> 		return;
> 	}
> 
>+	/*
>+	 * If last_server_id is set, we expect
>+	 * smbXsrv_client_global_verify_record()
>+	 * to detect the already dead global->server_id
>+	 * as state->db_rec is still locked and its
>+	 * value didn't change.
>+	 */
>+	SMB_ASSERT(last_server_id.pid == 0);
>+	last_server_id = global->server_id;
>+
> 	if (procid_is_local(&global->server_id)) {
> 		subreq = messaging_filtered_read_send(state,
> 						      state->ev,
>@@ -598,12 +631,28 @@ static void smb2srv_client_mc_negprot_next(struct tevent_req *req)
> 	if (procid_is_local(&global->server_id)) {
> 		status = smb2srv_client_connection_pass(state->smb2req,
> 							global);
>+		if (NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND)) {
>+			/*
>+			 * We remembered last_server_id = global->server_id
>+			 * above, so we'll treat it as dead in the
>+			 * next round to smbXsrv_client_global_verify_record().
>+			 */
>+			goto verify_again;
>+		}
> 		if (tevent_req_nterror(req, status)) {
> 			return;
> 		}
> 	} else {
> 		status = smb2srv_client_connection_drop(state->smb2req,
> 							global);
>+		if (NT_STATUS_EQUAL(status, NT_STATUS_OBJECT_NAME_NOT_FOUND)) {
>+			/*
>+			 * We remembered last_server_id = global->server_id
>+			 * above, so we'll treat it as dead in the
>+			 * next round to smbXsrv_client_global_verify_record().
>+			 */
>+			goto verify_again;
>+		}
> 		if (tevent_req_nterror(req, status)) {
> 			return;
> 		}
>-- 
>2.34.1
>

Flags:

metze: review+
bbaumbach: review? (slow)

Actions: View

Attachments on bug 15200: 17562 | 17563