From c34eed537e5cf77c88f281adeaec3fb68999e592 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 1ee4410c1cf5..c948b50d75b3 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 5d071b2e295720f9126708bddbcc881da3a72a6b 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 c948b50d75b3..648ca84e8c79 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 f5cdb16261298c2082b04d678616e38999376dbe 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 (similar to 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 648ca84e8c79..c671c99b6711 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 51cdbb8900cd80e48dfab0d987f9a584ca23a928 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 c671c99b6711..08dfd8668566 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 64f1454ce8d714bfced723f3c84b7176bd9e8f27 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 08dfd8668566..28e5035d21dc 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