laforge has submitted this change. ( https://gerrit.osmocom.org/c/osmo-remsim/+/42664?usp=email )
Change subject: Fix: Prevent race conditions when accessing slotmap in bankd ......................................................................
Fix: Prevent race conditions when accessing slotmap in bankd
If a client connects to a worker, the worker will check if there is an existing mapping between a reader and this client. If it exists, slotmap_by_client() will return a pointer. If the mapping is deleted at this time by the server, the worker uses a pointer to a mapping entry that has just been freed.
To prevent this, the worker locks the slot map and calls the new function slotmap_by_client_nolock(). After it has finished working with the returned pointer, it unlocks the slot map again. A possible delete by the main thread would be delayed.
Change-Id: I3464726f37beb7c47b4e1f00c018ffa4f3948906 --- M src/bankd/bankd_main.c M src/slotmap.c M src/slotmap.h 3 files changed, 21 insertions(+), 9 deletions(-)
Approvals: Jenkins Builder: Verified lynxis lazus: Looks good to me, approved
diff --git a/src/bankd/bankd_main.c b/src/bankd/bankd_main.c index 479adf0..fc8f480 100644 --- a/src/bankd/bankd_main.c +++ b/src/bankd/bankd_main.c @@ -700,8 +700,10 @@ { struct slot_mapping *slmap;
- slmap = slotmap_by_client(worker->bankd->slotmaps, &worker->client.clslot); + slotmaps_rdlock(worker->bankd->slotmaps); + slmap = slotmap_by_client_nolock(worker->bankd->slotmaps, &worker->client.clslot); if (!slmap) { + slotmaps_unlock(worker->bankd->slotmaps); LOGW(worker, "No slotmap (yet) for client C(%u:%u)\n", worker->client.clslot.client_id, worker->client.clslot.slot_nr); /* check in 10s if the map has been installed meanwhile by main thread */ @@ -712,6 +714,7 @@ slmap->client.client_id, slmap->client.slot_nr, slmap->bank.bank_id, slmap->bank.slot_nr); worker->slot = slmap->bank; + slotmaps_unlock(worker->bankd->slotmaps); worker_set_state_timeout(worker, BW_ST_CONN_CLIENT_MAPPED, 10); return worker_open_card(worker); } diff --git a/src/slotmap.c b/src/slotmap.c index b1be0ab..1883cf5 100644 --- a/src/slotmap.c +++ b/src/slotmap.c @@ -53,20 +53,27 @@ return (map->bank.bank_id << 16) | map->bank.slot_nr; }
+/* lookup of map by client:slot */ +struct slot_mapping *slotmap_by_client_nolock(struct slotmaps *maps, const struct client_slot *client) +{ + struct slot_mapping *map; + + llist_for_each_entry(map, &maps->mappings, list) { + if (client_slot_equals(&map->client, client)) + return map; + } + return NULL; +} + /* thread-safe lookup of map by client:slot */ struct slot_mapping *slotmap_by_client(struct slotmaps *maps, const struct client_slot *client) { struct slot_mapping *map;
slotmaps_rdlock(maps); - llist_for_each_entry(map, &maps->mappings, list) { - if (client_slot_equals(&map->client, client)) { - slotmaps_unlock(maps); - return map; - } - } + map = slotmap_by_client_nolock(maps, client); slotmaps_unlock(maps); - return NULL; + return map; }
/* thread-safe lookup of map by bank:slot */ @@ -83,7 +90,6 @@ } slotmaps_unlock(maps); return NULL; - }
/* thread-safe creating of a new bank<->client map */ diff --git a/src/slotmap.h b/src/slotmap.h index 3d07c8d..a14e1cf 100644 --- a/src/slotmap.h +++ b/src/slotmap.h @@ -70,6 +70,9 @@
uint32_t slotmap_get_id(const struct slot_mapping *map);
+/* lookup of map by client:slot */ +struct slot_mapping *slotmap_by_client_nolock(struct slotmaps *maps, const struct client_slot *client); + /* thread-safe lookup of map by client:slot */ struct slot_mapping *slotmap_by_client(struct slotmaps *maps, const struct client_slot *client);