Return-Path: Date: Tue, 16 Sep 2014 14:34:23 +0200 From: Alexander Aring To: Martin Townsend Cc: Martin Townsend , linux-zigbee-devel@lists.sourceforge.net, linux-bluetooth@vger.kernel.org, linux-wpan@vger.kernel.org, marcel@holtmann.org, jukka.rissanen@linux.intel.com Subject: Re: [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv Message-ID: <20140916123421.GA5576@omega> References: <1410865319-16310-1-git-send-email-martin.townsend@xsilon.com> <1410865319-16310-2-git-send-email-martin.townsend@xsilon.com> <20140916113614.GC4969@omega> <5418215F.5050308@xsilon.com> <20140916114759.GD4969@omega> <20140916115357.GE4969@omega> <20140916120247.GA5217@omega> <20140916121844.GA5349@omega> <54182C6B.6090801@xsilon.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 In-Reply-To: <54182C6B.6090801@xsilon.com> Sender: linux-wpan-owner@vger.kernel.org List-ID: On Tue, Sep 16, 2014 at 01:26:19PM +0100, Martin Townsend wrote: > Hi Alex, > > On 16/09/14 13:18, Alexander Aring wrote: > > On Tue, Sep 16, 2014 at 02:02:47PM +0200, Alexander Aring wrote: > >> On Tue, Sep 16, 2014 at 01:53:57PM +0200, Alexander Aring wrote: > >>> On Tue, Sep 16, 2014 at 01:47:59PM +0200, Alexander Aring wrote: > >>>> On Tue, Sep 16, 2014 at 12:39:11PM +0100, Martin Townsend wrote: > >>>>> Hi Alex, > >>>>> On 16/09/14 12:36, Alexander Aring wrote: > >>>>>> On Tue, Sep 16, 2014 at 12:01:59PM +0100, Martin Townsend wrote: > >>> ... > >>>> and this also smells like side effects for me, because we have the > >>>> local_skb which is sometimes freed inside of lowpan_process_data and > >>>> returning skb. Then we don't know which we should kfree_skb now, the skb > >>>> or local_skb now. Need to thing more about this to offer some solution, > >>>> somebody agree here with me? > >>>> > >>> I mean sometimes we do this *skb = *new and skb is the parameter and before we > >>> did a consume_skb(skb); then local_skb is already freed after this and > >>> returning an errno and we make kfree_skb(local_skb) will crash something, > >>> I suppose. > >> I meant skb = new for the expand skb thing. And we can't never free > >> kfree_skb(skb) here if (IS_ERR(skb) is true, but we can't decide if > >> we need a kfree_skb(local_skb) or not, because we do a > >> consume_skb($SKB_FROM_PARAMTER) in lowpan_process_data. > >> > > This all comes now in, because the ERR_PTR conversion. So we have two > > choices: > > > > - drop the ERR_PTR convertsion and make old behaviour > > - handle consume_skb/kfree_skb inside lowpan_process_data > > > > - Alex > > > How about a label for drop_local_skb? > > switch (skb->data[0] & 0xe0) { > case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */ > local_skb = skb_clone(skb, GFP_ATOMIC); > if (!local_skb) > goto drop; > > local_skb = process_data(local_skb, dev, chan); > if (IS_ERR(local_skb)) > goto drop_local_skb; > > local_skb->protocol = htons(ETH_P_IPV6); > local_skb->pkt_type = PACKET_HOST; > > if (give_skb_to_upper(local_skb, dev) > != NET_RX_SUCCESS) { > kfree_skb(local_skb); > goto drop; > } > > dev->stats.rx_bytes += skb->len; > dev->stats.rx_packets++; > > kfree_skb(skb); > break; > default: > break; > } > } > > return NET_RX_SUCCESS; > > drop_local_skb: > kfree_skb(local_skb); no this can't work, when IS_ERR(local_skb) is true, local_skb is an invalid pointer some "((void *) -errno)", you can rescue it with if (!IS_ERR(local_skb)), but... I don't know it looks complicated. :-) What I mean is in lowpan_process_data you have a paramater skb and a skb as return value. Sometimes we need a consume_skb($PARAMETER_SKB), because we make the copy_expand. After this the $PARAMETER_SKB is invalid and we have the $RETURN_SKB as our new skb. We don't know here if we need a kfree_skb($PARAMETER_SKB) or not because we don't know if we did a consume_skb($PARAMETER_SKB). I think the error handling need to be in lowpan_process_data again or make something which handle this case. I hope it was understandable what I mean here. - Alex