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
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
