From 8150e6f2c52b223f2fd440d5fbc8ec7ff2d8e75b Mon Sep 17 00:00:00 2001 From: Jeremy Allison 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 --- 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 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 --- 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 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 --- 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 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 --- 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 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 --- 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 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 Reviewed-by: Jeremy Allison --- 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