On 16/09/14 13:34, Alexander Aring wrote:
> 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
Yes I see the problem now, maybe it's better to revert back to skb_inout, less chance of introducing bugs and then we have a well defined return value.
- Martin.
--
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
