dbwrap_ctdb: avoid smbd/ctdb deadlocks: check whether we can work locally in db_ctdb_parse_record()

If the same process tries to re-lock the same record
it has already locked, don't go to the ctdbd again.

There are situations where we already have a lock on a record
and then do a dbwrap_parse_record() on that record, for instance
in locking code:

do_lock()
  -> grabs lock on brl record with brl_get_locks()
    -> calls brl_lock()
      -> calls brl_lock_posix or _windows_default()
        -> calls contend_level2_oplocks_begin()
          -> calls brl_locks_get_read_only()
            -> calls dbwrap_parse_record on the same brl record as above

In the local (tdb) case, this is not a problem, because
identical fcntl locks in the same process don't contend each other,
but calling out to ctdb for this lets smbd and ctdb deadlock.

db_ctdb_fetch_lock() already correclty checks first
whether we can simply try to lock locally. But db_ctdb_parse_record()
failed to do so for empty records, i.e. records that only
consist of the ctdb record header. (These can be deleted records
but can also be freshly created and still empty records.)

This patch lets db_ctdb_parse_record() not skip local access
for empty records, hence fixing the deadlock.

PLAN: In the long run, it would be better to solve this
generically on the dbwrap_layer, i.e. root the notion of
an already locked record there, and skip any call to the
db (tdb or ctdb backend) if we have it. This would also
solve the problem for all calls like fetch_locked, parse_record
and possibly others.  But this is the urgent fix for now.

Pair-Programmed-With: Volker Lendecke <vl@samba.org>
Signed-off-by: Michael Adam <obnox@samba.org>
Signed-off-by: Volker Lendecke <vl@samba.org>
Tested-by: Björn Baumbach <bb@sernet.de>

1 files changed, 1 insertions, 10 deletions

diff --git a/source3/lib/dbwrap/dbwrap_ctdb.c b/source3/lib/dbwrap/dbwrap_ctdb.c
index 87e644dbe5..09a51ceff8 100644
--- a/source3/lib/dbwrap/dbwrap_ctdb.c
+++ b/source3/lib/dbwrap/dbwrap_ctdb.c
@@ -109,16 +109,7 @@ static int db_ctdb_ltdb_parser(TDB_DATA key, TDB_DATA data,
 	if (data.dsize < sizeof(struct ctdb_ltdb_header)) {
 		return -1;
 	}
-	if (data.dsize == sizeof(struct ctdb_ltdb_header)) {
-		/*
-		 * Making this a separate case that needs fixing
-		 * separately. This is an empty record. ctdbd does not
-		 * distinguish between empty and deleted records. Samba right
-		 * now can live without empty records, so lets treat zero-size
-		 * (i.e. deleted) records as non-existing.
-		 */
-		return -1;
-	}
+
 	state->parser(
 		key, (struct ctdb_ltdb_header *)data.dptr,
 		make_tdb_data(data.dptr + sizeof(struct ctdb_ltdb_header),