s3:winbind: Avoid unnecessary locking in wb_parent_idmap_setup_send()

A function in tevent environment can span over several context loop iterations.
Every iteration 'unschedules' the current code and a different functions can
access not yet fully initialized structures.

A locking is used to avoid this. In tevent, we use tevent queues as a locking
mechanism. Every function trying to access lock protected data, puts itself to
a queue. The function must remove itself from the queue only after the complete
work is done.

A good coding practise is to lock only the smallest code path and not to use the
locking if not needed.

wb_parent_idmap_setup_send() uses queue "wb_parent_idmap_config_queue" for:
- testing if the setup is ready
- setting up all idmap domains

But "testing if the setup is ready" can be coded as an atomic operation without
needing a lock.

Signed-off-by: Pavel Filipenský <pfilipen@redhat.com>
Reviewed-by: Andreas Schneider <asn@samba.org>
Autobuild-User(master): Andreas Schneider <asn@cryptomilk.org>
Autobuild-Date(master): Fri Nov  4 10:06:28 UTC 2022 on sn-devel-184

diff --git a/source3/winbindd/winbindd.h b/source3/winbindd/winbindd.h
index 093a9fad11e1af741b308fd3956b69e00cba7997..5044fee0c68344c734d3d3026eb4d7f148a5fa4a 100644 (file)
--- a/source3/winbindd/winbindd.h
+++ b/source3/winbindd/winbindd.h
@@ -191,6 +191,7 @@ struct wb_parent_idmap_config_dom {
 struct wb_parent_idmap_config {
        struct tevent_queue *queue;
        uint32_t num_doms;
+       bool initialized;
        struct wb_parent_idmap_config_dom *doms;
 };
 

diff --git a/source3/winbindd/winbindd_idmap.c b/source3/winbindd/winbindd_idmap.c
index e6d2c284cf201129c4be764a386a7389ff1a65a9..75f7d37d86d0ad940f2c8454d9fcd35067de8c1a 100644 (file)
--- a/source3/winbindd/winbindd_idmap.c
+++ b/source3/winbindd/winbindd_idmap.c
@@ -131,6 +131,7 @@ static void wb_parent_idmap_setup_cleanup(struct tevent_req *req,
        }
 
        state->cfg->num_doms = 0;
+       state->cfg->initialized = false;
        TALLOC_FREE(state->cfg->doms);
        state->cfg = NULL;
 }
@@ -159,6 +160,11 @@ struct tevent_req *wb_parent_idmap_setup_send(TALLOC_CTX *mem_ctx,
                .dom_idx = 0,
        };
 
+       if (state->cfg->initialized) {
+               tevent_req_done(req);
+               return tevent_req_post(req, ev);
+       }
+
        if (state->cfg->queue == NULL) {
                state->cfg->queue = tevent_queue_create(NULL,
                                                "wb_parent_idmap_config_queue");
@@ -330,6 +336,7 @@ static void wb_parent_idmap_setup_lookupname_next(struct tevent_req *req)
                 * We're done, so start the idmap child
                 */
                setup_child(NULL, &static_idmap_child, "log.winbindd", "idmap");
+               static_parent_idmap_config.initialized = true;
                tevent_req_done(req);
                return;
        }