From 4a93dbceb69569928ff853ba8ac80e67a40d179f 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 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 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 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 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 | 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 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 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 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 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