From f2c45c39db49ffa72ef1c4fe163cbec55875a2c1 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher 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 Reviewed-by: Ralph Boehme (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 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 Reviewed-by: Ralph Boehme (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 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 Reviewed-by: Ralph Boehme (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 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 Reviewed-by: Ralph Boehme (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 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 Reviewed-by: Ralph Boehme (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