Re: [mISDN PATCH v2 09/19] Fix TEI and SAPI handling

[Karsten Keil <keil@b1-systems.de>]

Hi Sam,

thanks for the review.

On Freitag, 22. Mai 2009 23:53:14 Sam Ravnborg wrote:
> Hi Karsten.
>
> Flash from the past looking at some ISDN layer2.
> Some comments below.
>
> 	Sam
>
> >  struct layer2 *
> > -create_l2(struct mISDNchannel *ch, u_int protocol, u_long options,
> > u_long arg) +create_l2(struct mISDNchannel *ch, u_int protocol, u_long
> > options, u_int tei, +		u_int sapi)
>
> Unrealted to this specific patch...
> Are there any specific reason why mISDN prefer bsd style (u_int)
> rather then linux style (u32)?

No, this was some historic process with the ISDN code.
I think, if we want change this we should make a separate  patch to change 
this in all mISDN code later, I usually prefer shorter type names as well :-).
So as rule of dumb:
- Use   uN/sN for all values with constant width N ?

What should be used on functions which are usually defined as int and
only return 0 or some negative ERROR code, I think int should be OK here ?

>
> >  static struct layer2 *
> > -create_new_tei(struct manager *mgr, int tei)
> > +create_new_tei(struct manager *mgr, int tei, int sapi)
> >  {
>
> Here tei and sapi are passed as signed.
> But valid sapi range is [0..63] and tei [0..127].
> And create_l2 above uses unsigned.
>
> This looks confusing.

I think the signed was selected for error handling, but you are correct then 
it should be the same in all functions.

>
> >  	u_long		opt = 0;
> >  	u_long		flags;
> > @@ -791,7 +791,7 @@ create_new_tei(struct manager *mgr, int tei)
> >  	if (mgr->ch.st->dev->Dprotocols
> >  	  & ((1 << ISDN_P_TE_E1) | (1 << ISDN_P_NT_E1)))
> >  		test_and_set_bit(OPTION_L2_PMX, &opt);
> > -	l2 = create_l2(mgr->up, ISDN_P_LAPD_NT, (u_int)opt, (u_long)tei);
> > +	l2 = create_l2(mgr->up, ISDN_P_LAPD_NT, (u_int)opt, tei, sapi);
> >  	if (!l2) {
> >  		printk(KERN_WARNING "%s:no memory for layer2\n", __func__);
> >  		return NULL;
> > @@ -839,12 +839,15 @@ new_tei_req(struct manager *mgr, u_char *dp)
> >  	ri += dp[1];
> >  	if (!mgr->up)
> >  		goto denied;
> > -	tei = get_free_tei(mgr);
> > +	if (dp[3] != 0xff)
> > +		tei = dp[3] >> 1; /* 3GPP TS 08.56 6.1.11.2 */
> > +	else
> > +		tei = get_free_tei(mgr);
>
> You should check EA0 here before assuming that any values except
> EA=1, tei=127 equals ptmp.
>

make sense.

> >  	if (tei < 0) {
> >  		printk(KERN_WARNING "%s:No free tei\n", __func__);
> >  		goto denied;
> >  	}
>
> So get_free_tei() may return a negative value indicating no free tei.
> So that make my comment above void - but is this really the best way
> to return an error. Possibly it is..
>
We could define 255 as the error value, I will think about this, but in 
general I prefer negative values for error cases.

> > -	l2 = create_new_tei(mgr, tei);
> > +	l2 = create_new_tei(mgr, tei, 0);
>
> In general I would prefer to read: SAPI_CALLCONTROL rahter than a hardcoded
> 0.
>

Yes.

> >  	if (!l2)
> >  		goto denied;
> >  	else
> > @@ -976,8 +979,6 @@ create_teimgr(struct manager *mgr, struct channel_req
> > *crq) __func__, dev_name(&mgr->ch.st->dev->dev),
> >  			crq->protocol, crq->adr.dev, crq->adr.channel,
> >  			crq->adr.sapi, crq->adr.tei);
> > -	if (crq->adr.sapi != 0) /* not supported yet */
> > -		return -EINVAL;
> >  	if (crq->adr.tei > GROUP_TEI)
> >  		return -EINVAL;
> >  	if (crq->adr.tei < 64)
> > @@ -1025,7 +1026,7 @@ create_teimgr(struct manager *mgr, struct
> > channel_req *crq) return 0;
> >  	}
> >  	l2 = create_l2(crq->ch, crq->protocol, (u_int)opt,
> > -		(u_long)crq->adr.tei);
> > +		crq->adr.tei, crq->adr.sapi);
> >  	if (!l2)
> >  		return -ENOMEM;
> >  	l2->tm = kzalloc(sizeof(struct teimgr), GFP_KERNEL);
> > @@ -1166,7 +1167,7 @@ static int
> >  check_data(struct manager *mgr, struct sk_buff *skb)
> >  {
> >  	struct mISDNhead	*hh =  mISDN_HEAD_P(skb);
> > -	int			ret, tei;
> > +	int			ret, tei, sapi;
> >  	struct layer2		*l2;
> >
> >  	if (*debug & DEBUG_L2_CTRL)
> > @@ -1178,18 +1179,16 @@ check_data(struct manager *mgr, struct sk_buff
> > *skb) return -ENOTCONN;
> >  	if (skb->len != 3)
> >  		return -ENOTCONN;
> > -	if (skb->data[0] != 0)
> > -		/* only SAPI 0 command */
> > -		return -ENOTCONN;
> > +	sapi = skb->data[0] >> 2;
>
> PReviously there was an implicit check of EA0.
> This is missed not that you read sapi and discard the two lower bits.
>
> I also wonder if you remeber to check the command/response bit.
> That was implicitly tested to be 0 before and also ignored now.

Yes,  you are correct.

>
> >  	if (!(skb->data[1] & 1)) /* invalid EA1 */
> >  		return -EINVAL;
> > -	tei = skb->data[1] >> 0;
> > +	tei = skb->data[1] >> 1;
>
> This looks like a bug-fix...
>

Yes,  I think fixed TEI was never tested with P2MP,  I know
no device which really use it with SAPI 0, only in P2P we use a
fixed TEI  of 0.  Also the test suite used by certifications did
not check for correct fixed TEI handling, it only checks that a
fixed TEI is rejected in a dynamic TEI assignment.
With SAPI != 0 fixed TEIs are more common, so the bug was
detected now.

> >  	if (tei > 63) /* not a fixed tei */
> >  		return -ENOTCONN;
> >  	if ((skb->data[2] & ~0x10) != SABME)
> >  		return -ENOTCONN;
> >  	/* We got a SABME for a fixed TEI */
> > -	l2 = create_new_tei(mgr, tei);
> > +	l2 = create_new_tei(mgr, tei, sapi);
> >  	if (!l2)
> >  		return -ENOMEM;
> >  	ret = l2->ch.send(&l2->ch, skb);