From mboxrd@z Thu Jan 1 00:00:00 1970 From: Karsten Keil Subject: Re: [mISDN PATCH v2 09/19] Fix TEI and SAPI handling Date: Sat, 23 May 2009 15:00:37 +0200 Message-ID: <200905231500.38092.keil@b1-systems.de> References: <20090522215314.GA4592@uranus.ravnborg.org> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org, David Miller , i4ldeveloper@listserv.isdn4linux.de To: Sam Ravnborg Return-path: Received: from d112a2.x-mailer.de ([212.162.53.4]:52167 "EHLO d112a2.x-mailer.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751539AbZEWNAj (ORCPT ); Sat, 23 May 2009 09:00:39 -0400 In-Reply-To: <20090522215314.GA4592@uranus.ravnborg.org> Content-Disposition: inline Sender: netdev-owner@vger.kernel.org List-ID: 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);