The Samba-Bugzilla – Attachment 10072 Details for
Bug 10684
SMB1 blocking locks can fail notification on unlock, causing client timeout.
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
git-am fix for master.
bug-10684-master.patch (text/plain), 17.16 KB, created by
Jeremy Allison
on 2014-07-03 05:11:35 UTC
(
hide
)
Description:
git-am fix for master.
Filename:
MIME Type:
Creator:
Jeremy Allison
Created:
2014-07-03 05:11:35 UTC
Size:
17.16 KB
patch
obsolete
>From 6d54cf93ea67ffc00876a33534f5411a52a6dda5 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Wed, 2 Jul 2014 17:25:22 -0700 >Subject: [PATCH 1/5] s3: smbd: Locking - convert to using utility macro used > elsewhere. > >Bug #10684 - SMB1 blocking locks can fail notification on unlock, causing client timeout. > >https://bugzilla.samba.org/show_bug.cgi?id=10684 > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source3/smbd/blocking.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > >diff --git a/source3/smbd/blocking.c b/source3/smbd/blocking.c >index 5d198fc..b744500 100644 >--- a/source3/smbd/blocking.c >+++ b/source3/smbd/blocking.c >@@ -500,8 +500,7 @@ static bool process_lockingX(struct blocking_lock_record *blr) > return True; > } > >- if (!NT_STATUS_EQUAL(status,NT_STATUS_LOCK_NOT_GRANTED) && >- !NT_STATUS_EQUAL(status,NT_STATUS_FILE_LOCK_CONFLICT)) { >+ if (!ERROR_WAS_LOCK_DENIED(status)) { > /* > * We have other than a "can't get lock" > * error. Free any locks we had and return an error. >-- >1.9.1 > > >From f20638c37b241f04d33b53cf2b005fdc86091207 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Wed, 2 Jul 2014 20:18:42 -0700 >Subject: [PATCH 2/5] s3: smbd: Locking - add and use utility function > lock_timed_out(). > >Bug #10684 - SMB1 blocking locks can fail notification on unlock, causing client timeout. > >https://bugzilla.samba.org/show_bug.cgi?id=10684 > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source3/smbd/blocking.c | 24 +++++++++++++++++++++--- > 1 file changed, 21 insertions(+), 3 deletions(-) > >diff --git a/source3/smbd/blocking.c b/source3/smbd/blocking.c >index b744500..b8d3f1d 100644 >--- a/source3/smbd/blocking.c >+++ b/source3/smbd/blocking.c >@@ -430,6 +430,25 @@ static void blocking_lock_reply_error(struct blocking_lock_record *blr, NTSTATUS > } > > /**************************************************************************** >+ Utility function that returns true if a lock timed out. >+*****************************************************************************/ >+ >+static bool lock_timed_out(const struct blocking_lock_record *blr) >+{ >+ struct timeval tv_curr; >+ >+ if (timeval_is_zero(&blr->expire_time)) { >+ return false; /* Never times out. */ >+ } >+ >+ tv_curr = timeval_current(); >+ if (timeval_compare(&blr->expire_time, &tv_curr) <= 0) { >+ return true; >+ } >+ return false; >+} >+ >+/**************************************************************************** > Attempt to finish off getting all pending blocking locks for a lockingX call. > Returns True if we want to be removed from the list. > *****************************************************************************/ >@@ -734,11 +753,10 @@ static void received_unlock_msg(struct messaging_context *msg, > > void process_blocking_lock_queue(struct smbd_server_connection *sconn) > { >- struct timeval tv_curr = timeval_current(); > struct blocking_lock_record *blr, *next = NULL; > > if (sconn->using_smb2) { >- process_blocking_lock_queue_smb2(sconn, tv_curr); >+ process_blocking_lock_queue_smb2(sconn, timeval_current()); > return; > } > >@@ -794,7 +812,7 @@ void process_blocking_lock_queue(struct smbd_server_connection *sconn) > * If the time has expired, return a lock error. > */ > >- if (!timeval_is_zero(&blr->expire_time) && timeval_compare(&blr->expire_time, &tv_curr) <= 0) { >+ if (lock_timed_out(blr)) { > struct byte_range_lock *br_lck = brl_get_locks( > talloc_tos(), blr->fsp); > >-- >1.9.1 > > >From 24e10d78d7aec1ad59a6caa3df16f373bb5157e8 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Wed, 2 Jul 2014 20:40:49 -0700 >Subject: [PATCH 3/5] s3: smbd: Locking - treat lock timeout the same as any > other error. > >Allows the special case in process_blocking_lock_queue() >that talks back to the client to be removed. > >Bug #10684 - SMB1 blocking locks can fail notification on unlock, causing client timeout. > >https://bugzilla.samba.org/show_bug.cgi?id=10684 > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source3/smbd/blocking.c | 91 +++++++++++++++++++++---------------------------- > 1 file changed, 38 insertions(+), 53 deletions(-) > >diff --git a/source3/smbd/blocking.c b/source3/smbd/blocking.c >index b8d3f1d..79a5cc4 100644 >--- a/source3/smbd/blocking.c >+++ b/source3/smbd/blocking.c >@@ -464,6 +464,7 @@ static bool process_lockingX(struct blocking_lock_record *blr) > bool large_file_format = (locktype & LOCKING_ANDX_LARGE_FILES); > uint8_t *data; > NTSTATUS status = NT_STATUS_OK; >+ bool lock_timeout = lock_timed_out(blr); > > data = discard_const_p(uint8_t, blr->req->buf) > + ((large_file_format ? 20 : 10)*num_ulocks); >@@ -530,6 +531,14 @@ static bool process_lockingX(struct blocking_lock_record *blr) > } > > /* >+ * Return an error to the client if we timed out. >+ */ >+ if (lock_timeout) { >+ blocking_lock_reply_error(blr,NT_STATUS_FILE_LOCK_CONFLICT); >+ return true; >+ } >+ >+ /* > * Still can't get all the locks - keep waiting. > */ > >@@ -550,6 +559,8 @@ static bool process_trans2(struct blocking_lock_record *blr) > { > char params[2]; > NTSTATUS status; >+ bool lock_timeout = lock_timed_out(blr); >+ > struct byte_range_lock *br_lck = do_lock( > blr->fsp->conn->sconn->msg_ctx, > blr->fsp, >@@ -566,6 +577,15 @@ static bool process_trans2(struct blocking_lock_record *blr) > > if (!NT_STATUS_IS_OK(status)) { > if (ERROR_WAS_LOCK_DENIED(status)) { >+ if (lock_timeout) { >+ /* >+ * Return an error if we timed out >+ * and return true to get dequeued. >+ */ >+ blocking_lock_reply_error(blr, >+ NT_STATUS_FILE_LOCK_CONFLICT); >+ return true; >+ } > /* Still can't get the lock, just keep waiting. */ > return False; > } >@@ -765,6 +785,7 @@ void process_blocking_lock_queue(struct smbd_server_connection *sconn) > */ > > for (blr = sconn->smb1.locks.blocking_lock_queue; blr; blr = next) { >+ struct byte_range_lock *br_lck = NULL; > > next = blr->next; > >@@ -784,65 +805,29 @@ void process_blocking_lock_queue(struct smbd_server_connection *sconn) > SVAL(blr->req->inbuf,smb_flg), > false); > >- if(blocking_lock_record_process(blr)) { >- struct byte_range_lock *br_lck = brl_get_locks( >- talloc_tos(), blr->fsp); >- >- DEBUG(10, ("BLR_process returned true: cancelling and " >- "removing lock. BLR = %p\n", blr)); >- >- if (br_lck) { >- brl_lock_cancel(br_lck, >- blr->smblctx, >- messaging_server_id(sconn->msg_ctx), >- blr->offset, >- blr->count, >- blr->lock_flav, >- blr); >- TALLOC_FREE(br_lck); >- } >- >- DLIST_REMOVE(sconn->smb1.locks.blocking_lock_queue, blr); >- TALLOC_FREE(blr); >+ if(!blocking_lock_record_process(blr)) { >+ DEBUG(10, ("still waiting for lock. BLR = %p\n", blr)); > continue; > } > >- /* >- * We couldn't get the locks for this record on the list. >- * If the time has expired, return a lock error. >- */ >- >- if (lock_timed_out(blr)) { >- struct byte_range_lock *br_lck = brl_get_locks( >- talloc_tos(), blr->fsp); >- >- DEBUG(10, ("Lock timed out! BLR = %p\n", blr)); >+ br_lck = brl_get_locks(talloc_tos(), blr->fsp); > >- /* >- * Lock expired - throw away all previously >- * obtained locks and return lock error. >- */ >- >- if (br_lck) { >- DEBUG(5,("process_blocking_lock_queue: " >- "pending lock for %s, file %s " >- "timed out.\n", fsp_fnum_dbg(blr->fsp), >- fsp_str_dbg(blr->fsp))); >- >- brl_lock_cancel(br_lck, >- blr->smblctx, >- messaging_server_id(sconn->msg_ctx), >- blr->offset, >- blr->count, >- blr->lock_flav, >- blr); >- TALLOC_FREE(br_lck); >- } >+ DEBUG(10, ("BLR_process returned true: cancelling and " >+ "removing lock. BLR = %p\n", blr)); > >- blocking_lock_reply_error(blr,NT_STATUS_FILE_LOCK_CONFLICT); >- DLIST_REMOVE(sconn->smb1.locks.blocking_lock_queue, blr); >- TALLOC_FREE(blr); >+ if (br_lck) { >+ brl_lock_cancel(br_lck, >+ blr->smblctx, >+ messaging_server_id(sconn->msg_ctx), >+ blr->offset, >+ blr->count, >+ blr->lock_flav, >+ blr); >+ TALLOC_FREE(br_lck); > } >+ >+ DLIST_REMOVE(sconn->smb1.locks.blocking_lock_queue, blr); >+ TALLOC_FREE(blr); > } > > recalc_brl_timeout(sconn); >-- >1.9.1 > > >From 50030f66149950692c833c1bed461c2aa0e54cab Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Wed, 2 Jul 2014 20:51:24 -0700 >Subject: [PATCH 4/5] s3: smbd: Locking - re-add pending lock records if we > fail to acquire a lock (and the lock hasn't timed out). > >Keep the blocking lock record and the pending lock records consistent >if we are dealing with multiple blocking lock requests in one SMB1 LockingX >request. > >Ensure we re-add the records under the record lock, to avoid race >conditions. > >Bug #10684 - SMB1 blocking locks can fail notification on unlock, causing client timeout. > >https://bugzilla.samba.org/show_bug.cgi?id=10684 > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source3/smbd/blocking.c | 97 ++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 80 insertions(+), 17 deletions(-) > >diff --git a/source3/smbd/blocking.c b/source3/smbd/blocking.c >index 79a5cc4..fdb90a1 100644 >--- a/source3/smbd/blocking.c >+++ b/source3/smbd/blocking.c >@@ -459,8 +459,6 @@ static bool process_lockingX(struct blocking_lock_record *blr) > files_struct *fsp = blr->fsp; > uint16 num_ulocks = SVAL(blr->req->vwv+6, 0); > uint16 num_locks = SVAL(blr->req->vwv+7, 0); >- uint64_t count = (uint64_t)0, offset = (uint64_t)0; >- uint64_t smblctx; > bool large_file_format = (locktype & LOCKING_ANDX_LARGE_FILES); > uint8_t *data; > NTSTATUS status = NT_STATUS_OK; >@@ -478,9 +476,14 @@ static bool process_lockingX(struct blocking_lock_record *blr) > struct byte_range_lock *br_lck = NULL; > bool err; > >- smblctx = get_lock_pid( data, blr->lock_num, large_file_format); >- count = get_lock_count( data, blr->lock_num, large_file_format); >- offset = get_lock_offset( data, blr->lock_num, large_file_format, &err); >+ /* >+ * Ensure the blr record gets updated with >+ * any lock we might end up blocked on. >+ */ >+ >+ blr->smblctx = get_lock_pid( data, blr->lock_num, large_file_format); >+ blr->count = get_lock_count( data, blr->lock_num, large_file_format); >+ blr->offset = get_lock_offset( data, blr->lock_num, large_file_format, &err); > > /* > * We know err cannot be set as if it was the lock >@@ -489,9 +492,9 @@ static bool process_lockingX(struct blocking_lock_record *blr) > errno = 0; > br_lck = do_lock(fsp->conn->sconn->msg_ctx, > fsp, >- smblctx, >- count, >- offset, >+ blr->smblctx, >+ blr->count, >+ blr->offset, > ((locktype & LOCKING_ANDX_SHARED_LOCK) ? > READ_LOCK : WRITE_LOCK), > WINDOWS_LOCK, >@@ -500,6 +503,34 @@ static bool process_lockingX(struct blocking_lock_record *blr) > &blr->blocking_smblctx, > blr); > >+ if (ERROR_WAS_LOCK_DENIED(status) && !lock_timeout) { >+ /* >+ * If we didn't timeout, but still need to wait, >+ * re-add the pending lock entry whilst holding >+ * the brlock db lock. >+ */ >+ NTSTATUS status1 = >+ brl_lock(blr->fsp->conn->sconn->msg_ctx, >+ br_lck, >+ blr->smblctx, >+ messaging_server_id( >+ blr->fsp->conn->sconn->msg_ctx), >+ blr->offset, >+ blr->count, >+ blr->lock_type == READ_LOCK ? >+ PENDING_READ_LOCK : >+ PENDING_WRITE_LOCK, >+ blr->lock_flav, >+ true, /* Blocking lock. */ >+ NULL, >+ blr); >+ >+ if (!NT_STATUS_IS_OK(status1)) { >+ DEBUG(0,("failed to add PENDING_LOCK " >+ "record.\n")); >+ } >+ } >+ > TALLOC_FREE(br_lck); > > if (NT_STATUS_IS_ERR(status)) { >@@ -573,6 +604,33 @@ static bool process_trans2(struct blocking_lock_record *blr) > &status, > &blr->blocking_smblctx, > blr); >+ if (ERROR_WAS_LOCK_DENIED(status) && !lock_timeout) { >+ /* >+ * If we didn't timeout, but still need to wait, >+ * re-add the pending lock entry whilst holding >+ * the brlock db lock. >+ */ >+ NTSTATUS status1 = >+ brl_lock(blr->fsp->conn->sconn->msg_ctx, >+ br_lck, >+ blr->smblctx, >+ messaging_server_id( >+ blr->fsp->conn->sconn->msg_ctx), >+ blr->offset, >+ blr->count, >+ blr->lock_type == READ_LOCK ? >+ PENDING_READ_LOCK : >+ PENDING_WRITE_LOCK, >+ blr->lock_flav, >+ true, /* Blocking lock. */ >+ NULL, >+ blr); >+ >+ if (!NT_STATUS_IS_OK(status1)) { >+ DEBUG(0,("failed to add PENDING_LOCK record.\n")); >+ } >+ } >+ > TALLOC_FREE(br_lck); > > if (!NT_STATUS_IS_OK(status)) { >@@ -805,16 +863,13 @@ void process_blocking_lock_queue(struct smbd_server_connection *sconn) > SVAL(blr->req->inbuf,smb_flg), > false); > >- if(!blocking_lock_record_process(blr)) { >- DEBUG(10, ("still waiting for lock. BLR = %p\n", blr)); >- continue; >- } >+ /* >+ * Remove the pending lock we're waiting on. >+ * If we need to keep waiting blocking_lock_record_process() >+ * will re-add it. >+ */ > > br_lck = brl_get_locks(talloc_tos(), blr->fsp); >- >- DEBUG(10, ("BLR_process returned true: cancelling and " >- "removing lock. BLR = %p\n", blr)); >- > if (br_lck) { > brl_lock_cancel(br_lck, > blr->smblctx, >@@ -823,8 +878,16 @@ void process_blocking_lock_queue(struct smbd_server_connection *sconn) > blr->count, > blr->lock_flav, > blr); >- TALLOC_FREE(br_lck); > } >+ TALLOC_FREE(br_lck); >+ >+ if(!blocking_lock_record_process(blr)) { >+ DEBUG(10, ("still waiting for lock. BLR = %p\n", blr)); >+ continue; >+ } >+ >+ DEBUG(10, ("BLR_process returned true: removing BLR = %p\n", >+ blr)); > > DLIST_REMOVE(sconn->smb1.locks.blocking_lock_queue, blr); > TALLOC_FREE(blr); >-- >1.9.1 > > >From a66f6a82f1d259175f016c43f253ec02f731770b Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Tue, 1 Jul 2014 12:05:07 -0700 >Subject: [PATCH 5/5] s4: smbtorture: Add multi-lock test. Regression test for > bug #10684. > >Bug #10684 - SMB1 blocking locks can fail notification on unlock, causing client timeout. > >https://bugzilla.samba.org/show_bug.cgi?id=10684 > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source4/torture/raw/lock.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 97 insertions(+) > >diff --git a/source4/torture/raw/lock.c b/source4/torture/raw/lock.c >index a31a4d0..8dc2434 100644 >--- a/source4/torture/raw/lock.c >+++ b/source4/torture/raw/lock.c >@@ -2262,6 +2262,102 @@ done: > smbcli_deltree(cli->tree, BASEDIR); > return ret; > } >+/* >+ test multi Locking&X operation >+*/ >+static bool test_multilock(struct torture_context *tctx, >+ struct smbcli_state *cli) >+{ >+ union smb_lock io; >+ struct smb_lock_entry lock[2]; >+ NTSTATUS status; >+ bool ret = true; >+ int fnum; >+ const char *fname = BASEDIR "\\multilock_test.txt"; >+ time_t t; >+ struct smbcli_request *req; >+ struct smbcli_session_options options; >+ >+ torture_assert(tctx, torture_setup_dir(cli, BASEDIR), "Failed to setup up test directory: " BASEDIR); >+ >+ lpcfg_smbcli_session_options(tctx->lp_ctx, &options); >+ >+ torture_comment(tctx, "Testing LOCKING_ANDX multi-lock\n"); >+ io.generic.level = RAW_LOCK_LOCKX; >+ >+ /* Create the test file. */ >+ fnum = smbcli_open(cli->tree, fname, O_RDWR|O_CREAT, DENY_NONE); >+ torture_assert(tctx,(fnum != -1), talloc_asprintf(tctx, >+ "Failed to create %s - %s\n", >+ fname, smbcli_errstr(cli->tree))); >+ >+ /* >+ * Lock regions 100->109, 110->119 as >+ * two separate write locks in one request. >+ */ >+ io.lockx.level = RAW_LOCK_LOCKX; >+ io.lockx.in.file.fnum = fnum; >+ io.lockx.in.mode = LOCKING_ANDX_LARGE_FILES; >+ io.lockx.in.timeout = 0; >+ io.lockx.in.ulock_cnt = 0; >+ io.lockx.in.lock_cnt = 2; >+ io.lockx.in.mode = LOCKING_ANDX_EXCLUSIVE_LOCK; >+ lock[0].pid = cli->session->pid; >+ lock[0].offset = 100; >+ lock[0].count = 10; >+ lock[1].pid = cli->session->pid; >+ lock[1].offset = 120; >+ lock[1].count = 10; >+ io.lockx.in.locks = &lock[0]; >+ status = smb_raw_lock(cli->tree, &io); >+ CHECK_STATUS(status, NT_STATUS_OK); >+ >+ /* >+ * Now request the same locks on a different >+ * context as blocking locks with infinite timeout. >+ */ >+ >+ io.lockx.in.timeout = -1; >+ lock[0].pid = cli->session->pid+1; >+ lock[1].pid = cli->session->pid+1; >+ req = smb_raw_lock_send(cli->tree, &io); >+ torture_assert(tctx,(req != NULL), talloc_asprintf(tctx, >+ "Failed to setup timed locks (%s)\n", __location__)); >+ >+ /* Unlock lock[0] */ >+ io.lockx.in.timeout = 0; >+ io.lockx.in.ulock_cnt = 1; >+ io.lockx.in.lock_cnt = 0; >+ io.lockx.in.locks = &lock[0]; >+ lock[0].pid = cli->session->pid; >+ status = smb_raw_lock(cli->tree, &io); >+ CHECK_STATUS(status, NT_STATUS_OK); >+ >+ /* Start the clock. */ >+ t = time_mono(NULL); >+ >+ /* Unlock lock[1] */ >+ io.lockx.in.timeout = 0; >+ io.lockx.in.ulock_cnt = 1; >+ io.lockx.in.lock_cnt = 0; >+ io.lockx.in.locks = &lock[1]; >+ lock[1].pid = cli->session->pid; >+ status = smb_raw_lock(cli->tree, &io); >+ CHECK_STATUS(status, NT_STATUS_OK); >+ >+ /* receive the successful blocked lock requests */ >+ status = smbcli_request_simple_recv(req); >+ CHECK_STATUS(status, NT_STATUS_OK); >+ >+ /* Fail if this took more than 2 seconds. */ >+ torture_assert(tctx,!(time_mono(NULL) > t+2), talloc_asprintf(tctx, >+ "Blocking locks were not granted immediately (%s)\n", >+ __location__)); >+done: >+ smb_raw_exit(cli->session); >+ smbcli_deltree(cli->tree, BASEDIR); >+ return ret; >+} > > /* > basic testing of lock calls >@@ -2283,6 +2379,7 @@ struct torture_suite *torture_raw_lock(TALLOC_CTX *mem_ctx) > test_multiple_unlock); > torture_suite_add_1smb_test(suite, "zerobytelocks", test_zerobytelocks); > torture_suite_add_1smb_test(suite, "zerobyteread", test_zerobyteread); >+ torture_suite_add_1smb_test(suite, "multilock", test_multilock); > > return suite; > } >-- >1.9.1 >
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=10072">View the attachment on a separate page</a>.</b>
View Attachment As Raw
Actions:
View
Attachments on
bug 10684
:
10064
|
10070
|
10072
|
10076