From 3e8ca9b7a6b5c729f9ebb77de3b69676791705c0 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 d7a6fa35bf07..6b9887cdfb11 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 8615f492226fe68a8b2228ab3120bf4e6562bfb7 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 6b9887cdfb11..1cff66a2e3a7 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 da5a3b8274552bae9e818f50eb44c769e69261c1 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 1cff66a2e3a7..f0d34bbf4940 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 853a9fff4e2c2f29e96acec60d97b9bcd72d1a3b 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 f0d34bbf4940..0a958842d341 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 039f1067fc55d02cb7eba7eae35b945c56a09c8f 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 0a958842d341..f57bc724910d 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