From 9c83e302052f3fd8b91a27d4467765f72fa81945 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 4 Mar 2020 13:29:08 -0800 Subject: [PATCH 01/25] s3: VFS: vfs_aio_pthread. Fix leak of state struct on error. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Volker Lendecke (cherry picked from commit a1e247c3ba579ecc6ee03f5aad9679ed79fac5ac) --- source3/modules/vfs_aio_pthread.c | 1 + 1 file changed, 1 insertion(+) diff --git a/source3/modules/vfs_aio_pthread.c b/source3/modules/vfs_aio_pthread.c index d13ce2fdc63..37ba0c2c8a2 100644 --- a/source3/modules/vfs_aio_pthread.c +++ b/source3/modules/vfs_aio_pthread.c @@ -308,6 +308,7 @@ static int open_async(const files_struct *fsp, fsp->conn->sconn->pool, aio_open_worker, opd); if (subreq == NULL) { + TALLOC_FREE(opd); return -1; } tevent_req_set_callback(subreq, aio_open_handle_completion, opd); -- 2.20.1 From 7aca70199532a49e9c6e6402a61ee19dbd5020d5 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 4 Mar 2020 13:47:13 -0800 Subject: [PATCH 02/25] s3: VFS: vfs_aio_pthread: Replace state destructor with explicitly called teardown function. This will allow repurposing a real destructor to allow connections structs to be freed whilst the aio open request is in flight. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Volker Lendecke (cherry picked from commit 8db831a318cd4a10ec9c1d629ebff4ca35b8acfe) --- source3/modules/vfs_aio_pthread.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/source3/modules/vfs_aio_pthread.c b/source3/modules/vfs_aio_pthread.c index 37ba0c2c8a2..820e1b89c44 100644 --- a/source3/modules/vfs_aio_pthread.c +++ b/source3/modules/vfs_aio_pthread.c @@ -62,6 +62,7 @@ struct aio_open_private_data { static struct aio_open_private_data *open_pd_list; static void aio_open_do(struct aio_open_private_data *opd); +static void opd_free(struct aio_open_private_data *opd); /************************************************************************ Find the open private data by mid. @@ -145,7 +146,7 @@ static void aio_open_handle_completion(struct tevent_req *subreq) close(opd->ret_fd); opd->ret_fd = -1; } - TALLOC_FREE(opd); + opd_free(opd); } } @@ -207,16 +208,16 @@ static void aio_open_do(struct aio_open_private_data *opd) } /************************************************************************ - Open private data destructor. + Open private data teardown. ***********************************************************************/ -static int opd_destructor(struct aio_open_private_data *opd) +static void opd_free(struct aio_open_private_data *opd) { if (opd->dir_fd != -1) { close(opd->dir_fd); } DLIST_REMOVE(open_pd_list, opd); - return 0; + TALLOC_FREE(opd); } /************************************************************************ @@ -250,7 +251,7 @@ static struct aio_open_private_data *create_private_open_data(const files_struct /* Copy our current credentials. */ opd->ux_tok = copy_unix_token(opd, get_current_utok(fsp->conn)); if (opd->ux_tok == NULL) { - TALLOC_FREE(opd); + opd_free(opd); return NULL; } @@ -262,12 +263,12 @@ static struct aio_open_private_data *create_private_open_data(const files_struct fsp->fsp_name->base_name, &opd->dname, &fname) == false) { - TALLOC_FREE(opd); + opd_free(opd); return NULL; } opd->fname = talloc_strdup(opd, fname); if (opd->fname == NULL) { - TALLOC_FREE(opd); + opd_free(opd); return NULL; } @@ -277,11 +278,10 @@ static struct aio_open_private_data *create_private_open_data(const files_struct opd->dir_fd = open(opd->dname, O_RDONLY); #endif if (opd->dir_fd == -1) { - TALLOC_FREE(opd); + opd_free(opd); return NULL; } - talloc_set_destructor(opd, opd_destructor); DLIST_ADD_END(open_pd_list, opd); return opd; } @@ -308,7 +308,7 @@ static int open_async(const files_struct *fsp, fsp->conn->sconn->pool, aio_open_worker, opd); if (subreq == NULL) { - TALLOC_FREE(opd); + opd_free(opd); return -1; } tevent_req_set_callback(subreq, aio_open_handle_completion, opd); @@ -365,7 +365,7 @@ static bool find_completed_open(files_struct *fsp, smb_fname_str_dbg(fsp->fsp_name))); /* Now we can free the opd. */ - TALLOC_FREE(opd); + opd_free(opd); return true; } -- 2.20.1 From 3049ab93121de4be72dbcc315178658ca392a6e3 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 4 Mar 2020 16:39:39 -0800 Subject: [PATCH 03/25] s3: VFS: vfs_aio_pthread. Move xconn into state struct (opd). We will need this in future to cause a pending open to be rescheduled after the connection struct we're using has been shut down with an aio open in flight. This will allow a correct error reply to an awaiting client. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Volker Lendecke (cherry picked from commit ddb9038fe776b1d8239e563a4c9a70b4097645f3) --- source3/modules/vfs_aio_pthread.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/source3/modules/vfs_aio_pthread.c b/source3/modules/vfs_aio_pthread.c index 820e1b89c44..d5919f83b3f 100644 --- a/source3/modules/vfs_aio_pthread.c +++ b/source3/modules/vfs_aio_pthread.c @@ -51,6 +51,7 @@ struct aio_open_private_data { const char *fname; char *dname; connection_struct *conn; + struct smbXsrv_connection *xconn; const struct security_unix_token *ux_tok; uint64_t initial_allocation_size; /* Returns. */ @@ -91,7 +92,6 @@ static void aio_open_handle_completion(struct tevent_req *subreq) tevent_req_callback_data(subreq, struct aio_open_private_data); int ret; - struct smbXsrv_connection *xconn; ret = pthreadpool_tevent_job_recv(subreq); TALLOC_FREE(subreq); @@ -128,15 +128,8 @@ static void aio_open_handle_completion(struct tevent_req *subreq) opd->in_progress = false; - /* - * TODO: In future we need a proper algorithm - * to find the correct connection for a fsp. - * For now we only have one connection, so this is correct... - */ - xconn = opd->conn->sconn->client->connections; - /* Find outstanding event and reschedule. */ - if (!schedule_deferred_open_message_smb(xconn, opd->mid)) { + if (!schedule_deferred_open_message_smb(opd->xconn, opd->mid)) { /* * Outstanding event didn't exist or was * cancelled. Free up the fd and throw @@ -245,6 +238,12 @@ static struct aio_open_private_data *create_private_open_data(const files_struct .mid = fsp->mid, .in_progress = true, .conn = fsp->conn, + /* + * TODO: In future we need a proper algorithm + * to find the correct connection for a fsp. + * For now we only have one connection, so this is correct... + */ + .xconn = fsp->conn->sconn->client->connections, .initial_allocation_size = fsp->initial_allocation_size, }; -- 2.20.1 From 758022992f172496cf8ff7afb620c720d44dad6c Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 6 Mar 2020 09:30:26 -0800 Subject: [PATCH 04/25] s3: VFS: vfs_aio_pthread: Add a talloc context parameter to create_private_open_data(). Pass in NULL for now so no behavior change. We will be changing this from NULL to fsp->conn in a later commit. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Volker Lendecke (cherry picked from commit e566066605981549b670a5392683fbd81ce93d18) --- source3/modules/vfs_aio_pthread.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/source3/modules/vfs_aio_pthread.c b/source3/modules/vfs_aio_pthread.c index d5919f83b3f..be84e90cfb2 100644 --- a/source3/modules/vfs_aio_pthread.c +++ b/source3/modules/vfs_aio_pthread.c @@ -217,11 +217,12 @@ static void opd_free(struct aio_open_private_data *opd) Create and initialize a private data struct for async open. ***********************************************************************/ -static struct aio_open_private_data *create_private_open_data(const files_struct *fsp, +static struct aio_open_private_data *create_private_open_data(TALLOC_CTX *ctx, + const files_struct *fsp, int flags, mode_t mode) { - struct aio_open_private_data *opd = talloc_zero(NULL, + struct aio_open_private_data *opd = talloc_zero(ctx, struct aio_open_private_data); const char *fname = NULL; @@ -296,7 +297,7 @@ static int open_async(const files_struct *fsp, struct aio_open_private_data *opd = NULL; struct tevent_req *subreq = NULL; - opd = create_private_open_data(fsp, flags, mode); + opd = create_private_open_data(NULL, fsp, flags, mode); if (opd == NULL) { DEBUG(10, ("open_async: Could not create private data.\n")); return -1; -- 2.20.1 From b5b04c88203ec9ec6d2e507695dccba2efe8f9fd Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 5 Mar 2020 10:22:00 -0800 Subject: [PATCH 05/25] s3: VFS: vfs_aio_pthread: Make aio opens safe against connection teardown. Allocate state off fsp->conn, not NULL, and add a destructor that catches deallocation of conn which happens on connection shutdown or force close. Note - We don't allocate off fsp as the passed in fsp will get freed once we return EINPROGRESS/NT_STATUS_MORE_PROCESSING_REQUIRED. A new fsp pointer gets allocated on every re-run of the open code path. The destructor allows us to NULL out the saved conn struct pointer when conn is deallocated so we know not to access deallocated memory. This matches the async teardown code changes for bug #14301 in pread/pwrite/fsync vfs_default.c and vfs_glusterfs.c state is still correctly deallocated in all code paths so no memory leaks. This allows us to safely complete when the openat() returns and then return the error NT_STATUS_NETWORK_NAME_DELETED to the client open request. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Volker Lendecke (cherry picked from commit 6b567e0c138d1cf2bcf58c84872ed2b0e89d628d) --- source3/modules/vfs_aio_pthread.c | 66 ++++++++++++++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-) diff --git a/source3/modules/vfs_aio_pthread.c b/source3/modules/vfs_aio_pthread.c index be84e90cfb2..1ccf89a6d8c 100644 --- a/source3/modules/vfs_aio_pthread.c +++ b/source3/modules/vfs_aio_pthread.c @@ -95,6 +95,37 @@ static void aio_open_handle_completion(struct tevent_req *subreq) ret = pthreadpool_tevent_job_recv(subreq); TALLOC_FREE(subreq); + + /* + * We're no longer in flight. Remove the + * destructor used to preserve opd so + * a talloc_free actually removes it. + */ + talloc_set_destructor(opd, NULL); + + if (opd->conn == NULL) { + /* + * We were shutdown closed in flight. No one + * wants the result, and state has been reparented + * to the NULL context, so just free it so we + * don't leak memory. + */ + DBG_NOTICE("aio open request for %s/%s abandoned in flight\n", + opd->dname, + opd->fname); + if (opd->ret_fd != -1) { + close(opd->ret_fd); + opd->ret_fd = -1; + } + /* + * Find outstanding event and reschedule so the client + * gets an error message return from the open. + */ + schedule_deferred_open_message_smb(opd->xconn, opd->mid); + opd_free(opd); + return; + } + if (ret != 0) { bool ok; @@ -286,6 +317,22 @@ static struct aio_open_private_data *create_private_open_data(TALLOC_CTX *ctx, return opd; } +static int opd_inflight_destructor(struct aio_open_private_data *opd) +{ + /* + * Setting conn to NULL allows us to + * discover the connection was torn + * down which kills the fsp that owns + * opd. + */ + DBG_NOTICE("aio open request for %s/%s cancelled\n", + opd->dname, + opd->fname); + opd->conn = NULL; + /* Don't let opd go away. */ + return -1; +} + /***************************************************************** Setup an async open. *****************************************************************/ @@ -297,7 +344,18 @@ static int open_async(const files_struct *fsp, struct aio_open_private_data *opd = NULL; struct tevent_req *subreq = NULL; - opd = create_private_open_data(NULL, fsp, flags, mode); + /* + * Allocate off fsp->conn, not NULL or fsp. As we're going + * async fsp will get talloc_free'd when we return + * EINPROGRESS/NT_STATUS_MORE_PROCESSING_REQUIRED. A new fsp + * pointer gets allocated on every re-run of the + * open code path. Allocating on fsp->conn instead + * of NULL allows use to get notified via destructor + * if the conn is force-closed or we shutdown. + * opd is always safely freed in all codepath so no + * memory leaks. + */ + opd = create_private_open_data(fsp->conn, fsp, flags, mode); if (opd == NULL) { DEBUG(10, ("open_async: Could not create private data.\n")); return -1; @@ -318,6 +376,12 @@ static int open_async(const files_struct *fsp, opd->dname, opd->fname)); + /* + * Add a destructor to protect us from connection + * teardown whilst the open thread is in flight. + */ + talloc_set_destructor(opd, opd_inflight_destructor); + /* Cause the calling code to reschedule us. */ errno = EINPROGRESS; /* Maps to NT_STATUS_MORE_PROCESSING_REQUIRED. */ return -1; -- 2.20.1 From ae44875b2ff206cf3c39c55fbe78c553af797005 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 10 Mar 2020 10:55:22 -0700 Subject: [PATCH 06/25] s3: smbd: In aio_del_req_from_fsp() talloc_free(fsp->aio_requests[]) when fsp->num_aio_requests reaches zero. The add code in aio_add_req_to_fsp() re-tallocs this array on demand, and talloc freeing it here allows it to be used as the parent for a tevent wait queue, so callers can get notified when all outstanding aio on an fsp is finished. We'll deal with any performance issues in the next commit. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit 0c952bba1edf7c8173d05ccdc6fdaa7232d2c6aa) --- source3/smbd/aio.c | 1 + 1 file changed, 1 insertion(+) diff --git a/source3/smbd/aio.c b/source3/smbd/aio.c index 0f824f5aa1f..afe76608cd3 100644 --- a/source3/smbd/aio.c +++ b/source3/smbd/aio.c @@ -103,6 +103,7 @@ static int aio_del_req_from_fsp(struct aio_req_fsp_link *lnk) if (fsp->num_aio_requests == 0) { tevent_wait_done(fsp->deferred_close); + TALLOC_FREE(fsp->aio_requests); } return 0; } -- 2.20.1 From d4a8fc9aa4572417f3422732971dde80f436f1e2 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 11 Mar 2020 14:47:50 -0700 Subject: [PATCH 07/25] s3: smbd: Now we free fsp->aio_requests when it gets zero entries, talloc in chunks of 10 instead of 1. Prevents incremental +1 tallocs, and the original idea of this array was that it wasn't freed for io efficiency reasons. Add paranoia integer wrap protection also. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit b90bc0f28918133badbf6810d5e298fc326bd1aa) --- source3/smbd/aio.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/source3/smbd/aio.c b/source3/smbd/aio.c index afe76608cd3..cf35f3297ec 100644 --- a/source3/smbd/aio.c +++ b/source3/smbd/aio.c @@ -122,9 +122,19 @@ bool aio_add_req_to_fsp(files_struct *fsp, struct tevent_req *req) if (array_len <= fsp->num_aio_requests) { struct tevent_req **tmp; + if (fsp->num_aio_requests + 10 < 10) { + /* Integer wrap. */ + TALLOC_FREE(lnk); + return false; + } + + /* + * Allocate in blocks of 10 so we don't allocate + * on every aio request. + */ tmp = talloc_realloc( fsp, fsp->aio_requests, struct tevent_req *, - fsp->num_aio_requests+1); + fsp->num_aio_requests+10); if (tmp == NULL) { TALLOC_FREE(lnk); return false; -- 2.20.1 From 9f301bd2c3add46706aad04aaa81182ee17138eb Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 11 Mar 2020 17:25:59 -0700 Subject: [PATCH 08/25] s3: smbd: In async SMB1 reply_close() set fsp->closing = true, as we already do in SMB2 async close. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit b7d09b30ad14d51bbcbe368a11348754121f6ff8) --- source3/smbd/reply.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c index 40cd7483750..5887df41581 100644 --- a/source3/smbd/reply.c +++ b/source3/smbd/reply.c @@ -5659,6 +5659,13 @@ void reply_close(struct smb_request *req) DEBUG(10, ("closing with aio %u requests pending\n", fsp->num_aio_requests)); + /* + * Flag the file as close in progress. + * This will prevent any more IO being + * done on it. + */ + fsp->closing = true; + /* * We depend on the aio_extra destructor to take care of this * close request once fsp->num_aio_request drops to 0. -- 2.20.1 From 915baed66b1c9499c40826e8f674fb9835fdb3d3 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 11 Mar 2020 15:16:35 -0700 Subject: [PATCH 09/25] s3: smbd: Every place we check fsp->deferred_close, also check for fsp->closing. Eventually this will allow us to remove fsp->deferred_close from the fsp struct (and also source3/lib/tevent_wait.[ch]). BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit 4287ea138e82103cce0a939e504f9810636b4747) --- source3/modules/offload_token.c | 10 ++++++++++ source3/smbd/files.c | 14 ++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/source3/modules/offload_token.c b/source3/modules/offload_token.c index 3fb84dabdff..03bb3309f38 100644 --- a/source3/modules/offload_token.c +++ b/source3/modules/offload_token.c @@ -280,6 +280,16 @@ NTSTATUS vfs_offload_token_check_handles(uint32_t fsctl, return NT_STATUS_ACCESS_DENIED; } + if (src_fsp->closing) { + DBG_INFO("copy chunk src handle with closing in progress.\n"); + return NT_STATUS_ACCESS_DENIED; + } + + if (dst_fsp->closing) { + DBG_INFO("copy chunk dst handle with closing in progress.\n"); + return NT_STATUS_ACCESS_DENIED; + } + if (src_fsp->is_directory) { DBG_INFO("copy chunk no read on src directory handle (%s).\n", smb_fname_str_dbg(src_fsp->fsp_name)); diff --git a/source3/smbd/files.c b/source3/smbd/files.c index 99b2f343685..b06511147ab 100644 --- a/source3/smbd/files.c +++ b/source3/smbd/files.c @@ -598,6 +598,9 @@ files_struct *file_fsp(struct smb_request *req, uint16_t fid) if (req->chain_fsp->deferred_close) { return NULL; } + if (req->chain_fsp->closing) { + return NULL; + } return req->chain_fsp; } @@ -622,6 +625,10 @@ files_struct *file_fsp(struct smb_request *req, uint16_t fid) return NULL; } + if (fsp->closing) { + return NULL; + } + req->chain_fsp = fsp; return fsp; } @@ -669,6 +676,10 @@ struct files_struct *file_fsp_get(struct smbd_smb2_request *smb2req, return NULL; } + if (fsp->closing) { + return NULL; + } + return fsp; } @@ -682,6 +693,9 @@ struct files_struct *file_fsp_smb2(struct smbd_smb2_request *smb2req, if (smb2req->compat_chain_fsp->deferred_close) { return NULL; } + if (smb2req->compat_chain_fsp->closing) { + return NULL; + } return smb2req->compat_chain_fsp; } -- 2.20.1 From de75231e2829bf881d41461566f6b587ae447592 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 10 Mar 2020 12:25:58 -0700 Subject: [PATCH 10/25] s3: smbd: Don't allow force disconnect of a connection already being disconnected. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit ac800ca6bcb43c74a1a6ef508b900e2e6cb532dc) --- source3/smbd/conn_idle.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/source3/smbd/conn_idle.c b/source3/smbd/conn_idle.c index cd12e3f1266..698dc6e3a32 100644 --- a/source3/smbd/conn_idle.c +++ b/source3/smbd/conn_idle.c @@ -103,6 +103,11 @@ void conn_force_tdis( } tcon = conn->tcon; + if (!NT_STATUS_IS_OK(tcon->status)) { + /* In the process of already being disconnected. */ + continue; + } + do_close = check_fn(conn, private_data); if (!do_close) { continue; -- 2.20.1 From 80f0e76d6e052f50ab037fa97938f3c9ca250c5f Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 12 Mar 2020 09:28:21 -0700 Subject: [PATCH 11/25] s3: smbd: Add async internals of conn_force_tdis(). Commented out so it can be seen complete as a diff. The next commit will replace the old synchronous conn_force_tdis() code with the new async code. Uses a wait_queue to cause the force close requests to stay pending until all outstanding aio is finished on all file handles opened on the connection. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit 4f9e0459cd06f0332083a4a465f49b5f258838fa) --- source3/smbd/conn_idle.c | 148 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 148 insertions(+) diff --git a/source3/smbd/conn_idle.c b/source3/smbd/conn_idle.c index 698dc6e3a32..b54cdf82274 100644 --- a/source3/smbd/conn_idle.c +++ b/source3/smbd/conn_idle.c @@ -23,6 +23,7 @@ #include "smbd/smbd.h" #include "smbd/globals.h" #include "rpc_server/rpc_pipes.h" +#include "lib/util/tevent_ntstatus.h" /**************************************************************************** Update last used timestamps. @@ -139,3 +140,150 @@ void conn_force_tdis( change_to_root_user(); reload_services(sconn, conn_snum_used, true); } + +#if 0 +struct conn_force_tdis_state { + struct tevent_queue *wait_queue; +}; + +static void conn_force_tdis_wait_done(struct tevent_req *subreq); + +static struct tevent_req *conn_force_tdis_send(connection_struct *conn) +{ + struct tevent_req *req; + struct conn_force_tdis_state *state; + struct tevent_req *subreq; + files_struct *fsp; + + /* Create this off the NULL context. We must clean up on return. */ + req = tevent_req_create(NULL, &state, + struct conn_force_tdis_state); + if (req == NULL) { + return NULL; + } + state->wait_queue = tevent_queue_create(state, + "conn_force_tdis_wait_queue"); + if (tevent_req_nomem(state->wait_queue, req)) { + TALLOC_FREE(req); + return NULL; + } + + /* + * Make sure that no new request will be able to use this tcon. + * This ensures that once all outstanding fsp->aio_requests + * on this tcon are done, we are safe to close it. + */ + conn->tcon->status = NT_STATUS_NETWORK_NAME_DELETED; + + for (fsp = conn->sconn->files; fsp; fsp = fsp->next) { + if (fsp->conn != conn) { + continue; + } + /* + * Flag the file as close in progress. + * This will prevent any more IO being + * done on it. Not strictly needed, but + * doesn't hurt to flag it as closing. + */ + fsp->closing = true; + + if (fsp->num_aio_requests > 0) { + /* + * Now wait until all aio requests on this fsp are + * finished. + * + * We don't set a callback, as we just want to block the + * wait queue and the talloc_free() of fsp->aio_request + * will remove the item from the wait queue. + */ + subreq = tevent_queue_wait_send(fsp->aio_requests, + conn->sconn->ev_ctx, + state->wait_queue); + if (tevent_req_nomem(subreq, req)) { + TALLOC_FREE(req); + return NULL; + } + } + } + /* + * Now we add our own waiter to the end of the queue, + * this way we get notified when all pending requests are finished + * and reply to the outstanding SMB1 request. + */ + subreq = tevent_queue_wait_send(state, + conn->sconn->ev_ctx, + state->wait_queue); + if (tevent_req_nomem(subreq, req)) { + TALLOC_FREE(req); + return NULL; + } + + tevent_req_set_callback(subreq, conn_force_tdis_wait_done, req); + return req; +} + +static void conn_force_tdis_wait_done(struct tevent_req *subreq) +{ + struct tevent_req *req = tevent_req_callback_data( + subreq, struct tevent_req); + + tevent_queue_wait_recv(subreq); + TALLOC_FREE(subreq); + tevent_req_done(req); +} + +static NTSTATUS conn_force_tdis_recv(struct tevent_req *req) +{ + return tevent_req_simple_recv_ntstatus(req); +} + +static void conn_force_tdis_done(struct tevent_req *req) +{ + connection_struct *conn = tevent_req_callback_data( + req, connection_struct); + NTSTATUS status; + uint64_t vuid = UID_FIELD_INVALID; + struct smbXsrv_tcon *tcon = conn->tcon; + struct smbd_server_connection *sconn = conn->sconn; + + status = conn_force_tdis_recv(req); + TALLOC_FREE(req); + if (!NT_STATUS_IS_OK(status)) { + DBG_ERR("conn_force_tdis_recv of share '%s' " + "(wire_id=0x%08x) failed: %s\n", + tcon->global->share_name, + tcon->global->tcon_wire_id, + nt_errstr(status)); + return; + } + + if (conn->sconn->using_smb2) { + vuid = conn->vuid; + } + + DBG_WARNING("Closing " + "share '%s' (wire_id=0x%08x)\n", + tcon->global->share_name, + tcon->global->tcon_wire_id); + + conn = NULL; + status = smbXsrv_tcon_disconnect(tcon, vuid); + if (!NT_STATUS_IS_OK(status)) { + DBG_ERR("smbXsrv_tcon_disconnect() of share '%s' " + "(wire_id=0x%08x) failed: %s\n", + tcon->global->share_name, + tcon->global->tcon_wire_id, + nt_errstr(status)); + return; + } + + TALLOC_FREE(tcon); + + /* + * As we've been awoken, we may have changed + * uid in the meantime. Ensure we're still root. + */ + change_to_root_user(); + reload_services(sconn, conn_snum_used, true); +} +#endif -- 2.20.1 From c136392548b3265a3664fe926c34bad61fd07ff6 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 12 Mar 2020 09:47:26 -0700 Subject: [PATCH 12/25] s3: smbd: Replace synchronous conn_force_tdis() with the async version. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit 7891302ab8eeba8261b92171a4d429e2f538b89a) --- source3/smbd/conn_idle.c | 44 +++++++++++++++------------------------- 1 file changed, 16 insertions(+), 28 deletions(-) diff --git a/source3/smbd/conn_idle.c b/source3/smbd/conn_idle.c index b54cdf82274..cf5a417bff7 100644 --- a/source3/smbd/conn_idle.c +++ b/source3/smbd/conn_idle.c @@ -77,27 +77,27 @@ bool conn_idle_all(struct smbd_server_connection *sconn, time_t t) } /**************************************************************************** - Forcibly unmount a share. + Forcibly unmount a share - async All instances of the parameter 'sharename' share are unmounted. The special sharename '*' forces unmount of all shares. ****************************************************************************/ +static struct tevent_req *conn_force_tdis_send(connection_struct *conn); +static void conn_force_tdis_done(struct tevent_req *req); + void conn_force_tdis( struct smbd_server_connection *sconn, bool (*check_fn)(struct connection_struct *conn, void *private_data), void *private_data) { - connection_struct *conn, *next; + connection_struct *conn; /* SMB1 and SMB 2*/ - for (conn = sconn->connections; conn; conn = next) { + for (conn = sconn->connections; conn; conn = conn->next) { struct smbXsrv_tcon *tcon; bool do_close = false; - NTSTATUS status; - uint64_t vuid = UID_FIELD_INVALID; - - next = conn->next; + struct tevent_req *req; if (conn->tcon == NULL) { continue; @@ -114,34 +114,23 @@ void conn_force_tdis( continue; } + req = conn_force_tdis_send(conn); + if (req == NULL) { + DBG_WARNING("talloc_fail forcing async close of " + "share '%s'\n", + tcon->global->share_name); + continue; + } + DBG_WARNING("Forcing close of " "share '%s' (wire_id=0x%08x)\n", tcon->global->share_name, tcon->global->tcon_wire_id); - if (sconn->using_smb2) { - vuid = conn->vuid; - } - - conn = NULL; - status = smbXsrv_tcon_disconnect(tcon, vuid); - if (!NT_STATUS_IS_OK(status)) { - DEBUG(0, ("conn_force_tdis: " - "smbXsrv_tcon_disconnect() of share '%s' " - "(wire_id=0x%08x) failed: %s\n", - tcon->global->share_name, - tcon->global->tcon_wire_id, - nt_errstr(status))); - } - - TALLOC_FREE(tcon); + tevent_req_set_callback(req, conn_force_tdis_done, conn); } - - change_to_root_user(); - reload_services(sconn, conn_snum_used, true); } -#if 0 struct conn_force_tdis_state { struct tevent_queue *wait_queue; }; @@ -286,4 +275,3 @@ static void conn_force_tdis_done(struct tevent_req *req) change_to_root_user(); reload_services(sconn, conn_snum_used, true); } -#endif -- 2.20.1 From c1f9328e0c8ba555e27d258366f4a9ebbd843492 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 12 Mar 2020 09:51:53 -0700 Subject: [PATCH 13/25] s3: smbd: Add async internals of reply_tdis(). Waits until all aio requests on all fsp's under this conn struct are finished before returning to the client. Charges the profile time in the done function. Not strictly correct but better than the other SMB1 async code that double-charges profiling in both send and done at the moment. Done this way (commented out) so it is a clean diff and it's clear what is being added. A later commit will remove the old synchronous version. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit 86cc67d5a7de0a81131b11447dad57b2681d8e01) --- source3/smbd/reply.c | 173 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 173 insertions(+) diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c index 5887df41581..3c7bdff49ac 100644 --- a/source3/smbd/reply.c +++ b/source3/smbd/reply.c @@ -42,6 +42,7 @@ #include "smbprofile.h" #include "../lib/tsocket/tsocket.h" #include "lib/tevent_wait.h" +#include "lib/util/tevent_ntstatus.h" #include "libcli/smb/smb_signing.h" #include "lib/util/sys_rw_data.h" #include "librpc/gen_ndr/open_files.h" @@ -6067,6 +6068,178 @@ void reply_tdis(struct smb_request *req) return; } +#if 0 +struct reply_tdis_state { + struct tevent_queue *wait_queue; +}; + +static void reply_tdis_wait_done(struct tevent_req *subreq); + +/**************************************************************************** + Async SMB1 tdis. + Note, on failure here we deallocate and return NULL to allow the caller to + SMB1 return an error of ERRnomem immediately. +****************************************************************************/ + +static struct tevent_req *reply_tdis_send(struct smb_request *smb1req) +{ + struct tevent_req *req; + struct reply_tdis_state *state; + struct tevent_req *subreq; + connection_struct *conn = smb1req->conn; + files_struct *fsp; + + req = tevent_req_create(smb1req, &state, + struct reply_tdis_state); + if (req == NULL) { + return NULL; + } + state->wait_queue = tevent_queue_create(state, "reply_tdis_wait_queue"); + if (tevent_req_nomem(state->wait_queue, req)) { + TALLOC_FREE(req); + return NULL; + } + + /* + * Make sure that no new request will be able to use this tcon. + * This ensures that once all outstanding fsp->aio_requests + * on this tcon are done, we are safe to close it. + */ + conn->tcon->status = NT_STATUS_NETWORK_NAME_DELETED; + + for (fsp = conn->sconn->files; fsp; fsp = fsp->next) { + if (fsp->conn != conn) { + continue; + } + /* + * Flag the file as close in progress. + * This will prevent any more IO being + * done on it. Not strictly needed, but + * doesn't hurt to flag it as closing. + */ + fsp->closing = true; + + if (fsp->num_aio_requests > 0) { + /* + * Now wait until all aio requests on this fsp are + * finished. + * + * We don't set a callback, as we just want to block the + * wait queue and the talloc_free() of fsp->aio_request + * will remove the item from the wait queue. + */ + subreq = tevent_queue_wait_send(fsp->aio_requests, + conn->sconn->ev_ctx, + state->wait_queue); + if (tevent_req_nomem(subreq, req)) { + TALLOC_FREE(req); + return NULL; + } + } + } + + /* + * Now we add our own waiter to the end of the queue, + * this way we get notified when all pending requests are finished + * and reply to the outstanding SMB1 request. + */ + subreq = tevent_queue_wait_send(state, + conn->sconn->ev_ctx, + state->wait_queue); + if (tevent_req_nomem(subreq, req)) { + TALLOC_FREE(req); + return NULL; + } + + /* + * We're really going async - move the SMB1 request from + * a talloc stackframe above us to the sconn talloc-context. + * We need this to stick around until the wait_done + * callback is invoked. + */ + smb1req = talloc_move(smb1req->sconn, &smb1req); + + tevent_req_set_callback(subreq, reply_tdis_wait_done, req); + + return req; +} + +static void reply_tdis_wait_done(struct tevent_req *subreq) +{ + struct tevent_req *req = tevent_req_callback_data( + subreq, struct tevent_req); + + tevent_queue_wait_recv(subreq); + TALLOC_FREE(subreq); + tevent_req_done(req); +} + +static NTSTATUS reply_tdis_recv(struct tevent_req *req) +{ + return tevent_req_simple_recv_ntstatus(req); +} + +static void reply_tdis_done(struct tevent_req *req) +{ + struct smb_request *smb1req = tevent_req_callback_data( + req, struct smb_request); + NTSTATUS status; + struct smbXsrv_tcon *tcon = smb1req->conn->tcon; + bool ok; + + /* + * Take the profile charge here. Not strictly + * correct but better than the other SMB1 async + * code that double-charges at the moment. + */ + START_PROFILE(SMBtdis); + + status = reply_tdis_recv(req); + TALLOC_FREE(req); + if (!NT_STATUS_IS_OK(status)) { + TALLOC_FREE(smb1req); + END_PROFILE(SMBtdis); + exit_server(__location__ ": reply_tdis_recv failed"); + return; + } + + /* + * As we've been awoken, we may have changed + * directory in the meantime. + * reply_tdis() has the DO_CHDIR flag set. + */ + ok = chdir_current_service(smb1req->conn); + if (!ok) { + reply_force_doserror(smb1req, ERRSRV, ERRinvnid); + smb_request_done(smb1req); + END_PROFILE(SMBtdis); + } + + status = smbXsrv_tcon_disconnect(tcon, + smb1req->vuid); + if (!NT_STATUS_IS_OK(status)) { + TALLOC_FREE(smb1req); + END_PROFILE(SMBtdis); + exit_server(__location__ ": smbXsrv_tcon_disconnect failed"); + return; + } + + /* smbXsrv_tcon_disconnect frees smb1req->conn. */ + smb1req->conn = NULL; + + TALLOC_FREE(tcon); + + reply_outbuf(smb1req, 0, 0); + /* + * The following call is needed to push the + * reply data back out the socket after async + * return. Plus it frees smb1req. + */ + smb_request_done(smb1req); + END_PROFILE(SMBtdis); +} +#endif + /**************************************************************************** Reply to a echo. conn POINTER CAN BE NULL HERE ! -- 2.20.1 From 76cb55666d15788611a4eda5f268524e0a13e9aa Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 12 Mar 2020 09:55:30 -0700 Subject: [PATCH 14/25] s3: smbd: In reply_tdis(), replace req -> smb1req. Minimises the diff in the next commit. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit ca4521f1dd97bc5a05e381c652b05ae1eb8bd29b) --- source3/smbd/reply.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c index 3c7bdff49ac..e67f598db58 100644 --- a/source3/smbd/reply.c +++ b/source3/smbd/reply.c @@ -6026,28 +6026,28 @@ void reply_unlock(struct smb_request *req) conn POINTER CAN BE NULL HERE ! ****************************************************************************/ -void reply_tdis(struct smb_request *req) +void reply_tdis(struct smb_request *smb1req) { NTSTATUS status; - connection_struct *conn = req->conn; + connection_struct *conn = smb1req->conn; struct smbXsrv_tcon *tcon; START_PROFILE(SMBtdis); if (!conn) { DEBUG(4,("Invalid connection in tdis\n")); - reply_force_doserror(req, ERRSRV, ERRinvnid); + reply_force_doserror(smb1req, ERRSRV, ERRinvnid); END_PROFILE(SMBtdis); return; } tcon = conn->tcon; - req->conn = NULL; + smb1req->conn = NULL; /* * TODO: cancel all outstanding requests on the tcon */ - status = smbXsrv_tcon_disconnect(tcon, req->vuid); + status = smbXsrv_tcon_disconnect(tcon, smb1req->vuid); if (!NT_STATUS_IS_OK(status)) { DEBUG(0, ("reply_tdis: " "smbXsrv_tcon_disconnect() failed: %s\n", @@ -6063,7 +6063,7 @@ void reply_tdis(struct smb_request *req) TALLOC_FREE(tcon); - reply_outbuf(req, 0, 0); + reply_outbuf(smb1req, 0, 0); END_PROFILE(SMBtdis); return; } -- 2.20.1 From 050b2a5ee21ba3271a92f178f25cb42eb66ef1d4 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 12 Mar 2020 09:59:47 -0700 Subject: [PATCH 15/25] s3: smbd: reply_tdis() Update to modern coding standards. Minimizes the diff in the next commit. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit 71725f1c4adaa04ef04c0dd400c49399952ef5fa) --- source3/smbd/reply.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c index e67f598db58..d9e05c09830 100644 --- a/source3/smbd/reply.c +++ b/source3/smbd/reply.c @@ -6034,8 +6034,8 @@ void reply_tdis(struct smb_request *smb1req) START_PROFILE(SMBtdis); - if (!conn) { - DEBUG(4,("Invalid connection in tdis\n")); + if (conn == NULL) { + DBG_INFO("Invalid connection in tdis\n"); reply_force_doserror(smb1req, ERRSRV, ERRinvnid); END_PROFILE(SMBtdis); return; -- 2.20.1 From 868a7c75f796f9f55da9828b1c03b1538b8a11c2 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 12 Mar 2020 10:03:27 -0700 Subject: [PATCH 16/25] s3: smbd: Remove old synchronous SMB1 reply_tdis(). SMB1 tree disconnect is now fully async. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit 7613998e10c5f13c896667257fdef33824a45d2a) --- source3/smbd/reply.c | 45 +++++++++++++++++++------------------------- 1 file changed, 19 insertions(+), 26 deletions(-) diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c index d9e05c09830..9f79f7de6ce 100644 --- a/source3/smbd/reply.c +++ b/source3/smbd/reply.c @@ -6026,49 +6026,43 @@ void reply_unlock(struct smb_request *req) conn POINTER CAN BE NULL HERE ! ****************************************************************************/ +static struct tevent_req *reply_tdis_send(struct smb_request *smb1req); +static void reply_tdis_done(struct tevent_req *req); + void reply_tdis(struct smb_request *smb1req) { - NTSTATUS status; connection_struct *conn = smb1req->conn; - struct smbXsrv_tcon *tcon; + struct tevent_req *req; - START_PROFILE(SMBtdis); + /* + * Don't setup the profile charge here, take + * it in reply_tdis_done(). Not strictly correct + * but better than the other SMB1 async + * code that double-charges at the moment. + */ if (conn == NULL) { + /* Not going async, profile here. */ + START_PROFILE(SMBtdis); DBG_INFO("Invalid connection in tdis\n"); reply_force_doserror(smb1req, ERRSRV, ERRinvnid); END_PROFILE(SMBtdis); return; } - tcon = conn->tcon; - smb1req->conn = NULL; - - /* - * TODO: cancel all outstanding requests on the tcon - */ - status = smbXsrv_tcon_disconnect(tcon, smb1req->vuid); - if (!NT_STATUS_IS_OK(status)) { - DEBUG(0, ("reply_tdis: " - "smbXsrv_tcon_disconnect() failed: %s\n", - nt_errstr(status))); - /* - * If we hit this case, there is something completely - * wrong, so we better disconnect the transport connection. - */ + req = reply_tdis_send(smb1req); + if (req == NULL) { + /* Not going async, profile here. */ + START_PROFILE(SMBtdis); + reply_force_doserror(smb1req, ERRDOS, ERRnomem); END_PROFILE(SMBtdis); - exit_server(__location__ ": smbXsrv_tcon_disconnect failed"); return; } - - TALLOC_FREE(tcon); - - reply_outbuf(smb1req, 0, 0); - END_PROFILE(SMBtdis); + /* We're async. This will complete later. */ + tevent_req_set_callback(req, reply_tdis_done, smb1req); return; } -#if 0 struct reply_tdis_state { struct tevent_queue *wait_queue; }; @@ -6238,7 +6232,6 @@ static void reply_tdis_done(struct tevent_req *req) smb_request_done(smb1req); END_PROFILE(SMBtdis); } -#endif /**************************************************************************** Reply to a echo. -- 2.20.1 From d5d78f52567ae76147db719be8f940f8bdb7e234 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 12 Mar 2020 10:11:14 -0700 Subject: [PATCH 17/25] s3: smbd: Add async internals of reply_ulogoffX. Waits until all aio requests on all fsp's owned by this vuid are finished before returning to the client. Charges the profile time in the done function. Not strictly correct but better than the other SMB1 async code that double-charges profiling in both send and done at the moment. Done this way (commented out) so it is a clean diff and it's clear what is being added. A later commit will remove the old synchronous version. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit 4dd3012cb1b5e000ccf68d2601dbdbcb7ff538b5) --- source3/smbd/reply.c | 168 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 168 insertions(+) diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c index 9f79f7de6ce..c2e3719ac57 100644 --- a/source3/smbd/reply.c +++ b/source3/smbd/reply.c @@ -2664,6 +2664,174 @@ void reply_ulogoffX(struct smb_request *req) req->vuid = UID_FIELD_INVALID; } +#if 0 +struct reply_ulogoffX_state { + struct tevent_queue *wait_queue; + struct smbXsrv_session *session; +}; + +static void reply_ulogoffX_wait_done(struct tevent_req *subreq); + +/**************************************************************************** + Async SMB1 ulogoffX. + Note, on failure here we deallocate and return NULL to allow the caller to + SMB1 return an error of ERRnomem immediately. +****************************************************************************/ + +static struct tevent_req *reply_ulogoffX_send(struct smb_request *smb1req, + struct smbXsrv_session *session) +{ + struct tevent_req *req; + struct reply_ulogoffX_state *state; + struct tevent_req *subreq; + files_struct *fsp; + struct smbd_server_connection *sconn = session->client->sconn; + uint64_t vuid = session->global->session_wire_id; + + req = tevent_req_create(smb1req, &state, + struct reply_ulogoffX_state); + if (req == NULL) { + return NULL; + } + state->wait_queue = tevent_queue_create(state, + "reply_ulogoffX_wait_queue"); + if (tevent_req_nomem(state->wait_queue, req)) { + TALLOC_FREE(req); + return NULL; + } + state->session = session; + + /* + * Make sure that no new request will be able to use this session. + * This ensures that once all outstanding fsp->aio_requests + * on this session are done, we are safe to close it. + */ + session->status = NT_STATUS_USER_SESSION_DELETED; + + for (fsp = sconn->files; fsp; fsp = fsp->next) { + if (fsp->vuid != vuid) { + continue; + } + /* + * Flag the file as close in progress. + * This will prevent any more IO being + * done on it. + */ + fsp->closing = true; + + if (fsp->num_aio_requests > 0) { + /* + * Now wait until all aio requests on this fsp are + * finished. + * + * We don't set a callback, as we just want to block the + * wait queue and the talloc_free() of fsp->aio_request + * will remove the item from the wait queue. + */ + subreq = tevent_queue_wait_send(fsp->aio_requests, + sconn->ev_ctx, + state->wait_queue); + if (tevent_req_nomem(subreq, req)) { + TALLOC_FREE(req); + return NULL; + } + } + } + + /* + * Now we add our own waiter to the end of the queue, + * this way we get notified when all pending requests are finished + * and reply to the outstanding SMB1 request. + */ + subreq = tevent_queue_wait_send(state, + sconn->ev_ctx, + state->wait_queue); + if (tevent_req_nomem(subreq, req)) { + TALLOC_FREE(req); + return NULL; + } + + /* + * We're really going async - move the SMB1 request from + * a talloc stackframe above us to the sconn talloc-context. + * We need this to stick around until the wait_done + * callback is invoked. + */ + smb1req = talloc_move(sconn, &smb1req); + + tevent_req_set_callback(subreq, reply_ulogoffX_wait_done, req); + + return req; +} + +static void reply_ulogoffX_wait_done(struct tevent_req *subreq) +{ + struct tevent_req *req = tevent_req_callback_data( + subreq, struct tevent_req); + + tevent_queue_wait_recv(subreq); + TALLOC_FREE(subreq); + tevent_req_done(req); +} + +static NTSTATUS reply_ulogoffX_recv(struct tevent_req *req) +{ + return tevent_req_simple_recv_ntstatus(req); +} + +static void reply_ulogoffX_done(struct tevent_req *req) +{ + struct smb_request *smb1req = tevent_req_callback_data( + req, struct smb_request); + struct reply_ulogoffX_state *state = tevent_req_data(req, + struct reply_ulogoffX_state); + struct smbXsrv_session *session = state->session; + NTSTATUS status; + + /* + * Take the profile charge here. Not strictly + * correct but better than the other SMB1 async + * code that double-charges at the moment. + */ + START_PROFILE(SMBulogoffX); + + status = reply_ulogoffX_recv(req); + TALLOC_FREE(req); + if (!NT_STATUS_IS_OK(status)) { + TALLOC_FREE(smb1req); + END_PROFILE(SMBulogoffX); + exit_server(__location__ ": reply_ulogoffX_recv failed"); + return; + } + + status = smbXsrv_session_logoff(session); + if (!NT_STATUS_IS_OK(status)) { + TALLOC_FREE(smb1req); + END_PROFILE(SMBulogoffX); + exit_server(__location__ ": smbXsrv_session_logoff failed"); + return; + } + + TALLOC_FREE(session); + + reply_outbuf(smb1req, 2, 0); + SSVAL(smb1req->outbuf, smb_vwv0, 0xff); /* andx chain ends */ + SSVAL(smb1req->outbuf, smb_vwv1, 0); /* no andx offset */ + + DBG_NOTICE("ulogoffX vuid=%llu\n", + (unsigned long long)smb1req->vuid); + + smb1req->vuid = UID_FIELD_INVALID; + /* + * The following call is needed to push the + * reply data back out the socket after async + * return. Plus it frees smb1req. + */ + smb_request_done(smb1req); + END_PROFILE(SMBulogoffX); +} +#endif + /**************************************************************************** Reply to a mknew or a create. ****************************************************************************/ -- 2.20.1 From ba6f525b93a7020ed1898d59c79a8ec8b83fb7df Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 12 Mar 2020 10:15:10 -0700 Subject: [PATCH 18/25] s3: smbd: In reply_ulogoffX(), replace req -> smb1req. Minimises the diff in later commits. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit 5c073aa01b304f54a0039d9cd9dc74123191eb4b) --- source3/smbd/reply.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c index c2e3719ac57..66fb9b2bd63 100644 --- a/source3/smbd/reply.c +++ b/source3/smbd/reply.c @@ -2612,7 +2612,7 @@ void reply_open_and_X(struct smb_request *req) Reply to a SMBulogoffX. ****************************************************************************/ -void reply_ulogoffX(struct smb_request *req) +void reply_ulogoffX(struct smb_request *smb1req) { struct timeval now = timeval_current(); struct smbXsrv_session *session = NULL; @@ -2620,16 +2620,16 @@ void reply_ulogoffX(struct smb_request *req) START_PROFILE(SMBulogoffX); - status = smb1srv_session_lookup(req->xconn, - req->vuid, + status = smb1srv_session_lookup(smb1req->xconn, + smb1req->vuid, timeval_to_nttime(&now), &session); if (!NT_STATUS_IS_OK(status)) { DEBUG(3,("ulogoff, vuser id %llu does not map to user.\n", - (unsigned long long)req->vuid)); + (unsigned long long)smb1req->vuid)); - req->vuid = UID_FIELD_INVALID; - reply_force_doserror(req, ERRSRV, ERRbaduid); + smb1req->vuid = UID_FIELD_INVALID; + reply_force_doserror(smb1req, ERRSRV, ERRbaduid); END_PROFILE(SMBulogoffX); return; } @@ -2653,15 +2653,15 @@ void reply_ulogoffX(struct smb_request *req) TALLOC_FREE(session); - reply_outbuf(req, 2, 0); - SSVAL(req->outbuf, smb_vwv0, 0xff); /* andx chain ends */ - SSVAL(req->outbuf, smb_vwv1, 0); /* no andx offset */ + reply_outbuf(smb1req, 2, 0); + SSVAL(smb1req->outbuf, smb_vwv0, 0xff); /* andx chain ends */ + SSVAL(smb1req->outbuf, smb_vwv1, 0); /* no andx offset */ DEBUG(3, ("ulogoffX vuid=%llu\n", - (unsigned long long)req->vuid)); + (unsigned long long)smb1req->vuid)); END_PROFILE(SMBulogoffX); - req->vuid = UID_FIELD_INVALID; + smb1req->vuid = UID_FIELD_INVALID; } #if 0 -- 2.20.1 From 4495645dab0728af824dfd0ab17ba24511e476ef Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 12 Mar 2020 10:16:42 -0700 Subject: [PATCH 19/25] s3: smbd: reply_ulogoffX() Update to modern coding standards. Minimizes the diff in the later commits. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit 9cda76ad29db0cfbffa3dbb0764ec5dda24490f9) --- source3/smbd/reply.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c index 66fb9b2bd63..7b5ed7ef0db 100644 --- a/source3/smbd/reply.c +++ b/source3/smbd/reply.c @@ -2625,8 +2625,8 @@ void reply_ulogoffX(struct smb_request *smb1req) timeval_to_nttime(&now), &session); if (!NT_STATUS_IS_OK(status)) { - DEBUG(3,("ulogoff, vuser id %llu does not map to user.\n", - (unsigned long long)smb1req->vuid)); + DBG_WARNING("ulogoff, vuser id %llu does not map to user.\n", + (unsigned long long)smb1req->vuid); smb1req->vuid = UID_FIELD_INVALID; reply_force_doserror(smb1req, ERRSRV, ERRbaduid); -- 2.20.1 From f1116b3cd4d414b9a1267d052aeeb5eef664c9d7 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 12 Mar 2020 10:19:31 -0700 Subject: [PATCH 20/25] s3: smbd: Remove old synchronous SMB1 reply_ulogoffX(). SMB1 user logoff is now fully async. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit 446b64ca66591d8ae5b4bf1aabdd46a1e8cb1c1c) --- source3/smbd/reply.c | 48 +++++++++++++++++++------------------------- 1 file changed, 21 insertions(+), 27 deletions(-) diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c index 7b5ed7ef0db..87f78df85d9 100644 --- a/source3/smbd/reply.c +++ b/source3/smbd/reply.c @@ -2612,19 +2612,31 @@ void reply_open_and_X(struct smb_request *req) Reply to a SMBulogoffX. ****************************************************************************/ +static struct tevent_req *reply_ulogoffX_send(struct smb_request *smb1req, + struct smbXsrv_session *session); +static void reply_ulogoffX_done(struct tevent_req *req); + void reply_ulogoffX(struct smb_request *smb1req) { struct timeval now = timeval_current(); struct smbXsrv_session *session = NULL; + struct tevent_req *req; NTSTATUS status; - START_PROFILE(SMBulogoffX); + /* + * Don't setup the profile charge here, take + * it in reply_ulogoffX_done(). Not strictly correct + * but better than the other SMB1 async + * code that double-charges at the moment. + */ status = smb1srv_session_lookup(smb1req->xconn, smb1req->vuid, timeval_to_nttime(&now), &session); if (!NT_STATUS_IS_OK(status)) { + /* Not going async, profile here. */ + START_PROFILE(SMBulogoffX); DBG_WARNING("ulogoff, vuser id %llu does not map to user.\n", (unsigned long long)smb1req->vuid); @@ -2634,37 +2646,20 @@ void reply_ulogoffX(struct smb_request *smb1req) return; } - /* - * TODO: cancel all outstanding requests on the session - */ - status = smbXsrv_session_logoff(session); - if (!NT_STATUS_IS_OK(status)) { - DEBUG(0, ("reply_ulogoff: " - "smbXsrv_session_logoff() failed: %s\n", - nt_errstr(status))); - /* - * If we hit this case, there is something completely - * wrong, so we better disconnect the transport connection. - */ + req = reply_ulogoffX_send(smb1req, session); + if (req == NULL) { + /* Not going async, profile here. */ + START_PROFILE(SMBulogoffX); + reply_force_doserror(smb1req, ERRDOS, ERRnomem); END_PROFILE(SMBulogoffX); - exit_server(__location__ ": smbXsrv_session_logoff failed"); return; } - TALLOC_FREE(session); - - reply_outbuf(smb1req, 2, 0); - SSVAL(smb1req->outbuf, smb_vwv0, 0xff); /* andx chain ends */ - SSVAL(smb1req->outbuf, smb_vwv1, 0); /* no andx offset */ - - DEBUG(3, ("ulogoffX vuid=%llu\n", - (unsigned long long)smb1req->vuid)); - - END_PROFILE(SMBulogoffX); - smb1req->vuid = UID_FIELD_INVALID; + /* We're async. This will complete later. */ + tevent_req_set_callback(req, reply_ulogoffX_done, smb1req); + return; } -#if 0 struct reply_ulogoffX_state { struct tevent_queue *wait_queue; struct smbXsrv_session *session; @@ -2830,7 +2825,6 @@ static void reply_ulogoffX_done(struct tevent_req *req) smb_request_done(smb1req); END_PROFILE(SMBulogoffX); } -#endif /**************************************************************************** Reply to a mknew or a create. -- 2.20.1 From ba5120611008766494070baba44c0c3f547b62b2 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 12 Mar 2020 10:59:16 -0700 Subject: [PATCH 21/25] s3: smbd: Add async internals of reply_exit(). Waits until all aio requests on all fsp's owned by this vuid are finished before returning to the client. Charges the profile time in the done function. Not strictly correct but better than the other SMB1 async code that double-charges profiling in both send and done at the moment. Done this way (commented out) so it is a clean diff and it's clear what is being added. A later commit will remove the old synchronous version. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit 8f58feab58afbc7aa214fac2a1728dda68303c6b) --- source3/smbd/reply.c | 201 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 201 insertions(+) diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c index 87f78df85d9..7b145da4f47 100644 --- a/source3/smbd/reply.c +++ b/source3/smbd/reply.c @@ -5767,6 +5767,207 @@ void reply_exit(struct smb_request *req) return; } +#if 0 +struct reply_exit_state { + struct tevent_queue *wait_queue; +}; + +static void reply_exit_wait_done(struct tevent_req *subreq); + +/**************************************************************************** + Async SMB1 exit. + Note, on failure here we deallocate and return NULL to allow the caller to + SMB1 return an error of ERRnomem immediately. +****************************************************************************/ + +static struct tevent_req *reply_exit_send(struct smb_request *smb1req) +{ + struct tevent_req *req; + struct reply_exit_state *state; + struct tevent_req *subreq; + files_struct *fsp; + struct smbd_server_connection *sconn = smb1req->sconn; + + req = tevent_req_create(smb1req, &state, + struct reply_exit_state); + if (req == NULL) { + return NULL; + } + state->wait_queue = tevent_queue_create(state, + "reply_exit_wait_queue"); + if (tevent_req_nomem(state->wait_queue, req)) { + TALLOC_FREE(req); + return NULL; + } + + for (fsp = sconn->files; fsp; fsp = fsp->next) { + if (fsp->file_pid != smb1req->smbpid) { + continue; + } + if (fsp->vuid != smb1req->vuid) { + continue; + } + /* + * Flag the file as close in progress. + * This will prevent any more IO being + * done on it. + */ + fsp->closing = true; + + if (fsp->num_aio_requests > 0) { + /* + * Now wait until all aio requests on this fsp are + * finished. + * + * We don't set a callback, as we just want to block the + * wait queue and the talloc_free() of fsp->aio_request + * will remove the item from the wait queue. + */ + subreq = tevent_queue_wait_send(fsp->aio_requests, + sconn->ev_ctx, + state->wait_queue); + if (tevent_req_nomem(subreq, req)) { + TALLOC_FREE(req); + return NULL; + } + } + } + + /* + * Now we add our own waiter to the end of the queue, + * this way we get notified when all pending requests are finished + * and reply to the outstanding SMB1 request. + */ + subreq = tevent_queue_wait_send(state, + sconn->ev_ctx, + state->wait_queue); + if (tevent_req_nomem(subreq, req)) { + TALLOC_FREE(req); + return NULL; + } + + /* + * We're really going async - move the SMB1 request from + * a talloc stackframe above us to the conn talloc-context. + * We need this to stick around until the wait_done + * callback is invoked. + */ + smb1req = talloc_move(sconn, &smb1req); + + tevent_req_set_callback(subreq, reply_exit_wait_done, req); + + return req; +} + +static void reply_exit_wait_done(struct tevent_req *subreq) +{ + struct tevent_req *req = tevent_req_callback_data( + subreq, struct tevent_req); + + tevent_queue_wait_recv(subreq); + TALLOC_FREE(subreq); + tevent_req_done(req); +} + +static NTSTATUS reply_exit_recv(struct tevent_req *req) +{ + return tevent_req_simple_recv_ntstatus(req); +} + +static void reply_exit_done(struct tevent_req *req) +{ + struct smb_request *smb1req = tevent_req_callback_data( + req, struct smb_request); + struct smbd_server_connection *sconn = smb1req->sconn; + struct smbXsrv_connection *xconn = smb1req->xconn; + NTTIME now = timeval_to_nttime(&smb1req->request_time); + struct smbXsrv_session *session = NULL; + files_struct *fsp, *next; + NTSTATUS status; + + /* + * Take the profile charge here. Not strictly + * correct but better than the other SMB1 async + * code that double-charges at the moment. + */ + START_PROFILE(SMBexit); + + status = reply_exit_recv(req); + TALLOC_FREE(req); + if (!NT_STATUS_IS_OK(status)) { + TALLOC_FREE(smb1req); + END_PROFILE(SMBexit); + exit_server(__location__ ": reply_exit_recv failed"); + return; + } + + /* + * Ensure the session is still valid. + */ + status = smb1srv_session_lookup(xconn, + smb1req->vuid, + now, + &session); + if (!NT_STATUS_IS_OK(status)) { + reply_force_doserror(smb1req, ERRSRV, ERRinvnid); + smb_request_done(smb1req); + END_PROFILE(SMBexit); + } + + /* + * Ensure the vuid is still valid - no one + * called reply_ulogoffX() in the meantime. + * reply_exit() doesn't have AS_USER set, so + * use set_current_user_info() directly. + * This is the same logic as in switch_message(). + */ + if (session->global->auth_session_info != NULL) { + set_current_user_info( + session->global->auth_session_info->unix_info->sanitized_username, + session->global->auth_session_info->unix_info->unix_name, + session->global->auth_session_info->info->domain_name); + } + + /* No more aio - do the actual closes. */ + for (fsp = sconn->files; fsp; fsp = next) { + bool ok; + next = fsp->next; + + if (fsp->file_pid != smb1req->smbpid) { + continue; + } + if (fsp->vuid != smb1req->vuid) { + continue; + } + if (!fsp->closing) { + continue; + } + + /* + * reply_exit() has the DO_CHDIR flag set. + */ + ok = chdir_current_service(fsp->conn); + if (!ok) { + reply_force_doserror(smb1req, ERRSRV, ERRinvnid); + smb_request_done(smb1req); + END_PROFILE(SMBexit); + } + close_file(NULL, fsp, SHUTDOWN_CLOSE); + } + + reply_outbuf(smb1req, 0, 0); + /* + * The following call is needed to push the + * reply data back out the socket after async + * return. Plus it frees smb1req. + */ + smb_request_done(smb1req); + DBG_INFO("reply_exit complete\n"); + END_PROFILE(SMBexit); + return; +} +#endif + struct reply_close_state { files_struct *fsp; struct smb_request *smbreq; -- 2.20.1 From d396723b6520a11613abfda77eefe6bf5b0d7063 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 12 Mar 2020 11:01:05 -0700 Subject: [PATCH 22/25] s3: smbd: Remove old synchronous SMB1 reply_exit(). SMB1 exit is now fully async. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit 1de0daa715f3324e3620ae8152b7fbaeb40ee9d9) --- source3/smbd/reply.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c index 7b145da4f47..2d4bb07305e 100644 --- a/source3/smbd/reply.c +++ b/source3/smbd/reply.c @@ -5753,21 +5753,33 @@ void reply_flush(struct smb_request *req) conn POINTER CAN BE NULL HERE ! ****************************************************************************/ -void reply_exit(struct smb_request *req) -{ - START_PROFILE(SMBexit); - - file_close_pid(req->sconn, req->smbpid, req->vuid); +static struct tevent_req *reply_exit_send(struct smb_request *smb1req); +static void reply_exit_done(struct tevent_req *req); - reply_outbuf(req, 0, 0); +void reply_exit(struct smb_request *smb1req) +{ + struct tevent_req *req; - DEBUG(3,("exit\n")); + /* + * Don't setup the profile charge here, take + * it in reply_exit_done(). Not strictly correct + * but better than the other SMB1 async + * code that double-charges at the moment. + */ + req = reply_exit_send(smb1req); + if (req == NULL) { + /* Not going async, profile here. */ + START_PROFILE(SMBexit); + reply_force_doserror(smb1req, ERRDOS, ERRnomem); + END_PROFILE(SMBexit); + return; + } - END_PROFILE(SMBexit); + /* We're async. This will complete later. */ + tevent_req_set_callback(req, reply_exit_done, smb1req); return; } -#if 0 struct reply_exit_state { struct tevent_queue *wait_queue; }; @@ -5966,7 +5978,6 @@ static void reply_exit_done(struct tevent_req *req) END_PROFILE(SMBexit); return; } -#endif struct reply_close_state { files_struct *fsp; -- 2.20.1 From 9f695881c4601bb9369e5bd20e2c881aad5c3b8b Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 12 Mar 2020 11:02:19 -0700 Subject: [PATCH 23/25] s3: smbd: Remove file_close_pid(). The old synchronous reply_exit() was the only user. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit 584933439c70af3d2fd047e62a3456c1c2eca45e) --- source3/smbd/files.c | 17 ----------------- source3/smbd/proto.h | 2 -- 2 files changed, 19 deletions(-) diff --git a/source3/smbd/files.c b/source3/smbd/files.c index b06511147ab..a982c0a5980 100644 --- a/source3/smbd/files.c +++ b/source3/smbd/files.c @@ -188,23 +188,6 @@ void file_close_conn(connection_struct *conn) } } -/**************************************************************************** - Close all open files for a pid and a vuid. -****************************************************************************/ - -void file_close_pid(struct smbd_server_connection *sconn, uint16_t smbpid, - uint64_t vuid) -{ - files_struct *fsp, *next; - - for (fsp=sconn->files;fsp;fsp=next) { - next = fsp->next; - if ((fsp->file_pid == smbpid) && (fsp->vuid == vuid)) { - close_file(NULL, fsp, SHUTDOWN_CLOSE); - } - } -} - /**************************************************************************** Initialise file structures. ****************************************************************************/ diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h index 96d574023a5..911fe6dad3c 100644 --- a/source3/smbd/proto.h +++ b/source3/smbd/proto.h @@ -379,8 +379,6 @@ void fsp_set_gen_id(files_struct *fsp); NTSTATUS file_new(struct smb_request *req, connection_struct *conn, files_struct **result); void file_close_conn(connection_struct *conn); -void file_close_pid(struct smbd_server_connection *sconn, uint16_t smbpid, - uint64_t vuid); bool file_init_global(void); bool file_init(struct smbd_server_connection *sconn); void file_close_user(struct smbd_server_connection *sconn, uint64_t vuid); -- 2.20.1 From 8b7bc0e44885cc2512402d475938c08a343d4f3a Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Mon, 2 Mar 2020 13:11:06 -0800 Subject: [PATCH 24/25] smbd: enforce AIO requests draining Assert we have no aio on a close. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Ralph Boehme Reviewed-by: Jeremy Allison Combined squash of commits: (cherry picked from commit 410e7599bd2ae9b35429f60a529bb7c4aa88df25) (cherry picked from commit acb0b01761330864a23932f643f7ad4e3d374634) (cherry picked from commit f94cd10a211e2eae966ba4bd26921556bbe513fc) (cherry picked from commit 0ae4f368c6c8d2c8c7aa34069007a984055df0da) (cherry picked from commit 86dd5a080969e14ab0d131d8cb1054ec624a41ba) --- source3/smbd/close.c | 80 ++++++++++---------------------------------- 1 file changed, 17 insertions(+), 63 deletions(-) diff --git a/source3/smbd/close.c b/source3/smbd/close.c index f45371e656c..d5af62a277c 100644 --- a/source3/smbd/close.c +++ b/source3/smbd/close.c @@ -31,6 +31,7 @@ #include "auth.h" #include "messages.h" #include "../librpc/gen_ndr/open_files.h" +#include "lib/util/tevent_ntstatus.h" /**************************************************************************** Run a file if it is a magic script. @@ -635,6 +636,20 @@ static NTSTATUS ntstatus_keeperror(NTSTATUS s1, NTSTATUS s2) return s2; } +static void assert_no_pending_aio(struct files_struct *fsp, + enum file_close_type close_type) +{ + unsigned num_requests = fsp->num_aio_requests; + + if (num_requests == 0) { + return; + } + + DBG_ERR("fsp->num_aio_requests=%u\n", num_requests); + smb_panic("can not close with outstanding aio requests"); + return; +} + /**************************************************************************** Close a file. @@ -651,45 +666,7 @@ static NTSTATUS close_normal_file(struct smb_request *req, files_struct *fsp, connection_struct *conn = fsp->conn; bool is_durable = false; - if (fsp->num_aio_requests != 0) { - - if (close_type != SHUTDOWN_CLOSE) { - /* - * reply_close and the smb2 close must have - * taken care of this. No other callers of - * close_file should ever have created async - * I/O. - * - * We need to panic here because if we close() - * the fd while we have outstanding async I/O - * requests, in the worst case we could end up - * writing to the wrong file. - */ - DEBUG(0, ("fsp->num_aio_requests=%u\n", - fsp->num_aio_requests)); - smb_panic("can not close with outstanding aio " - "requests"); - } - - /* - * For shutdown close, just drop the async requests - * including a potential close request pending for - * this fsp. Drop the close request first, the - * destructor for the aio_requests would execute it. - */ - TALLOC_FREE(fsp->deferred_close); - - while (fsp->num_aio_requests != 0) { - /* - * The destructor of the req will remove - * itself from the fsp. - * Don't use TALLOC_FREE here, this will overwrite - * what the destructor just wrote into - * aio_requests[0]. - */ - talloc_free(fsp->aio_requests[0]); - } - } + assert_no_pending_aio(fsp, close_type); while (talloc_array_length(fsp->blocked_smb1_lock_reqs) != 0) { smbd_smb1_brl_finish_by_req( @@ -1134,30 +1111,7 @@ static NTSTATUS close_directory(struct smb_request *req, files_struct *fsp, notify_status = NT_STATUS_OK; } - if (fsp->num_aio_requests != 0) { - if (close_type != SHUTDOWN_CLOSE) { - /* - * We panic here because if we close() the fd while we - * have outstanding async I/O requests, an async IO - * request might use the fd. For directories the fd is - * read-only, so this is not as bad as with files, but - * still, better safe then sorry. - */ - DBG_ERR("fsp->num_aio_requests=%u\n", - fsp->num_aio_requests); - smb_panic("close with outstanding aio requests"); - return NT_STATUS_INTERNAL_ERROR; - } - - while (fsp->num_aio_requests != 0) { - /* - * The destructor of the req will remove itself from the - * fsp. Don't use TALLOC_FREE here, this will overwrite - * what the destructor just wrote into aio_requests[0]. - */ - talloc_free(fsp->aio_requests[0]); - } - } + assert_no_pending_aio(fsp, close_type); /* * NT can set delete_on_close of the last open -- 2.20.1 From 7d2e8b46a2d155e40dc76d30e75e8824fececf6e Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 3 Mar 2020 13:31:18 -0800 Subject: [PATCH 25/25] s3: tests: Add samba3.blackbox.force-close-share Checks server stays up whilst writing to a force closed share. Uses existing aio_delay_inject share to delay writes while we force close the share. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison Reviewed-by: Volker Lendecke Autobuild-User(master): Jeremy Allison Autobuild-Date(master): Sun Mar 8 19:34:14 UTC 2020 on sn-devel-184 (cherry picked from commit bb22be08b077b7d5911ccdeb1012f4dea85647e5) (cherry picked from commit 566658d914176c41942e3c6aba404ae369aeb123) --- .../script/tests/test_force_close_share.sh | 100 ++++++++++++++++++ source3/selftest/tests.py | 9 ++ 2 files changed, 109 insertions(+) create mode 100755 source3/script/tests/test_force_close_share.sh diff --git a/source3/script/tests/test_force_close_share.sh b/source3/script/tests/test_force_close_share.sh new file mode 100755 index 00000000000..da78b5a752e --- /dev/null +++ b/source3/script/tests/test_force_close_share.sh @@ -0,0 +1,100 @@ +#!/bin/bash +# +# Test smbcontrol close-share command. +# +# Copyright (C) 2020 Volker Lendecke +# Copyright (C) 2020 Jeremy Allison +# +# Note this is designed to be run against +# the aio_delay_inject share which is preconfigured +# with 2 second delays on pread/pwrite. + +if [ $# -lt 5 ]; then + echo Usage: test_share_force_close.sh \ + SERVERCONFFILE SMBCLIENT SMBCONTROL IP aio_delay_inject_sharename +exit 1 +fi + +CONF=$1 +SMBCLIENT=$2 +SMBCONTROL=$3 +SERVER=$4 +SHARE=$5 + +incdir=$(dirname $0)/../../../testprogs/blackbox +. $incdir/subunit.sh + +failed=0 + +# Create the smbclient communication pipes. +rm -f smbclient-stdin smbclient-stdout smbclient-stderr +mkfifo smbclient-stdin smbclient-stdout smbclient-stderr + +# Create a large-ish testfile +rm testfile +head -c 20MB /dev/zero >testfile + +CLI_FORCE_INTERACTIVE=1; export CLI_FORCE_INTERACTIVE + +${SMBCLIENT} //${SERVER}/${SHARE} ${CONF} -U${USER}%${PASSWORD} \ + < smbclient-stdin > smbclient-stdout 2>smbclient-stderr & +CLIENT_PID=$! + +sleep 1 + +exec 100>smbclient-stdin 101&100 +echo "put testfile" >&100 + +sleep 1 + +# Close the aio_delay_inject share whilst we have outstanding writes. + +testit "smbcontrol" ${SMBCONTROL} ${CONF} smbd close-share ${SHARE} || + failed=$(expr $failed + 1) + +sleep 1 + +# If we get one or more NT_STATUS_NETWORK_NAME_DELETED +# or NT_STATUS_INVALID_HANDLE on stderr from the writes we +# know the server stayed up and didn't crash when the +# close-share removed the share. +# +# BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 +# +COUNT=$(head -n 2 <&102 | + grep -e NT_STATUS_NETWORK_NAME_DELETED -e NT_STATUS_INVALID_HANDLE | + wc -l) + +testit "Verify close-share did cancel the file put" \ + test $COUNT -ge 1 || failed=$(expr $failed + 1) + +kill ${CLIENT_PID} + +# Rerun smbclient to remove the testfile on the server. +rm -f smbclient-stdin smbclient-stdout smbclient-stderr testfile +mkfifo smbclient-stdin smbclient-stdout + +${SMBCLIENT} //${SERVER}/${SHARE} ${CONF} -U${USER}%${PASSWORD} \ + < smbclient-stdin > smbclient-stdout & +CLIENT_PID=$! + +sleep 1 + +exec 100>smbclient-stdin 101&100 + +sleep 1 + +kill ${CLIENT_PID} + +rm -f smbclient-stdin smbclient-stdout testfile + +testok $0 $failed diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py index dddcb03230c..6bdbb6f97a4 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -802,6 +802,15 @@ plantestsuite("samba3.blackbox.close-denied-share", "simpleserver:local", '$SERVER_IP', "tmp"]) +plantestsuite("samba3.blackbox.force-close-share", "simpleserver:local", + [os.path.join(samba3srcdir, + "script/tests/test_force_close_share.sh"), + configuration, + os.path.join(bindir(), "smbclient"), + os.path.join(bindir(), "smbcontrol"), + '$SERVER_IP', + "aio_delay_inject"]) + plantestsuite("samba3.blackbox.open-eintr", "simpleserver:local", [os.path.join(samba3srcdir, "script/tests/test_open_eintr.sh"), -- 2.20.1