From c77c212afd169769084bda9eec7220b8b404347e Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 1 Jul 2014 14:05:08 -0700 Subject: [PATCH 1/4] s3: smbd: Locking - Remove any pending lock record *before* attempting to acquire it again. The removes duplicate code from the return cases, but is incomplete until the following patches which re-add a pending lock record if we fail to acquire. Breaking up the patch this way is clearer (to me at least). 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 --- source3/smbd/blocking.c | 59 ++++++++++++++++--------------------------------- 1 file changed, 19 insertions(+), 40 deletions(-) diff --git a/source3/smbd/blocking.c b/source3/smbd/blocking.c index 5d198fc..5b1de65 100644 --- a/source3/smbd/blocking.c +++ b/source3/smbd/blocking.c @@ -748,6 +748,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; @@ -767,24 +768,27 @@ 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); + /* + * 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); + 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); + } + + if(blocking_lock_record_process(blr)) { 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); continue; @@ -796,32 +800,7 @@ void process_blocking_lock_queue(struct smbd_server_connection *sconn) */ if (!timeval_is_zero(&blr->expire_time) && timeval_compare(&blr->expire_time, &tv_curr) <= 0) { - struct byte_range_lock *br_lck = brl_get_locks( - talloc_tos(), blr->fsp); - DEBUG(10, ("Lock timed out! BLR = %p\n", blr)); - - /* - * 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); - } - blocking_lock_reply_error(blr,NT_STATUS_FILE_LOCK_CONFLICT); DLIST_REMOVE(sconn->smb1.locks.blocking_lock_queue, blr); TALLOC_FREE(blr); -- 2.0.0.526.g5318336 From 6a1ee0d54e8d1ad5ca04a36737204059af8fb111 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 2 Jul 2014 11:01:18 -0700 Subject: [PATCH 2/4] s3: smbd: Locking - add utility function lock_timed_out(). Will be used more extensively in the next patch. 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 --- 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 5b1de65..bca13b2 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. *****************************************************************************/ @@ -735,11 +754,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; } @@ -799,7 +817,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)) { DEBUG(10, ("Lock timed out! BLR = %p\n", blr)); blocking_lock_reply_error(blr,NT_STATUS_FILE_LOCK_CONFLICT); DLIST_REMOVE(sconn->smb1.locks.blocking_lock_queue, blr); -- 2.0.0.526.g5318336 From 7cb52a29107818bb649ed4f775af8c855ca75501 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 2 Jul 2014 11:14:27 -0700 Subject: [PATCH 3/4] s3: smbd: Locking - re-add pending lock records if we fail to acquire a lock (and the lock hasn't timed out). In conjunction with the previous patches, this keeps the blocking lock record and the pending lock records consistent if we are dealing with multiple blocking lock requests in one SMB1 LockingX request. 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 --- source3/smbd/blocking.c | 64 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 56 insertions(+), 8 deletions(-) diff --git a/source3/smbd/blocking.c b/source3/smbd/blocking.c index bca13b2..4f7af3d 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; @@ -477,9 +475,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 @@ -488,9 +491,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, @@ -499,6 +502,28 @@ static bool process_lockingX(struct blocking_lock_record *blr) &blr->blocking_smblctx, blr); + if (ERROR_WAS_LOCK_DENIED(status) && !lock_timed_out(blr)) { + /* + * Re-add the pending lock entry if it's still active. + */ + NTSTATUS status1 = brl_lock(fsp->conn->sconn->msg_ctx, + br_lck, + blr->smblctx, + messaging_server_id(fsp->conn->sconn->msg_ctx), + blr->offset, + blr->count, + locktype == 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)) { @@ -563,6 +588,29 @@ static bool process_trans2(struct blocking_lock_record *blr) &status, &blr->blocking_smblctx, blr); + + if (ERROR_WAS_LOCK_DENIED(status) && !lock_timed_out(blr)) { + /* + * Re-add the pending lock entry. + */ + 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)) { -- 2.0.0.526.g5318336 From 937531d363a96847d3c21af2fe19f0a33b1abde3 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 1 Jul 2014 12:05:07 -0700 Subject: [PATCH 4/4] 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 --- 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; } -- 2.0.0.526.g5318336