The Samba-Bugzilla – Attachment 8847 Details for
Bug 9722
Samba does not properly handle Oplock breaks in compound requests
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
git-am fix for master and 4.0.next.
bug-9722.patchset (text/plain), 16.42 KB, created by
Jeremy Allison
on 2013-05-02 22:17:18 UTC
(
hide
)
Description:
git-am fix for master and 4.0.next.
Filename:
MIME Type:
Creator:
Jeremy Allison
Created:
2013-05-02 22:17:18 UTC
Size:
16.42 KB
patch
obsolete
>From 8150e6f2c52b223f2fd440d5fbc8ec7ff2d8e75b Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@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@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@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@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@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@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@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@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@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@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@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@gmail.com> >Reviewed-by: Jeremy Allison <jra@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 >
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=8847">View the attachment on a separate page</a>.</b>
View Attachment As Raw
Actions:
View
Attachments on
bug 9722
:
8646
|
8647
|
8684
|
8844
|
8846
|
8847
|
8869
|
8873