[PATCH] Fix bug #13058 - Kernel oplock can remain not broken if oplock holder closes the file and another process catches the lease.
Jeremy Allison
jra at samba.org
Wed Nov 8 20:48:38 UTC 2017
Hi Ralph,
As discussed on the phone, the fix is to revert
commit b35a296a27a0807c780f2a9e7af2f2e93feefaa8.
The proof is the test case added after. It shows
a race condition, which can be forced one way
or the other by adding (or not adding):
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 89a267b0634..a65e75370ad 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -3269,6 +3269,9 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
schedule_defer_open(lck, fsp->file_id, request_time,
req);
TALLOC_FREE(lck);
+ if (oplock_request == 0) {
+ sleep(5);
+ }
DEBUG(10, ("Sent oplock break request to kernel "
"oplock holder\n"));
return NT_STATUS_SHARING_VIOLATION;
into smbd to force it to process either the
create request causing the break first, or
the create request sent as a result of the
kernel oplock break.
Please review and push if happy.
I'm planning to follow up with the fix for
kernel oplock use by a non-smbd process, but
this needs more thought on how to add a test
case for it.
Cheers,
Jeremy.
-------------- next part --------------
From 9460cf54381aed2311c2b70d0973903f39447281 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
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 <jra at samba.org>
---
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 d507786a2e8a133333e4423b1e96215a14118f1e Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
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 <jra at samba.org>
---
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
More information about the samba-technical
mailing list