pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/32358 )

Change subject: ms: Make ms_{attach,detach}_tbf expectancies more robust ......................................................................

ms: Make ms_{attach,detach}_tbf expectancies more robust

* Make sure that the tbf being attached has already the MS assigned. * Check no re-attaching of alredy attached TBF ever happens. * Document and early skip case where a non-attached TBF detach is attempted. * Avoid recursive call mess to tbf_set_ms(tbf, NULL); during detach. * MsTest needs to be modified since it uses fake TBF objects which use different set of calls than the regular TBFs in osmo-pcu. Since the ul_tbf object is reused, it needs to be assigned ul_tbf->ms again before re-assigning it, as per what happens usually in tbf_set_ms() in osmo-pcu.

Change-Id: Ia18fe2de1fb3bf72f530157e5f30de64f2b11e12 --- M src/gprs_ms.c M tests/ms/MsTest.cpp M tests/ms/MsTest.err 3 files changed, 60 insertions(+), 31 deletions(-)

Approvals: Jenkins Builder: Verified laforge: Looks good to me, but someone else must approve osmith: Looks good to me, but someone else must approve pespin: Looks good to me, approved

diff --git a/src/gprs_ms.c b/src/gprs_ms.c index 357470b..da50426 100644 --- a/src/gprs_ms.c +++ b/src/gprs_ms.c @@ -348,11 +348,30 @@ } }

+/* If a TBF is attached to an MS, it is either in ms->{dl,ul}_tbf or in ms->old_tbfs list */ +static bool ms_tbf_is_attached(const struct GprsMs *ms, const struct gprs_rlcmac_tbf *tbf) +{ + const struct llist_item *pos; + OSMO_ASSERT(ms); + OSMO_ASSERT(tbf); + OSMO_ASSERT(tbf_ms(tbf) == ms); + + if (tbf == ul_tbf_as_tbf_const(ms->ul_tbf)) + return true; + + if (tbf == dl_tbf_as_tbf_const(ms->dl_tbf)) + return true; + + llist_for_each_entry(pos, &ms->old_tbfs, list) { + const struct gprs_rlcmac_tbf *tmp_tbf = (struct gprs_rlcmac_tbf *)pos->entry; + if (tmp_tbf == tbf) + return true; + } + return false; +} + static void ms_attach_ul_tbf(struct GprsMs *ms, struct gprs_rlcmac_ul_tbf *tbf) { - if (ms->ul_tbf == tbf) - return; - LOGPMS(ms, DRLCMAC, LOGL_INFO, "Attaching UL TBF: %s\n", tbf_name((struct gprs_rlcmac_tbf *)tbf));

ms_ref(ms, __func__); @@ -369,9 +388,6 @@

static void ms_attach_dl_tbf(struct GprsMs *ms, struct gprs_rlcmac_dl_tbf *tbf) { - if (ms->dl_tbf == tbf) - return; - LOGPMS(ms, DRLCMAC, LOGL_INFO, "Attaching DL TBF: %s\n", tbf_name((struct gprs_rlcmac_tbf *)tbf));

ms_ref(ms, __func__); @@ -390,6 +406,7 @@ { OSMO_ASSERT(ms); OSMO_ASSERT(tbf); + OSMO_ASSERT(!ms_tbf_is_attached(ms, tbf));

if (tbf_direction(tbf) == GPRS_RLCMAC_DL_TBF) ms_attach_dl_tbf(ms, tbf_as_dl_tbf(tbf)); @@ -399,34 +416,26 @@

void ms_detach_tbf(struct GprsMs *ms, struct gprs_rlcmac_tbf *tbf) { - if (tbf == (struct gprs_rlcmac_tbf *)(ms->ul_tbf)) { + OSMO_ASSERT(tbf_ms(tbf) == ms); + + /* In general this should not happen, but it can happen if during TBF + * allocation something fails before tbf->setup() called ms_attach_tbf(). */ + if (!ms_tbf_is_attached(ms, tbf)) + return; + + if (tbf == ul_tbf_as_tbf(ms->ul_tbf)) { ms->ul_tbf = NULL; - } else if (tbf == (struct gprs_rlcmac_tbf *)(ms->dl_tbf)) { + } else if (tbf == dl_tbf_as_tbf(ms->dl_tbf)) { ms->dl_tbf = NULL; } else { - bool found = false; - - struct llist_item *pos, *tmp; - llist_for_each_entry_safe(pos, tmp, &ms->old_tbfs, list) { - struct gprs_rlcmac_tbf *tmp_tbf = (struct gprs_rlcmac_tbf *)pos->entry; - if (tmp_tbf == tbf) { - llist_del(&pos->list); - found = true; - break; - } - } - - /* Protect against recursive calls via set_ms() */ - if (!found) - return; + /* We know from ms_tbf_is_attached()==true check above that tbf + * is in ms->old_tbfs, no need to look it up again. */ + llist_del(tbf_ms_list(tbf)); }

LOGPMS(ms, DRLCMAC, LOGL_INFO, "Detaching TBF: %s\n", tbf_name(tbf));

- if (tbf_ms(tbf) == ms) - tbf_set_ms(tbf, NULL); - if (!ms->dl_tbf && !ms->ul_tbf) { ms_set_reserved_slots(ms, NULL, 0, 0); ms->first_common_ts = NULL; diff --git a/tests/ms/MsTest.cpp b/tests/ms/MsTest.cpp index a8febd0..40e8381 100644 --- a/tests/ms/MsTest.cpp +++ b/tests/ms/MsTest.cpp @@ -443,7 +443,7 @@ OSMO_ASSERT(ms != NULL); ul_tbf = alloc_ul_tbf(bts, ms); ms_attach_tbf(ms, ul_tbf); - ms_detach_tbf(ms, ul_tbf); + tbf_set_ms(ul_tbf, NULL); ms = bts_get_ms_by_tlli(bts, tlli + 0, GSM_RESERVED_TMSI); OSMO_ASSERT(ms == NULL); ms = bts_get_ms_by_tlli(bts, tlli + 1, GSM_RESERVED_TMSI); @@ -452,8 +452,8 @@ /* delete ms */ ms = bts_get_ms_by_tlli(bts, tlli + 1, GSM_RESERVED_TMSI); OSMO_ASSERT(ms != NULL); - ms_attach_tbf(ms, ul_tbf); - ms_detach_tbf(ms, ul_tbf); + tbf_set_ms(ul_tbf, ms); + tbf_set_ms(ul_tbf, NULL); ms = bts_get_ms_by_tlli(bts, tlli + 1, GSM_RESERVED_TMSI); OSMO_ASSERT(ms == NULL);

diff --git a/tests/ms/MsTest.err b/tests/ms/MsTest.err index 3d271ae..3f2f520 100644 --- a/tests/ms/MsTest.err +++ b/tests/ms/MsTest.err @@ -63,10 +63,10 @@ MS(IMSI-001001987654321:TLLI-0xffeeddbb:TA-220:MSCLS-0-0:UL): - ms_attach_ul_tbf: now used by 0 (-) MS(IMSI-001001987654321:TLLI-0xffeeddbb:TA-220:MSCLS-0-0) Detaching TBF: TBF(UL:STATE-NEW:GPRS:IMSI-001001987654321:TLLI-0xffeeddbb) MS(IMSI-001001987654321:TLLI-0xffeeddbb:TA-220:MSCLS-0-0) Destroying MS object -MS(IMSI-001001987654322:TLLI-0xffeeddbc:TA-220:MSCLS-0-0) Attaching UL TBF: TBF(UL:STATE-NEW:GPRS) +MS(IMSI-001001987654322:TLLI-0xffeeddbc:TA-220:MSCLS-0-0) Attaching UL TBF: TBF(UL:STATE-NEW:GPRS:IMSI-001001987654322:TLLI-0xffeeddbc) MS(IMSI-001001987654322:TLLI-0xffeeddbc:TA-220:MSCLS-0-0): + ms_attach_ul_tbf: now used by 1 (ms_attach_ul_tbf) MS(IMSI-001001987654322:TLLI-0xffeeddbc:TA-220:MSCLS-0-0:UL): - ms_attach_ul_tbf: now used by 0 (-) -MS(IMSI-001001987654322:TLLI-0xffeeddbc:TA-220:MSCLS-0-0) Detaching TBF: TBF(UL:STATE-NEW:GPRS) +MS(IMSI-001001987654322:TLLI-0xffeeddbc:TA-220:MSCLS-0-0) Detaching TBF: TBF(UL:STATE-NEW:GPRS:IMSI-001001987654322:TLLI-0xffeeddbc) MS(IMSI-001001987654322:TLLI-0xffeeddbc:TA-220:MSCLS-0-0) Destroying MS object Creating MS object Modifying MS object, UL TLLI: 0xffffffff -> 0xffeeddbb, not yet confirmed