Re: [PATCH v2 bluetooth-next 1/4] 6lowpan: remove skb_deliver from IPHC

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

Hi Martin,

On Mon, Oct 20, 2014 at 03:39:48PM +0100, Martin Townsend wrote:
> Separating skb delivery from decompression ensures that we can support further
> decompression schemes and removes the mixed return value of error codes with
> NET_RX_FOO.
> 
> Signed-off-by: Martin Townsend <mtownsend1973@gmail.com>
> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>

All your patches have two different "Signed-off-by". Please use only one
here.

> ---
>  include/net/6lowpan.h         |  4 +---
>  net/6lowpan/iphc.c            | 32 ++++++--------------------------
>  net/bluetooth/6lowpan.c       | 12 +++++++++++-
>  net/ieee802154/6lowpan_rtnl.c | 26 ++++++++++++++------------
>  4 files changed, 32 insertions(+), 42 deletions(-)
> 
...
> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> index 6c5c2ef..03787e0 100644
> --- a/net/bluetooth/6lowpan.c
> +++ b/net/bluetooth/6lowpan.c
> @@ -290,7 +290,7 @@ static int process_data(struct sk_buff *skb, struct net_device *netdev,
>  	return lowpan_process_data(skb, netdev,
>  				   saddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
>  				   daddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
> -				   iphc0, iphc1, give_skb_to_upper);
> +				   iphc0, iphc1);
>  
>  drop:
>  	kfree_skb(skb);
> @@ -350,6 +350,16 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
>  			if (ret != NET_RX_SUCCESS)
>  				goto drop;
>  
> +			local_skb->protocol = htons(ETH_P_IPV6);
> +			local_skb->pkt_type = PACKET_HOST;
> +			local_skb->dev = dev;
> +
> +			if (give_skb_to_upper(local_skb, dev)
> +					!= NET_RX_SUCCESS) {

There is still a  NET_RX_FOO and errno conversion in function
"give_skb_to_upper". Please check that, maybe introduce a new patch for
this one.

> +				kfree_skb(local_skb);
> +				goto drop;
> +			}
> +
>  			dev->stats.rx_bytes += skb->len;
>  			dev->stats.rx_packets++;
>  
> diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
> index 0c1a49b..898d317 100644
> --- a/net/ieee802154/6lowpan_rtnl.c
> +++ b/net/ieee802154/6lowpan_rtnl.c
> @@ -146,8 +146,9 @@ static int lowpan_give_skb_to_devices(struct sk_buff *skb,
>  		if (lowpan_dev_info(entry->ldev)->real_dev == skb->dev) {
>  			skb_cp = skb_copy(skb, GFP_ATOMIC);
>  			if (!skb_cp) {
> -				stat = -ENOMEM;
> -				break;
> +				kfree_skb(skb);
> +				rcu_read_unlock();
> +				return NET_RX_DROP;
>  			}
>  
>  			skb_cp->dev = entry->ldev;
> @@ -155,6 +156,11 @@ static int lowpan_give_skb_to_devices(struct sk_buff *skb,
>  		}
>  	rcu_read_unlock();
>  
> +	if (stat == NET_RX_SUCCESS)
> +		consume_skb(skb);
> +	else
> +		kfree_skb(skb);
> +

You will not see it because somebody forgot brackets in the above
"list_for_each_entry_rcu" loop. But variable "stat" in this case is only
for the last entry of netif_rx call.

Also if netif_rx returns NET_RX_DROP, which is the else branch in this
case. In case of netif_rx the skb will be freed somewhere else.

Calltrace:

- netif_rx
- netif_rx_internal
- enqueue_to_backlog
  - kfree_skb(skb);
  - return NET_RX_DROP;


- Alex