From d40b9d0a100bfe01c658ecaa6324db6812ff01cb Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 1 Jul 2014 14:05:08 -0700 Subject: [PATCH 1/3] 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 next patch which re-adds a pending lock record if we fail to acquire. Breaking up the patch into two 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 2f0ab2e5405fca8b066d3795d1d30222679e375b Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 1 Jul 2014 14:38:10 -0700 Subject: [PATCH 2/3] s3: smbd: Locking - re-add pending lock records if we fail to acquire a lock. In conjunction with the previous patch, 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 5b1de65..4b3b61a 100644 --- a/source3/smbd/blocking.c +++ b/source3/smbd/blocking.c @@ -440,8 +440,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; @@ -458,9 +456,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 @@ -469,9 +472,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, @@ -480,6 +483,28 @@ static bool process_lockingX(struct blocking_lock_record *blr) &blr->blocking_smblctx, blr); + if (ERROR_WAS_LOCK_DENIED(status)) { + /* + * Re-add the pending lock entry. + */ + 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)) { @@ -544,6 +569,29 @@ static bool process_trans2(struct blocking_lock_record *blr) &status, &blr->blocking_smblctx, blr); + + if (ERROR_WAS_LOCK_DENIED(status)) { + /* + * 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 f797a8034cfa730e6f26f04ab70c1fd1c4cb0262 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 1 Jul 2014 12:05:07 -0700 Subject: [PATCH 3/3] 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