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

[Alexander Aring <alex.aring@gmail.com>]

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.
> 		}
> 	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.




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. :-)

- Alex