Attachment 17563 Details for Bug 15200 – fix for 4.17

[patch] fix for 4.17

fix-bug-15200-v4-17.patch (text/plain), 12.09 KB, created by Björn Baumbach on 2022-10-14 15:51:39 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Björn Baumbach

Created: 2022-10-14 15:51:39 UTC

Size: 12.09 KB

patch

obsolete

>From 4a93dbceb69569928ff853ba8ac80e67a40d179f 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 d7a6fa35bf0..6b9887cdfb1 100644
>--- a/source3/smbd/smbXsrv_client.c
>+++ b/source3/smbd/smbXsrv_client.c
>@@ -1111,6 +1111,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 47710dff1657fee30f5676dfa3a16a0eb6436a36 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 6b9887cdfb1..1cff66a2e3a 100644
>--- a/source3/smbd/smbXsrv_client.c
>+++ b/source3/smbd/smbXsrv_client.c
>@@ -231,8 +231,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 97c369d36643c7b4c72dc068b5e5ae653574ad17 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 | 31 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
>
>diff --git a/source3/smbd/smbXsrv_client.c b/source3/smbd/smbXsrv_client.c
>index 1cff66a2e3a..f0d34bbf494 100644
>--- a/source3/smbd/smbXsrv_client.c
>+++ b/source3/smbd/smbXsrv_client.c
>@@ -612,6 +612,20 @@ static void smb2srv_client_mc_negprot_next(struct tevent_req *req)
> 		tevent_req_set_callback(subreq, smb2srv_client_mc_negprot_done, req);
> 	}
> 
>+	if (procid_is_local(&global->server_id)) {
>+		status = smb2srv_client_connection_pass(state->smb2req,
>+							global);
>+		if (tevent_req_nterror(req, status)) {
>+			return;
>+		}
>+	} else {
>+		status = smb2srv_client_connection_drop(state->smb2req,
>+							global);
>+		if (tevent_req_nterror(req, status)) {
>+			return;
>+		}
>+	}
>+
> 	/*
> 	 * If the record changed, but we are not happy with the change yet,
> 	 * we better remove ourself from the waiter list
>@@ -643,22 +657,7 @@ static void smb2srv_client_mc_negprot_next(struct tevent_req *req)
> 	}
> 	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;
>-		}
>-	}
>-
>+	TALLOC_FREE(global);
> 	TALLOC_FREE(state->db_rec);
> 	return;
> }
>-- 
>2.34.1
>
>
>From fb2770553343e4c56a2bb009cdb3406d2e88ebda 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 f0d34bbf494..0a958842d34 100644
>--- a/source3/smbd/smbXsrv_client.c
>+++ b/source3/smbd/smbXsrv_client.c
>@@ -471,6 +471,7 @@ struct smb2srv_client_mc_negprot_state {
> 	struct db_record *db_rec;
> 	uint64_t watch_instance;
> 	uint32_t last_seqnum;
>+	struct tevent_req *filter_subreq;
> };
> 
> static void smb2srv_client_mc_negprot_cleanup(struct tevent_req *req,
>@@ -534,6 +535,7 @@ static void smb2srv_client_mc_negprot_next(struct tevent_req *req)
> 	NTSTATUS status;
> 	uint32_t seqnum = 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,
>@@ -610,6 +612,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)) {
>@@ -692,6 +695,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 ec3ac93b380c6ab882ad2ea442a9ce6745d5a287 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 0a958842d34..f57bc724910 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,
> 					uint32_t *pseqnum)
> {
>@@ -198,6 +199,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();
> 
>@@ -254,6 +256,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;
>@@ -534,6 +552,7 @@ static void smb2srv_client_mc_negprot_next(struct tevent_req *req)
> 	struct tevent_req *subreq = NULL;
> 	NTSTATUS status;
> 	uint32_t seqnum = 0;
>+	struct server_id last_server_id = { .pid = 0, };
> 
> 	TALLOC_FREE(state->filter_subreq);
> 	SMB_ASSERT(state->db_rec == NULL);
>@@ -545,10 +564,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,
> 					    &seqnum);
> 	if (is_free) {
>@@ -602,6 +625,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,
>@@ -618,12 +651,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