From f0eb0d4c51147a22249e6c72afa41bb84c7618fa Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Wed, 20 Jul 2016 12:36:24 +0200 Subject: [PATCH 1/3] smbd: ignore ctdb tombstone records in fetch_share_mode_unlocked_parser() dbwrap_parse_record() can return ctdb tombstone records from the lctdb, ignore them. Bug: https://bugzilla.samba.org/show_bug.cgi?id=12005 Signed-off-by: Ralph Boehme Reviewed-by: Stefan Metzmacher (cherry picked from commit 7147859c7afc1344e76485e2cbc286679110d96e) --- source3/locking/share_mode_lock.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/source3/locking/share_mode_lock.c b/source3/locking/share_mode_lock.c index b5a63f8..f738323 100644 --- a/source3/locking/share_mode_lock.c +++ b/source3/locking/share_mode_lock.c @@ -628,6 +628,12 @@ static void fetch_share_mode_unlocked_parser( struct share_mode_lock *lck = talloc_get_type_abort( private_data, struct share_mode_lock); + if (data.dsize == 0) { + /* Likely a ctdb tombstone record, ignore it */ + lck->data = NULL; + return; + } + lck->data = parse_share_modes(lck, key, data); } -- 2.7.4 From 1939776359c85890eba2713f8a67110b596098f0 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Sat, 23 Jul 2016 11:08:13 +0200 Subject: [PATCH 2/3] s4/torture: add a test for ctdb-tombstrone-record deadlock This tests for a possible deadlock between smbd and ctdb dealing with ctdb tombstone records. Commit 925625b52886d40b50fc631bad8bdc81970f7598 explains the deadlock in more details and contains the fix. It's a fix for a regression introduced by the patch for bug 10008 (1cae59ce112c). If you ever want to use this test against that specific commit: $ git checkout 925625b52886d40b50fc631bad8bdc81970f7598 $ git cherry-pick THIS_COMMIT This should not deadlock on a ctdb cluster. $ git revert 925625b52886d40b50fc631bad8bdc81970f7598 This will deadlock. Bug: https://bugzilla.samba.org/show_bug.cgi?id=12005 Pair-Programmed-With: Michael Adam Signed-off-by: Ralph Boehme Signed-off-by: Michael Adam (cherry picked from commit b17e2f5c740fb081c007ed2e1c23138ffcba1469) --- source4/torture/smb2/lock.c | 64 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/source4/torture/smb2/lock.c b/source4/torture/smb2/lock.c index 68e353d..3900abf 100644 --- a/source4/torture/smb2/lock.c +++ b/source4/torture/smb2/lock.c @@ -3033,6 +3033,69 @@ done: return ret; } +/** + * Test lock interaction between smbd and ctdb with tombstone records. + * + * Re-locking an unlocked record could lead to a deadlock between + * smbd and ctdb. Make sure we don't regress. + * + * https://bugzilla.samba.org/show_bug.cgi?id=12005 + * https://bugzilla.samba.org/show_bug.cgi?id=10008 + */ +static bool test_deadlock(struct torture_context *torture, + struct smb2_tree *tree) +{ + NTSTATUS status; + bool ret = true; + struct smb2_handle _h; + struct smb2_handle *h = NULL; + uint8_t buf[200]; + const char *fname = BASEDIR "\\deadlock.txt"; + + if (!lpcfg_clustering(torture->lp_ctx)) { + torture_skip(torture, "Test must be run on a ctdb cluster\n"); + return true; + } + + status = torture_smb2_testdir(tree, BASEDIR, &_h); + torture_assert_ntstatus_ok(torture, status, + "torture_smb2_testdir failed"); + smb2_util_close(tree, _h); + + status = torture_smb2_testfile(tree, fname, &_h); + torture_assert_ntstatus_ok_goto(torture, status, ret, done, + "torture_smb2_testfile failed"); + h = &_h; + + ZERO_STRUCT(buf); + status = smb2_util_write(tree, *h, buf, 0, ARRAY_SIZE(buf)); + torture_assert_ntstatus_ok_goto(torture, status, ret, done, + "smb2_util_write failed"); + + status = test_smb2_lock(tree, *h, 0, 1, true); + torture_assert_ntstatus_ok_goto(torture, status, ret, done, + "test_smb2_lock failed"); + + status = test_smb2_unlock(tree, *h, 0, 1); + torture_assert_ntstatus_ok_goto(torture, status, ret, done, + "test_smb2_unlock failed"); + + status = test_smb2_lock(tree, *h, 0, 1, true); + torture_assert_ntstatus_ok_goto(torture, status, ret, done, + "test_smb2_lock failed"); + + status = test_smb2_unlock(tree, *h, 0, 1); + torture_assert_ntstatus_ok_goto(torture, status, ret, done, + "test_smb2_unlock failed"); + +done: + if (h != NULL) { + smb2_util_close(tree, *h); + } + smb2_deltree(tree, BASEDIR); + return ret; +} + /* basic testing of SMB2 locking */ struct torture_suite *torture_smb2_lock_init(void) @@ -3068,6 +3131,7 @@ struct torture_suite *torture_smb2_lock_init(void) torture_suite_add_2smb2_test(suite, "overlap", test_overlap); torture_suite_add_1smb2_test(suite, "truncate", test_truncate); torture_suite_add_1smb2_test(suite, "replay", test_replay); + torture_suite_add_1smb2_test(suite, "ctdb-delrec-deadlock", test_deadlock); suite->description = talloc_strdup(suite, "SMB2-LOCK tests"); -- 2.7.4 From f946992ee67c903e0ab166dc9cec36da54a9fe0d Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Mon, 8 Aug 2016 16:58:51 +0200 Subject: [PATCH 3/3] dbwrap_ctdb: treat empty records in ltdb as non-existing When fetching records from remote ctdb nodes via ctdbd_parse() or in db_ctdb_traverse(), we already check for tombstone records and skip them. This was originally also done for the ltdb checks. See also bug: https://bugzilla.samba.org/show_bug.cgi?id=10008 (commit 1cae59ce112ccb51b45357a52b902f80fce1eef1). Commit 925625b52886d40b50fc631bad8bdc81970f7598 reverted part of the patch of bug 10008 due to a deadlock it introduced. This patch re-introduces the consistent treatment of empty records in the ltdb but avoids the deadlock by correctly signalling NT_STATUS_NOT_FOUND if an empty record is found authoritatively in the ltdb and not calling ctdb in this case. BUG: https://bugzilla.samba.org/show_bug.cgi?id=12005 Pair-Programmed-With: Michael Adam Signed-off-by: Ralph Boehme Signed-off-by: Michael Adam Autobuild-User(master): Michael Adam Autobuild-Date(master): Tue Aug 9 04:38:44 CEST 2016 on sn-devel-144 (cherry picked from commit 25df582739918b7afd4e5497eaffe279e2d92cd1) --- source3/lib/dbwrap/dbwrap_ctdb.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/source3/lib/dbwrap/dbwrap_ctdb.c b/source3/lib/dbwrap/dbwrap_ctdb.c index d86d078..10f5f50 100644 --- a/source3/lib/dbwrap/dbwrap_ctdb.c +++ b/source3/lib/dbwrap/dbwrap_ctdb.c @@ -1221,6 +1221,7 @@ struct db_ctdb_parse_record_state { uint32_t my_vnn; bool ask_for_readonly_copy; bool done; + bool empty_record; }; static void db_ctdb_parse_record_parser( @@ -1240,7 +1241,16 @@ static void db_ctdb_parse_record_parser_nonpersistent( (struct db_ctdb_parse_record_state *)private_data; if (db_ctdb_can_use_local_hdr(header, state->my_vnn, true)) { - state->parser(key, data, state->private_data); + /* + * A record consisting only of the ctdb header can be + * a validly created empty record or a tombstone + * record of a deleted record (not vacuumed yet). Mark + * it accordingly. + */ + state->empty_record = (data.dsize == 0); + if (!state->empty_record) { + state->parser(key, data, state->private_data); + } state->done = true; } else { /* @@ -1267,6 +1277,7 @@ static NTSTATUS db_ctdb_parse_record(struct db_context *db, TDB_DATA key, state.parser = parser; state.private_data = private_data; state.my_vnn = ctdbd_vnn(ctx->conn); + state.empty_record = false; if (ctx->transaction != NULL) { struct db_ctdb_transaction_handle *h = ctx->transaction; @@ -1298,6 +1309,20 @@ static NTSTATUS db_ctdb_parse_record(struct db_context *db, TDB_DATA key, status = db_ctdb_ltdb_parse( ctx, key, db_ctdb_parse_record_parser_nonpersistent, &state); if (NT_STATUS_IS_OK(status) && state.done) { + if (state.empty_record) { + /* + * We know authoritatively, that this is an empty + * record. Since ctdb does not distinguish between empty + * and deleted records, this can be a record stored as + * empty or a not-yet-vacuumed tombstone record of a + * deleted record. Now Samba right now can live without + * empty records, so we can safely report this record + * as non-existing. + * + * See bugs 10008 and 12005. + */ + return NT_STATUS_NOT_FOUND; + } return NT_STATUS_OK; } -- 2.7.4