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:28:06AM +0100, Martin Townsend wrote:
> 
> 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?

yes, you are right. [0] Now other things makes more sense for me.
Thanks.

I mean there is another deliver function netif_receive_skb and on
comment always stand "Return values (usually ignored)", depends on
context what you need. But netif_rx in this context is right.

Here we should not ignore the return value, because we already are in
the packet layer (the packet layer func callback). The netif_receive_skb 
function we need for putting frames from the driver (some tasklet context)
into the packet layer.


[0] is some function which is called mainly after "netif_rx" function,
"netif_receive_skb" will call this function, too.

- Alex

[0] http://lxr.free-electrons.com/source/net/core/dev.c#L3280