From a38341a4487529ec4ed2a2d6922ab4c833a2585c Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 2 Jul 2014 17:25:22 -0700 Subject: [PATCH 1/7] 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 Reviewed-by: Volker Lendecke (cherry picked from commit 517fa80bd385c6adcfee03ea6b25599013ad88f5) --- 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 5f6dda3..54ce016 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. -- 2.0.0.526.g5318336 From bc26dd51531547864143a669210f00349b8dfb6a Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 2 Jul 2014 20:18:42 -0700 Subject: [PATCH 2/7] 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 Reviewed-by: Volker Lendecke (cherry picked from commit 12be57ef3b2d1b670be7a83f29cd580938030015) --- 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 54ce016..3035850 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); -- 2.0.0.526.g5318336 From ae9797dfe0456551d6c49cfed8880927e58685c3 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 2 Jul 2014 20:40:49 -0700 Subject: [PATCH 3/7] 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 Reviewed-by: Volker Lendecke (cherry picked from commit cc9de6eb091159a84228b988c49261c46c301233) --- 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 3035850..b6edb65 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); -- 2.0.0.526.g5318336 From 8b090c67f09ee457d555de789dcacc304a8e1ceb Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 2 Jul 2014 20:51:24 -0700 Subject: [PATCH 4/7] 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 Reviewed-by: Volker Lendecke (cherry picked from commit 954401f8b2b16b3e2ef9655e8ce94d657becce36) --- 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 b6edb65..d5b8347 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); -- 2.0.0.526.g5318336 From 7a6d3e3e6834ad09839e64bca83d77aedb48596d Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 1 Jul 2014 12:05:07 -0700 Subject: [PATCH 5/7] 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 Reviewed-by: Volker Lendecke (cherry picked from commit 64346a134dac2bd023f7473202ca38d35ffd3c89) --- 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 From 389bc60be5e59d8b1b3dc782d56fd936908c74c5 Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Thu, 3 Jul 2014 10:05:39 +0000 Subject: [PATCH 6/7] torture4: Adapt comment to code Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison (cherry picked from commit 4205463ef1815d6e86e1d1f1f57651ca30407469) --- source4/torture/raw/lock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source4/torture/raw/lock.c b/source4/torture/raw/lock.c index 8dc2434..2d5616f 100644 --- a/source4/torture/raw/lock.c +++ b/source4/torture/raw/lock.c @@ -2292,7 +2292,7 @@ static bool test_multilock(struct torture_context *tctx, fname, smbcli_errstr(cli->tree))); /* - * Lock regions 100->109, 110->119 as + * Lock regions 100->109, 120->129 as * two separate write locks in one request. */ io.lockx.level = RAW_LOCK_LOCKX; -- 2.0.0.526.g5318336 From 2be067e8b7fa3487e64f4c81a593daecd9e6ff27 Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Thu, 3 Jul 2014 10:05:55 +0000 Subject: [PATCH 7/7] torture4: Make raw.lock.multilock fail after 20 seconds Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison Autobuild-User(master): Jeremy Allison Autobuild-Date(master): Fri Jul 4 00:04:10 CEST 2014 on sn-devel-104 (cherry picked from commit 0c97b7eb5359b95c0d51a3b5524e82e34243d2d1) --- source4/torture/raw/lock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source4/torture/raw/lock.c b/source4/torture/raw/lock.c index 2d5616f..e131e27 100644 --- a/source4/torture/raw/lock.c +++ b/source4/torture/raw/lock.c @@ -2317,7 +2317,7 @@ static bool test_multilock(struct torture_context *tctx, * context as blocking locks with infinite timeout. */ - io.lockx.in.timeout = -1; + io.lockx.in.timeout = 20000; lock[0].pid = cli->session->pid+1; lock[1].pid = cli->session->pid+1; req = smb_raw_lock_send(cli->tree, &io); -- 2.0.0.526.g5318336