Re: [PATCH v3 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv

[Martin Townsend <martin.townsend@xsilon.com>]

On 16/09/14 11:17, Alexander Aring wrote:
> On Tue, Sep 16, 2014 at 11:04:13AM +0100, Martin Townsend wrote:
>> Hi Alex,
>>
>> On the lowpan_give_skb_to_devices change.
>>
>> As we are iterating over a list of lowpan_devices and could potentially copy the skb more than once, what happens if the first device returns NET_RX_DROP and then the second time it return NET_RX_SUCCESS?  The stat variable is overwritten so stat only ever reflects the return value of netif_rx for the last device?
>>
>> Maybe it's better to completely remove the if else at the end and always consume the skb?  For the case whereskb_copy fails then we should kfree_skb,
>> e.g.
>>
>> static int lowpan_give_skb_to_devices(struct sk_buff *skb,
>> 				      struct net_device *dev)
>> {
>> 	struct lowpan_dev_record *entry;
>> 	struct sk_buff *skb_cp;
>> 	int stat = NET_RX_SUCCESS;
>>
>> 	rcu_read_lock();
>> 	list_for_each_entry_rcu(entry, &lowpan_devices, list)
>> 		if (lowpan_dev_info(entry->ldev)->real_dev == skb->dev) {
>> 			skb_cp = skb_copy(skb, GFP_ATOMIC);
>> 			if (!skb_cp) {
>> 				kfree_skb(skb);
>> 				rcu_read_unlock();
>> 				return NET_RX_DROP;
>> 			}
>>
>> 			skb_cp->dev = entry->ldev;
>> 			stat = netif_rx(skb_cp);
> here we should do a:
>
> if (stat == NET_RX_DROP)
> 	kfree_skb(skb_cp);
>
> or? It doesn't deliver and then we "could" lost the pointer.
Doesn't netif_rx always free the buffer?
>> 		}
>> 	rcu_read_unlock();
>>
>> 	consume_skb(skb);
>>
>> 	return stat;
>> }
>>
>>
>>  what are your thoughts?
>>
> for consume_skb:
>
> for me it's ok to make this behaviour. We never deliver the skb, always
> skb_cp. So if we are before the deliver call (netif_rx) this should
> never failed and we should consume the skb from which we did some copies.
yep
>
>
>
>
> btw.
>
> I see now that's skb_copy... mhhh. But this another issue. There exist
> skb_clone and skb_copy. skb_clone make a copy of struct sk_buff and data
> buffer is shared. I am currently not sure if we also can use a skb_clone
> here instead skb_copy, because the IPv6 doesn't manipulate the data buffer
> (I think it doesn't change the data buffer -> only parse) I need to think
> more about this, just a performance hint. But I really also doesn't know
> what sense makes multiple lowpan devices for one wpan interface. :-)
skb_clone could be a future patch.

Also I have been wondering why there are multiple lowpan device multiplexed onto a wpan.  Again maybe a future patch.
>
> - Alex
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
- Martin.