[PATCH] Hold a read lock during the samba_dsdb stack init() phase
Andrew Bartlett
abartlet at samba.org
Wed Apr 11 01:10:20 UTC 2018
On Tue, 2018-04-10 at 22:13 +1200, Andrew Bartlett wrote:
> The init_module hook is not locked by ldb, as the module may need to do
> things, but due to the multi-db nature of Samba's stack we need to lock
> it, so we do so in samba_dsdb.
>
> This should/may avoid lock ordering issues that show up as
> prepare_commit locking failures. (These seemed to show up a lot while
> I was trying to merge the LMDB patches, but I've seen them without so
> they are unrelated).
>
> CI results will be here:
>
> https://gitlab.com/catalyst-samba/samba/pipelines/20250607
>
> Please review and push if the CI indicates.
The new CI run is here:
https://gitlab.com/catalyst-samba/samba/pipelines/20296527
I've addressed the failure in the samba_tool test which detected that
the schema was being reloaded too often. This new patch passes the
schema tests while also reloading the schema less often.
Please review and push if the CI indicates.
Thanks!
Andrew Bartlett
--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team https://samba.org
Samba Development and Support, Catalyst IT
https://catalyst.net.nz/services/samba
-------------- next part --------------
From 261fba205d31c7027c389c93669c0d94c8ab94ea Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Thu, 15 Mar 2018 13:42:17 +1300
Subject: [PATCH 01/13] ldb_wrap: Remove ldb_transaction_cancel_noerr from
ldb_wrap_fork_hook()
Writing to a TDB, without locks (these are per-process) in a forked child is never going to
end well, if a transaction is open at this point we have bigger problems.
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
lib/ldb-samba/ldb_wrap.c | 15 +++------------
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/lib/ldb-samba/ldb_wrap.c b/lib/ldb-samba/ldb_wrap.c
index 9959b04ed95..8c3bf6f7bf3 100644
--- a/lib/ldb-samba/ldb_wrap.c
+++ b/lib/ldb-samba/ldb_wrap.c
@@ -329,20 +329,11 @@ int samba_ldb_connect(struct ldb_context *ldb, struct loadparm_context *lp_ctx,
}
/*
- when we fork() we need to make sure that any open ldb contexts have
- any open transactions cancelled (ntdb databases doesn't need reopening,
- as we don't use clear_if_first).
- */
+ call tdb_reopen_all() in case there is a TDB open so we are
+ not blocked from re-opening it inside ldb_tdb.
+*/
void ldb_wrap_fork_hook(void)
{
- struct ldb_wrap *w;
-
- for (w=ldb_wrap_list; w; w=w->next) {
- if (ldb_transaction_cancel_noerr(w->ldb) != LDB_SUCCESS) {
- smb_panic("Failed to cancel child transactions\n");
- }
- }
-
if (tdb_reopen_all(1) != 0) {
smb_panic("tdb_reopen_all failed\n");
}
--
2.11.0
From f32fb0a5a84c83263b1917a3418c2c738fa93152 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Mon, 9 Apr 2018 14:52:47 +1200
Subject: [PATCH 02/13] dsdb: Ensure to cancel the transaction if we fail to
save the prefixMap
This rare error case forgot to call ldb_transaction_cancel()
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
source4/dsdb/repl/replicated_objects.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/source4/dsdb/repl/replicated_objects.c b/source4/dsdb/repl/replicated_objects.c
index 0c44960cf5f..4c8890f0553 100644
--- a/source4/dsdb/repl/replicated_objects.c
+++ b/source4/dsdb/repl/replicated_objects.c
@@ -921,6 +921,7 @@ WERROR dsdb_replicated_objects_commit(struct ldb_context *ldb,
}
DEBUG(0,("Failed to save updated prefixMap: %s\n",
win_errstr(werr)));
+ ldb_transaction_cancel(ldb);
TALLOC_FREE(tmp_ctx);
return werr;
}
--
2.11.0
From 892c0808510300bc5307201ad0200fd7dc32500e Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Mon, 9 Apr 2018 17:51:57 +1200
Subject: [PATCH 03/13] dsdb: Do not create a transaction in partition_init()
This will allow us to lock the databases for read during all of the Samba init
hooks.
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
.../dsdb/samdb/ldb_modules/partition_metadata.c | 59 ++++++++++------------
1 file changed, 27 insertions(+), 32 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/partition_metadata.c b/source4/dsdb/samdb/ldb_modules/partition_metadata.c
index e3ad0d8c6c2..3e60393a2c9 100644
--- a/source4/dsdb/samdb/ldb_modules/partition_metadata.c
+++ b/source4/dsdb/samdb/ldb_modules/partition_metadata.c
@@ -319,38 +319,6 @@ int partition_metadata_init(struct ldb_module *module)
return ret;
}
- /*
- * We need to fill in the sequence number from the DB, so we
- * need to get a lock over all the databases. We only read
- * from the main partitions, but write to metadata so to avoid
- * lock ordering we just get a transaction over the lot.
- */
- ret = partition_start_trans(module);
- if (ret != LDB_SUCCESS) {
- TALLOC_FREE(data->metadata);
- return ret;
- }
-
- ret = partition_metadata_set_sequence_number(module);
- if (ret != LDB_SUCCESS) {
- TALLOC_FREE(data->metadata);
- partition_del_trans(module);
- return ret;
- }
-
- ret = partition_prepare_commit(module);
- if (ret != LDB_SUCCESS) {
- TALLOC_FREE(data->metadata);
- partition_del_trans(module);
- return ret;
- }
-
- ret = partition_end_trans(module);
- if (ret != LDB_SUCCESS) {
- /* Nothing much we can do */
- TALLOC_FREE(data->metadata);
- }
-
return ret;
}
@@ -370,6 +338,13 @@ int partition_metadata_sequence_number(struct ldb_module *module, uint64_t *valu
return ret;
}
+ /*
+ * This means we will give a 0 until the first write
+ * tranaction, which is actually pretty reasonable.
+ *
+ * All modern databases will have the metadata.tdb from
+ * the time of the first transaction in provision anyway.
+ */
ret = partition_metadata_get_uint64(module,
LDB_METADATA_SEQ_NUM,
value,
@@ -410,6 +385,26 @@ int partition_metadata_sequence_number_increment(struct ldb_module *module, uint
return ret;
}
+ if (*value == 0) {
+ /*
+ * We are in a transaction now, so we can get the
+ * sequence number from the partitions.
+ */
+ ret = partition_metadata_set_sequence_number(module);
+ if (ret != LDB_SUCCESS) {
+ TALLOC_FREE(data->metadata);
+ partition_del_trans(module);
+ return ret;
+ }
+
+ ret = partition_metadata_get_uint64(module,
+ LDB_METADATA_SEQ_NUM,
+ value, 0);
+ if (ret != LDB_SUCCESS) {
+ return ret;
+ }
+ }
+
(*value)++;
ret = partition_metadata_set_uint64(module, LDB_METADATA_SEQ_NUM, *value, false);
return ret;
--
2.11.0
From 0c5bdb966d548b035989051c0e94d735cef486d9 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Mon, 9 Apr 2018 21:15:25 +1200
Subject: [PATCH 04/13] dsdb: Allow search before init() call in
encrypted_secrets
Simply do not decrypt anything until the init call is run.
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
source4/dsdb/samdb/ldb_modules/encrypted_secrets.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source4/dsdb/samdb/ldb_modules/encrypted_secrets.c b/source4/dsdb/samdb/ldb_modules/encrypted_secrets.c
index 87ec9e4eb53..ef69bb0831c 100644
--- a/source4/dsdb/samdb/ldb_modules/encrypted_secrets.c
+++ b/source4/dsdb/samdb/ldb_modules/encrypted_secrets.c
@@ -1365,7 +1365,7 @@ static int es_search_post_process(struct ldb_module *module,
/*
* Decrypt any encrypted secret attributes
*/
- if (data->encrypt_secrets) {
+ if (data && data->encrypt_secrets) {
int err = decrypt_secret_attributes(ldb, msg, data);
if (err != LDB_SUCCESS) {
return err;
--
2.11.0
From 5f7409b703a868e69fc032865fe6c10254037df2 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Wed, 11 Apr 2018 12:51:49 +1200
Subject: [PATCH 05/13] selftest: Make a transaction before @INDEXLIST etc is
checked in dsdb_schema_attributes.py
This helps us remove the write to the database from the (soon to be read locked) init
code.
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
python/samba/tests/dsdb_schema_attributes.py | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/python/samba/tests/dsdb_schema_attributes.py b/python/samba/tests/dsdb_schema_attributes.py
index 2bebbb569f2..c3b956f3b99 100644
--- a/python/samba/tests/dsdb_schema_attributes.py
+++ b/python/samba/tests/dsdb_schema_attributes.py
@@ -193,6 +193,14 @@ systemOnly: FALSE
samdb2 = samba.tests.connect_samdb(self.lp.samdb_url())
+ # We now only update the @ATTRIBUTES when a transaction happens
+ # rather than making a read of the DB do writes.
+ #
+ # This avoids locking issues and is more expected
+
+ samdb2.transaction_start()
+ samdb2.transaction_commit()
+
res = self.samdb.search(base="@ATTRIBUTES", scope=ldb.SCOPE_BASE,
attrs=["@TEST_EXTRA"])
self.assertEquals(len(res), 1)
@@ -220,6 +228,14 @@ systemOnly: FALSE
samdb2 = samba.tests.connect_samdb(self.lp.samdb_url())
+ # We now only update the @INDEXLIST when a transaction happens
+ # rather than making a read of the DB do writes.
+ #
+ # This avoids locking issues and is more expected
+
+ samdb2.transaction_start()
+ samdb2.transaction_commit()
+
res = self.samdb.search(base="@INDEXLIST", scope=ldb.SCOPE_BASE,
attrs=["@TEST_EXTRA"])
self.assertEquals(len(res), 1)
--
2.11.0
From 6f61cfaf8ec4f2335a2b30e25a44451a3333c82e Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Mon, 9 Apr 2018 21:59:01 +1200
Subject: [PATCH 06/13] dsdb: Wait until a transaction starts to call
dsdb_schema_set_indices_and_attributes()
This avoids starting a transaction in schema_load_init() and allows it
to operate with a read lock held, which will avoid locking issues
(deadlock detected due to lock odering if we do not have a global
read lock).
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
source4/dsdb/samdb/ldb_modules/schema_load.c | 60 +++++++---------------------
1 file changed, 15 insertions(+), 45 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/schema_load.c b/source4/dsdb/samdb/ldb_modules/schema_load.c
index 2099fac1159..55fb63a0f30 100644
--- a/source4/dsdb/samdb/ldb_modules/schema_load.c
+++ b/source4/dsdb/samdb/ldb_modules/schema_load.c
@@ -42,6 +42,12 @@ struct schema_load_private_data {
uint64_t schema_seq_num_read_lock;
uint64_t schema_seq_num_cache;
int tdb_seqnum;
+
+ /*
+ * Please write out the updated schema on the next transaction
+ * start
+ */
+ bool need_write;
};
static int dsdb_schema_from_db(struct ldb_module *module,
@@ -495,7 +501,6 @@ static int schema_load_init(struct ldb_module *module)
struct ldb_context *ldb = ldb_module_get_ctx(module);
struct schema_load_private_data *private_data;
int ret;
- bool need_write = false;
private_data = talloc_zero(module, struct schema_load_private_data);
if (private_data == NULL) {
@@ -510,50 +515,7 @@ static int schema_load_init(struct ldb_module *module)
return ret;
}
- ret = schema_load(ldb, module, &need_write);
-
- if (ret == LDB_SUCCESS && need_write) {
- TALLOC_CTX *frame = talloc_stackframe();
- struct dsdb_schema *schema = NULL;
-
- ret = ldb_transaction_start(ldb);
- if (ret != LDB_SUCCESS) {
- ldb_debug_set(ldb, LDB_DEBUG_FATAL,
- "schema_load_init: transaction start failed");
- return LDB_ERR_OPERATIONS_ERROR;
- }
-
- schema = dsdb_get_schema(ldb, frame);
- if (schema == NULL) {
- ldb_debug_set(ldb, LDB_DEBUG_FATAL,
- "schema_load_init: dsdb_get_schema failed");
- ldb_transaction_cancel(ldb);
- return LDB_ERR_OPERATIONS_ERROR;
- }
- ret = dsdb_schema_set_indices_and_attributes(ldb, schema,
- SCHEMA_WRITE);
-
- TALLOC_FREE(frame);
-
- if (ret != LDB_SUCCESS) {
- ldb_asprintf_errstring(ldb, "Failed to write new "
- "@INDEXLIST and @ATTRIBUTES "
- "records for updated schema: %s",
- ldb_errstring(ldb));
- ldb_transaction_cancel(ldb);
- return ret;
- }
-
- ret = ldb_transaction_commit(ldb);
- if (ret != LDB_SUCCESS) {
- ldb_debug_set(ldb, LDB_DEBUG_FATAL,
- "schema_load_init: transaction commit failed");
- return LDB_ERR_OPERATIONS_ERROR;
- }
- }
-
-
- return ret;
+ return schema_load(ldb, module, &private_data->need_write);
}
static int schema_search(struct ldb_module *module, struct ldb_request *req)
@@ -586,6 +548,14 @@ static int schema_load_start_transaction(struct ldb_module *module)
"schema_load_init: dsdb_get_schema failed");
return LDB_ERR_OPERATIONS_ERROR;
}
+
+ if (private_data->need_write) {
+ ret = dsdb_schema_set_indices_and_attributes(ldb,
+ schema,
+ SCHEMA_WRITE);
+ private_data->need_write = false;
+ }
+
private_data->in_transaction++;
return ret;
--
2.11.0
From 9a2e391b40ce945fd2f25683dda5d2c421472dbb Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Tue, 10 Apr 2018 07:58:07 +1200
Subject: [PATCH 07/13] dsdb: Create rootdse_get_private_data()
This will get the private data on the first call, allowing that not to be
the init() hook.
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
source4/dsdb/samdb/ldb_modules/rootdse.c | 63 ++++++++++++++++++++++++--------
1 file changed, 48 insertions(+), 15 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/rootdse.c b/source4/dsdb/samdb/ldb_modules/rootdse.c
index 0d486218c0b..0d4cbc60f24 100644
--- a/source4/dsdb/samdb/ldb_modules/rootdse.c
+++ b/source4/dsdb/samdb/ldb_modules/rootdse.c
@@ -863,11 +863,46 @@ static int rootdse_search(struct ldb_module *module, struct ldb_request *req)
return ldb_next_request(module, down_req);
}
+static struct rootdse_private_data *rootdse_get_private_data(struct ldb_module *module)
+{
+ void *priv = ldb_module_get_private(module);
+ struct rootdse_private_data *data = NULL;
+
+ if (priv != NULL) {
+ data = talloc_get_type_abort(priv,
+ struct rootdse_private_data);
+ }
+
+ if (data != NULL) {
+ return data;
+ }
+
+ data = talloc_zero(module, struct rootdse_private_data);
+ if (data == NULL) {
+ return NULL;
+ }
+
+ data->num_controls = 0;
+ data->controls = NULL;
+ data->num_partitions = 0;
+ data->partitions = NULL;
+ data->block_anonymous = true;
+
+ ldb_module_set_private(module, data);
+ return data;
+}
+
+
static int rootdse_register_control(struct ldb_module *module, struct ldb_request *req)
{
- struct rootdse_private_data *priv = talloc_get_type(ldb_module_get_private(module), struct rootdse_private_data);
+ struct rootdse_private_data *priv =
+ rootdse_get_private_data(module);
char **list;
+ if (priv == NULL) {
+ return ldb_module_oom(module);
+ }
+
list = talloc_realloc(priv, priv->controls, char *, priv->num_controls + 1);
if (!list) {
return ldb_oom(ldb_module_get_ctx(module));
@@ -886,9 +921,14 @@ static int rootdse_register_control(struct ldb_module *module, struct ldb_reques
static int rootdse_register_partition(struct ldb_module *module, struct ldb_request *req)
{
- struct rootdse_private_data *priv = talloc_get_type(ldb_module_get_private(module), struct rootdse_private_data);
+ struct rootdse_private_data *priv =
+ rootdse_get_private_data(module);
struct ldb_dn **list;
+ if (priv == NULL) {
+ return ldb_module_oom(module);
+ }
+
list = talloc_realloc(priv, priv->partitions, struct ldb_dn *, priv->num_partitions + 1);
if (!list) {
return ldb_oom(ldb_module_get_ctx(module));
@@ -924,28 +964,21 @@ static int rootdse_request(struct ldb_module *module, struct ldb_request *req)
static int rootdse_init(struct ldb_module *module)
{
int ret;
- struct ldb_context *ldb;
struct ldb_result *res;
- struct rootdse_private_data *data;
const char *attrs[] = { "msDS-Behavior-Version", NULL };
const char *ds_attrs[] = { "dsServiceName", NULL };
TALLOC_CTX *mem_ctx;
- ldb = ldb_module_get_ctx(module);
+ struct ldb_context *ldb
+ = ldb_module_get_ctx(module);
+
+ struct rootdse_private_data *data
+ = rootdse_get_private_data(module);
- data = talloc_zero(module, struct rootdse_private_data);
if (data == NULL) {
- return ldb_oom(ldb);
+ return ldb_module_oom(module);
}
- data->num_controls = 0;
- data->controls = NULL;
- data->num_partitions = 0;
- data->partitions = NULL;
- data->block_anonymous = true;
-
- ldb_module_set_private(module, data);
-
ldb_set_default_dns(ldb);
ret = ldb_next_init(module);
--
2.11.0
From 4edd0485b4ffa465273a6fe0855d47b186522e21 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Tue, 10 Apr 2018 07:54:20 +1200
Subject: [PATCH 08/13] dsdb: Move ldb_set_default_dns() into
rootdse_get_private_data()
This call needs to be done at the very first chance, in this case
during the first call to the lock_read() hook, otherwise the
schema_data module can't find the schema.
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
source4/dsdb/samdb/ldb_modules/rootdse.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/rootdse.c b/source4/dsdb/samdb/ldb_modules/rootdse.c
index 0d4cbc60f24..751fe15d1a1 100644
--- a/source4/dsdb/samdb/ldb_modules/rootdse.c
+++ b/source4/dsdb/samdb/ldb_modules/rootdse.c
@@ -867,6 +867,8 @@ static struct rootdse_private_data *rootdse_get_private_data(struct ldb_module *
{
void *priv = ldb_module_get_private(module);
struct rootdse_private_data *data = NULL;
+ struct ldb_context *ldb
+ = ldb_module_get_ctx(module);
if (priv != NULL) {
data = talloc_get_type_abort(priv,
@@ -889,6 +891,9 @@ static struct rootdse_private_data *rootdse_get_private_data(struct ldb_module *
data->block_anonymous = true;
ldb_module_set_private(module, data);
+
+ ldb_set_default_dns(ldb);
+
return data;
}
@@ -979,8 +984,6 @@ static int rootdse_init(struct ldb_module *module)
return ldb_module_oom(module);
}
- ldb_set_default_dns(ldb);
-
ret = ldb_next_init(module);
if (ret != LDB_SUCCESS) {
--
2.11.0
From 3087137fe59c048696319da09bc5b4d94b60d74d Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Tue, 10 Apr 2018 16:34:21 +1200
Subject: [PATCH 09/13] dsdb: Allow search before init() is called in
extended_dn_out
This matches the earlier check of p && p->normalise.
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
source4/dsdb/samdb/ldb_modules/extended_dn_out.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source4/dsdb/samdb/ldb_modules/extended_dn_out.c b/source4/dsdb/samdb/ldb_modules/extended_dn_out.c
index ad4603fbbb9..6a869d0c482 100644
--- a/source4/dsdb/samdb/ldb_modules/extended_dn_out.c
+++ b/source4/dsdb/samdb/ldb_modules/extended_dn_out.c
@@ -498,7 +498,7 @@ static int extended_callback(struct ldb_request *req, struct ldb_reply *ares,
continue;
}
- if (p->normalise) {
+ if (p && p->normalise) {
/* If we are also in 'normalise' mode, then
* fix the attribute names to be in the
* correct case */
--
2.11.0
From eb0f583c5bd32f1fafa4347d7064381e506a6c14 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Wed, 11 Apr 2018 11:58:22 +1200
Subject: [PATCH 10/13] dsdb: Load schema during the read_lock() hook, not the
search
This should trigger slightly less often and is the more correct place, as
we only load it during the first lock when not in a transaction.
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
source4/dsdb/samdb/ldb_modules/schema_load.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/schema_load.c b/source4/dsdb/samdb/ldb_modules/schema_load.c
index 55fb63a0f30..9ed363a5973 100644
--- a/source4/dsdb/samdb/ldb_modules/schema_load.c
+++ b/source4/dsdb/samdb/ldb_modules/schema_load.c
@@ -518,16 +518,6 @@ static int schema_load_init(struct ldb_module *module)
return schema_load(ldb, module, &private_data->need_write);
}
-static int schema_search(struct ldb_module *module, struct ldb_request *req)
-{
- struct ldb_context *ldb = ldb_module_get_ctx(module);
-
- /* Try the schema refresh now */
- dsdb_get_schema(ldb, NULL);
-
- return ldb_next_request(module, req);
-}
-
static int schema_load_start_transaction(struct ldb_module *module)
{
struct schema_load_private_data *private_data =
@@ -632,8 +622,10 @@ static int schema_read_lock(struct ldb_module *module)
return ldb_next_read_lock(module);
}
+ private_data->in_read_transaction++;
+
if (private_data->in_transaction == 0 &&
- private_data->in_read_transaction == 0) {
+ private_data->in_read_transaction == 1) {
/*
* This appears to fail during the init path, so do not bother
* checking the return, and return 0 (reload schema).
@@ -643,8 +635,10 @@ static int schema_read_lock(struct ldb_module *module)
&schema_seq_num, 0);
private_data->schema_seq_num_read_lock = schema_seq_num;
+
+ /* Try the schema refresh now */
+ dsdb_get_schema(ldb_module_get_ctx(module), NULL);
}
- private_data->in_read_transaction++;
return ldb_next_read_lock(module);
@@ -673,7 +667,6 @@ static const struct ldb_module_ops ldb_schema_load_module_ops = {
.name = "schema_load",
.init_context = schema_load_init,
.extended = schema_load_extended,
- .search = schema_search,
.start_transaction = schema_load_start_transaction,
.end_transaction = schema_load_end_transaction,
.del_transaction = schema_load_del_transaction,
--
2.11.0
From 821e97ae5617a27df92c0e760ddf1a60a127d4d2 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Wed, 11 Apr 2018 12:29:18 +1200
Subject: [PATCH 11/13] dsdb: Rework schema reload during the read lock
Rather than refusing the reload based on making cached sequence numbers match
just load it once at the time the DB is globally locked, if required.
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
source4/dsdb/samdb/ldb_modules/schema_load.c | 69 ++++++++++++----------------
1 file changed, 29 insertions(+), 40 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/schema_load.c b/source4/dsdb/samdb/ldb_modules/schema_load.c
index 9ed363a5973..cfd88fec467 100644
--- a/source4/dsdb/samdb/ldb_modules/schema_load.c
+++ b/source4/dsdb/samdb/ldb_modules/schema_load.c
@@ -39,7 +39,6 @@ struct schema_load_private_data {
uint64_t in_transaction;
uint64_t in_read_transaction;
struct tdb_wrap *metadata;
- uint64_t schema_seq_num_read_lock;
uint64_t schema_seq_num_cache;
int tdb_seqnum;
@@ -201,16 +200,26 @@ static struct dsdb_schema *dsdb_schema_refresh(struct ldb_module *module, struct
return schema;
}
- if (private_data->in_transaction > 0) {
-
+ if (schema != NULL) {
/*
- * If the refresh is not an expected part of a larger
- * transaction, then we don't allow a schema reload during a
- * transaction. This stops others from modifying our schema
- * behind our backs
+ * If we have a schema already (not in the startup)
+ * and we are in a read or write transaction, then
+ * avoid a schema reload, it can't have changed
*/
- if (ldb_get_opaque(ldb, "dsdb_schema_refresh_expected") != (void *)1) {
- return schema;
+ if (private_data->in_transaction > 0
+ || private_data->in_read_transaction > 0 ) {
+ /*
+ * If the refresh is not an expected part of a
+ * larger transaction, then we don't allow a
+ * schema reload during a transaction. This
+ * stops others from modifying our schema
+ * behind our backs
+ */
+ if (ldb_get_opaque(ldb,
+ "dsdb_schema_refresh_expected")
+ != (void *)1) {
+ return schema;
+ }
}
}
@@ -229,19 +238,9 @@ static struct dsdb_schema *dsdb_schema_refresh(struct ldb_module *module, struct
* continue to hit the database to get the highest USN.
*/
- if (private_data->in_read_transaction > 0) {
- /*
- * We must give a static value of the metadata sequence number
- * during a read lock, otherwise, we will fail to load the
- * schema at runtime.
- */
- schema_seq_num = private_data->schema_seq_num_read_lock;
- ret = LDB_SUCCESS;
- } else {
- ret = schema_metadata_get_uint64(private_data,
- DSDB_METADATA_SCHEMA_SEQ_NUM,
- &schema_seq_num, 0);
- }
+ ret = schema_metadata_get_uint64(private_data,
+ DSDB_METADATA_SCHEMA_SEQ_NUM,
+ &schema_seq_num, 0);
if (schema != NULL) {
if (ret == LDB_SUCCESS) {
@@ -616,32 +615,26 @@ static int schema_read_lock(struct ldb_module *module)
{
struct schema_load_private_data *private_data =
talloc_get_type(ldb_module_get_private(module), struct schema_load_private_data);
- uint64_t schema_seq_num = 0;
+ int ret;
if (private_data == NULL) {
return ldb_next_read_lock(module);
}
- private_data->in_read_transaction++;
+ ret = ldb_next_read_lock(module);
+ if (ret != LDB_SUCCESS) {
+ return ret;
+ }
if (private_data->in_transaction == 0 &&
- private_data->in_read_transaction == 1) {
- /*
- * This appears to fail during the init path, so do not bother
- * checking the return, and return 0 (reload schema).
- */
- schema_metadata_get_uint64(private_data,
- DSDB_METADATA_SCHEMA_SEQ_NUM,
- &schema_seq_num, 0);
-
- private_data->schema_seq_num_read_lock = schema_seq_num;
-
+ private_data->in_read_transaction == 0) {
/* Try the schema refresh now */
dsdb_get_schema(ldb_module_get_ctx(module), NULL);
}
- return ldb_next_read_lock(module);
+ private_data->in_read_transaction++;
+ return LDB_SUCCESS;
}
static int schema_read_unlock(struct ldb_module *module)
@@ -653,10 +646,6 @@ static int schema_read_unlock(struct ldb_module *module)
return ldb_next_read_unlock(module);
}
- if (private_data->in_transaction == 0 &&
- private_data->in_read_transaction == 1) {
- private_data->schema_seq_num_read_lock = 0;
- }
private_data->in_read_transaction--;
return ldb_next_read_unlock(module);
--
2.11.0
From f6767ead762281d80e6f864e4e6d85ec2aa32848 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Mon, 9 Apr 2018 18:13:59 +1200
Subject: [PATCH 12/13] dsdb: ensure we take out a read lock during the
dsdb_init
We have to also take it out in the partitions code when we load the
partition backends.
This ensures that the init handlers hold a whole-db lock just as the
search code does.
To ensure the locking count in schema_load is balanced, the
private data is now created in the first lock_read() call.
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
source4/dsdb/samdb/ldb_modules/partition.c | 24 +++++++++++++++++++++
source4/dsdb/samdb/ldb_modules/partition_init.c | 15 +++----------
source4/dsdb/samdb/ldb_modules/samba_dsdb.c | 17 +++++++++++++--
source4/dsdb/samdb/ldb_modules/schema_load.c | 28 ++++++++++++-------------
4 files changed, 55 insertions(+), 29 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/partition.c b/source4/dsdb/samdb/ldb_modules/partition.c
index 37e714d6e1b..9fb1b9dee12 100644
--- a/source4/dsdb/samdb/ldb_modules/partition.c
+++ b/source4/dsdb/samdb/ldb_modules/partition.c
@@ -1236,6 +1236,30 @@ int partition_read_lock(struct ldb_module *module)
* ordering
*/
+ if (data == NULL) {
+ TALLOC_CTX *mem_ctx = talloc_new(module);
+
+ data = talloc_zero(mem_ctx, struct partition_private_data);
+ if (data == NULL) {
+ talloc_free(mem_ctx);
+ return ldb_operr(ldb);
+ }
+
+ /*
+ * When used from Samba4, this message is set by the
+ * samba4 module, as a fixed value not read from the
+ * DB. This avoids listing modules in the DB
+ */
+ data->forced_module_msg = talloc_get_type(
+ ldb_get_opaque(ldb,
+ DSDB_OPAQUE_PARTITION_MODULE_MSG_OPAQUE_NAME),
+ struct ldb_message);
+
+ ldb_module_set_private(module, talloc_steal(module,
+ data));
+ talloc_free(mem_ctx);
+ }
+
/*
* This will lock the metadata partition (sam.ldb) and
* will also call event loops, so we do it before we
diff --git a/source4/dsdb/samdb/ldb_modules/partition_init.c b/source4/dsdb/samdb/ldb_modules/partition_init.c
index 9a6ac0c05a9..9a8bb7e211d 100644
--- a/source4/dsdb/samdb/ldb_modules/partition_init.c
+++ b/source4/dsdb/samdb/ldb_modules/partition_init.c
@@ -863,18 +863,9 @@ int partition_init(struct ldb_module *module)
return ldb_operr(ldb);
}
- data = talloc_zero(mem_ctx, struct partition_private_data);
- if (data == NULL) {
- return ldb_operr(ldb);
- }
-
- /* When used from Samba4, this message is set by the samba4
- * module, as a fixed value not read from the DB. This avoids
- * listing modules in the DB */
- data->forced_module_msg = talloc_get_type(
- ldb_get_opaque(ldb,
- DSDB_OPAQUE_PARTITION_MODULE_MSG_OPAQUE_NAME),
- struct ldb_message);
+ /* We actually got this during the read_lock call */
+ data = talloc_get_type_abort(ldb_module_get_private(module),
+ struct partition_private_data);
/* This loads the partitions */
ret = partition_reload_if_required(module, data, NULL);
diff --git a/source4/dsdb/samdb/ldb_modules/samba_dsdb.c b/source4/dsdb/samdb/ldb_modules/samba_dsdb.c
index 2605c1e511e..54ec6a26068 100644
--- a/source4/dsdb/samdb/ldb_modules/samba_dsdb.c
+++ b/source4/dsdb/samdb/ldb_modules/samba_dsdb.c
@@ -249,7 +249,7 @@ static bool check_required_features(struct ldb_message_element *el)
static int samba_dsdb_init(struct ldb_module *module)
{
struct ldb_context *ldb = ldb_module_get_ctx(module);
- int ret, len, i, j;
+ int ret, lock_ret, len, i, j;
TALLOC_CTX *tmp_ctx = talloc_new(module);
struct ldb_result *res;
struct ldb_message *rootdse_msg = NULL, *partition_msg;
@@ -627,7 +627,20 @@ static int samba_dsdb_init(struct ldb_module *module)
/* Set this as the 'next' module, so that we effectivly append it to module chain */
ldb_module_set_next(module, module_chain);
- return ldb_next_init(module);
+ ret = ldb_next_read_lock(module);
+ if (ret != LDB_SUCCESS) {
+ return ret;
+ }
+
+ ret = ldb_next_init(module);
+
+ lock_ret = ldb_next_read_unlock(module);
+
+ if (lock_ret != LDB_SUCCESS) {
+ return lock_ret;
+ }
+
+ return ret;
}
static const struct ldb_module_ops ldb_samba_dsdb_module_ops = {
diff --git a/source4/dsdb/samdb/ldb_modules/schema_load.c b/source4/dsdb/samdb/ldb_modules/schema_load.c
index cfd88fec467..e977e8b0ad5 100644
--- a/source4/dsdb/samdb/ldb_modules/schema_load.c
+++ b/source4/dsdb/samdb/ldb_modules/schema_load.c
@@ -498,17 +498,11 @@ static int schema_load(struct ldb_context *ldb,
static int schema_load_init(struct ldb_module *module)
{
struct ldb_context *ldb = ldb_module_get_ctx(module);
- struct schema_load_private_data *private_data;
+ struct schema_load_private_data *private_data =
+ talloc_get_type_abort(ldb_module_get_private(module),
+ struct schema_load_private_data);
int ret;
- private_data = talloc_zero(module, struct schema_load_private_data);
- if (private_data == NULL) {
- return ldb_oom(ldb);
- }
- private_data->module = module;
-
- ldb_module_set_private(module, private_data);
-
ret = ldb_next_init(module);
if (ret != LDB_SUCCESS) {
return ret;
@@ -618,7 +612,14 @@ static int schema_read_lock(struct ldb_module *module)
int ret;
if (private_data == NULL) {
- return ldb_next_read_lock(module);
+ private_data = talloc_zero(module, struct schema_load_private_data);
+ if (private_data == NULL) {
+ return ldb_module_oom(module);
+ }
+
+ private_data->module = module;
+
+ ldb_module_set_private(module, private_data);
}
ret = ldb_next_read_lock(module);
@@ -640,11 +641,8 @@ static int schema_read_lock(struct ldb_module *module)
static int schema_read_unlock(struct ldb_module *module)
{
struct schema_load_private_data *private_data =
- talloc_get_type(ldb_module_get_private(module), struct schema_load_private_data);
-
- if (private_data == NULL) {
- return ldb_next_read_unlock(module);
- }
+ talloc_get_type_abort(ldb_module_get_private(module),
+ struct schema_load_private_data);
private_data->in_read_transaction--;
--
2.11.0
From 9d0f2a1c566a235a627524883287e59488e11d3d Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Tue, 10 Apr 2018 13:34:56 +1200
Subject: [PATCH 13/13] dsdb: Use talloc_get_type_abort() in
schema_load_{start,end}_transaction
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
source4/dsdb/samdb/ldb_modules/schema_load.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/schema_load.c b/source4/dsdb/samdb/ldb_modules/schema_load.c
index e977e8b0ad5..7cbbeb699ee 100644
--- a/source4/dsdb/samdb/ldb_modules/schema_load.c
+++ b/source4/dsdb/samdb/ldb_modules/schema_load.c
@@ -514,7 +514,8 @@ static int schema_load_init(struct ldb_module *module)
static int schema_load_start_transaction(struct ldb_module *module)
{
struct schema_load_private_data *private_data =
- talloc_get_type(ldb_module_get_private(module), struct schema_load_private_data);
+ talloc_get_type_abort(ldb_module_get_private(module),
+ struct schema_load_private_data);
struct ldb_context *ldb = ldb_module_get_ctx(module);
struct dsdb_schema *schema;
int ret;
@@ -547,7 +548,8 @@ static int schema_load_start_transaction(struct ldb_module *module)
static int schema_load_end_transaction(struct ldb_module *module)
{
struct schema_load_private_data *private_data =
- talloc_get_type(ldb_module_get_private(module), struct schema_load_private_data);
+ talloc_get_type_abort(ldb_module_get_private(module),
+ struct schema_load_private_data);
struct ldb_context *ldb = ldb_module_get_ctx(module);
if (private_data->in_transaction == 0) {
--
2.11.0
More information about the samba-technical
mailing list