Change in ...osmo-sgsn[master]: gprs_gmm: Introduce assert to guard against unexpected condition
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 ) Change subject: gprs_gmm: Introduce assert to guard against unexpected condition .. Patch Set 3: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: I8e1eaeb9b3ebee8e45704b4fe007190c7db609e4 Gerrit-Change-Number: 15167 Gerrit-PatchSet: 3 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-Comment-Date: Sat, 17 Aug 2019 14:10:23 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: gprs_gmm: Introduce assert to guard against unexpected condition
pespin has submitted this change and it was merged. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 ) Change subject: gprs_gmm: Introduce assert to guard against unexpected condition .. gprs_gmm: Introduce assert to guard against unexpected condition This may well be the culprit of OS#3957, were already freed llme is accessed from mmctx context later on, upon some timer is triggered in mmctx. Related: OS#3957 Change-Id: I8e1eaeb9b3ebee8e45704b4fe007190c7db609e4 --- M src/gprs/gprs_gmm.c 1 file changed, 5 insertions(+), 0 deletions(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, but someone else must approve fixeria: Looks good to me, but someone else must approve pespin: Looks good to me, approved diff --git a/src/gprs/gprs_gmm.c b/src/gprs/gprs_gmm.c index 0fcf1bb..718fc97 100644 --- a/src/gprs/gprs_gmm.c +++ b/src/gprs/gprs_gmm.c @@ -1741,6 +1741,11 @@ "The MM context cannot be used, RA: %03d-%0*d-%d-%d\n", mmctx->ra.mcc, mmctx->ra.mnc_3_digits, mmctx->ra.mnc, mmctx->ra.lac, mmctx->ra.rac); + /* mmctx is set to NULL and gprs_llgmm_unassign(llme) will be + called below, let's make sure we don't keep dangling llme + pointers in mmctx (OS#3957). */ + if (mmctx->ran_type == MM_CTX_T_GERAN_Gb) + OSMO_ASSERT(mmctx->gb.llme == NULL); mmctx = NULL; } -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: I8e1eaeb9b3ebee8e45704b4fe007190c7db609e4 Gerrit-Change-Number: 15167 Gerrit-PatchSet: 3 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-MessageType: merged
Change in ...osmo-sgsn[master]: gprs_gmm: Introduce assert to guard against unexpected condition
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 ) Change subject: gprs_gmm: Introduce assert to guard against unexpected condition .. Patch Set 3: +1+1=+2 -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: I8e1eaeb9b3ebee8e45704b4fe007190c7db609e4 Gerrit-Change-Number: 15167 Gerrit-PatchSet: 3 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-Comment-Date: Sat, 17 Aug 2019 14:10:17 + Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: gprs_gmm: Introduce assert to guard against unexpected condition
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 ) Change subject: gprs_gmm: Introduce assert to guard against unexpected condition .. Patch Set 3: Code-Review+1 Much cleaner now. -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: I8e1eaeb9b3ebee8e45704b4fe007190c7db609e4 Gerrit-Change-Number: 15167 Gerrit-PatchSet: 3 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-Comment-Date: Sat, 17 Aug 2019 08:46:54 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: gprs_gmm: Introduce assert to guard against unexpected condition
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 ) Change subject: gprs_gmm: Introduce assert to guard against unexpected condition .. Patch Set 3: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: I8e1eaeb9b3ebee8e45704b4fe007190c7db609e4 Gerrit-Change-Number: 15167 Gerrit-PatchSet: 3 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-CC: fixeria Gerrit-Comment-Date: Sat, 17 Aug 2019 01:22:50 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: gprs_gmm: Introduce assert to guard against unexpected condition
Hello lynxis lazus, neels, laforge, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 to look at the new patch set (#3). Change subject: gprs_gmm: Introduce assert to guard against unexpected condition .. gprs_gmm: Introduce assert to guard against unexpected condition This may well be the culprit of OS#3957, were already freed llme is accessed from mmctx context later on, upon some timer is triggered in mmctx. Related: OS#3957 Change-Id: I8e1eaeb9b3ebee8e45704b4fe007190c7db609e4 --- M src/gprs/gprs_gmm.c 1 file changed, 5 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-sgsn refs/changes/67/15167/3 -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: I8e1eaeb9b3ebee8e45704b4fe007190c7db609e4 Gerrit-Change-Number: 15167 Gerrit-PatchSet: 3 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-CC: fixeria Gerrit-MessageType: newpatchset
Change in ...osmo-sgsn[master]: gprs_gmm: Introduce assert to guard against unexpected condition
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 ) Change subject: gprs_gmm: Introduce assert to guard against unexpected condition .. Patch Set 2: (1 comment) https://gerrit.osmocom.org/#/c/15167/2/src/gprs/gprs_gmm.c File src/gprs/gprs_gmm.c: https://gerrit.osmocom.org/#/c/15167/2/src/gprs/gprs_gmm.c@1747 PS2, Line 1747: mmctx->ran_type == MM_CTX_T_GERAN_Gb > I'm also confused. […] This is an assert I add to try to catch keith's osmo-sgsn isntance crashing (we started debugging it in OsmoDevCon). I don't want to directly workaround the issue, I really want osmo-sgsn to assert and exit in this situation, so we can make sure this is the condition he's hitting. Since lots of subscribers are using the sgsn at that time, it's impossible otherwise to find out. Killing osmo-sgsn here is not a big issue since anyway it would crash later on due to the bug we spotted. Regarding the assert condition: If ran_type is MM_CTX_T_UTRAN_Iu, it's perfectly fine to have a null llme, so I really want to check in the Gb case (because anyway that's what's deployed in the setup where we saw the crash). I want to make sure that "it doesn't happen that llme is NULL when on Gb", and the condition expresses so. The expression I'm using is exactly the same you are proposing. But Ok, I'll change it if you think it's simpler. -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: I8e1eaeb9b3ebee8e45704b4fe007190c7db609e4 Gerrit-Change-Number: 15167 Gerrit-PatchSet: 2 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-CC: fixeria Gerrit-Comment-Date: Thu, 15 Aug 2019 09:16:49 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria Comment-In-Reply-To: neels Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: gprs_gmm: Introduce assert to guard against unexpected condition
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 ) Change subject: gprs_gmm: Introduce assert to guard against unexpected condition .. Patch Set 2: Code-Review-1 (1 comment) https://gerrit.osmocom.org/#/c/15167/2/src/gprs/gprs_gmm.c File src/gprs/gprs_gmm.c: https://gerrit.osmocom.org/#/c/15167/2/src/gprs/gprs_gmm.c@1747 PS2, Line 1747: mmctx->ran_type == MM_CTX_T_GERAN_Gb > Oh, sorry, I was confused. Can we simplify the code a bit? […] I'm also confused. The condition matching the current patch would have to be if (mmctx->ran_type == MM_CTX_T_GERAN_Gb) OSMO_ASSERT(mmctx->gb.llme); But it makes no sense when reading the comment above it. To make sure of no dangling pointers I also expected an 'gb.llme = NULL' assignment instead of an assert? Pau, can you explain better what the intention is and possibly fix the condition? -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: I8e1eaeb9b3ebee8e45704b4fe007190c7db609e4 Gerrit-Change-Number: 15167 Gerrit-PatchSet: 2 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-CC: fixeria Gerrit-Comment-Date: Wed, 14 Aug 2019 23:20:25 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: fixeria Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: gprs_gmm: Introduce assert to guard against unexpected condition
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 ) Change subject: gprs_gmm: Introduce assert to guard against unexpected condition .. Patch Set 2: (1 comment) https://gerrit.osmocom.org/#/c/15167/2/src/gprs/gprs_gmm.c File src/gprs/gprs_gmm.c: https://gerrit.osmocom.org/#/c/15167/2/src/gprs/gprs_gmm.c@1747 PS2, Line 1747: mmctx->ran_type == MM_CTX_T_GERAN_Gb > So OsmoSGSN would crash if mmctx->ran_type != MM_CTX_T_GERAN_Gb? Is it > expected? […] Oh, sorry, I was confused. Can we simplify the code a bit? if (mmctx->ran_type == MM_CTX_T_GERAN_Gb) OSMO_ASSERT(mmctx->gb.llme == NULL); -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: I8e1eaeb9b3ebee8e45704b4fe007190c7db609e4 Gerrit-Change-Number: 15167 Gerrit-PatchSet: 2 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: pespin Gerrit-CC: fixeria Gerrit-Comment-Date: Wed, 14 Aug 2019 22:53:54 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: gprs_gmm: Introduce assert to guard against unexpected condition
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 ) Change subject: gprs_gmm: Introduce assert to guard against unexpected condition .. Patch Set 2: (1 comment) https://gerrit.osmocom.org/#/c/15167/2/src/gprs/gprs_gmm.c File src/gprs/gprs_gmm.c: https://gerrit.osmocom.org/#/c/15167/2/src/gprs/gprs_gmm.c@1747 PS2, Line 1747: mmctx->ran_type == MM_CTX_T_GERAN_Gb So OsmoSGSN would crash if mmctx->ran_type != MM_CTX_T_GERAN_Gb? Is it expected? The whole condition evaluates to (mmctx->ran_type != MM_CTX_T_GERAN_Gb || mmctx->gb.llme != NULL). -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: I8e1eaeb9b3ebee8e45704b4fe007190c7db609e4 Gerrit-Change-Number: 15167 Gerrit-PatchSet: 2 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: pespin Gerrit-CC: fixeria Gerrit-Comment-Date: Wed, 14 Aug 2019 22:49:37 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: gprs_gmm: Introduce assert to guard against unexpected condition
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 ) Change subject: gprs_gmm: Introduce assert to guard against unexpected condition .. Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: I8e1eaeb9b3ebee8e45704b4fe007190c7db609e4 Gerrit-Change-Number: 15167 Gerrit-PatchSet: 1 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: pespin Gerrit-Comment-Date: Tue, 13 Aug 2019 10:54:39 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-sgsn[master]: gprs_gmm: Introduce assert to guard against unexpected condition
pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 Change subject: gprs_gmm: Introduce assert to guard against unexpected condition .. gprs_gmm: Introduce assert to guard against unexpected condition This may well be the culprit of OS#3957, were already freed llme is accessed from mmctx context. Related: OS#3957 Change-Id: I8e1eaeb9b3ebee8e45704b4fe007190c7db609e4 --- M src/gprs/gprs_gmm.c 1 file changed, 4 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-sgsn refs/changes/67/15167/1 diff --git a/src/gprs/gprs_gmm.c b/src/gprs/gprs_gmm.c index c8bc0f7..672864f 100644 --- a/src/gprs/gprs_gmm.c +++ b/src/gprs/gprs_gmm.c @@ -1741,6 +1741,10 @@ "The MM context cannot be used, RA: %03d-%0*d-%d-%d\n", mmctx->ra.mcc, mmctx->ra.mnc_3_digits, mmctx->ra.mnc, mmctx->ra.lac, mmctx->ra.rac); + /* mmctx is set to NULL and gprs_llgmm_unassign(llme) will be + called below, let's make sure we don't keep dangling llme + pointers in mmctx (OS#3957). */ + OSMO_ASSERT(!(mmctx->ran_type == MM_CTX_T_GERAN_Gb && mmctx->gb.llme == NULL)); mmctx = NULL; } -- To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15167 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-sgsn Gerrit-Branch: master Gerrit-Change-Id: I8e1eaeb9b3ebee8e45704b4fe007190c7db609e4 Gerrit-Change-Number: 15167 Gerrit-PatchSet: 1 Gerrit-Owner: pespin Gerrit-MessageType: newchange
