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);
--
To view, visit https://gerrit.osmocom.org/c/osmo-remsim/+/42664?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: osmo-remsim
Gerrit-Branch: master
Gerrit-Change-Id: I3464726f37beb7c47b4e1f00c018ffa4f3948906
Gerrit-Change-Number: 42664
Gerrit-PatchSet: 1
Gerrit-Owner: jolly <[email protected]>
