Failure with compound requests and aio enabled
Jeremy Allison
jra at samba.org
Thu Sep 21 21:33:34 UTC 2017
On Thu, Sep 21, 2017 at 12:54:15PM -0700, Christof Schmitt wrote:
>
> Thank you for the explanation, i was not aware of this bug. The testcase
> was extracting the Linux kernel source code from a tar file on a Mac
> client. The problem indeed only occurs on symlinks. The Samba server is
> configured with 'aio read size' and 'aio write size' set to 1, as files
> can be potentially migrated to offline storage and read/write calls can
> block until the data is available.
>
> Trying to understand how this breaks the spec: The client sends the
> CREATE-WRITE-CLOSE as a compound request. Then the server decides
> whether any of those requests are processed asynchronously. If that is
> the case, then the server may send an error back to the client:
>
> https://msdn.microsoft.com/en-us/library/cc246764.aspx
> If the NextCommand field in the SMB2 header of the request is not
> equal to 0, the server MUST process the received request as a compounded
> series of requests. The server MAY<215> fail requests in a compound
> chain which require asynchronous processing.
>
> https://msdn.microsoft.com/en-us/library/cc246805.aspx#Appendix_A_215
> <215> Section 3.3.5.2.7: In Windows Vista and Windows Server 2008, when
> an operation in a compound request requires asynchronous processing,
> Windows-based servers fail them with STATUS_INTERNAL_ERROR except for
> the following two cases: when a create request in the compound request
> triggers an oplock break, or when the operation is last in the compound
> request.
>
> Is that the correct reference to the spec? So i would assume that the
> client cannot know beforehand whether the server might try to process a
> request asynchronously, so the client has to react to the INTERNAL_ERROR
> and retry without compound requests?
>
> Interestingly the product behaviour note <215> also implies that newer
> versions do not send an error in this case.
>
> As this is a bug with existing clients: Would it make sense to have a
> similar check for SMB2 requests to not go async when they are part of a
> compound chain? This would be similar to the req_is_in_chain() check for
> SMB1 requests. I implemented that in the attached patches; any comments
> on that?
Nice idea. Can you explain why you need the new bool in_compound_chain ?
Can't you use the existing compound_related bool ?
Jeremy.
> From 759d4b9d9f360ac21d26dbd80cd24867476eca59 Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Thu, 21 Sep 2017 12:04:36 -0700
> Subject: [PATCH 1/4] smbd: Add flag indicating whether SMB2 request was part
> of a compound chain
>
> Signed-off-by: Christof Schmitt <cs at samba.org>
> ---
> source3/smbd/globals.h | 2 ++
> source3/smbd/smb2_server.c | 7 +++++++
> 2 files changed, 9 insertions(+)
>
> diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h
> index ae5ecf4..9cc1dbc 100644
> --- a/source3/smbd/globals.h
> +++ b/source3/smbd/globals.h
> @@ -710,6 +710,8 @@ struct smbd_smb2_request {
> struct tevent_timer *async_te;
> bool compound_related;
>
> + bool in_compound_chain;
> +
> /*
> * Give the implementation of an SMB2 req a way to tell the SMB2 request
> * processing engine that the internal request is going async, while
> diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
> index d95631f..4df3571 100644
> --- a/source3/smbd/smb2_server.c
> +++ b/source3/smbd/smb2_server.c
> @@ -2259,6 +2259,7 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req)
> bool signing_required = false;
> bool encryption_desired = false;
> bool encryption_required = false;
> + size_t next_command_ofs;
>
> inhdr = SMBD_SMB2_IN_HDR_PTR(req);
>
> @@ -2445,6 +2446,12 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req)
>
> if (flags & SMB2_HDR_FLAG_CHAINED) {
> req->compound_related = true;
> + req->in_compound_chain = true;
> + }
> +
> + next_command_ofs = IVAL(inhdr, SMB2_HDR_NEXT_COMMAND);
> + if (next_command_ofs != 0) {
> + req->in_compound_chain = true;
> }
>
> if (call->need_session) {
> --
> 1.8.3.1
>
>
> From ee39a32781085fb901e1a1f8c67deb535512b266 Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Thu, 21 Sep 2017 12:08:01 -0700
> Subject: [PATCH 2/4] smbd/aio: Do not go async for SMB2 compound requests
>
> Signed-off-by: Christof Schmitt <cs at samba.org>
> ---
> source3/smbd/aio.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/source3/smbd/aio.c b/source3/smbd/aio.c
> index f455d04..dab7e35 100644
> --- a/source3/smbd/aio.c
> +++ b/source3/smbd/aio.c
> @@ -697,6 +697,11 @@ NTSTATUS schedule_smb2_aio_read(connection_struct *conn,
> return NT_STATUS_RETRY;
> }
>
> + if (smbreq->smb2req->in_compound_chain) {
> + /* Don't try to go async for requests in compound chain */
> + return NT_STATUS_RETRY;
> + }
> +
> /* Create the out buffer. */
> *preadbuf = data_blob_talloc(ctx, NULL, smb_maxcnt);
> if (preadbuf->data == NULL) {
> @@ -841,6 +846,11 @@ NTSTATUS schedule_aio_smb2_write(connection_struct *conn,
> return NT_STATUS_RETRY;
> }
>
> + if (smbreq->smb2req->in_compound_chain) {
> + /* Don't try to go async for requests in compound chain */
> + return NT_STATUS_RETRY;
> + }
> +
> if (smbreq->unread_bytes) {
> /* Can't do async with recvfile. */
> return NT_STATUS_RETRY;
> --
> 1.8.3.1
>
>
> From 2801efed3f5a90a1e8a7bfffe927a619cf06df48 Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Wed, 20 Sep 2017 16:07:50 -0700
> Subject: [PATCH 3/4] torture: Add testcase for compound CREATE-WRITE-CLOSE
> request
>
> Signed-off-by: Christof Schmitt <cs at samba.org>
> ---
> source4/torture/smb2/compound.c | 73 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 73 insertions(+)
>
> diff --git a/source4/torture/smb2/compound.c b/source4/torture/smb2/compound.c
> index e501870..c592308 100644
> --- a/source4/torture/smb2/compound.c
> +++ b/source4/torture/smb2/compound.c
> @@ -671,6 +671,77 @@ done:
> return ret;
> }
>
> +static bool test_compound_create_write_close(struct torture_context *tctx,
> + struct smb2_tree *tree)
> +{
> + struct smb2_handle handle = { .data = { UINT64_MAX, UINT64_MAX } };
> + struct smb2_create create;
> + struct smb2_write write;
> + struct smb2_close close;
> + const char *fname = "compound_create_write_close.dat";
> + struct smb2_request *req[3];
> + NTSTATUS status;
> + bool ret = false;
> +
> + smb2_util_unlink(tree, fname);
> +
> + ZERO_STRUCT(create);
> + create.in.security_flags = 0x00;
> + create.in.oplock_level = 0;
> + create.in.impersonation_level = NTCREATEX_IMPERSONATION_IMPERSONATION;
> + create.in.create_flags = 0x00000000;
> + create.in.reserved = 0x00000000;
> + create.in.desired_access = SEC_RIGHTS_FILE_ALL;
> + create.in.file_attributes = FILE_ATTRIBUTE_NORMAL;
> + create.in.share_access = NTCREATEX_SHARE_ACCESS_READ |
> + NTCREATEX_SHARE_ACCESS_WRITE |
> + NTCREATEX_SHARE_ACCESS_DELETE;
> + create.in.create_disposition = NTCREATEX_DISP_OPEN_IF;
> + create.in.create_options = NTCREATEX_OPTIONS_SEQUENTIAL_ONLY |
> + NTCREATEX_OPTIONS_ASYNC_ALERT |
> + NTCREATEX_OPTIONS_NON_DIRECTORY_FILE |
> + 0x00200000;
> + create.in.fname = fname;
> +
> + smb2_transport_compound_start(tree->session->transport, 3);
> +
> + req[0] = smb2_create_send(tree, &create);
> +
> + smb2_transport_compound_set_related(tree->session->transport, true);
> +
> + ZERO_STRUCT(write);
> + write.in.file.handle = handle;
> + write.in.offset = 0;
> + write.in.data = data_blob_talloc(tctx, NULL, 1024);
> +
> + req[1] = smb2_write_send(tree, &write);
> +
> + ZERO_STRUCT(close);
> + close.in.file.handle = handle;
> +
> + req[2] = smb2_close_send(tree, &close);
> +
> + status = smb2_create_recv(req[0], tree, &create);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "CREATE failed.");
> +
> + status = smb2_write_recv(req[1], &write);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "WRITE failed.");
> +
> + status = smb2_close_recv(req[2], &close);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "CLOSE failed.");
> +
> + status = smb2_util_unlink(tree, fname);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "File deletion failed.");
> +
> + ret = true;
> +done:
> + return ret;
> +}
> +
> static bool test_compound_unrelated1(struct torture_context *tctx,
> struct smb2_tree *tree)
> {
> @@ -1230,6 +1301,8 @@ struct torture_suite *torture_smb2_compound_init(TALLOC_CTX *ctx)
> torture_suite_add_1smb2_test(suite, "interim2", test_compound_interim2);
> torture_suite_add_1smb2_test(suite, "compound-break", test_compound_break);
> torture_suite_add_1smb2_test(suite, "compound-padding", test_compound_padding);
> + torture_suite_add_1smb2_test(suite, "create-write-close",
> + test_compound_create_write_close);
>
> suite->description = talloc_strdup(suite, "SMB2-COMPOUND tests");
>
> --
> 1.8.3.1
>
>
> From a3e7823009a620e3ceff0076643abd0cddd8c364 Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Wed, 20 Sep 2017 16:13:38 -0700
> Subject: [PATCH 4/4] selftest: Also run smbtorture smb2.compound with aio
> enabled
>
> Signed-off-by: Christof Schmitt <cs at samba.org>
> ---
> selftest/knownfail | 1 +
> source3/selftest/tests.py | 4 ++++
> 2 files changed, 5 insertions(+)
>
> diff --git a/selftest/knownfail b/selftest/knownfail
> index aa89dab..953b181 100644
> --- a/selftest/knownfail
> +++ b/selftest/knownfail
> @@ -172,6 +172,7 @@
> ^samba3.smb2.setinfo.setinfo
> ^samba3.smb2.session.*reauth5 # some special anonymous checks?
> ^samba3.smb2.compound.interim2 # wrong return code (STATUS_CANCELLED)
> +^samba3.smb2.compound.aio.interim2 # wrong return code (STATUS_CANCELLED)
> ^samba3.smb2.replay.channel-sequence
> ^samba3.smb2.replay.replay3
> ^samba3.smb2.replay.replay4
> diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
> index 139bba0..73479fc 100755
> --- a/source3/selftest/tests.py
> +++ b/source3/selftest/tests.py
> @@ -518,6 +518,10 @@ for t in tests:
> elif t == "rpc.samr.users.privileges":
> plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD --option=torture:nt4_dc=true')
> plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD')
> + elif t == "smb2.compound":
> + 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')
> else:
> plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
> plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD')
> --
> 1.8.3.1
>
More information about the samba-technical
mailing list