Change in osmo-bts[master]: rsl: prevent race condition during timeslot re-configuration

[laforge]

laforge has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/25674 )

Change subject: rsl: prevent race condition during timeslot re-configuration
......................................................................

rsl: prevent race condition during timeslot re-configuration

It may happen that the BSC requests logical channel activation on a
dynamic timeslot, which is in a process of switching from one pchan
type to another due to a preceding channel activation request.

In this case 'struct gsm_bts_trx_ts' already holds an msgb with the
preceding RSL CHANnel ACTIVation message, that is normally handled
once the PHY completes the process of timeslot re-configuration.

On receipt of subsequent RSL CHANnel ACTIVation messages, in function
dyn_ts_l1_reconnect() we overwrite the preceeding msgb (memleak), by
the most recent one.  And once the timeslot re-configuration is done,
only the most recent CHANnel ACTIVation message gets ACKed.

In order to avoid this, let's move the msgb ownership to 'struct
gsm_lchan', so it cannot be overwritten by the CHANnel ACTIVation
message that is related to a different lchan on the same timeslot.

Change-Id: Ia625c2827fca883ea712076706d5ef21ed793ba6
Related: I3b602ac9dbe0ab3e80eb30de573c9b48a79872d8
Fixes: OS#5245
---
M include/osmo-bts/gsm_data.h
M include/osmo-bts/lchan.h
M src/common/rsl.c
3 files changed, 22 insertions(+), 19 deletions(-)

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



diff --git a/include/osmo-bts/gsm_data.h b/include/osmo-bts/gsm_data.h
index ad0f78d..dcb357f 100644
--- a/include/osmo-bts/gsm_data.h
+++ b/include/osmo-bts/gsm_data.h
@@ -83,7 +83,6 @@
 	struct {
 		enum gsm_phys_chan_config pchan_is;
 		enum gsm_phys_chan_config pchan_want;
-		struct msgb *pending_chan_activ;
 	} dyn;
 
 	unsigned int flags;
diff --git a/include/osmo-bts/lchan.h b/include/osmo-bts/lchan.h
index 4cf957a..fdb3144 100644
--- a/include/osmo-bts/lchan.h
+++ b/include/osmo-bts/lchan.h
@@ -278,6 +278,8 @@
 	int s;
 	/* Kind of the release/activation. E.g. RSL or PCU */
 	enum lchan_rel_act_kind rel_act_kind;
+	/* Pending RSL CHANnel ACTIVation message */
+	struct msgb *pending_chan_activ;
 	/* RTP header Marker bit to indicate beginning of speech after pause  */
 	bool rtp_tx_marker;
 
diff --git a/src/common/rsl.c b/src/common/rsl.c
index f03d510..229a2af 100644
--- a/src/common/rsl.c
+++ b/src/common/rsl.c
@@ -1523,7 +1523,7 @@
  * Store the CHAN_ACTIV msg, connect the L1 timeslot in the proper type and
  * then invoke rsl_rx_chan_activ() with msg.
  */
-static int dyn_ts_l1_reconnect(struct gsm_bts_trx_ts *ts, struct msgb *msg)
+static int dyn_ts_l1_reconnect(struct gsm_bts_trx_ts *ts)
 {
 	DEBUGP(DRSL, "%s dyn_ts_l1_reconnect\n", gsm_ts_and_pchan_name(ts));
 
@@ -1544,9 +1544,6 @@
 		return -EINVAL;
 	}
 
-	/* We will feed this back to rsl_rx_chan_activ() later */
-	ts->dyn.pending_chan_activ = msg;
-
 	/* Disconnect, continue connecting from cb_ts_disconnected(). */
 	DEBUGP(DRSL, "%s Disconnect\n", gsm_ts_and_pchan_name(ts));
 	return bts_model_ts_disconnect(ts);
@@ -1652,9 +1649,12 @@
 			 * mode than this activation needs it to be.
 			 * Re-connect, then come back to rsl_rx_chan_activ().
 			 */
-			rc = dyn_ts_l1_reconnect(ts, msg);
+			rc = dyn_ts_l1_reconnect(ts);
 			if (rc)
 				return rsl_tx_chan_act_nack(lchan, RSL_ERR_NORMAL_UNSPEC);
+			/* will be fed back to rsl_rx_chan_activ() later */
+			OSMO_ASSERT(lchan->pending_chan_activ == NULL);
+			lchan->pending_chan_activ = msg;
 			/* indicate that the msgb should not be freed. */
 			return 1;
 		}
@@ -3181,8 +3181,7 @@
 
 static void osmo_dyn_ts_connected(struct gsm_bts_trx_ts *ts, int rc)
 {
-	struct msgb *msg = ts->dyn.pending_chan_activ;
-	ts->dyn.pending_chan_activ = NULL;
+	unsigned int ln;
 
 	if (rc) {
 		LOGP(DRSL, LOGL_NOTICE, "%s PDCH ACT OSMO operation failed (%d) in bts model\n",
@@ -3191,20 +3190,23 @@
 		return;
 	}
 
-	if (!msg) {
-		LOGP(DRSL, LOGL_ERROR,
-		     "%s TS re-connected, but no chan activ msg pending\n",
-		     gsm_ts_and_pchan_name(ts));
-		return;
-	}
-
 	ts->dyn.pchan_is = ts->dyn.pchan_want;
 	DEBUGP(DRSL, "%s Connected\n", gsm_ts_and_pchan_name(ts));
 
-	/* continue where we left off before re-connecting the TS. */
-	rc = rsl_rx_chan_activ(msg);
-	if (rc != 1)
-		msgb_free(msg);
+	/* Handle postponed RSL CHANnel ACTIVation messages (if any) */
+	for (ln = 0; ln < ARRAY_SIZE(ts->lchan); ln++) {
+		struct gsm_lchan *lchan = &ts->lchan[ln];
+
+		if (lchan->pending_chan_activ == NULL)
+			continue;
+
+		struct msgb *msg = lchan->pending_chan_activ;
+		lchan->pending_chan_activ = NULL;
+
+		/* Continue where we left off before re-connecting the TS */
+		if (rsl_rx_chan_activ(msg) != 1)
+			msgb_free(msg);
+	}
 }
 
 void cb_ts_connected(struct gsm_bts_trx_ts *ts, int rc)

-- 
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/25674
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: Ia625c2827fca883ea712076706d5ef21ed793ba6
Gerrit-Change-Number: 25674
Gerrit-PatchSet: 5
Gerrit-Owner: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-CC: neels <nhofmeyr at sysmocom.de>
Gerrit-CC: osmith <osmith at sysmocom.de>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20211008/bf670ec3/attachment.htm>