The Samba-Bugzilla – Attachment 17663 Details for
Bug 15172
Compound SMB2 FLUSH+CLOSE requests from MacOSX are not handled correctly.
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
git-am fix for 4.17.next.
bug-15224-4.17 (text/plain), 16.11 KB, created by
Jeremy Allison
on 2022-11-17 19:24:05 UTC
(
hide
)
Description:
git-am fix for 4.17.next.
Filename:
MIME Type:
Creator:
Jeremy Allison
Created:
2022-11-17 19:24:05 UTC
Size:
16.11 KB
patch
obsolete
>From 688743168939ea06ac9f3e3a64e2192e3627c9c9 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Tue, 18 Oct 2022 16:22:33 -0700 >Subject: [PATCH 1/4] s4: torture: Add an async SMB2_OP_FLUSH + SMB2_OP_CLOSE > test to smb2.compound_async. > >Shows we fail sending an SMB2_OP_FLUSH + SMB2_OP_CLOSE >compound. Internally the flush goes async and >we free the req, then we process the close. >When the flush completes it tries to access >already freed data. > >Found using the Apple MacOSX client at SNIA SDC 2022. > >Add knownfail. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15172 > >Signed-off-by: Jeremy Allison <jra@samba.org> >Reviewed-by: Ralph Boehme <slow@samba.org> >(cherry picked from commit 17a110c1b58196eb8ecf3c76eb97e8508976c544) >--- > selftest/knownfail.d/compound_async | 1 + > source3/selftest/tests.py | 2 + > source4/torture/smb2/compound.c | 117 ++++++++++++++++++++++++++++ > source4/torture/smb2/smb2.c | 1 + > 4 files changed, 121 insertions(+) > create mode 100644 selftest/knownfail.d/compound_async > >diff --git a/selftest/knownfail.d/compound_async b/selftest/knownfail.d/compound_async >new file mode 100644 >index 00000000000..c18465b7204 >--- /dev/null >+++ b/selftest/knownfail.d/compound_async >@@ -0,0 +1 @@ >+^samba3.smb2.compound_async.flush_close\(fileserver\) >diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py >index 246d2b05633..050b23fe159 100755 >--- a/source3/selftest/tests.py >+++ b/source3/selftest/tests.py >@@ -1054,6 +1054,8 @@ for t in tests: > plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD') > plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/aio -U$USERNAME%$PASSWORD', 'aio') > plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD') >+ elif t == "smb2.compound_async": >+ plansmbtorture4testsuite(t, "fileserver", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD') > elif t == "smb2.ea": > plansmbtorture4testsuite(t, "fileserver", '//$SERVER/ea_acl_xattr --option=torture:acl_xattr_name=hackme -U$USERNAME%$PASSWORD') > elif t == "rpc.samba3.netlogon" or t == "rpc.samba3.sessionkey": >diff --git a/source4/torture/smb2/compound.c b/source4/torture/smb2/compound.c >index cf19361130f..e78d78e3a98 100644 >--- a/source4/torture/smb2/compound.c >+++ b/source4/torture/smb2/compound.c >@@ -2057,6 +2057,110 @@ done: > return ret; > } > >+static bool test_compound_async_flush_close(struct torture_context *tctx, >+ struct smb2_tree *tree) >+{ >+ struct smb2_handle fhandle = { .data = { 0, 0 } }; >+ struct smb2_handle relhandle = { .data = { UINT64_MAX, UINT64_MAX } }; >+ struct smb2_close cl; >+ struct smb2_flush fl; >+ const char *fname = "compound_async_flush_close"; >+ struct smb2_request *req[2]; >+ NTSTATUS status; >+ bool ret = false; >+ >+ /* Start clean. */ >+ smb2_util_unlink(tree, fname); >+ >+ /* Create a file. */ >+ status = torture_smb2_testfile_access(tree, >+ fname, >+ &fhandle, >+ SEC_RIGHTS_FILE_ALL); >+ CHECK_STATUS(status, NT_STATUS_OK); >+ >+ /* Now do a compound flush + close handle. */ >+ smb2_transport_compound_start(tree->session->transport, 2); >+ >+ ZERO_STRUCT(fl); >+ fl.in.file.handle = fhandle; >+ >+ req[0] = smb2_flush_send(tree, &fl); >+ torture_assert_not_null_goto(tctx, req[0], ret, done, >+ "smb2_flush_send failed\n"); >+ >+ smb2_transport_compound_set_related(tree->session->transport, true); >+ >+ ZERO_STRUCT(cl); >+ cl.in.file.handle = relhandle; >+ req[1] = smb2_close_send(tree, &cl); >+ torture_assert_not_null_goto(tctx, req[1], ret, done, >+ "smb2_close_send failed\n"); >+ >+ status = smb2_flush_recv(req[0], &fl); >+ /* >+ * On Windows, this flush will usually >+ * succeed as we have nothing to flush, >+ * so allow NT_STATUS_OK. Once bug #15172 >+ * is fixed Samba will do the flush synchronously >+ * so allow NT_STATUS_OK. >+ */ >+ if (!NT_STATUS_IS_OK(status)) { >+ /* >+ * If we didn't get NT_STATUS_OK, we *must* >+ * get NT_STATUS_INTERNAL_ERROR if the flush >+ * goes async. >+ * >+ * For pre-bugfix #15172 Samba, the flush goes async and >+ * we should get NT_STATUS_INTERNAL_ERROR. >+ */ >+ torture_assert_ntstatus_equal_goto(tctx, >+ status, >+ NT_STATUS_INTERNAL_ERROR, >+ ret, >+ done, >+ "smb2_flush_recv didn't return " >+ "NT_STATUS_INTERNAL_ERROR.\n"); >+ } >+ status = smb2_close_recv(req[1], &cl); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_close_recv failed."); >+ >+ ZERO_STRUCT(fhandle); >+ >+ /* >+ * Do several more operations on the tree, spaced >+ * out by 1 sec sleeps to make sure the server didn't >+ * crash on the close. The sleeps are required to >+ * make test test for a crash reliable, as we ensure >+ * the pthread fsync internally finishes and accesses >+ * freed memory. Without them the test occassionally >+ * passes as we disconnect before the pthread fsync >+ * finishes. >+ */ >+ status = smb2_util_unlink(tree, fname); >+ CHECK_STATUS(status, NT_STATUS_OK); >+ >+ sleep(1); >+ status = smb2_util_unlink(tree, fname); >+ CHECK_STATUS(status, NT_STATUS_OBJECT_NAME_NOT_FOUND); >+ >+ sleep(1); >+ status = smb2_util_unlink(tree, fname); >+ CHECK_STATUS(status, NT_STATUS_OBJECT_NAME_NOT_FOUND); >+ >+ ret = true; >+ >+ done: >+ >+ if (fhandle.data[0] != 0) { >+ smb2_util_close(tree, fhandle); >+ } >+ >+ smb2_util_unlink(tree, fname); >+ return ret; >+} >+ > struct torture_suite *torture_smb2_compound_init(TALLOC_CTX *ctx) > { > struct torture_suite *suite = torture_suite_create(ctx, "compound"); >@@ -2107,3 +2211,16 @@ struct torture_suite *torture_smb2_compound_find_init(TALLOC_CTX *ctx) > > return suite; > } >+ >+struct torture_suite *torture_smb2_compound_async_init(TALLOC_CTX *ctx) >+{ >+ struct torture_suite *suite = torture_suite_create(ctx, >+ "compound_async"); >+ >+ torture_suite_add_1smb2_test(suite, "flush_close", >+ test_compound_async_flush_close); >+ >+ suite->description = talloc_strdup(suite, "SMB2-COMPOUND-ASYNC tests"); >+ >+ return suite; >+} >diff --git a/source4/torture/smb2/smb2.c b/source4/torture/smb2/smb2.c >index 3a519e34c95..c717db50b70 100644 >--- a/source4/torture/smb2/smb2.c >+++ b/source4/torture/smb2/smb2.c >@@ -174,6 +174,7 @@ NTSTATUS torture_smb2_init(TALLOC_CTX *ctx) > torture_suite_add_suite(suite, torture_smb2_lease_init(suite)); > torture_suite_add_suite(suite, torture_smb2_compound_init(suite)); > torture_suite_add_suite(suite, torture_smb2_compound_find_init(suite)); >+ torture_suite_add_suite(suite, torture_smb2_compound_async_init(suite)); > torture_suite_add_suite(suite, torture_smb2_oplocks_init(suite)); > torture_suite_add_suite(suite, torture_smb2_kernel_oplocks_init(suite)); > torture_suite_add_suite(suite, torture_smb2_streams_init(suite)); >-- >2.34.1 > > >From 98aa77663477fc2e7681b96317226db73efd1183 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Thu, 20 Oct 2022 14:22:25 -0700 >Subject: [PATCH 2/4] s4: torture: Add an async SMB2_OP_FLUSH + SMB2_OP_FLUSH > test to smb2.compound_async. > >Shows we fail sending an SMB2_OP_FLUSH + SMB2_OP_FLUSH >compound if we immediately close the file afterward. > >Internally the flushes go async and we free the req, then >we process the close. When the flushes complete they try to access >already freed data. > >Extra test which will allow me to test when the final >component (flush) of the compound goes async and returns >NT_STATUS_PENDING. > >Add knownfail. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15172 > >Signed-off-by: Jeremy Allison <jra@samba.org> >Reviewed-by: Ralph Boehme <slow@samba.org> >(cherry picked from commit 6f149dfd9d8d2619a9e18975ebcf5e69df2b7766) >--- > selftest/knownfail.d/compound_async | 1 + > source4/torture/smb2/compound.c | 115 ++++++++++++++++++++++++++++ > 2 files changed, 116 insertions(+) > >diff --git a/selftest/knownfail.d/compound_async b/selftest/knownfail.d/compound_async >index c18465b7204..e1be97649f3 100644 >--- a/selftest/knownfail.d/compound_async >+++ b/selftest/knownfail.d/compound_async >@@ -1 +1,2 @@ > ^samba3.smb2.compound_async.flush_close\(fileserver\) >+^samba3.smb2.compound_async.flush_flush\(fileserver\) >diff --git a/source4/torture/smb2/compound.c b/source4/torture/smb2/compound.c >index e78d78e3a98..47a550f0873 100644 >--- a/source4/torture/smb2/compound.c >+++ b/source4/torture/smb2/compound.c >@@ -2161,6 +2161,119 @@ static bool test_compound_async_flush_close(struct torture_context *tctx, > return ret; > } > >+static bool test_compound_async_flush_flush(struct torture_context *tctx, >+ struct smb2_tree *tree) >+{ >+ struct smb2_handle fhandle = { .data = { 0, 0 } }; >+ struct smb2_handle relhandle = { .data = { UINT64_MAX, UINT64_MAX } }; >+ struct smb2_flush fl1; >+ struct smb2_flush fl2; >+ const char *fname = "compound_async_flush_flush"; >+ struct smb2_request *req[2]; >+ NTSTATUS status; >+ bool ret = false; >+ >+ /* Start clean. */ >+ smb2_util_unlink(tree, fname); >+ >+ /* Create a file. */ >+ status = torture_smb2_testfile_access(tree, >+ fname, >+ &fhandle, >+ SEC_RIGHTS_FILE_ALL); >+ CHECK_STATUS(status, NT_STATUS_OK); >+ >+ /* Now do a compound flush + flush handle. */ >+ smb2_transport_compound_start(tree->session->transport, 2); >+ >+ ZERO_STRUCT(fl1); >+ fl1.in.file.handle = fhandle; >+ >+ req[0] = smb2_flush_send(tree, &fl1); >+ torture_assert_not_null_goto(tctx, req[0], ret, done, >+ "smb2_flush_send (1) failed\n"); >+ >+ smb2_transport_compound_set_related(tree->session->transport, true); >+ >+ ZERO_STRUCT(fl2); >+ fl2.in.file.handle = relhandle; >+ >+ req[1] = smb2_flush_send(tree, &fl2); >+ torture_assert_not_null_goto(tctx, req[1], ret, done, >+ "smb2_flush_send (2) failed\n"); >+ >+ status = smb2_flush_recv(req[0], &fl1); >+ /* >+ * On Windows, this flush will usually >+ * succeed as we have nothing to flush, >+ * so allow NT_STATUS_OK. Once bug #15172 >+ * is fixed Samba will do the flush synchronously >+ * so allow NT_STATUS_OK. >+ */ >+ if (!NT_STATUS_IS_OK(status)) { >+ /* >+ * If we didn't get NT_STATUS_OK, we *must* >+ * get NT_STATUS_INTERNAL_ERROR if the flush >+ * goes async. >+ * >+ * For pre-bugfix #15172 Samba, the flush goes async and >+ * we should get NT_STATUS_INTERNAL_ERROR. >+ */ >+ torture_assert_ntstatus_equal_goto(tctx, >+ status, >+ NT_STATUS_INTERNAL_ERROR, >+ ret, >+ done, >+ "smb2_flush_recv (1) didn't return " >+ "NT_STATUS_INTERNAL_ERROR.\n"); >+ } >+ >+ /* >+ * If the flush is the last entry in a compound, >+ * it should always succeed even if it goes async. >+ */ >+ status = smb2_flush_recv(req[1], &fl2); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_flush_recv (2) failed."); >+ >+ status = smb2_util_close(tree, fhandle); >+ torture_assert_ntstatus_ok_goto(tctx, status, ret, done, >+ "smb2_util_close failed."); >+ ZERO_STRUCT(fhandle); >+ >+ /* >+ * Do several more operations on the tree, spaced >+ * out by 1 sec sleeps to make sure the server didn't >+ * crash on the close. The sleeps are required to >+ * make test test for a crash reliable, as we ensure >+ * the pthread fsync internally finishes and accesses >+ * freed memory. Without them the test occassionally >+ * passes as we disconnect before the pthread fsync >+ * finishes. >+ */ >+ status = smb2_util_unlink(tree, fname); >+ CHECK_STATUS(status, NT_STATUS_OK); >+ >+ sleep(1); >+ status = smb2_util_unlink(tree, fname); >+ CHECK_STATUS(status, NT_STATUS_OBJECT_NAME_NOT_FOUND); >+ >+ sleep(1); >+ status = smb2_util_unlink(tree, fname); >+ CHECK_STATUS(status, NT_STATUS_OBJECT_NAME_NOT_FOUND); >+ >+ ret = true; >+ >+ done: >+ >+ if (fhandle.data[0] != 0) { >+ smb2_util_close(tree, fhandle); >+ } >+ >+ smb2_util_unlink(tree, fname); >+ return ret; >+} >+ > struct torture_suite *torture_smb2_compound_init(TALLOC_CTX *ctx) > { > struct torture_suite *suite = torture_suite_create(ctx, "compound"); >@@ -2219,6 +2332,8 @@ struct torture_suite *torture_smb2_compound_async_init(TALLOC_CTX *ctx) > > torture_suite_add_1smb2_test(suite, "flush_close", > test_compound_async_flush_close); >+ torture_suite_add_1smb2_test(suite, "flush_flush", >+ test_compound_async_flush_flush); > > suite->description = talloc_strdup(suite, "SMB2-COMPOUND-ASYNC tests"); > >-- >2.34.1 > > >From efc02161e847e0fa577886b2ebc4908ac1e32fde Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Thu, 20 Oct 2022 15:08:14 -0700 >Subject: [PATCH 3/4] s3: smbd: Add utility function > smbd_smb2_is_last_in_compound(). > >Not yet used. Returns true if we're processing the last SMB2 request in a >compound. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15172 > >Signed-off-by: Jeremy Allison <jra@samba.org> >Reviewed-by: Ralph Boehme <slow@samba.org> >(cherry picked from commit e668c3a82cd566b405c976d45659dd79786948de) >--- > source3/smbd/globals.h | 1 + > source3/smbd/smb2_server.c | 6 ++++++ > 2 files changed, 7 insertions(+) > >diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h >index efcf02f0d24..125ef64f070 100644 >--- a/source3/smbd/globals.h >+++ b/source3/smbd/globals.h >@@ -238,6 +238,7 @@ void smbd_server_disconnect_client_ex(struct smbXsrv_client *client, > const char *smb2_opcode_name(uint16_t opcode); > bool smbd_is_smb2_header(const uint8_t *inbuf, size_t size); > bool smbd_smb2_is_compound(const struct smbd_smb2_request *req); >+bool smbd_smb2_is_last_in_compound(const struct smbd_smb2_request *req); > > NTSTATUS smbd_add_connection(struct smbXsrv_client *client, int sock_fd, > NTTIME now, struct smbXsrv_connection **_xconn); >diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c >index 1cd5953f116..8e32a477947 100644 >--- a/source3/smbd/smb2_server.c >+++ b/source3/smbd/smb2_server.c >@@ -229,6 +229,12 @@ bool smbd_smb2_is_compound(const struct smbd_smb2_request *req) > return req->in.vector_count >= (2*SMBD_SMB2_NUM_IOV_PER_REQ); > } > >+bool smbd_smb2_is_last_in_compound(const struct smbd_smb2_request *req) >+{ >+ return (req->current_idx + SMBD_SMB2_NUM_IOV_PER_REQ == >+ req->in.vector_count); >+} >+ > static NTSTATUS smbd_initialize_smb2(struct smbXsrv_connection *xconn, > uint64_t expected_seq_low) > { >-- >2.34.1 > > >From 813986b3e01ef61d4f98de6fef4959cc51b8aecc Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Thu, 20 Oct 2022 15:19:05 -0700 >Subject: [PATCH 4/4] s3: smbd: Cause SMB2_OP_FLUSH to go synchronous in a > compound anywhere but the last operation in the list. >MIME-Version: 1.0 >Content-Type: text/plain; charset=UTF-8 >Content-Transfer-Encoding: 8bit > >Async read and write go synchronous in the same case, >so do the same here. > >Remove knownfail. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15172 > >Signed-off-by: Jeremy Allison <jra@samba.org> >Reviewed-by: Ralph Boehme <slow@samba.org> > >Autobuild-User(master): Ralph Böhme <slow@samba.org> >Autobuild-Date(master): Thu Nov 17 05:55:42 UTC 2022 on sn-devel-184 > >(cherry picked from commit 26adf3344337f4e8d5d2107e6ba42e5ea7656372) >--- > selftest/knownfail.d/compound_async | 2 -- > source3/smbd/smb2_flush.c | 14 ++++++++++++++ > 2 files changed, 14 insertions(+), 2 deletions(-) > delete mode 100644 selftest/knownfail.d/compound_async > >diff --git a/selftest/knownfail.d/compound_async b/selftest/knownfail.d/compound_async >deleted file mode 100644 >index e1be97649f3..00000000000 >--- a/selftest/knownfail.d/compound_async >+++ /dev/null >@@ -1,2 +0,0 @@ >-^samba3.smb2.compound_async.flush_close\(fileserver\) >-^samba3.smb2.compound_async.flush_flush\(fileserver\) >diff --git a/source3/smbd/smb2_flush.c b/source3/smbd/smb2_flush.c >index e73666f0afc..5c8c171e418 100644 >--- a/source3/smbd/smb2_flush.c >+++ b/source3/smbd/smb2_flush.c >@@ -126,6 +126,8 @@ static struct tevent_req *smbd_smb2_flush_send(TALLOC_CTX *mem_ctx, > struct tevent_req *subreq; > struct smbd_smb2_flush_state *state; > struct smb_request *smbreq; >+ bool is_compound = false; >+ bool is_last_in_compound = false; > > req = tevent_req_create(mem_ctx, &state, > struct smbd_smb2_flush_state); >@@ -195,6 +197,18 @@ static struct tevent_req *smbd_smb2_flush_send(TALLOC_CTX *mem_ctx, > > tevent_req_set_callback(subreq, smbd_smb2_flush_done, req); > >+ is_compound = smbd_smb2_is_compound(smb2req); >+ is_last_in_compound = smbd_smb2_is_last_in_compound(smb2req); >+ >+ if (is_compound && !is_last_in_compound) { >+ /* >+ * Can't go async if we're not the >+ * last request in a compound request. >+ * Cause this request to complete synchronously. >+ */ >+ smb2_request_set_async_internal(state->smb2req, true); >+ } >+ > /* Ensure any close request knows about this outstanding IO. */ > if (!aio_add_req_to_fsp(fsp, req)) { > tevent_req_nterror(req, NT_STATUS_NO_MEMORY); >-- >2.34.1 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Flags:
slow
:
review+
Actions:
View
Attachments on
bug 15172
:
17515
|
17526
|
17527
|
17528
| 17663 |
17664