[PATCH] Patch for bug 13802
Ralph Böhme
slow at samba.org
Fri Feb 22 17:05:02 UTC 2019
Hi Volker,
as discussed, attached is an updated patchset with improved commit messages.
I added an additional commit that makes the xids argument to wb_xids2sids_send()
const.
In case anyone else is wathching, this patchset addresses an issue in idmapping
in winbindd.
https://bugzilla.samba.org/show_bug.cgi?id=13802
Under certain, yet not fully clear conditions, the bug can cause broken POSIX id
to SID mapping in the UNIX range S-1-22. It can also cause xid2sid idmap
requests to churn the idmap cache and all such requests end up going through the
idmap backend, in the case of idmap_ad with potentially devastating results on
large and busy system.
CI: https://gitlab.com/samba-team/devel/samba/pipelines/48734322
That one is missing the const commit.
CI matching attached patchset is still running:
https://gitlab.com/samba-team/devel/samba/pipelines/48782761
Please review&push if happy. Thanks!
-slow
--
Ralph Boehme, Samba Team https://samba.org/
Samba Developer, SerNet GmbH https://sernet.de/en/samba/
GPG-Fingerprint FAE2C6088A24252051C559E4AA1E9B7126399E46
-------------- next part --------------
From 7fafdb301286b87ce5405c87c643a5a2e87086b7 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Thu, 21 Feb 2019 18:34:51 +0100
Subject: [PATCH 1/7] winbindd: make a copy of xid's in wb_xids2sids_send()
This is in preparation of setting the result of the mapping in the top-
level callback wb_xids2sids_done(), not in the per-idmap-domain callback
wb_xids2sids_dom_done().
When caching the mapping we need the id-type from the backend, so we
need a way to pass up that information from wb_xids2sids_dom_done() up
to wb_xids2sids_done()
The xids array copy gets passed from wb_xids2sids_send() to
wb_xids2sids_dom_send(), so wb_xids2sids_dom_done() can then directly
update the top-level copy.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13802
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/winbindd/wb_xids2sids.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/source3/winbindd/wb_xids2sids.c b/source3/winbindd/wb_xids2sids.c
index 310a645cdff..38e581eee8d 100644
--- a/source3/winbindd/wb_xids2sids.c
+++ b/source3/winbindd/wb_xids2sids.c
@@ -452,9 +452,14 @@ struct tevent_req *wb_xids2sids_send(TALLOC_CTX *mem_ctx,
return NULL;
}
state->ev = ev;
- state->xids = xids;
state->num_xids = num_xids;
+ state->xids = talloc_array(state, struct unixid, num_xids);
+ if (tevent_req_nomem(state->xids, req)) {
+ return tevent_req_post(req, ev);
+ }
+ memcpy(state->xids, xids, num_xids * sizeof(struct unixid));
+
state->sids = talloc_zero_array(state, struct dom_sid, num_xids);
if (tevent_req_nomem(state->sids, req)) {
return tevent_req_post(req, ev);
--
2.17.2
From ed769dd3f91ab096ce5a7a053c6cfd1a2b24adcf Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 22 Feb 2019 16:29:07 +0100
Subject: [PATCH 2/7] winbindd: make xids a const argument to
wb_xids2sids_send()
The previous commit made an internal copy of xids, this commit makes it
more obivious that we musn't mess with the xids argument but treat it as
an in-parameter and don't write to it.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13802
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/winbindd/wb_xids2sids.c | 2 +-
source3/winbindd/winbindd_proto.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/source3/winbindd/wb_xids2sids.c b/source3/winbindd/wb_xids2sids.c
index 38e581eee8d..891a4c2a4bd 100644
--- a/source3/winbindd/wb_xids2sids.c
+++ b/source3/winbindd/wb_xids2sids.c
@@ -440,7 +440,7 @@ static void wb_xids2sids_init_dom_maps_done(struct tevent_req *subreq);
struct tevent_req *wb_xids2sids_send(TALLOC_CTX *mem_ctx,
struct tevent_context *ev,
- struct unixid *xids,
+ const struct unixid *xids,
uint32_t num_xids)
{
struct tevent_req *req, *subreq;
diff --git a/source3/winbindd/winbindd_proto.h b/source3/winbindd/winbindd_proto.h
index c524d2050df..2a829b0171a 100644
--- a/source3/winbindd/winbindd_proto.h
+++ b/source3/winbindd/winbindd_proto.h
@@ -919,7 +919,7 @@ NTSTATUS winbindd_sids_to_xids_recv(struct tevent_req *req,
struct winbindd_response *response);
struct tevent_req *wb_xids2sids_send(TALLOC_CTX *mem_ctx,
struct tevent_context *ev,
- struct unixid *xids,
+ const struct unixid *xids,
uint32_t num_xids);
NTSTATUS wb_xids2sids_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx,
struct dom_sid **sids);
--
2.17.2
From a2feefa9ce412b6086db42af48c8d78cdfda8849 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Thu, 21 Feb 2019 18:39:46 +0100
Subject: [PATCH 3/7] winbindd: convert id to a pointer in
wb_xids2sids_dom_done()
No change in behaviour.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13802
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/winbindd/wb_xids2sids.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/source3/winbindd/wb_xids2sids.c b/source3/winbindd/wb_xids2sids.c
index 891a4c2a4bd..3c72d9447a2 100644
--- a/source3/winbindd/wb_xids2sids.c
+++ b/source3/winbindd/wb_xids2sids.c
@@ -357,9 +357,9 @@ static void wb_xids2sids_dom_done(struct tevent_req *subreq)
dom_sid_idx = 0;
for (i=0; i<state->num_all_xids; i++) {
- struct unixid id = state->all_xids[i];
+ struct unixid *id = &state->all_xids[i];
- if ((id.id < dom_map->low_id) || (id.id > dom_map->high_id)) {
+ if ((id->id < dom_map->low_id) || (id->id > dom_map->high_id)) {
/* out of range */
continue;
}
--
2.17.2
From 685cf952f4af0c170d9f6866cf2d1a7d9c3ccef9 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Thu, 21 Feb 2019 18:40:20 +0100
Subject: [PATCH 4/7] winbindd: update xid in wb_xids2sids_state->xids with
what we got
In preparation of priming the idmap cache in the top-level
wb_xids2sids_done(), not in the per-idmap-domain callback
wb_xids2sids_dom_done().
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13802
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/winbindd/wb_xids2sids.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/source3/winbindd/wb_xids2sids.c b/source3/winbindd/wb_xids2sids.c
index 3c72d9447a2..c088560adec 100644
--- a/source3/winbindd/wb_xids2sids.c
+++ b/source3/winbindd/wb_xids2sids.c
@@ -369,6 +369,7 @@ static void wb_xids2sids_dom_done(struct tevent_req *subreq)
}
sid_copy(&state->all_sids[i], &state->dom_sids[dom_sid_idx]);
+ *id = state->dom_xids[dom_sid_idx];
/*
* Prime the cache after an xid2sid call. It's
--
2.17.2
From 827ea08c7963133a0b67768e63cd211a38c29579 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Thu, 21 Feb 2019 16:52:21 +0100
Subject: [PATCH 5/7] winbindd: switch send-next/done order
In preparation of adding more logic to the done step. No change in
behaviour.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13802
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/winbindd/wb_xids2sids.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/source3/winbindd/wb_xids2sids.c b/source3/winbindd/wb_xids2sids.c
index c088560adec..331a04c4fb0 100644
--- a/source3/winbindd/wb_xids2sids.c
+++ b/source3/winbindd/wb_xids2sids.c
@@ -554,18 +554,22 @@ static void wb_xids2sids_done(struct tevent_req *subreq)
state->dom_idx += 1;
- if (state->dom_idx >= num_domains) {
- tevent_req_done(req);
+ if (state->dom_idx < num_domains) {
+ subreq = wb_xids2sids_dom_send(state,
+ state->ev,
+ &dom_maps[state->dom_idx],
+ state->xids,
+ state->num_xids,
+ state->sids);
+ if (tevent_req_nomem(subreq, req)) {
+ return;
+ }
+ tevent_req_set_callback(subreq, wb_xids2sids_done, req);
return;
}
- subreq = wb_xids2sids_dom_send(
- state, state->ev, &dom_maps[state->dom_idx],
- state->xids, state->num_xids, state->sids);
- if (tevent_req_nomem(subreq, req)) {
- return;
- }
- tevent_req_set_callback(subreq, wb_xids2sids_done, req);
+ tevent_req_done(req);
+ return;
}
NTSTATUS wb_xids2sids_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx,
--
2.17.2
From a1a1043b2e555f55fbb73208ea6e6b7430f579b2 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 22 Feb 2019 11:00:00 +0100
Subject: [PATCH 6/7] winbindd: track whether a result from xid2sid was coming
from the cache
This is needed in perparation of moving the step to update the idmap
cache from the per-idmap-domain callback wb_xids2sids_dom_done() to the
top-level callback wb_xids2sids_done().
Currently the sequence of action is:
* check cache, if not found:
* ask backends
* cache result from backend
* return results
Iow, if we got something from the cache, we don't write the cache.
The next commit defers updating the cache to the top-level callback, so
the sequence becomes
* check cache, if not found:
* ask backends
* cache results
* return results
This has two problems:
* it needlessly writes to the cache what we just got from it
* it possibly overwrites the ID_TYPE_BOTH for a SID-to-xid mapping in
the following case:
- existing ID_TYPE_BOTH mapping in the cache, eg:
IDMAP/SID2XID/S-1-5-21-2180672342-2513613279-2566592647-512 -> Value: 3000000:B
- someone calls wb_xids2sids_send() with xid.id=3000000,xid.type=ID_TYPE_GID
- cache lookup with idmap_cache_find_gid2sid() succeeds
- when caching results we'd call idmap_cache_set_sid2unixid() with the
callers xid.type=ID_TYPE_GID, so idmap_cache_set_sid2unixid() will
overwrite the SID-to-xid mapping with ID_TYPE_GID
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13802
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/winbindd/wb_xids2sids.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/source3/winbindd/wb_xids2sids.c b/source3/winbindd/wb_xids2sids.c
index 331a04c4fb0..9dd8f22da26 100644
--- a/source3/winbindd/wb_xids2sids.c
+++ b/source3/winbindd/wb_xids2sids.c
@@ -432,6 +432,7 @@ struct wb_xids2sids_state {
struct unixid *xids;
size_t num_xids;
struct dom_sid *sids;
+ bool *cached;
size_t dom_idx;
};
@@ -466,6 +467,11 @@ struct tevent_req *wb_xids2sids_send(TALLOC_CTX *mem_ctx,
return tevent_req_post(req, ev);
}
+ state->cached = talloc_zero_array(state, bool, num_xids);
+ if (tevent_req_nomem(state->cached, req)) {
+ return tevent_req_post(req, ev);
+ }
+
if (winbindd_use_idmap_cache()) {
uint32_t i;
@@ -493,6 +499,7 @@ struct tevent_req *wb_xids2sids_send(TALLOC_CTX *mem_ctx,
dom_sid_str_buf(&sid, &buf));
sid_copy(&state->sids[i], &sid);
+ state->cached[i] = true;
}
}
}
--
2.17.2
From 184dec342402664e660bffcf2277476efccfa131 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Thu, 21 Feb 2019 16:55:09 +0100
Subject: [PATCH 7/7] winbindd: set idmap cache entries as the last step in
async wb_xids2sids
Bug: https://bugzilla.samba.org/show_bug.cgi?id=13802
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/winbindd/wb_xids2sids.c | 33 ++++++++++++++++++++++-----------
1 file changed, 22 insertions(+), 11 deletions(-)
diff --git a/source3/winbindd/wb_xids2sids.c b/source3/winbindd/wb_xids2sids.c
index 9dd8f22da26..4f2a73deadf 100644
--- a/source3/winbindd/wb_xids2sids.c
+++ b/source3/winbindd/wb_xids2sids.c
@@ -371,17 +371,6 @@ static void wb_xids2sids_dom_done(struct tevent_req *subreq)
sid_copy(&state->all_sids[i], &state->dom_sids[dom_sid_idx]);
*id = state->dom_xids[dom_sid_idx];
- /*
- * Prime the cache after an xid2sid call. It's
- * important that we use state->dom_xids for the xid
- * value, not state->all_xids: state->all_xids carries
- * what we asked for, e.g. a
- * ID_TYPE_UID. state->dom_xids holds something the
- * idmap child possibly changed to ID_TYPE_BOTH.
- */
- idmap_cache_set_sid2unixid(
- &state->all_sids[i], &state->dom_xids[dom_sid_idx]);
-
dom_sid_idx += 1;
}
@@ -551,6 +540,7 @@ static void wb_xids2sids_done(struct tevent_req *subreq)
struct wb_xids2sids_state *state = tevent_req_data(
req, struct wb_xids2sids_state);
size_t num_domains = talloc_array_length(dom_maps);
+ int i;
NTSTATUS status;
status = wb_xids2sids_dom_recv(subreq);
@@ -575,6 +565,27 @@ static void wb_xids2sids_done(struct tevent_req *subreq)
return;
}
+
+ for (i = 0; i < state->num_xids; i++) {
+ /*
+ * Prime the cache after an xid2sid call. It's important that we
+ * use the xid value returned from the backend for the xid value
+ * passed to idmap_cache_set_sid2unixid(), not the input to
+ * wb_xids2sids_send: the input carries what was asked for,
+ * e.g. a ID_TYPE_UID. The result from the backend something the
+ * idmap child possibly changed to ID_TYPE_BOTH.
+ *
+ * And of course If the value was from the cache don't update
+ * the cache.
+ */
+
+ if (state->cached[i]) {
+ continue;
+ }
+
+ idmap_cache_set_sid2unixid(&state->sids[i], &state->xids[i]);
+ }
+
tevent_req_done(req);
return;
}
--
2.17.2
More information about the samba-technical
mailing list