jolly has uploaded this change for review. ( 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
All threads (main and workers) can use slotmap_by_bank() and slotmap_by_client() to get a mapping entry from the slot map. While working with this entry (reading values), it could be deleted by a different thread.
For example, 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.
Change-Id: I3464726f37beb7c47b4e1f00c018ffa4f3948906 --- M src/bankd/bankd_main.c M src/slotmap.c M src/slotmap.h 3 files changed, 46 insertions(+), 19 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-remsim refs/changes/64/42664/1
diff --git a/src/bankd/bankd_main.c b/src/bankd/bankd_main.c index 99a5949..c6b0f1c 100644 --- a/src/bankd/bankd_main.c +++ b/src/bankd/bankd_main.c @@ -162,7 +162,7 @@ { struct bank_slot bs = map->bank;
- slotmap_del(g_bankd->slotmaps, map); + _slotmap_del(g_bankd->slotmaps, map);
/* kill/reset the respective worker, if any! */ send_signal_to_worker(&bs, NULL, SIGMAPDEL); @@ -209,11 +209,13 @@ rspro2client_slot(&cs, &creq->client);
/* check if slot map exists */ - map = slotmap_by_bank(g_bankd->slotmaps, &bs); + slotmaps_rdlock(g_bankd->slotmaps); + map = _slotmap_by_bank(g_bankd->slotmaps, &bs); if (map) { if (client_slot_equals(&map->client, &cs)) { LOGPFSML(srvc->fi, LOGL_ERROR, "ignoring identical slotmap\n"); resp = rspro_gen_CreateMappingRes(ResultCode_ok); + slotmaps_unlock(g_bankd->slotmaps); goto send_resp; } else { LOGPFSML(srvc->fi, LOGL_NOTICE, "slot already connected to client %d:%d. Removing old mapping.\n", @@ -221,6 +223,7 @@ bankd_srvc_remove_mapping(map); } } + slotmaps_unlock(g_bankd->slotmaps);
/* check if client map exists */ map = slotmap_by_client(g_bankd->slotmaps, &cs); @@ -256,7 +259,8 @@ } else { rspro2bank_slot(&bs, &rreq->bank); /* Remove a mapping */ - map = slotmap_by_bank(g_bankd->slotmaps, &bs); + slotmaps_rdlock(g_bankd->slotmaps); + map = _slotmap_by_bank(g_bankd->slotmaps, &bs); if (!map) { LOGPFSML(srvc->fi, LOGL_ERROR, "B(%lu:%lu) could not find to-be-deleted slotmap\n", rreq->bank.bankId, rreq->bank.slotNr); resp = rspro_gen_RemoveMappingRes(ResultCode_unknownSlotmap); @@ -271,6 +275,7 @@ resp = rspro_gen_RemoveMappingRes(ResultCode_ok); } } + slotmaps_unlock(g_bankd->slotmaps); } server_conn_send_rspro(srvc, resp); break; @@ -700,8 +705,10 @@ { struct slot_mapping *slmap;
- slmap = slotmap_by_client(worker->bankd->slotmaps, &worker->client.clslot); + slotmaps_rdlock(worker->bankd->slotmaps); + slmap = _slotmap_by_client(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 +719,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); } @@ -1104,13 +1112,15 @@ g_worker->client.fd = -1; if (g_worker->state >= BW_ST_CONN_CLIENT_MAPPED) { struct slot_mapping *slmap; - slmap = slotmap_by_client(g_worker->bankd->slotmaps, &g_worker->client.clslot); + slotmaps_rdlock(g_worker->bankd->slotmaps); + slmap = _slotmap_by_client(g_worker->bankd->slotmaps, &g_worker->client.clslot); if (slmap) { - slotmap_del(g_bankd->slotmaps, slmap); + _slotmap_del(g_bankd->slotmaps, slmap); g_worker->slot.bank_id = 0xffff; g_worker->slot.slot_nr = 0xffff; worker_set_state(g_worker, BW_ST_CONN_CLIENT_UNMAPPED, true); } + slotmaps_unlock(g_worker->bankd->slotmaps); } g_worker->client.clslot.client_id = g_worker->client.clslot.slot_nr = 0; } diff --git a/src/slotmap.c b/src/slotmap.c index b1be0ab..6e46a5a 100644 --- a/src/slotmap.c +++ b/src/slotmap.c @@ -53,20 +53,40 @@ return (map->bank.bank_id << 16) | map->bank.slot_nr; }
+/* lookup of map by client:slot */ +struct slot_mapping *_slotmap_by_client(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(maps, client); slotmaps_unlock(maps); + return map; +} + +/* lookup of map by bank:slot */ +struct slot_mapping *_slotmap_by_bank(struct slotmaps *maps, const struct bank_slot *bank) +{ + struct slot_mapping *map; + + llist_for_each_entry(map, &maps->mappings, list) { + if (bank_slot_equals(&map->bank, bank)) + return map; + } return NULL; + }
/* thread-safe lookup of map by bank:slot */ @@ -75,14 +95,9 @@ struct slot_mapping *map;
slotmaps_rdlock(maps); - llist_for_each_entry(map, &maps->mappings, list) { - if (bank_slot_equals(&map->bank, bank)) { - slotmaps_unlock(maps); - return map; - } - } + map = _slotmap_by_bank(maps, bank); slotmaps_unlock(maps); - return NULL; + return map;
}
diff --git a/src/slotmap.h b/src/slotmap.h index 3d07c8d..6c78e4e 100644 --- a/src/slotmap.h +++ b/src/slotmap.h @@ -72,9 +72,11 @@
/* 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 *_slotmap_by_client(struct slotmaps *maps, const struct client_slot *client);
/* thread-safe lookup of map by bank:slot */ struct slot_mapping *slotmap_by_bank(struct slotmaps *maps, const struct bank_slot *bank); +struct slot_mapping *_slotmap_by_bank(struct slotmaps *maps, const struct bank_slot *bank);
/* thread-safe creating of a new bank<->client map */ struct slot_mapping *slotmap_add(struct slotmaps *maps, const struct bank_slot *bank, const struct client_slot *client);