[PATCH v2 bluetooth-next 0/4] 6lowpan: Move skb delivery out of IPHC

[PATCH v2 bluetooth-next 0/4] 6lowpan: Move skb delivery out of IPHC

[Martin Townsend]

This series moves skb delivery out of IPHC and into the receive routines of
both bluetooth and 802.15.4.  The reason is that we need to support more
(de)compression schemes in the future.  It also means that calling 
lowpan_process_data now only returns error codes or 0 for success so 
this has been cleaned up.  The final patch then renames occurences of
lowpan_process_data and process_data to something more meaningful.

v1 -> v2

* Handle freeing of skb in lowpan_give_skb_to_devices for 802.15.14
* Added another skb_consume in bluetooth code for uncompressed IPv6 headers
* Added missing skb_consume for local_skb for bluetooth in compreesed IPv6 header
  path
* Combine patch 4 into 1.

Martin Townsend (4):
  6lowpan: remove skb_deliver from IPHC
  6lowpan: fix process_data return values
  bluetooth:6lowpan: use consume_skb when packet processed successfully
  ieee802154: 6lowpan: rename process_data and lowpan_process_data

 include/net/6lowpan.h         | 12 ++++++------
 net/6lowpan/iphc.c            | 42 ++++++++++++----------------------------
 net/bluetooth/6lowpan.c       | 34 +++++++++++++++++++++-----------
 net/ieee802154/6lowpan_rtnl.c | 45 +++++++++++++++++++++++--------------------
 4 files changed, 65 insertions(+), 68 deletions(-)

-- 
1.9.1

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

[Martin Townsend]

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>
---
 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/include/net/6lowpan.h b/include/net/6lowpan.h
index d184df1..abfa359 100644
--- a/include/net/6lowpan.h
+++ b/include/net/6lowpan.h
@@ -372,12 +372,10 @@ lowpan_uncompress_size(const struct sk_buff *skb, u16 *dgram_offset)
 	return skb->len + uncomp_header - ret;
 }
 
-typedef int (*skb_delivery_cb)(struct sk_buff *skb, struct net_device *dev);
-
 int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 		const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
 		const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
-		u8 iphc0, u8 iphc1, skb_delivery_cb skb_deliver);
+		u8 iphc0, u8 iphc1);
 int lowpan_header_compress(struct sk_buff *skb, struct net_device *dev,
 			unsigned short type, const void *_daddr,
 			const void *_saddr, unsigned int len);
diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 747b3cc..45714fe 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -171,29 +171,6 @@ static int uncompress_context_based_src_addr(struct sk_buff *skb,
 	return 0;
 }
 
-static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
-		       struct net_device *dev, skb_delivery_cb deliver_skb)
-{
-	int stat;
-
-	skb_push(skb, sizeof(struct ipv6hdr));
-	skb_reset_network_header(skb);
-	skb_copy_to_linear_data(skb, hdr, sizeof(struct ipv6hdr));
-
-	skb->protocol = htons(ETH_P_IPV6);
-	skb->pkt_type = PACKET_HOST;
-	skb->dev = dev;
-
-	raw_dump_table(__func__, "raw skb data dump before receiving",
-		       skb->data, skb->len);
-
-	stat = deliver_skb(skb, dev);
-
-	consume_skb(skb);
-
-	return stat;
-}
-
 /* Uncompress function for multicast destination address,
  * when M bit is set.
  */
@@ -327,7 +304,7 @@ static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
 int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 			const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
 			const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
-			u8 iphc0, u8 iphc1, skb_delivery_cb deliver_skb)
+			u8 iphc0, u8 iphc1)
 {
 	struct ipv6hdr hdr = {};
 	u8 tmp, num_context = 0;
@@ -492,10 +469,13 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 		hdr.version, ntohs(hdr.payload_len), hdr.nexthdr,
 		hdr.hop_limit, &hdr.daddr);
 
-	raw_dump_table(__func__, "raw header dump", (u8 *)&hdr, sizeof(hdr));
+	skb_push(skb, sizeof(hdr));
+	skb_reset_network_header(skb);
+	skb_copy_to_linear_data(skb, &hdr, sizeof(hdr));
 
-	return skb_deliver(skb, &hdr, dev, deliver_skb);
+	raw_dump_table(__func__, "raw header dump", (u8 *)&hdr, sizeof(hdr));
 
+	return 0;
 drop:
 	kfree_skb(skb);
 	return -EINVAL;
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) {
+				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);
+
 	return stat;
 }
 
@@ -190,8 +196,7 @@ static int process_data(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
 
 	return lowpan_process_data(skb, skb->dev, sap, sa.addr_type,
 				   IEEE802154_ADDR_LEN, dap, da.addr_type,
-				   IEEE802154_ADDR_LEN, iphc0, iphc1,
-				   lowpan_give_skb_to_devices);
+				   IEEE802154_ADDR_LEN, iphc0, iphc1);
 
 drop:
 	kfree_skb(skb);
@@ -528,15 +533,8 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
 
 	/* check that it's our buffer */
 	if (skb->data[0] == LOWPAN_DISPATCH_IPV6) {
-		skb->protocol = htons(ETH_P_IPV6);
-		skb->pkt_type = PACKET_HOST;
-
 		/* Pull off the 1-byte of 6lowpan header. */
 		skb_pull(skb, 1);
-
-		ret = lowpan_give_skb_to_devices(skb, NULL);
-		if (ret == NET_RX_DROP)
-			goto drop;
 	} else {
 		switch (skb->data[0] & 0xe0) {
 		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
@@ -565,7 +563,11 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
 		}
 	}
 
-	return NET_RX_SUCCESS;
+	/* Pass IPv6 packet up to the next layer */
+	skb->protocol = htons(ETH_P_IPV6);
+	skb->pkt_type = PACKET_HOST;
+	return lowpan_give_skb_to_devices(skb, NULL);
+
 drop_skb:
 	kfree_skb(skb);
 drop:
-- 
1.9.1

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

[Alexander Aring]

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

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

[Alexander Aring]

On Mon, Oct 20, 2014 at 09:18:55PM +0200, Alexander Aring wrote:

...

> >  
> > 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;
> 

I think, we need here something like:

stat = netif_rx(foo);
if (stat == NET_RX_SUCCESS)
	consume_skb(foo);

inside the loop.

- Alex

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

[Martin Townsend]

Hi Alex,

On 20/10/14 20:18, Alexander Aring wrote:
> 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.
I think I know how this occurred, 2 difference computers with 2 
different git configs :)  I'll fix this in the next series.
>> ---
>>   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.
I thought give_skb_to_upper only returned NET_RX_FOO return values? I'll 
double check and fix.
>
>> +				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.
This is a bug that's probably outside the scope of this patch.
> 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;
The copy will be freed not the skb that is passed to the function.
     skb_cp = skb_copy(skb, GFP_ATOMIC);
     ....
     stat = netif_rx(skb_cp);

What to do with stat that is set multiple times in a loop.  We could 
call skb_consume if stat is always NET_RX_SUCCESS or call skb_consume if 
stat is at least NET_RX_SUCCESS once, or maybe remove the loop further 
out the call chain  ...   if possible??

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

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

[Alexander Aring]

Hi Martin,

On Mon, Oct 20, 2014 at 10:35:25PM +0100, Martin Townsend wrote:
> Hi Alex,
> 
> On 20/10/14 20:18, Alexander Aring wrote:
> >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.
> I think I know how this occurred, 2 difference computers with 2 different
> git configs :)  I'll fix this in the next series.
> >>---
> >>  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.
> I thought give_skb_to_upper only returned NET_RX_FOO return values? I'll
> double check and fix.
> >

<qoute>
static int give_skb_to_upper(struct sk_buff *skb, struct net_device *dev)
{
	struct sk_buff *skb_cp;

	skb_cp = skb_copy(skb, GFP_ATOMIC);
	if (!skb_cp)
		return -ENOMEM;

Returning errno here.


	return netif_rx(skb_cp);

Returning NET_RX_FOO here. Here should be the same behaviour like below.
Only without a loop. Because the skb_share_check and only one lowpan
interface, I think we don't need the skb_copy here. This is out of scope
in this patch.

}
</qoute>

> >>+				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.
> This is a bug that's probably outside the scope of this patch.
> >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;
> The copy will be freed not the skb that is passed to the function.

mhh, okay.

>     skb_cp = skb_copy(skb, GFP_ATOMIC);
>     ....
>     stat = netif_rx(skb_cp);
> 
> What to do with stat that is set multiple times in a loop.  We could call
> skb_consume if stat is always NET_RX_SUCCESS or call skb_consume if stat is
> at least NET_RX_SUCCESS once, or maybe remove the loop further out the call
> chain  ...   if possible??
> 

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);
			if (stat == NET_RX_DROP)
				break;
                }
        rcu_read_unlock();

	consume_skb(skb);

        return stat;
}

We don't need no explicit run of kfree_skb or consume_skb here. in
this case "skb" should only a skb where we create copies from. After
that we don't dropping the skb. Okay, we only drop it if allocation
failed. I this okay now or I overlooked something here?

- Alex

[PATCH v2 bluetooth-next 2/4] 6lowpan: fix process_data return values

[Martin Townsend]

As process_data now returns just error codes fix up the calls to this
function to only drop the skb if an error code is returned.

Signed-off-by: Martin Townsend <mtownsend1973@gmail.com>
Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
---
 net/bluetooth/6lowpan.c       | 2 +-
 net/ieee802154/6lowpan_rtnl.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index 03787e0..702bf3c 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -347,7 +347,7 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
 				goto drop;
 
 			ret = process_data(local_skb, dev, chan);
-			if (ret != NET_RX_SUCCESS)
+			if (ret < 0)
 				goto drop;
 
 			local_skb->protocol = htons(ETH_P_IPV6);
diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
index 898d317..a160d0b 100644
--- a/net/ieee802154/6lowpan_rtnl.c
+++ b/net/ieee802154/6lowpan_rtnl.c
@@ -539,14 +539,14 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
 		switch (skb->data[0] & 0xe0) {
 		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
 			ret = process_data(skb, &hdr);
-			if (ret == NET_RX_DROP)
+			if (ret < 0)
 				goto drop;
 			break;
 		case LOWPAN_DISPATCH_FRAG1:	/* first fragment header */
 			ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAG1);
 			if (ret == 1) {
 				ret = process_data(skb, &hdr);
-				if (ret == NET_RX_DROP)
+				if (ret < 0)
 					goto drop;
 			}
 			break;
@@ -554,7 +554,7 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
 			ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN);
 			if (ret == 1) {
 				ret = process_data(skb, &hdr);
-				if (ret == NET_RX_DROP)
+				if (ret < 0)
 					goto drop;
 			}
 			break;
-- 
1.9.1

[PATCH v2 bluetooth-next 3/4] bluetooth:6lowpan: use consume_skb when packet processed successfully

[Martin Townsend]

From: Martin Townsend <mtownsend1973@gmail.com>

Signed-off-by: Martin Townsend <mtownsend1973@gmail.com>
Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
---
 net/bluetooth/6lowpan.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index 702bf3c..6c3182f 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -337,8 +337,8 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
 		dev->stats.rx_bytes += skb->len;
 		dev->stats.rx_packets++;
 
-		kfree_skb(local_skb);
-		kfree_skb(skb);
+		consume_skb(local_skb);
+		consume_skb(skb);
 	} else {
 		switch (skb->data[0] & 0xe0) {
 		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
@@ -363,7 +363,8 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
 			dev->stats.rx_bytes += skb->len;
 			dev->stats.rx_packets++;
 
-			kfree_skb(skb);
+			consume_skb(local_skb);
+			consume_skb(skb);
 			break;
 		default:
 			break;
-- 
1.9.1

[PATCH v2 bluetooth-next 4/4] ieee802154: 6lowpan: rename process_data and lowpan_process_data

[Martin Townsend]

From: Martin Townsend <mtownsend1973@gmail.com>

As we have decouple decompression from data delivery we can now rename all
occurences of process_data in receive path.

Signed-off-by: Martin Townsend <mtownsend1973@gmail.com>
Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
---
 include/net/6lowpan.h         | 10 ++++++----
 net/6lowpan/iphc.c            | 12 +++++++-----
 net/bluetooth/6lowpan.c       | 15 ++++++++-------
 net/ieee802154/6lowpan_rtnl.c | 15 ++++++++-------
 4 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
index abfa359..dc03d77 100644
--- a/include/net/6lowpan.h
+++ b/include/net/6lowpan.h
@@ -372,10 +372,12 @@ lowpan_uncompress_size(const struct sk_buff *skb, u16 *dgram_offset)
 	return skb->len + uncomp_header - ret;
 }
 
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
-		const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
-		const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
-		u8 iphc0, u8 iphc1);
+int
+lowpan_header_decompress(struct sk_buff *skb, struct net_device *dev,
+			 const u8 *saddr, const u8 saddr_type,
+			 const u8 saddr_len, const u8 *daddr,
+			 const u8 daddr_type, const u8 daddr_len,
+			 u8 iphc0, u8 iphc1);
 int lowpan_header_compress(struct sk_buff *skb, struct net_device *dev,
 			unsigned short type, const void *_daddr,
 			const void *_saddr, unsigned int len);
diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 45714fe..73a7065 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -301,10 +301,12 @@ err:
 /* TTL uncompression values */
 static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
 
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
-			const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
-			const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
-			u8 iphc0, u8 iphc1)
+int
+lowpan_header_decompress(struct sk_buff *skb, struct net_device *dev,
+			 const u8 *saddr, const u8 saddr_type,
+			 const u8 saddr_len, const u8 *daddr,
+			 const u8 daddr_type, const u8 daddr_len,
+			 u8 iphc0, u8 iphc1)
 {
 	struct ipv6hdr hdr = {};
 	u8 tmp, num_context = 0;
@@ -480,7 +482,7 @@ drop:
 	kfree_skb(skb);
 	return -EINVAL;
 }
-EXPORT_SYMBOL_GPL(lowpan_process_data);
+EXPORT_SYMBOL_GPL(lowpan_header_decompress);
 
 static u8 lowpan_compress_addr_64(u8 **hc_ptr, u8 shift,
 				  const struct in6_addr *ipaddr,
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index 6c3182f..13b326d 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -257,8 +257,8 @@ static int give_skb_to_upper(struct sk_buff *skb, struct net_device *dev)
 	return netif_rx(skb_cp);
 }
 
-static int process_data(struct sk_buff *skb, struct net_device *netdev,
-			struct l2cap_chan *chan)
+static int iphc_decompress(struct sk_buff *skb, struct net_device *netdev,
+			   struct l2cap_chan *chan)
 {
 	const u8 *saddr, *daddr;
 	u8 iphc0, iphc1;
@@ -287,10 +287,11 @@ static int process_data(struct sk_buff *skb, struct net_device *netdev,
 	if (lowpan_fetch_skb_u8(skb, &iphc1))
 		goto drop;
 
-	return lowpan_process_data(skb, netdev,
-				   saddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
-				   daddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
-				   iphc0, iphc1);
+	return lowpan_header_decompress(skb, netdev,
+					saddr, IEEE802154_ADDR_LONG,
+					EUI64_ADDR_LEN, daddr,
+					IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
+					iphc0, iphc1);
 
 drop:
 	kfree_skb(skb);
@@ -346,7 +347,7 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
 			if (!local_skb)
 				goto drop;
 
-			ret = process_data(local_skb, dev, chan);
+			ret = iphc_decompress(local_skb, dev, chan);
 			if (ret < 0)
 				goto drop;
 
diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
index a160d0b..b98eede 100644
--- a/net/ieee802154/6lowpan_rtnl.c
+++ b/net/ieee802154/6lowpan_rtnl.c
@@ -164,7 +164,8 @@ static int lowpan_give_skb_to_devices(struct sk_buff *skb,
 	return stat;
 }
 
-static int process_data(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
+static int
+iphc_decompress(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
 {
 	u8 iphc0, iphc1;
 	struct ieee802154_addr_sa sa, da;
@@ -194,9 +195,9 @@ static int process_data(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
 	else
 		dap = &da.hwaddr;
 
-	return lowpan_process_data(skb, skb->dev, sap, sa.addr_type,
-				   IEEE802154_ADDR_LEN, dap, da.addr_type,
-				   IEEE802154_ADDR_LEN, iphc0, iphc1);
+	return lowpan_header_decompress(skb, skb->dev, sap, sa.addr_type,
+					IEEE802154_ADDR_LEN, dap, da.addr_type,
+					IEEE802154_ADDR_LEN, iphc0, iphc1);
 
 drop:
 	kfree_skb(skb);
@@ -538,14 +539,14 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
 	} else {
 		switch (skb->data[0] & 0xe0) {
 		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
-			ret = process_data(skb, &hdr);
+			ret = iphc_decompress(skb, &hdr);
 			if (ret < 0)
 				goto drop;
 			break;
 		case LOWPAN_DISPATCH_FRAG1:	/* first fragment header */
 			ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAG1);
 			if (ret == 1) {
-				ret = process_data(skb, &hdr);
+				ret = iphc_decompress(skb, &hdr);
 				if (ret < 0)
 					goto drop;
 			}
@@ -553,7 +554,7 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
 		case LOWPAN_DISPATCH_FRAGN:	/* next fragments headers */
 			ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN);
 			if (ret == 1) {
-				ret = process_data(skb, &hdr);
+				ret = iphc_decompress(skb, &hdr);
 				if (ret < 0)
 					goto drop;
 			}
-- 
1.9.1