Failure with compound requests and aio enabled
Christof Schmitt
cs at samba.org
Thu Sep 21 19:54:15 UTC 2017
On Wed, Sep 20, 2017 at 05:14:49PM -0700, Jeremy Allison wrote:
> On Wed, Sep 20, 2017 at 04:57:35PM -0700, Jeremy Allison via samba-technical wrote:
> > On Wed, Sep 20, 2017 at 04:47:18PM -0700, Christof Schmitt via samba-technical wrote:
> > > Mac clients sometimes send a combination of CREATE-WRITE-CLOSE requests
> > > as compound requests. This works in the default config, but fails on a
> > > share with 'aio read size' and 'aio write size' enabled.
> >
> > Yes, this is a known bug and Mac clients are BREAKING
> > THE SPEC here. They don't care 'cos it works to Windows
> > servers.
>
> The reason this is a spec break is that any writes
> in a compound are allowed to go async, and the mac
> client doesn't cope with our response here.
>
> As the mainly use this to write minshal-french
> symlinks which are 1k or less, you can get around
> this by setting 'aio write size = 2k'.
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?
Christof
-------------- next part --------------
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