[PATCH] Fix bug 9722 - Samba does not properly handle Oplock breaks in compound requests
Jeremy Allison
jra at samba.org
Fri May 3 12:19:04 MDT 2013
Here is a bugfix for master for Bug 9722, discovered by Richard.
It includes a torture test (written by Richard) that
reproduces the Windows client test case that discovered
the bug.
Passes all testing here (I've also tested with SMB3
encryption turned on :-).
Please review and push if you approve.
Cheers,
Jeremy.
-------------- next part --------------
From 8150e6f2c52b223f2fd440d5fbc8ec7ff2d8e75b Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 2 May 2013 11:12:47 -0700
Subject: [PATCH 1/6] Only do the 1 second delay for sharing violations for
SMB1, not SMB2.
Match Windows behavior.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/open.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 7d02e52..53f8b8e 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -2526,10 +2526,11 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
/*
* If we're returning a share violation, ensure we
- * cope with the braindead 1 second delay.
+ * cope with the braindead 1 second delay (SMB1 only).
*/
if (!(oplock_request & INTERNAL_OPEN_ONLY) &&
+ !conn->sconn->using_smb2 &&
lp_defer_sharing_violations()) {
struct timeval timeout;
struct deferred_open_record state;
--
1.8.1.2
From 53772fe4c810c08298b65ab4b3b6b054b4fbca9a Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 2 May 2013 12:34:54 -0700
Subject: [PATCH 2/6] Ensure we don't try and cancel anything that is in a
compound-related request.
Too hard to deal with splitting off the replies.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/smb2_server.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index 57e9c7b..9a55d6a 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -1599,6 +1599,14 @@ static NTSTATUS smbd_smb2_request_process_cancel(struct smbd_smb2_request *req)
uint64_t message_id;
uint64_t async_id;
+ if (cur->compound_related) {
+ /*
+ * Never cancel anything in a compound request.
+ * Way too hard to deal with the result.
+ */
+ continue;
+ }
+
outhdr = SMBD_SMB2_OUT_HDR_PTR(cur);
message_id = BVAL(outhdr, SMB2_HDR_MESSAGE_ID);
--
1.8.1.2
From 3e82ab8d18b1c0cb8c9de12c8fc4b2da99b56e04 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 2 May 2013 13:08:16 -0700
Subject: [PATCH 3/6] Move a variable into the area of code where it's used.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/smb2_server.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index 9a55d6a..1738b5e 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -1272,7 +1272,6 @@ NTSTATUS smbd_smb2_request_pending_queue(struct smbd_smb2_request *req,
uint32_t defer_time)
{
NTSTATUS status;
- int idx = req->current_idx;
struct timeval defer_endtime;
uint8_t *outhdr = NULL;
uint32_t flags;
@@ -1296,7 +1295,7 @@ NTSTATUS smbd_smb2_request_pending_queue(struct smbd_smb2_request *req,
return NT_STATUS_OK;
}
- if (req->in.vector_count > idx + SMBD_SMB2_NUM_IOV_PER_REQ) {
+ if (req->in.vector_count > req->current_idx + SMBD_SMB2_NUM_IOV_PER_REQ) {
/*
* We're trying to go async in a compound
* request chain. This is not allowed.
@@ -1318,6 +1317,7 @@ NTSTATUS smbd_smb2_request_pending_queue(struct smbd_smb2_request *req,
}
if (req->out.vector_count >= (2*SMBD_SMB2_NUM_IOV_PER_REQ)) {
+ int idx = req->current_idx;
/*
* This is a compound reply. We
* must do an interim response
--
1.8.1.2
From 31b07d47c7d0ca413c728829eb2787826f4d1029 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 2 May 2013 13:55:53 -0700
Subject: [PATCH 4/6] The core of the fix to allow opens to go async inside a
compound request.
This is only allowed for opens that cause an oplock break, otherwise it
is not allowed. See [MS-SMB2].pdf note <194> on Section 3.3.5.2.7.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/smb2_server.c | 96 +++++++++++++++++++++++++---------------------
1 file changed, 53 insertions(+), 43 deletions(-)
diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index 1738b5e..8f804a6 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -1298,16 +1298,26 @@ NTSTATUS smbd_smb2_request_pending_queue(struct smbd_smb2_request *req,
if (req->in.vector_count > req->current_idx + SMBD_SMB2_NUM_IOV_PER_REQ) {
/*
* We're trying to go async in a compound
- * request chain. This is not allowed.
- * Cancel the outstanding request.
+ * request chain.
+ * This is only allowed for opens that
+ * cause an oplock break, otherwise it
+ * is not allowed. See [MS-SMB2].pdf
+ * note <194> on Section 3.3.5.2.7.
*/
- bool ok = tevent_req_cancel(req->subreq);
- if (ok) {
- return NT_STATUS_OK;
+ const uint8_t *inhdr = SMBD_SMB2_IN_HDR_PTR(req);
+
+ if (SVAL(inhdr, SMB2_HDR_OPCODE) != SMB2_OP_CREATE) {
+ /*
+ * Cancel the outstanding request.
+ */
+ bool ok = tevent_req_cancel(req->subreq);
+ if (ok) {
+ return NT_STATUS_OK;
+ }
+ TALLOC_FREE(req->subreq);
+ return smbd_smb2_request_error(req,
+ NT_STATUS_INTERNAL_ERROR);
}
- TALLOC_FREE(req->subreq);
- return smbd_smb2_request_error(req,
- NT_STATUS_INTERNAL_ERROR);
}
if (DEBUGLEVEL >= 10) {
@@ -1316,51 +1326,51 @@ NTSTATUS smbd_smb2_request_pending_queue(struct smbd_smb2_request *req,
print_req_vectors(req);
}
- if (req->out.vector_count >= (2*SMBD_SMB2_NUM_IOV_PER_REQ)) {
- int idx = req->current_idx;
+ if (req->current_idx > 1) {
/*
- * This is a compound reply. We
- * must do an interim response
- * followed by the async response
- * to match W2K8R2.
+ * We're going async in a compound
+ * chain after the first request has
+ * already been processed. Send an
+ * interim response containing the
+ * set of replies already generated.
*/
+ int idx = req->current_idx;
+
status = smb2_send_async_interim_response(req);
if (!NT_STATUS_IS_OK(status)) {
return status;
}
data_blob_clear_free(&req->first_key);
- /*
- * We're splitting off the last SMB2
- * request in a compound set, and the
- * smb2_send_async_interim_response()
- * call above just sent all the replies
- * for the previous SMB2 requests in
- * this compound set. So we're no longer
- * in the "compound_related_in_progress"
- * state, and this is no longer a compound
- * request.
- */
- req->compound_related = false;
- req->sconn->smb2.compound_related_in_progress = false;
-
req->current_idx = 1;
- /* Re-arrange the in.vectors. */
- memmove(&req->in.vector[req->current_idx],
- &req->in.vector[idx],
- sizeof(req->in.vector[0])*SMBD_SMB2_NUM_IOV_PER_REQ);
- req->in.vector_count = req->current_idx + SMBD_SMB2_NUM_IOV_PER_REQ;
-
- /* Re-arrange the out.vectors. */
- memmove(&req->out.vector[req->current_idx],
- &req->out.vector[idx],
- sizeof(req->out.vector[0])*SMBD_SMB2_NUM_IOV_PER_REQ);
- req->out.vector_count = req->current_idx + SMBD_SMB2_NUM_IOV_PER_REQ;
-
- outhdr = SMBD_SMB2_OUT_HDR_PTR(req);
- flags = (IVAL(outhdr, SMB2_HDR_FLAGS) & ~SMB2_HDR_FLAG_CHAINED);
- SIVAL(outhdr, SMB2_HDR_FLAGS, flags);
+ /*
+ * Re-arrange the in.vectors to remove what
+ * we just sent.
+ */
+ memmove(&req->in.vector[1],
+ &req->in.vector[idx],
+ sizeof(req->in.vector[0])*(req->in.vector_count - idx));
+ req->in.vector_count = 1 + (req->in.vector_count - idx);
+
+ /* Re-arrange the out.vectors to match. */
+ memmove(&req->out.vector[1],
+ &req->out.vector[idx],
+ sizeof(req->out.vector[0])*(req->out.vector_count - idx));
+ req->out.vector_count = 1 + (req->out.vector_count - idx);
+
+ if (req->in.vector_count == 1 + SMBD_SMB2_NUM_IOV_PER_REQ) {
+ /*
+ * We only have one remaining request as
+ * we've processed everything else.
+ * This is no longer a compound request.
+ */
+ req->compound_related = false;
+ req->sconn->smb2.compound_related_in_progress = false;
+ outhdr = SMBD_SMB2_OUT_HDR_PTR(req);
+ flags = (IVAL(outhdr, SMB2_HDR_FLAGS) & ~SMB2_HDR_FLAG_CHAINED);
+ SIVAL(outhdr, SMB2_HDR_FLAGS, flags);
+ }
}
data_blob_clear_free(&req->last_key);
--
1.8.1.2
From a8632c2e6c7cc9d626f2a74e8e634509a071c746 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 2 May 2013 14:16:22 -0700
Subject: [PATCH 5/6] Remove the compound_related_in_progress state from the
smb2 global state.
And also remove the restriction that we can't read a new
request whilst we're in this state.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/globals.h | 1 -
source3/smbd/smb2_server.c | 11 -----------
2 files changed, 12 deletions(-)
diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h
index 46baeed..d618aea 100644
--- a/source3/smbd/globals.h
+++ b/source3/smbd/globals.h
@@ -794,7 +794,6 @@ struct smbd_server_connection {
uint32_t max_trans;
uint32_t max_read;
uint32_t max_write;
- bool compound_related_in_progress;
} smb2;
struct smbXsrv_connection *conn;
diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index 8f804a6..b031c6d 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -1366,7 +1366,6 @@ NTSTATUS smbd_smb2_request_pending_queue(struct smbd_smb2_request *req,
* This is no longer a compound request.
*/
req->compound_related = false;
- req->sconn->smb2.compound_related_in_progress = false;
outhdr = SMBD_SMB2_OUT_HDR_PTR(req);
flags = (IVAL(outhdr, SMB2_HDR_FLAGS) & ~SMB2_HDR_FLAG_CHAINED);
SIVAL(outhdr, SMB2_HDR_FLAGS, flags);
@@ -2042,7 +2041,6 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req)
if (flags & SMB2_HDR_FLAG_CHAINED) {
req->compound_related = true;
- req->sconn->smb2.compound_related_in_progress = true;
}
if (call->need_session) {
@@ -2406,7 +2404,6 @@ static NTSTATUS smbd_smb2_request_reply(struct smbd_smb2_request *req)
if (req->compound_related) {
req->compound_related = false;
- req->sconn->smb2.compound_related_in_progress = false;
}
smb2_setup_nbt_length(req->out.vector, req->out.vector_count);
@@ -3161,14 +3158,6 @@ static NTSTATUS smbd_smb2_request_next_incoming(struct smbd_server_connection *s
size_t cur_send_queue_len;
struct tevent_req *subreq;
- if (sconn->smb2.compound_related_in_progress) {
- /*
- * Can't read another until the related
- * compound is done.
- */
- return NT_STATUS_OK;
- }
-
if (tevent_queue_length(sconn->smb2.recv_queue) > 0) {
/*
* if there is already a smbd_smb2_request_read
--
1.8.1.2
From db81ca70fe2ae75e0ed0188e717d467139073fee Mon Sep 17 00:00:00 2001
From: Richard Sharpe <realrichardsharpe at gmail.com>
Date: Thu, 2 May 2013 14:36:05 -0700
Subject: [PATCH 6/6] Tests processing an oplock break within a compound SMB2
request.
Signed-off-by: Richard Sharpe <realrichardsharpe at gmail.com>
Reviewed-by: Jeremy Allison <jra at samba.org>
---
selftest/knownfail | 1 +
source4/torture/smb2/compound.c | 163 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 164 insertions(+)
diff --git a/selftest/knownfail b/selftest/knownfail
index 0c96eee..cb7630f 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -107,6 +107,7 @@
^samba4.smb2.rename.no_share_delete_no_delete_access\(.*\)$
^samba4.smb2.rename.msword
^samba4.smb2.compound.related3
+^samba4.smb2.compound.compound-break
^samba4.winbind.struct.*.show_sequence # Not yet working in winbind
^samba4.*base.delaywrite.*update of write time and SMBwrite truncate\(.*\)$
^samba4.*base.delaywrite.*update of write time and SMBwrite truncate expand\(.*\)$
diff --git a/source4/torture/smb2/compound.c b/source4/torture/smb2/compound.c
index 4a47e14..9b3cacc 100644
--- a/source4/torture/smb2/compound.c
+++ b/source4/torture/smb2/compound.c
@@ -34,6 +34,168 @@
goto done; \
}} while (0)
+static struct {
+ struct smb2_handle handle;
+ uint8_t level;
+ struct smb2_break br;
+ int count;
+ int failures;
+ NTSTATUS failure_status;
+} break_info;
+
+static void torture_oplock_break_callback(struct smb2_request *req)
+{
+ NTSTATUS status;
+ struct smb2_break br;
+
+ ZERO_STRUCT(br);
+ status = smb2_break_recv(req, &break_info.br);
+ if (!NT_STATUS_IS_OK(status)) {
+ break_info.failures++;
+ break_info.failure_status = status;
+ }
+
+ return;
+}
+
+/* A general oplock break notification handler. This should be used when a
+ * test expects to break from batch or exclusive to a lower level. */
+static bool torture_oplock_handler(struct smb2_transport *transport,
+ const struct smb2_handle *handle,
+ uint8_t level,
+ void *private_data)
+{
+ struct smb2_tree *tree = private_data;
+ const char *name;
+ struct smb2_request *req;
+ ZERO_STRUCT(break_info.br);
+
+ break_info.handle = *handle;
+ break_info.level = level;
+ break_info.count++;
+
+ switch (level) {
+ case SMB2_OPLOCK_LEVEL_II:
+ name = "level II";
+ break;
+ case SMB2_OPLOCK_LEVEL_NONE:
+ name = "none";
+ break;
+ default:
+ name = "unknown";
+ break_info.failures++;
+ }
+ printf("Acking to %s [0x%02X] in oplock handler\n", name, level);
+
+ break_info.br.in.file.handle = *handle;
+ break_info.br.in.oplock_level = level;
+ break_info.br.in.reserved = 0;
+ break_info.br.in.reserved2 = 0;
+
+ req = smb2_break_send(tree, &break_info.br);
+ req->async.fn = torture_oplock_break_callback;
+ req->async.private_data = NULL;
+ return true;
+}
+
+static bool test_compound_break(struct torture_context *tctx,
+ struct smb2_tree *tree)
+{
+ const char *fname1 = "some-file.pptx";
+ NTSTATUS status;
+ bool ret = true;
+ union smb_open io1;
+ struct smb2_create io2;
+ struct smb2_getinfo gf;
+ struct smb2_request *req[2];
+ struct smb2_handle h1;
+ struct smb2_handle h;
+
+ tree->session->transport->oplock.handler = torture_oplock_handler;
+ tree->session->transport->oplock.private_data = tree;
+
+ ZERO_STRUCT(break_info);
+
+ /*
+ base ntcreatex parms
+ */
+ ZERO_STRUCT(io1.smb2);
+ io1.generic.level = RAW_OPEN_SMB2;
+ io1.smb2.in.desired_access = (SEC_STD_SYNCHRONIZE|
+ SEC_STD_READ_CONTROL|
+ SEC_FILE_READ_ATTRIBUTE|
+ SEC_FILE_READ_EA|
+ SEC_FILE_READ_DATA);
+ io1.smb2.in.alloc_size = 0;
+ io1.smb2.in.file_attributes = FILE_ATTRIBUTE_NORMAL;
+ io1.smb2.in.share_access = NTCREATEX_SHARE_ACCESS_READ|
+ NTCREATEX_SHARE_ACCESS_WRITE|
+ NTCREATEX_SHARE_ACCESS_DELETE;
+ io1.smb2.in.create_disposition = NTCREATEX_DISP_OPEN_IF;
+ io1.smb2.in.create_options = 0;
+ io1.smb2.in.impersonation_level = SMB2_IMPERSONATION_ANONYMOUS;
+ io1.smb2.in.security_flags = 0;
+ io1.smb2.in.fname = fname1;
+
+ torture_comment(tctx, "TEST2: open a file with an batch "
+ "oplock (share mode: all)\n");
+ io1.smb2.in.oplock_level = SMB2_OPLOCK_LEVEL_BATCH;
+
+ status = smb2_create(tree, tctx, &(io1.smb2));
+ torture_assert_ntstatus_ok(tctx, status, "Error opening the file");
+
+ h1 = io1.smb2.out.file.handle;
+
+ torture_comment(tctx, "TEST2: Opening second time with compound\n");
+
+ ZERO_STRUCT(io2);
+
+ io2.in.desired_access = (SEC_STD_SYNCHRONIZE|
+ SEC_FILE_READ_ATTRIBUTE|
+ SEC_FILE_READ_EA);
+ io2.in.alloc_size = 0;
+ io2.in.file_attributes = FILE_ATTRIBUTE_NORMAL;
+ io2.in.share_access = NTCREATEX_SHARE_ACCESS_READ|
+ NTCREATEX_SHARE_ACCESS_WRITE|
+ NTCREATEX_SHARE_ACCESS_DELETE;
+ io2.in.create_disposition = NTCREATEX_DISP_OPEN;
+ io2.in.create_options = 0;
+ io2.in.impersonation_level = SMB2_IMPERSONATION_ANONYMOUS;
+ io2.in.security_flags = 0;
+ io2.in.fname = fname1;
+ io2.in.oplock_level = 0;
+
+ smb2_transport_compound_start(tree->session->transport, 2);
+
+ req[0] = smb2_create_send(tree, &io2);
+
+ smb2_transport_compound_set_related(tree->session->transport, true);
+
+ h.data[0] = UINT64_MAX;
+ h.data[1] = UINT64_MAX;
+
+ ZERO_STRUCT(gf);
+ gf.in.file.handle = h;
+ gf.in.info_type = SMB2_GETINFO_FILE;
+ gf.in.info_class = 0x16;
+ gf.in.output_buffer_length = 0x1000;
+ gf.in.input_buffer_length = 0;
+
+ req[1] = smb2_getinfo_send(tree, &gf);
+
+ status = smb2_create_recv(req[0], tree, &io2);
+ CHECK_STATUS(status, NT_STATUS_OK);
+
+ status = smb2_getinfo_recv(req[1], tree, &gf);
+ CHECK_STATUS(status, NT_STATUS_OK);
+
+done:
+
+ smb2_util_close(tree, h1);
+ smb2_util_unlink(tree, fname1);
+ return ret;
+}
+
static bool test_compound_related1(struct torture_context *tctx,
struct smb2_tree *tree)
{
@@ -717,6 +879,7 @@ struct torture_suite *torture_smb2_compound_init(void)
torture_suite_add_1smb2_test(suite, "invalid3", test_compound_invalid3);
torture_suite_add_1smb2_test(suite, "interim1", test_compound_interim1);
torture_suite_add_1smb2_test(suite, "interim2", test_compound_interim2);
+ torture_suite_add_1smb2_test(suite, "compound-break", test_compound_break);
suite->description = talloc_strdup(suite, "SMB2-COMPOUND tests");
--
1.8.1.2
More information about the samba-technical
mailing list