Attachment 17664 Details for Bug 15172 – git-am fix for 4.16.next.

[patch] git-am fix for 4.16.next.

bug-15224-4.16 (text/plain), 16.20 KB, created by Jeremy Allison on 2022-11-17 19:25:02 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeremy Allison

Created: 2022-11-17 19:25:02 UTC

Size: 16.20 KB

patch

obsolete

>From 99729216fde8ee7b649b7cf9cb3745e511413218 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>
>(back-ported 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 72fa5efee3f..e5b0e74c61f 100755
>--- a/source3/selftest/tests.py
>+++ b/source3/selftest/tests.py
>@@ -1001,6 +1001,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 == "rpc.samba3.netlogon" or t == "rpc.samba3.sessionkey":
>         plansmbtorture4testsuite(t, "nt4_dc_smb1", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD --option=torture:wksname=samba3rpctest')
>         plansmbtorture4testsuite(t, "ad_dc_smb1", '//$SERVER/tmp -U$USERNAME%$PASSWORD --option=torture:wksname=samba3rpctest')
>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 f60477a6c00..0216cdb9ddd 100644
>--- a/source4/torture/smb2/smb2.c
>+++ b/source4/torture/smb2/smb2.c
>@@ -173,6 +173,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 a82db861a583e711701b1f97b7448250e2b3fa39 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 60a3c2a567ac964fa72b07c9ffd4bc4ac3abfb33 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 eef38f00a4e..efd2032ce1a 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 f4e16cb7da9..f0b4814628b 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 235dedf64b3cbfe38b059ed0c8d393669679fbb4 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
>

Flags:

slow: review+

Actions: View

Attachments on bug 15172: 17515 | 17526 | 17527 | 17528 | 17663 | 17664