>From 6d54cf93ea67ffc00876a33534f5411a52a6dda5 Mon Sep 17 00:00:00 2001 From: Jeremy Allison 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 --- 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 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 --- 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 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 --- 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 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 --- 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 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 --- 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