From 0b69465b6a8c06d424d23505290eeb06f112f156 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 3 Nov 2017 21:47:01 +0000 Subject: [PATCH 1/2] Revert "s3/smbd: fix deferred open with streams and kernel oplocks" This reverts commit b35a296a27a0807c780f2a9e7af2f2e93feefaa8. This was the cause of BUG: https://bugzilla.samba.org/show_bug.cgi?id=13058 1. client of smbd-1 opens the file and sets the oplock. 2. client of smbd-2 tries to open the file. open() fails(EAGAIN) and open is deferred. 3. client of smbd-1 sends oplock break request to the client. 4. client of smbd-1 closes the file. 5. client of smbd-1 opens the file and sets the oplock. 6. client of smbd-2 calls defer_open_done(), sees that the file lease was not changed and does not reschedule open. and is no longer needed now vfs_streams_xattr.c no longer opens the base file internally. Signed-off-by: Jeremy Allison --- source3/smbd/open.c | 115 +++++----------------------------------------------- 1 file changed, 11 insertions(+), 104 deletions(-) diff --git a/source3/smbd/open.c b/source3/smbd/open.c index 7781a6f86a7..89a267b0634 100644 --- a/source3/smbd/open.c +++ b/source3/smbd/open.c @@ -1896,23 +1896,6 @@ static bool delay_for_oplock(files_struct *fsp, return delay; } -/** - * Return lease or oplock state from a share mode - **/ -static uint32_t get_lease_type_from_share_mode(const struct share_mode_data *d) -{ - uint32_t e_lease_type = 0; - uint32_t i; - - for (i=0; i < d->num_share_modes; i++) { - struct share_mode_entry *e = &d->share_modes[i]; - - e_lease_type |= get_lease_type(d, e); - } - - return e_lease_type; -} - static bool file_has_brlocks(files_struct *fsp) { struct byte_range_lock *br_lck; @@ -2325,11 +2308,6 @@ static struct deferred_open_record *deferred_open_record_create( struct defer_open_state { struct smbXsrv_connection *xconn; uint64_t mid; - struct file_id file_id; - struct timeval request_time; - struct timeval timeout; - bool kernel_oplock; - uint32_t lease_type; }; static void defer_open_done(struct tevent_req *req); @@ -2348,7 +2326,6 @@ static void defer_open(struct share_mode_lock *lck, struct timeval timeout, struct smb_request *req, bool delayed_for_oplocks, - bool kernel_oplock, struct file_id id) { struct deferred_open_record *open_rec = NULL; @@ -2360,12 +2337,11 @@ static void defer_open(struct share_mode_lock *lck, abs_timeout = timeval_sum(&request_time, &timeout); DBG_DEBUG("request time [%s] timeout [%s] mid [%" PRIu64 "] " - "delayed_for_oplocks [%s] kernel_oplock [%s] file_id [%s]\n", + "delayed_for_oplocks [%s] file_id [%s]\n", timeval_string(talloc_tos(), &request_time, false), timeval_string(talloc_tos(), &abs_timeout, false), req->mid, delayed_for_oplocks ? "yes" : "no", - kernel_oplock ? "yes" : "no", file_id_string_tos(&id)); open_rec = deferred_open_record_create(delayed_for_oplocks, @@ -2382,11 +2358,6 @@ static void defer_open(struct share_mode_lock *lck, } watch_state->xconn = req->xconn; watch_state->mid = req->mid; - watch_state->file_id = lck->data->id; - watch_state->request_time = request_time; - watch_state->timeout = timeout; - watch_state->kernel_oplock = kernel_oplock; - watch_state->lease_type = get_lease_type_from_share_mode(lck->data); DBG_DEBUG("defering mid %" PRIu64 "\n", req->mid); @@ -2416,12 +2387,8 @@ static void defer_open_done(struct tevent_req *req) { struct defer_open_state *state = tevent_req_callback_data( req, struct defer_open_state); - struct tevent_req *watch_req = NULL; - struct share_mode_lock *lck = NULL; - bool schedule_req = true; - struct timeval timeout; NTSTATUS status; - bool ok; + bool ret; status = dbwrap_watched_watch_recv(req, talloc_tos(), NULL, NULL, NULL); @@ -2433,72 +2400,13 @@ static void defer_open_done(struct tevent_req *req) * Even if it failed, retry anyway. TODO: We need a way to * tell a re-scheduled open about that error. */ - if (NT_STATUS_EQUAL(status, NT_STATUS_IO_TIMEOUT) && - state->kernel_oplock) - { - /* - * If we reschedule but the kernel oplock is still hold - * we would block in the second open as that will be a - * blocking open attempt. - */ - exit_server("Kernel oplock holder didn't " - "respond to break message"); - } - } - - if (state->kernel_oplock) { - lck = get_existing_share_mode_lock(talloc_tos(), state->file_id); - if (lck != NULL) { - uint32_t lease_type; - - lease_type = get_lease_type_from_share_mode(lck->data); - - if ((lease_type != 0) && - (lease_type == state->lease_type)) - { - DBG_DEBUG("Unchanged lease: %" PRIu32 "\n", - lease_type); - schedule_req = false; - } - } - } - - if (schedule_req) { - DBG_DEBUG("scheduling mid %" PRIu64 "\n", state->mid); - - ok = schedule_deferred_open_message_smb(state->xconn, - state->mid); - if (!ok) { - exit_server("schedule_deferred_open_message_smb failed"); - } - TALLOC_FREE(lck); - TALLOC_FREE(state); - return; - } - - DBG_DEBUG("Keep waiting for oplock release for [%s/%s%s] " - "mid: %" PRIu64 "\n", - lck->data->servicepath, - lck->data->base_name, - lck->data->stream_name ? lck->data->stream_name : "", - state->mid); - - watch_req = dbwrap_watched_watch_send(state, - state->xconn->ev_ctx, - lck->data->record, - (struct server_id){0}); - if (watch_req == NULL) { - exit_server("Could not watch share mode record"); } - tevent_req_set_callback(watch_req, defer_open_done, state); - timeout = timeval_sum(&state->request_time, &state->timeout); - ok = tevent_req_set_endtime(watch_req, state->xconn->ev_ctx, timeout); - if (!ok) { - exit_server("tevent_req_set_endtime failed"); - } + DEBUG(10, ("scheduling mid %llu\n", (unsigned long long)state->mid)); - TALLOC_FREE(lck); + ret = schedule_deferred_open_message_smb(state->xconn, state->mid); + SMB_ASSERT(ret); + TALLOC_FREE(state); } /** @@ -2649,8 +2557,7 @@ static NTSTATUS fcb_or_dos_open(struct smb_request *req, static void schedule_defer_open(struct share_mode_lock *lck, struct file_id id, struct timeval request_time, - struct smb_request *req, - bool kernel_oplock) + struct smb_request *req) { /* This is a relative time, added to the absolute request_time value to get the absolute timeout time. @@ -2674,7 +2581,7 @@ static void schedule_defer_open(struct share_mode_lock *lck, return; } - defer_open(lck, request_time, timeout, req, true, kernel_oplock, id); + defer_open(lck, request_time, timeout, req, true, id); } /**************************************************************************** @@ -3360,7 +3267,7 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn, first_open_attempt); if (delay) { schedule_defer_open(lck, fsp->file_id, request_time, - req, true); + req); TALLOC_FREE(lck); DEBUG(10, ("Sent oplock break request to kernel " "oplock holder\n")); @@ -3493,7 +3400,7 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn, first_open_attempt); if (delay) { schedule_defer_open(lck, fsp->file_id, - request_time, req, false); + request_time, req); TALLOC_FREE(lck); fd_close(fsp); return NT_STATUS_SHARING_VIOLATION; @@ -3597,7 +3504,7 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn, if (!request_timed_out(request_time, timeout)) { defer_open(lck, request_time, timeout, req, - false, false, id); + false, id); } } -- 2.15.0.448.gf294e3d99a-goog From 81cdfc901810323516cf91eb39b8d6e18d38a180 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 3 Nov 2017 12:02:17 -0700 Subject: [PATCH 2/2] s4: torture: kernel_oplocks. Create a regression test case for bug #13058. It implements the following test case: 1. client of smbd-1 opens the file and sets the oplock. 2. client of smbd-2 tries to open the file. open() fails(EAGAIN) and open is deferred. 3. client of smbd-1 sends oplock break request to the client. 4. client of smbd-1 closes the file. 5. client of smbd-1 opens the file and sets the oplock. 6. client of smbd-2 calls defer_open_done(), sees that the file lease was not changed and does not reschedule open. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13058 Signed-off-by: Jeremy Allison --- source4/torture/smb2/oplock.c | 117 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+) diff --git a/source4/torture/smb2/oplock.c b/source4/torture/smb2/oplock.c index e0db5ecb50d..3290ed42d8c 100644 --- a/source4/torture/smb2/oplock.c +++ b/source4/torture/smb2/oplock.c @@ -4674,6 +4674,122 @@ done: return ret; } +/** + * Recreate regression test from bug: + * + * https://bugzilla.samba.org/show_bug.cgi?id=13058 + * + * 1. smbd-1 opens the file and sets the oplock + * 2. smbd-2 tries to open the file. open() fails(EAGAIN) and open is deferred. + * 3. smbd-1 sends oplock break request to the client. + * 4. smbd-1 closes the file. + * 5. smbd-1 opens the file and sets the oplock. + * 6. smbd-2 calls defer_open_done(), and should re-break the oplock. + **/ + +static bool test_smb2_kernel_oplocks7(struct torture_context *tctx, + struct smb2_tree *tree, + struct smb2_tree *tree2) +{ + const char *fname = "test_kernel_oplock7.dat"; + NTSTATUS status; + bool ret = true; + struct smb2_create create; + struct smb2_handle h1 = {{0}}, h2 = {{0}}; + struct smb2_create create_2; + struct smb2_create io; + struct smb2_request *req; + + smb2_util_unlink(tree, fname); + status = torture_smb2_testfile(tree, fname, &h1); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "Error creating testfile\n"); + smb2_util_close(tree, h1); + ZERO_STRUCT(h1); + + /* Close the open file on break. */ + tree->session->transport->oplock.handler = torture_oplock_handler_close; + tree->session->transport->oplock.private_data = tree; + ZERO_STRUCT(break_info); + + /* 1 - open file with oplock */ + ZERO_STRUCT(create); + create.in.desired_access = SEC_RIGHTS_FILE_ALL; + create.in.file_attributes = FILE_ATTRIBUTE_NORMAL; + create.in.share_access = NTCREATEX_SHARE_ACCESS_MASK; + create.in.create_disposition = NTCREATEX_DISP_OPEN; + create.in.impersonation_level = SMB2_IMPERSONATION_ANONYMOUS; + create.in.fname = fname; + create.in.oplock_level = SMB2_OPLOCK_LEVEL_EXCLUSIVE; + + status = smb2_create(tree, tctx, &create); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "Error opening the file\n"); + CHECK_VAL(create.out.oplock_level, SMB2_OPLOCK_LEVEL_EXCLUSIVE); + + /* 2 - open file to break oplock */ + ZERO_STRUCT(create_2); + create_2.in.desired_access = SEC_RIGHTS_FILE_ALL; + create_2.in.file_attributes = FILE_ATTRIBUTE_NORMAL; + create_2.in.share_access = NTCREATEX_SHARE_ACCESS_MASK; + create_2.in.create_disposition = NTCREATEX_DISP_OPEN; + create_2.in.impersonation_level = SMB2_IMPERSONATION_ANONYMOUS; + create_2.in.fname = fname; + create_2.in.oplock_level = SMB2_OPLOCK_LEVEL_NONE; + + /* Open on tree2 - should cause a break on tree */ + req = smb2_create_send(tree2, &create_2); + torture_assert(tctx, req != NULL, "smb2_create_send"); + + /* The oplock break handler should close the file. */ + /* Steps 3 & 4. */ + torture_wait_for_oplock_break(tctx); + + tree->session->transport->oplock.handler = torture_oplock_handler; + + /* + * 5 - re-open on tree. NB. There is a race here + * depending on which smbd goes first. We either get + * an oplock level of SMB2_OPLOCK_LEVEL_EXCLUSIVE if + * the close and re-open on tree is processed first, or + * SMB2_OPLOCK_LEVEL_NONE if the pending create on + * tree2 is processed first. + */ + status = smb2_create(tree, tctx, &create); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "Error opening the file\n"); + + h1 = create.out.file.handle; + if (create.out.oplock_level != SMB2_OPLOCK_LEVEL_EXCLUSIVE && + create.out.oplock_level != SMB2_OPLOCK_LEVEL_NONE) { + torture_result(tctx, + TORTURE_FAIL, + "(%s): wrong value for oplock got 0x%x\n", + __location__, + (unsigned int)create.out.oplock_level); + ret = false; + goto done; + + } + + /* 6 - retrieve the second open. */ + status = smb2_create_recv(req, tctx, &io); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "Error opening the file\n"); + h2 = io.out.file.handle; + CHECK_VAL(io.out.oplock_level, SMB2_OPLOCK_LEVEL_NONE); + +done: + if (!smb2_util_handle_empty(h1)) { + smb2_util_close(tree, h1); + } + if (!smb2_util_handle_empty(h2)) { + smb2_util_close(tree2, h2); + } + smb2_util_unlink(tree, fname); + return ret; +} + struct torture_suite *torture_smb2_kernel_oplocks_init(TALLOC_CTX *ctx) { struct torture_suite *suite = @@ -4685,6 +4801,7 @@ struct torture_suite *torture_smb2_kernel_oplocks_init(TALLOC_CTX *ctx) torture_suite_add_1smb2_test(suite, "kernel_oplocks4", test_smb2_kernel_oplocks4); torture_suite_add_1smb2_test(suite, "kernel_oplocks5", test_smb2_kernel_oplocks5); torture_suite_add_2smb2_test(suite, "kernel_oplocks6", test_smb2_kernel_oplocks6); + torture_suite_add_2smb2_test(suite, "kernel_oplocks7", test_smb2_kernel_oplocks7); suite->description = talloc_strdup(suite, "SMB2-KERNEL-OPLOCK tests"); -- 2.15.0.448.gf294e3d99a-goog