Change in osmo-bsc[master]: bts_nokia_site: Fix LAPD segfault during reset procedure

[tnt]

tnt has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/18009 )

Change subject: bts_nokia_site: Fix LAPD segfault during reset procedure
......................................................................

bts_nokia_site: Fix LAPD segfault during reset procedure

The existing Nokia *Site code destroyed the LAPD SAP instance for OML
while processing an OML message.  Once the stack frame returned back
to the LAPD code, the LAPD SAP was gone -> segfault.

Let's work around this by moving deletion of the LAPD SAP out-of-line
by starting a timer 0ms in the future.  Not particularly nice, but
effective.

Change-Id: I6270c7210f600e53f845561898245d2fd30a368d
Closes: OS#1761
---
M include/osmocom/bsc/gsm_data.h
M src/osmo-bsc/bts_nokia_site.c
2 files changed, 32 insertions(+), 25 deletions(-)

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



diff --git a/include/osmocom/bsc/gsm_data.h b/include/osmocom/bsc/gsm_data.h
index 6996905..1d02e39 100644
--- a/include/osmocom/bsc/gsm_data.h
+++ b/include/osmocom/bsc/gsm_data.h
@@ -1143,7 +1143,7 @@
 				no_loc_rel_cnf:1,	/* don't wait for RSL REL CONF */
 				bts_reset_timer_cnf,	/* timer for BTS RESET */
 				did_reset:1,		/* we received a RESET ACK */
-				wait_reset:1;		/* we are waiting for reset to complete */
+				wait_reset:2;		/* we are waiting for reset to complete */
 			struct osmo_timer_list reset_timer;
 		} nokia;
 	};
diff --git a/src/osmo-bsc/bts_nokia_site.c b/src/osmo-bsc/bts_nokia_site.c
index 66972c2..cde0afa 100644
--- a/src/osmo-bsc/bts_nokia_site.c
+++ b/src/osmo-bsc/bts_nokia_site.c
@@ -41,6 +41,12 @@
 
 #include <osmocom/abis/lapd.h>
 
+enum reset_timer_state {
+	RESET_T_NONE = 0,
+	RESET_T_STOP_LAPD = 1,		/* first timer expiration: stop LAPD SAP */
+	RESET_T_RESTART_LAPD = 2,	/* second timer expiration: restart LAPD SAP */
+};
+
 /* TODO: put in a separate file ? */
 
 extern int abis_nm_sendmsg(struct gsm_bts *bts, struct msgb *msg);
@@ -1461,18 +1467,28 @@
 	struct gsm_e1_subslot *e1_link = &bts->oml_e1_link;
 	struct e1inp_line *line;
 
-	bts->nokia.wait_reset = 0;
-
 	/* OML link */
 	line = e1inp_line_find(e1_link->e1_nr);
 	if (!line) {
-		LOGP(DLINP, LOGL_ERROR, "BTS %u OML link referring to "
-		     "non-existing E1 line %u\n", bts->nr, e1_link->e1_nr);
+		LOGP(DLINP, LOGL_ERROR, "BTS %u OML link referring to non-existing E1 line %u\n",
+		     bts->nr, e1_link->e1_nr);
 		return;
 	}
 
-	start_sabm_in_line(line, 0, -1);	/* stop all first */
-	start_sabm_in_line(line, 1, SAPI_OML);	/* start only OML */
+	switch (bts->nokia.wait_reset) {
+	case RESET_T_NONE:				/* shouldn't happen */
+		break;
+	case RESET_T_STOP_LAPD:
+		start_sabm_in_line(line, 0, -1);	/* stop all first */
+		bts->nokia.wait_reset = RESET_T_RESTART_LAPD;
+		osmo_timer_schedule(&bts->nokia.reset_timer, bts->nokia.bts_reset_timer_cnf, 0);
+		break;
+	case RESET_T_RESTART_LAPD:
+		bts->nokia.wait_reset = 0;
+		start_sabm_in_line(line, 0, -1);	/* stop all first */
+		start_sabm_in_line(line, 1, SAPI_OML);	/* start only OML */
+		break;
+	}
 }
 
 /* TODO: put in a separate file ? */
@@ -1574,25 +1590,16 @@
 			   (function handle_ts1_read()) and ignoring the received data.
 			   It seems to be necessary for the MetroSite too.
 			 */
-			bts->nokia.wait_reset = 1;
 
-			osmo_timer_setup(&bts->nokia.reset_timer,
-					 reset_timer_cb, bts);
-			osmo_timer_schedule(&bts->nokia.reset_timer, bts->nokia.bts_reset_timer_cnf, 0);
-
-			struct gsm_e1_subslot *e1_link = &bts->oml_e1_link;
-			struct e1inp_line *line;
-			/* OML link */
-			line = e1inp_line_find(e1_link->e1_nr);
-			if (!line) {
-				LOGP(DLINP, LOGL_ERROR,
-				     "BTS %u OML link referring to "
-				     "non-existing E1 line %u\n", bts->nr,
-				     e1_link->e1_nr);
-				return -ENOMEM;
-			}
-
-			start_sabm_in_line(line, 0, -1);	/* stop all first */
+			/* we cannot delete / stop the OML LAPD SAP right here, as we are in
+			 * the middle of processing an LAPD I frame and are subsequently returning
+			 * back to the LAPD I frame processing code that assumes the SAP is still
+			 * active. So we first schedule the timer at 0ms in the future, where we
+			 * kill all LAPD SAP and re-arm the timer for the reset duration, after which
+			 * we re-create them */
+			bts->nokia.wait_reset = RESET_T_STOP_LAPD;
+			osmo_timer_setup(&bts->nokia.reset_timer, reset_timer_cb, bts);
+			osmo_timer_schedule(&bts->nokia.reset_timer, 0, 0);
 		}
 
 		/* ACK for CONF DATA message ? */

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

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I6270c7210f600e53f845561898245d2fd30a368d
Gerrit-Change-Number: 18009
Gerrit-PatchSet: 2
Gerrit-Owner: laforge <laforge at osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Reviewer: tnt <tnt at 246tNt.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200508/c62cfbe7/attachment.htm>