[1/4] gprs_gmm: ensure llme present upon Attach Req (CID #57686)

---
 openbsc/src/gprs/gprs_gmm.c | 4 ++++
 1 file changed, 4 insertions(+)

> On 14 Apr 2016, at 15:21, Neels Hofmeyr <nhofmeyr@sysmocom.de> wrote:
> 
> ---
> openbsc/src/gprs/gprs_gmm.c | 4 ++++
> 1 file changed, 4 insertions(+)
> 
> diff --git a/openbsc/src/gprs/gprs_gmm.c b/openbsc/src/gprs/gprs_gmm.c
> index 5f0a5fd..438e047 100644
> --- a/openbsc/src/gprs/gprs_gmm.c
> +++ b/openbsc/src/gprs/gprs_gmm.c
> @@ -838,6 +838,10 @@ static int gsm48_rx_gmm_att_req(struct sgsn_mm_ctx *ctx, struct msgb *msg,
> 	int rc;
> 
> 	LOGMMCTXP(LOGL_INFO, ctx, "-> GMM ATTACH REQUEST ");
> +	if (!llme) {

same as with my other mail. The lower layer should create a llme on the fly. So please have a look at the coverity user interface to see _where_ the llme is NULL and then we can have a look at that issue.

holger

On Fri, Apr 22, 2016 at 03:47:29PM +0200, Holger Freyther wrote:
> 
> > On 14 Apr 2016, at 15:21, Neels Hofmeyr <nhofmeyr@sysmocom.de> wrote:
> > 
> > ---
> > openbsc/src/gprs/gprs_gmm.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> > 
> > diff --git a/openbsc/src/gprs/gprs_gmm.c b/openbsc/src/gprs/gprs_gmm.c
> > index 5f0a5fd..438e047 100644
> > --- a/openbsc/src/gprs/gprs_gmm.c
> > +++ b/openbsc/src/gprs/gprs_gmm.c
> > @@ -838,6 +838,10 @@ static int gsm48_rx_gmm_att_req(struct sgsn_mm_ctx *ctx, struct msgb *msg,
> > 	int rc;
> > 
> > 	LOGMMCTXP(LOGL_INFO, ctx, "-> GMM ATTACH REQUEST ");
> > +	if (!llme) {
> 
> same as with my other mail. The lower layer should create a llme on the fly. So please have a look at the coverity user interface to see _where_ the llme is NULL and then we can have a look at that issue.

There is, obviously, an explicit NULL on the sysmocom/iu branch, where no LLME
is applicable.

openbsc/openbsc/src/gprs/gprs_gmm.c:2462:		rc = gsm0408_rcv_gmm(mmctx, msg, NULL);


Coverity concerns itself about a

"Comparing llme to null implies that llme might be null."

so no actual NULL is reported. (though coverity could have reported above
explicit NULL which I found manually)

Actually, the llme NULL check in gsm0408_rcv_gmm() is introduced in the
sysmocom/iu branch along with mentioned explicit NULL:

1628        if (llme && !mmctx &&
1629            gh->msg_type != GSM48_MT_GMM_ATTACH_REQ &&
1630            gh->msg_type != GSM48_MT_GMM_RA_UPD_REQ) {
1631                LOGP(DMM, LOGL_NOTICE, "Cannot handle GMM for unknown MM CTX\n");

So my suggestion above to clean up llme only when it's not NULL does still make
a lot of sense to me.

Since I'm not familiar with GRPS code, maybe Daniel could have a quick look?
I'm having a déjà vu, didn't I write this before?
Would be nice if I could stop banging my head against your wall now ;)

~Neels

> On 25 Apr 2016, at 12:51, Neels Hofmeyr <nhofmeyr@sysmocom.de> wrote:
> 
>> 
> 
> There is, obviously, an explicit NULL on the sysmocom/iu branch, where no LLME
> is applicable.
> 
> openbsc/openbsc/src/gprs/gprs_gmm.c:2462:		rc = gsm0408_rcv_gmm(mmctx, msg, NULL);
> 

when looking at the patch I thought you would be on master.


> So my suggestion above to clean up llme only when it's not NULL does still make
> a lot of sense to me.
> 
> Since I'm not familiar with GRPS code, maybe Daniel could have a quick look?
> I'm having a déjà vu, didn't I write this before?
> Would be nice if I could stop banging my head against your wall now ;)

I would need to look at the code but if the GMM code is used on top of two different layers, then it should not look too much at the LLME? Specially as with send/receive it might not use the same LLME anyway?

So yes, for your Iu branch add the NULL check, once we merge to master we need to see if there is valuable information inside the llme that we need to store somewhere else/or just log it.

holger

On Mon, Apr 25, 2016 at 08:35:39PM +0200, Holger Freyther wrote:
> 
> > On 25 Apr 2016, at 12:51, Neels Hofmeyr <nhofmeyr@sysmocom.de> wrote:
> > 
> >> 
> > 
> > There is, obviously, an explicit NULL on the sysmocom/iu branch, where no LLME
> > is applicable.
> > 
> > openbsc/openbsc/src/gprs/gprs_gmm.c:2462:		rc = gsm0408_rcv_gmm(mmctx, msg, NULL);
> > 
> 
> when looking at the patch I thought you would be on master.

IIUC the coverity build uses the iu branches?

But right, I submitted the patch against master, while on the iu branches
I would normally just commit. So, a mixup from my side, sorry!

I'm still glad you found this issue; I do in general highly esteem your
reviews, pretty much always of pinnacle quality in an amazingly short
time. Kudos!

> So yes, for your Iu branch add the NULL check, once we merge to master we need to see if there is valuable information inside the llme that we need to store somewhere else/or just log it.

My guess is that for Iu subscriptions, there will simply be no LLME, and
there won't be information in an LLME that is NULL ;)
But agreed.

The merge-Iu-to-master review will be ... extreme, probably. Daniel and I
should at some point take a look at refactoring the commit sequence to
group by semantics and iron out trial-and-error commits.

But it won't happen without an A-interface, I guess, so it is quite far
down the line at this point (unless we keep NITB somehow, which would
probably also be a bit of work).

~Neels

Hi Neels,

I'm a bit troubled by the following statements:

On Thu, Apr 28, 2016 at 01:27:11PM +0200, Neels Hofmeyr wrote:
> The merge-Iu-to-master review will be ... extreme, probably. Daniel and I
> should at some point take a look at refactoring the commit sequence to
> group by semantics and iron out trial-and-error commits.
> 
> But it won't happen without an A-interface, I guess, so it is quite far
> down the line at this point (unless we keep NITB somehow, which would
> probably also be a bit of work).

It was clearly not the intention to keep the code out of master for more
than the time it takes to implement those features.

So yes, very clearly, for the CS side the NITB has to stay intact after
a merge of the Iu, until the A interface is implemented.  So please
don't think of this as something "far down the line".

For the PS side, I don't see any reason at all why a merge to master
should be postponed much longer (after it works end-to-end, with GSUP
and external HLR).  The newly-introduced libiu should be easy to add
without any risk to break existing code.  And the decisions about Gb vs.
IuPS are all made at runtime when looking at the respective subscriber /
mm_context.

Regards,
	Harald

On Thu, Apr 28, 2016 at 02:52:12PM +0200, Harald Welte wrote:
> It was clearly not the intention to keep the code out of master for more
> than the time it takes to implement those features.
> 
> So yes, very clearly, for the CS side the NITB has to stay intact after
> a merge of the Iu, until the A interface is implemented.  So please
> don't think of this as something "far down the line".

This is a very high level decision.

So far the explicit goal was to replace CSCN's libbsc with a proper A
interface. So instead of placing junctures to decide whether to use BSC land
structures (like the bts pointer) or not, I removed BSC land references.

The A-interface aim is more neat in the sense that we will have explicit
junctures that can decide whether to send a given message to A or IuCS. This
obviously removes some NITB direct-BSC-access features.

OTOH, the NITB-with-IuCS would be more like the tightly connected MSC+BSC with
an IuCS side channel "stuck to its side". It's then questionnable whether to
remove the direct-BSC-access features at all.

Implementing the A-interface is clearly more work before being able to merge.
But if we are going to sacrifice NITB for an A-interface anyway, it would be
overall less work to keep NITB and IuCS separate until A is done.

Keeping the NITB requires some un-rewiring, talking about the changes to
gsm_subscriber_connection and reverting a few clear cuts from the previous code
paths that I so far have in the iu branch. Shouldn't be too much work really.

However, to me, keeping the NITB along with IuCS does sound like a complete
change of direction compared to the previous instructions you gave me, i.e. to
go for an A. Either way is fine, but let's be clear on which is the goal!

Thinkable would also be to have all three: IuCS, NITB and a future A interface.
That is to a degree sacrificing the neat clarity of separating MSC and BSC for
good, and it would be the non-UNIX style "Eierlegende Wollmilchsau" [1].

These are my thoughts on CS so far, I am happy to go whichever way the
community prefers. Let's establish consensus on the goal/roadmap as soon as
possible.

My wish to review the branch diff so far is still valid and probably goes hand
in hand with this decision. It might be good if I have a look at the key
junctures in the code paths and send a report/overview to this list?

> For the PS side, I don't see any reason at all why a merge to master
> should be postponed much longer.

Yes, of course depending on Daniel's opinion, this is a completely different
thing and merging is less of a complex decision.

~Neels

[1] https://en.wiktionary.org/wiki/eierlegende_Wollmilchsau