[PATCH][linux-bluetooth 0/3] Fix lowpan_rcv

[PATCH][linux-bluetooth 0/3] Fix lowpan_rcv

[Martin Townsend]

This series aims to cleanup the lowpan receive path and fix a few issues. 
RFC compliancy by supported fragmented uncompressed IPv6 headers
Fix incorrect return values in lowpan_rcv
To achieve this it also refactors the receive path to
  1) free skb only from lowpan_rcv and not functions that it calls
  2) move skb delivery from IPHC

Martin Townsend (3):
  6lowpan: skb freed locally from lowpan_rcv
  6lowpan: Move skb delivery from IPHC.
  6lowpan: Refactored lowpan_rcv so it's RFC compliant

I have only compile tested the changes for bluetooth as I don't have any HW
available so would be grateful for any testing from the Bluetooth developers.
I have done some minimal testing for IEEE802.15.4


 include/net/6lowpan.h         | 22 ++++++++++-
 net/6lowpan/iphc.c            | 80 ++++++++++++++++----------------------
 net/bluetooth/6lowpan.c       | 20 +++++++---
 net/ieee802154/6lowpan_rtnl.c | 90 +++++++++++++++++++++++++------------------
 4 files changed, 120 insertions(+), 92 deletions(-)

-- 
1.9.1

[PATCH][linux-bluetooth 0/3] Fix lowpan_rcv

[Martin Townsend]

This series aims to cleanup the lowpan receive path and fix a few issues. 
RFC compliancy by supported fragmented uncompressed IPv6 headers
Fix incorrect return values in lowpan_rcv
To achieve this it also refactors the receive path to
  1) free skb only from lowpan_rcv and not functions that it calls
  2) move skb delivery from IPHC

Martin Townsend (3):
  6lowpan: skb freed locally from lowpan_rcv
  6lowpan: Move skb delivery from IPHC.
  6lowpan: Refactored lowpan_rcv so it's RFC compliant

 include/net/6lowpan.h         | 22 ++++++++++-
 net/6lowpan/iphc.c            | 80 ++++++++++++++++----------------------
 net/bluetooth/6lowpan.c       | 20 +++++++---
 net/ieee802154/6lowpan_rtnl.c | 90 +++++++++++++++++++++++++------------------
 4 files changed, 120 insertions(+), 92 deletions(-)

-- 
1.9.1

[PATCH][linux-bluetooth 1/3] 6lowpan: skb freed locally from lowpan_rcv

[Martin Townsend]

Currently there are a number of error paths in the lowpan_rcv function that
free the skb before returning, this patch simplifies the receive path by
ensuring that the skb is only freed from this function.

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

diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
index d184df1..883e7a2 100644
--- a/include/net/6lowpan.h
+++ b/include/net/6lowpan.h
@@ -374,7 +374,7 @@ lowpan_uncompress_size(const struct sk_buff *skb, u16 *dgram_offset)
 
 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,
+int lowpan_process_data(struct sk_buff **skb_inout, 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);
diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 142eef5..eca24bf 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -179,11 +179,11 @@ static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
 
 	new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
 			      GFP_ATOMIC);
-	kfree_skb(skb);
-
 	if (!new)
 		return -ENOMEM;
 
+	kfree_skb(skb);
+
 	skb_push(new, sizeof(struct ipv6hdr));
 	skb_reset_network_header(new);
 	skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
@@ -196,6 +196,8 @@ static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
 		       new->data, new->len);
 
 	stat = deliver_skb(new, dev);
+	if (stat < 0)
+		stat = NET_RX_DROP;
 
 	kfree_skb(new);
 
@@ -332,7 +334,7 @@ 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,
+int lowpan_process_data(struct sk_buff **skb_inout, 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)
@@ -340,6 +342,8 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 	struct ipv6hdr hdr = {};
 	u8 tmp, num_context = 0;
 	int err;
+	int err_ret = -EINVAL;
+	struct sk_buff *skb = *skb_inout;
 
 	raw_dump_table(__func__, "raw skb data dump uncompressed",
 		       skb->data, skb->len);
@@ -460,7 +464,6 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 	/* UDP data uncompression */
 	if (iphc0 & LOWPAN_IPHC_NH_C) {
 		struct udphdr uh;
-		struct sk_buff *new;
 
 		if (uncompress_udp_header(skb, &uh))
 			goto drop;
@@ -468,14 +471,15 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 		/* replace the compressed UDP head by the uncompressed UDP
 		 * header
 		 */
-		new = skb_copy_expand(skb, sizeof(struct udphdr),
+		skb = skb_copy_expand(skb, sizeof(struct udphdr),
 				      skb_tailroom(skb), GFP_ATOMIC);
-		kfree_skb(skb);
-
-		if (!new)
-			return -ENOMEM;
+		if (!skb) {
+			err_ret = -ENOMEM;
+			goto drop;
+		}
 
-		skb = new;
+		kfree_skb(*skb_inout);
+		*skb_inout = skb;
 
 		skb_push(skb, sizeof(struct udphdr));
 		skb_reset_transport_header(skb);
@@ -502,8 +506,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 	return skb_deliver(skb, &hdr, dev, deliver_skb);
 
 drop:
-	kfree_skb(skb);
-	return -EINVAL;
+	return err_ret;
 }
 EXPORT_SYMBOL_GPL(lowpan_process_data);
 
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index 206b65c..f5f7ee0 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -215,7 +215,7 @@ static int give_skb_to_upper(struct sk_buff *skb, struct net_device *dev)
 	return ret;
 }
 
-static int process_data(struct sk_buff *skb, struct net_device *netdev,
+static int process_data(struct sk_buff **skb_inout, struct net_device *netdev,
 			struct l2cap_chan *chan)
 {
 	const u8 *saddr, *daddr;
@@ -223,6 +223,7 @@ static int process_data(struct sk_buff *skb, struct net_device *netdev,
 	struct lowpan_dev *dev;
 	struct lowpan_peer *peer;
 	unsigned long flags;
+	struct sk_buff *skb = *skb_inout;
 
 	dev = lowpan_dev(netdev);
 
@@ -245,13 +246,12 @@ 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,
+	return lowpan_process_data(skb_inout, netdev,
 				   saddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
 				   daddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
 				   iphc0, iphc1, give_skb_to_upper);
 
 drop:
-	kfree_skb(skb);
 	return -EINVAL;
 }
 
@@ -300,7 +300,11 @@ 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 = process_data(&local_skb, dev, chan);
+			if (ret < 0) {
+				kfree_skb(local_skb);
+				goto drop;
+			}
 			if (ret != NET_RX_SUCCESS)
 				goto drop;
 
diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
index 6591d27..fdb6c10 100644
--- a/net/ieee802154/6lowpan_rtnl.c
+++ b/net/ieee802154/6lowpan_rtnl.c
@@ -167,11 +167,12 @@ 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 process_data(struct sk_buff **skb_inout, const struct ieee802154_hdr *hdr)
 {
 	u8 iphc0, iphc1;
 	struct ieee802154_addr_sa sa, da;
 	void *sap, *dap;
+	struct sk_buff *skb = *skb_inout;
 
 	raw_dump_table(__func__, "raw skb data dump", skb->data, skb->len);
 	/* at least two bytes will be used for the encoding */
@@ -197,13 +198,12 @@ 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,
+	return lowpan_process_data(skb_inout, skb->dev, sap, sa.addr_type,
 				   IEEE802154_ADDR_LEN, dap, da.addr_type,
 				   IEEE802154_ADDR_LEN, iphc0, iphc1,
 				   lowpan_give_skb_to_devices);
 
 drop:
-	kfree_skb(skb);
 	return -EINVAL;
 }
 
@@ -490,14 +490,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 = process_data(&skb, &hdr);
 			if (ret == NET_RX_DROP)
 				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 = process_data(&skb, &hdr);
 				if (ret == NET_RX_DROP)
 					goto drop;
 			}
@@ -505,7 +505,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 = process_data(&skb, &hdr);
 				if (ret == NET_RX_DROP)
 					goto drop;
 			}
-- 
1.9.1

Re: [PATCH][linux-bluetooth 1/3] 6lowpan: skb freed locally from lowpan_rcv

[Alexander Aring]

Hi Martin,

On Wed, Sep 10, 2014 at 03:06:06PM +0100, Martin Townsend wrote:
> Currently there are a number of error paths in the lowpan_rcv function that
> free the skb before returning, this patch simplifies the receive path by
> ensuring that the skb is only freed from this function.
> 
> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
> ---
>  include/net/6lowpan.h         |  2 +-
>  net/6lowpan/iphc.c            | 27 +++++++++++++++------------
>  net/bluetooth/6lowpan.c       | 12 ++++++++----
>  net/ieee802154/6lowpan_rtnl.c | 12 ++++++------
>  4 files changed, 30 insertions(+), 23 deletions(-)
> 
> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
> index d184df1..883e7a2 100644
> --- a/include/net/6lowpan.h
> +++ b/include/net/6lowpan.h
> @@ -374,7 +374,7 @@ lowpan_uncompress_size(const struct sk_buff *skb, u16 *dgram_offset)
>  
>  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,
> +int lowpan_process_data(struct sk_buff **skb_inout, 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);
> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
> index 142eef5..eca24bf 100644
> --- a/net/6lowpan/iphc.c
> +++ b/net/6lowpan/iphc.c
> @@ -179,11 +179,11 @@ static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
>  
>  	new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
>  			      GFP_ATOMIC);
> -	kfree_skb(skb);
> -
>  	if (!new)
>  		return -ENOMEM;
>  
> +	kfree_skb(skb);
> +
>  	skb_push(new, sizeof(struct ipv6hdr));
>  	skb_reset_network_header(new);
>  	skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
> @@ -196,6 +196,8 @@ static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
>  		       new->data, new->len);
>  
>  	stat = deliver_skb(new, dev);
> +	if (stat < 0)
> +		stat = NET_RX_DROP;
>  

We should be ensure that "lowpan_process_data" return an errno not
NET_RX_DROP which is 1, so the usual check on error with if (err < 0)
doesn't work here.

Now the skb_deliver function returns the return value of skb_deliver and
we should not returning NET_RX_DROP here.

The deliver_skb variable is a callback, so please be sure that all existsing
functionpointer make a conversion from NET_RX_DROP to errno (maybe -EIO).
The thing is that netif_rx returns NET_RX_DROP and we should directly convert
it to an errno after calling.

>  	kfree_skb(new);
>  
> @@ -332,7 +334,7 @@ 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,
> +int lowpan_process_data(struct sk_buff **skb_inout, 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)
> @@ -340,6 +342,8 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>  	struct ipv6hdr hdr = {};
>  	u8 tmp, num_context = 0;
>  	int err;
> +	int err_ret = -EINVAL;

simple, use err here? No more variables to putting on the stack.

> +	struct sk_buff *skb = *skb_inout;
>  
>  	raw_dump_table(__func__, "raw skb data dump uncompressed",
>  		       skb->data, skb->len);
> @@ -460,7 +464,6 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>  	/* UDP data uncompression */
>  	if (iphc0 & LOWPAN_IPHC_NH_C) {
>  		struct udphdr uh;
> -		struct sk_buff *new;
>  
>  		if (uncompress_udp_header(skb, &uh))
>  			goto drop;
> @@ -468,14 +471,15 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>  		/* replace the compressed UDP head by the uncompressed UDP
>  		 * header
>  		 */
> -		new = skb_copy_expand(skb, sizeof(struct udphdr),
> +		skb = skb_copy_expand(skb, sizeof(struct udphdr),
>  				      skb_tailroom(skb), GFP_ATOMIC);
> -		kfree_skb(skb);
> -
> -		if (!new)
> -			return -ENOMEM;
> +		if (!skb) {
> +			err_ret = -ENOMEM;
> +			goto drop;
> +		}
>  
> -		skb = new;
> +		kfree_skb(*skb_inout);
> +		*skb_inout = skb;
>  
>  		skb_push(skb, sizeof(struct udphdr));
>  		skb_reset_transport_header(skb);
> @@ -502,8 +506,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>  	return skb_deliver(skb, &hdr, dev, deliver_skb);
>  
>  drop:
> -	kfree_skb(skb);
> -	return -EINVAL;
> +	return err_ret;

return err; - when you remove the err_ret variable. Then we always
return the right indicator for the error. Sometimes useful for
debugging.

>  }
>  EXPORT_SYMBOL_GPL(lowpan_process_data);
>  
> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> index 206b65c..f5f7ee0 100644
> --- a/net/bluetooth/6lowpan.c
> +++ b/net/bluetooth/6lowpan.c
> @@ -215,7 +215,7 @@ static int give_skb_to_upper(struct sk_buff *skb, struct net_device *dev)
>  	return ret;
>  }
>  
> -static int process_data(struct sk_buff *skb, struct net_device *netdev,
> +static int process_data(struct sk_buff **skb_inout, struct net_device *netdev,
>  			struct l2cap_chan *chan)
>  {
>  	const u8 *saddr, *daddr;
> @@ -223,6 +223,7 @@ static int process_data(struct sk_buff *skb, struct net_device *netdev,
>  	struct lowpan_dev *dev;
>  	struct lowpan_peer *peer;
>  	unsigned long flags;
> +	struct sk_buff *skb = *skb_inout;
>  
>  	dev = lowpan_dev(netdev);
>  
> @@ -245,13 +246,12 @@ 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,
> +	return lowpan_process_data(skb_inout, netdev,
>  				   saddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
>  				   daddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
>  				   iphc0, iphc1, give_skb_to_upper);
>  
>  drop:
> -	kfree_skb(skb);
>  	return -EINVAL;
>  }
>  
> @@ -300,7 +300,11 @@ 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 = process_data(&local_skb, dev, chan);
> +			if (ret < 0) {
> +				kfree_skb(local_skb);
> +				goto drop;
> +			}
>  			if (ret != NET_RX_SUCCESS)
>  				goto drop;

remove checking on NET_RX_SUCCESS. Exactly at this place we should
convert the errno again to the NET_RX_DROP, because we are in the packet
layer receive function. You already did that in if (ret < 0), but ret !=
NET_RX_SUCCESS should never occur then.

>  
> diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
> index 6591d27..fdb6c10 100644
> --- a/net/ieee802154/6lowpan_rtnl.c
> +++ b/net/ieee802154/6lowpan_rtnl.c
> @@ -167,11 +167,12 @@ 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 process_data(struct sk_buff **skb_inout, const struct ieee802154_hdr *hdr)
>  {
>  	u8 iphc0, iphc1;
>  	struct ieee802154_addr_sa sa, da;
>  	void *sap, *dap;
> +	struct sk_buff *skb = *skb_inout;
>  
>  	raw_dump_table(__func__, "raw skb data dump", skb->data, skb->len);
>  	/* at least two bytes will be used for the encoding */
> @@ -197,13 +198,12 @@ 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,
> +	return lowpan_process_data(skb_inout, skb->dev, sap, sa.addr_type,
>  				   IEEE802154_ADDR_LEN, dap, da.addr_type,
>  				   IEEE802154_ADDR_LEN, iphc0, iphc1,
>  				   lowpan_give_skb_to_devices);
>  
>  drop:
> -	kfree_skb(skb);
>  	return -EINVAL;
>  }
>  
> @@ -490,14 +490,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 = process_data(&skb, &hdr);
>  			if (ret == NET_RX_DROP)
>  				goto drop;

the conversion from errno to NET_RX_DROP here.

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 = process_data(&skb, &hdr);
>  				if (ret == NET_RX_DROP)
>  					goto drop;

same here.

>  			}
> @@ -505,7 +505,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 = process_data(&skb, &hdr);
>  				if (ret == NET_RX_DROP)
>  					goto drop;

same here.

Frag handling for freeing skb's on error is different here... which
makes me lot of thinking currently. If we have a error in lowpan_frag_rcv,
the return value is -1 but skb already freed. We should return NET_RX_DROP
here. Need to think about that..., for now also check on else if (ret == -1)
and start conversion to NET_RX_DROP. Please make this as an new patch
after that. Also send it to bluetooth (simple because it's a bug and depends
on this then).

- Alex

Re: [PATCH][linux-bluetooth 1/3] 6lowpan: skb freed locally from lowpan_rcv

[Alexander Aring]

On Thu, Sep 11, 2014 at 09:58:30AM +0200, Alexander Aring wrote:
...
> >  
> > @@ -490,14 +490,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 = process_data(&skb, &hdr);
> >  			if (ret == NET_RX_DROP)
> >  				goto drop;
> 
> the conversion from errno to NET_RX_DROP here.
> 
> if (ret < 0)
>         goto drop;
> 

should be more drop_skb here, since we removed the kfree_skb in
process_data. ... I know the error handling is a mess, but was also
before I start to working with it. :-)

Thanks, that you accept this challenge to fix it. ;-)

- Alex

Re: [PATCH][linux-bluetooth 1/3] 6lowpan: skb freed locally from lowpan_rcv

[Martin Townsend]

Hi Alex,

Thanks for reviewing,  this patch is part of the solution with the next patch, please take this into consideration.  Sorry I should have put this in the cover letter.
The idea is to finally split skb delivery out of IPHC so the iphc decompress function(s) just return an error code at which point we can free skb and return from lowpan_rcv.  If IPHC is successful then skb delivery is done from receive function removing the need for all these mixed return codes.  I think this will make the code simpler, ... hopefully :)

- Martin. 

On 11/09/14 08:58, Alexander Aring wrote:
> Hi Martin,
>
> On Wed, Sep 10, 2014 at 03:06:06PM +0100, Martin Townsend wrote:
>> Currently there are a number of error paths in the lowpan_rcv function that
>> free the skb before returning, this patch simplifies the receive path by
>> ensuring that the skb is only freed from this function.
>>
>> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
>> ---
>>  include/net/6lowpan.h         |  2 +-
>>  net/6lowpan/iphc.c            | 27 +++++++++++++++------------
>>  net/bluetooth/6lowpan.c       | 12 ++++++++----
>>  net/ieee802154/6lowpan_rtnl.c | 12 ++++++------
>>  4 files changed, 30 insertions(+), 23 deletions(-)
>>
>> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
>> index d184df1..883e7a2 100644
>> --- a/include/net/6lowpan.h
>> +++ b/include/net/6lowpan.h
>> @@ -374,7 +374,7 @@ lowpan_uncompress_size(const struct sk_buff *skb, u16 *dgram_offset)
>>  
>>  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,
>> +int lowpan_process_data(struct sk_buff **skb_inout, 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);
>> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
>> index 142eef5..eca24bf 100644
>> --- a/net/6lowpan/iphc.c
>> +++ b/net/6lowpan/iphc.c
>> @@ -179,11 +179,11 @@ static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
>>  
>>  	new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
>>  			      GFP_ATOMIC);
>> -	kfree_skb(skb);
>> -
>>  	if (!new)
>>  		return -ENOMEM;
>>  
>> +	kfree_skb(skb);
>> +
>>  	skb_push(new, sizeof(struct ipv6hdr));
>>  	skb_reset_network_header(new);
>>  	skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
>> @@ -196,6 +196,8 @@ static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
>>  		       new->data, new->len);
>>  
>>  	stat = deliver_skb(new, dev);
>> +	if (stat < 0)
>> +		stat = NET_RX_DROP;
>>  
> We should be ensure that "lowpan_process_data" return an errno not
> NET_RX_DROP which is 1, so the usual check on error with if (err < 0)
> doesn't work here.
>
> Now the skb_deliver function returns the return value of skb_deliver and
> we should not returning NET_RX_DROP here.
>
> The deliver_skb variable is a callback, so please be sure that all existsing
> functionpointer make a conversion from NET_RX_DROP to errno (maybe -EIO).
> The thing is that netif_rx returns NET_RX_DROP and we should directly convert
> it to an errno after calling.
>
>>  	kfree_skb(new);
>>  
>> @@ -332,7 +334,7 @@ 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,
>> +int lowpan_process_data(struct sk_buff **skb_inout, 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)
>> @@ -340,6 +342,8 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>  	struct ipv6hdr hdr = {};
>>  	u8 tmp, num_context = 0;
>>  	int err;
>> +	int err_ret = -EINVAL;
> simple, use err here? No more variables to putting on the stack.
ok.
>
>> +	struct sk_buff *skb = *skb_inout;
>>  
>>  	raw_dump_table(__func__, "raw skb data dump uncompressed",
>>  		       skb->data, skb->len);
>> @@ -460,7 +464,6 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>  	/* UDP data uncompression */
>>  	if (iphc0 & LOWPAN_IPHC_NH_C) {
>>  		struct udphdr uh;
>> -		struct sk_buff *new;
>>  
>>  		if (uncompress_udp_header(skb, &uh))
>>  			goto drop;
>> @@ -468,14 +471,15 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>  		/* replace the compressed UDP head by the uncompressed UDP
>>  		 * header
>>  		 */
>> -		new = skb_copy_expand(skb, sizeof(struct udphdr),
>> +		skb = skb_copy_expand(skb, sizeof(struct udphdr),
>>  				      skb_tailroom(skb), GFP_ATOMIC);
>> -		kfree_skb(skb);
>> -
>> -		if (!new)
>> -			return -ENOMEM;
>> +		if (!skb) {
>> +			err_ret = -ENOMEM;
>> +			goto drop;
>> +		}
>>  
>> -		skb = new;
>> +		kfree_skb(*skb_inout);
>> +		*skb_inout = skb;
>>  
>>  		skb_push(skb, sizeof(struct udphdr));
>>  		skb_reset_transport_header(skb);
>> @@ -502,8 +506,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>  	return skb_deliver(skb, &hdr, dev, deliver_skb);
>>  
>>  drop:
>> -	kfree_skb(skb);
>> -	return -EINVAL;
>> +	return err_ret;
> return err; - when you remove the err_ret variable. Then we always
> return the right indicator for the error. Sometimes useful for
> debugging.
>
>>  }
>>  EXPORT_SYMBOL_GPL(lowpan_process_data);
>>  
>> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
>> index 206b65c..f5f7ee0 100644
>> --- a/net/bluetooth/6lowpan.c
>> +++ b/net/bluetooth/6lowpan.c
>> @@ -215,7 +215,7 @@ static int give_skb_to_upper(struct sk_buff *skb, struct net_device *dev)
>>  	return ret;
>>  }
>>  
>> -static int process_data(struct sk_buff *skb, struct net_device *netdev,
>> +static int process_data(struct sk_buff **skb_inout, struct net_device *netdev,
>>  			struct l2cap_chan *chan)
>>  {
>>  	const u8 *saddr, *daddr;
>> @@ -223,6 +223,7 @@ static int process_data(struct sk_buff *skb, struct net_device *netdev,
>>  	struct lowpan_dev *dev;
>>  	struct lowpan_peer *peer;
>>  	unsigned long flags;
>> +	struct sk_buff *skb = *skb_inout;
>>  
>>  	dev = lowpan_dev(netdev);
>>  
>> @@ -245,13 +246,12 @@ 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,
>> +	return lowpan_process_data(skb_inout, netdev,
>>  				   saddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
>>  				   daddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
>>  				   iphc0, iphc1, give_skb_to_upper);
>>  
>>  drop:
>> -	kfree_skb(skb);
>>  	return -EINVAL;
>>  }
>>  
>> @@ -300,7 +300,11 @@ 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 = process_data(&local_skb, dev, chan);
>> +			if (ret < 0) {
>> +				kfree_skb(local_skb);
>> +				goto drop;
>> +			}
>>  			if (ret != NET_RX_SUCCESS)
>>  				goto drop;
> remove checking on NET_RX_SUCCESS. Exactly at this place we should
> convert the errno again to the NET_RX_DROP, because we are in the packet
> layer receive function. You already did that in if (ret < 0), but ret !=
> NET_RX_SUCCESS should never occur then.
>
>>  
>> diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
>> index 6591d27..fdb6c10 100644
>> --- a/net/ieee802154/6lowpan_rtnl.c
>> +++ b/net/ieee802154/6lowpan_rtnl.c
>> @@ -167,11 +167,12 @@ 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 process_data(struct sk_buff **skb_inout, const struct ieee802154_hdr *hdr)
>>  {
>>  	u8 iphc0, iphc1;
>>  	struct ieee802154_addr_sa sa, da;
>>  	void *sap, *dap;
>> +	struct sk_buff *skb = *skb_inout;
>>  
>>  	raw_dump_table(__func__, "raw skb data dump", skb->data, skb->len);
>>  	/* at least two bytes will be used for the encoding */
>> @@ -197,13 +198,12 @@ 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,
>> +	return lowpan_process_data(skb_inout, skb->dev, sap, sa.addr_type,
>>  				   IEEE802154_ADDR_LEN, dap, da.addr_type,
>>  				   IEEE802154_ADDR_LEN, iphc0, iphc1,
>>  				   lowpan_give_skb_to_devices);
>>  
>>  drop:
>> -	kfree_skb(skb);
>>  	return -EINVAL;
>>  }
>>  
>> @@ -490,14 +490,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 = process_data(&skb, &hdr);
>>  			if (ret == NET_RX_DROP)
>>  				goto drop;
> the conversion from errno to NET_RX_DROP here.
>
> 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 = process_data(&skb, &hdr);
>>  				if (ret == NET_RX_DROP)
>>  					goto drop;
> same here.
>
>>  			}
>> @@ -505,7 +505,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 = process_data(&skb, &hdr);
>>  				if (ret == NET_RX_DROP)
>>  					goto drop;
> same here.
>
> Frag handling for freeing skb's on error is different here... which
> makes me lot of thinking currently. If we have a error in lowpan_frag_rcv,
> the return value is -1 but skb already freed. We should return NET_RX_DROP
> here. Need to think about that..., for now also check on else if (ret == -1)
> and start conversion to NET_RX_DROP. Please make this as an new patch
> after that. Also send it to bluetooth (simple because it's a bug and depends
> on this then).
>
> - Alex

[PATCH][linux-bluetooth 2/3] 6lowpan: Move skb delivery from IPHC.

[Martin Townsend]

Passing the skb from 6lowpan up to the higher layers is not a
function of IPHC.  By moving it out of IPHC we also remove the
need to support error code returns with NET_RX codes.
It also makes the lowpan_rcv function more extendable as we
can support more compression schemes.

Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
---
 include/net/6lowpan.h         |  5 ++--
 net/6lowpan/iphc.c            | 63 +++++++++++++++++--------------------------
 net/bluetooth/6lowpan.c       | 16 ++++++-----
 net/ieee802154/6lowpan_rtnl.c | 43 +++++++++++++++++------------
 4 files changed, 63 insertions(+), 64 deletions(-)

diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
index 883e7a2..d71c062 100644
--- a/include/net/6lowpan.h
+++ b/include/net/6lowpan.h
@@ -374,10 +374,11 @@ lowpan_uncompress_size(const struct sk_buff *skb, u16 *dgram_offset)
 
 typedef int (*skb_delivery_cb)(struct sk_buff *skb, struct net_device *dev);
 
-int lowpan_process_data(struct sk_buff **skb_inout, struct net_device *dev,
+int lowpan_iphc_header_uncompress(
+		struct sk_buff **skb_inout, 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 eca24bf..33985b9 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -171,39 +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)
-{
-	struct sk_buff *new;
-	int stat;
-
-	new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
-			      GFP_ATOMIC);
-	if (!new)
-		return -ENOMEM;
-
-	kfree_skb(skb);
-
-	skb_push(new, sizeof(struct ipv6hdr));
-	skb_reset_network_header(new);
-	skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
-
-	new->protocol = htons(ETH_P_IPV6);
-	new->pkt_type = PACKET_HOST;
-	new->dev = dev;
-
-	raw_dump_table(__func__, "raw skb data dump before receiving",
-		       new->data, new->len);
-
-	stat = deliver_skb(new, dev);
-	if (stat < 0)
-		stat = NET_RX_DROP;
-
-	kfree_skb(new);
-
-	return stat;
-}
-
 /* Uncompress function for multicast destination address,
  * when M bit is set.
  */
@@ -334,10 +301,11 @@ err:
 /* TTL uncompression values */
 static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
 
-int lowpan_process_data(struct sk_buff **skb_inout, 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)
+int lowpan_iphc_header_uncompress(struct sk_buff **skb_inout,
+				  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;
@@ -503,12 +471,29 @@ int lowpan_process_data(struct sk_buff **skb_inout, struct net_device *dev,
 
 	raw_dump_table(__func__, "raw header dump", (u8 *)&hdr, sizeof(hdr));
 
-	return skb_deliver(skb, &hdr, dev, deliver_skb);
+	/* Setup skb with new uncompressed IPv6 header. */
+	skb = skb_copy_expand(skb, sizeof(hdr), skb_tailroom(skb),
+			      GFP_ATOMIC);
+	if (!skb)
+		return -ENOMEM;
+
+	kfree_skb(*skb_inout);
+	*skb_inout = skb;
+
+	skb_push(skb, sizeof(hdr));
+	skb_reset_network_header(skb);
+	skb_copy_to_linear_data(skb, &hdr, sizeof(hdr));
+	skb->dev = dev;
+
+	raw_dump_table(__func__, "raw skb data dump before receiving",
+		       skb->data, skb->len);
+
+	return 0;
 
 drop:
 	return err_ret;
 }
-EXPORT_SYMBOL_GPL(lowpan_process_data);
+EXPORT_SYMBOL_GPL(lowpan_iphc_header_uncompress);
 
 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 f5f7ee0..6a76d44 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -246,10 +246,10 @@ static int process_data(struct sk_buff **skb_inout, struct net_device *netdev,
 	if (lowpan_fetch_skb_u8(skb, &iphc1))
 		goto drop;
 
-	return lowpan_process_data(skb_inout, netdev,
+	return lowpan_iphc_header_uncompress(skb_inout, netdev,
 				   saddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
 				   daddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
-				   iphc0, iphc1, give_skb_to_upper);
+				   iphc0, iphc1);
 
 drop:
 	return -EINVAL;
@@ -280,9 +280,6 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
 		local_skb->protocol = htons(ETH_P_IPV6);
 		local_skb->pkt_type = PACKET_HOST;
 
-		skb_reset_network_header(local_skb);
-		skb_set_transport_header(local_skb, sizeof(struct ipv6hdr));
-
 		if (give_skb_to_upper(local_skb, dev) != NET_RX_SUCCESS) {
 			kfree_skb(local_skb);
 			goto drop;
@@ -305,8 +302,15 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
 				kfree_skb(local_skb);
 				goto drop;
 			}
-			if (ret != NET_RX_SUCCESS)
+
+			local_skb->protocol = htons(ETH_P_IPV6);
+			local_skb->pkt_type = PACKET_HOST;
+
+			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 fdb6c10..4f4b02d 100644
--- a/net/ieee802154/6lowpan_rtnl.c
+++ b/net/ieee802154/6lowpan_rtnl.c
@@ -167,7 +167,8 @@ static int lowpan_give_skb_to_devices(struct sk_buff *skb,
 	return stat;
 }
 
-static int process_data(struct sk_buff **skb_inout, const struct ieee802154_hdr *hdr)
+static int iphc_uncompress_hdr(struct sk_buff **skb_inout,
+			       const struct ieee802154_hdr *hdr)
 {
 	u8 iphc0, iphc1;
 	struct ieee802154_addr_sa sa, da;
@@ -198,10 +199,10 @@ static int process_data(struct sk_buff **skb_inout, const struct ieee802154_hdr
 	else
 		dap = &da.hwaddr;
 
-	return lowpan_process_data(skb_inout, skb->dev, sap, sa.addr_type,
+	return lowpan_iphc_header_uncompress(skb_inout, 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:
 	return -EINVAL;
@@ -478,36 +479,35 @@ 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;
+		ret = NET_RX_SUCCESS;
 	} else {
 		switch (skb->data[0] & 0xe0) {
 		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
-			ret = process_data(&skb, &hdr);
-			if (ret == NET_RX_DROP)
+			ret = iphc_uncompress_hdr(&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);
-				if (ret == NET_RX_DROP)
+				ret = iphc_uncompress_hdr(&skb, &hdr);
+				if (ret < 0)
 					goto drop;
+			} else {
+				return NET_RX_SUCCESS;
 			}
 			break;
 		case LOWPAN_DISPATCH_FRAGN:	/* next fragments headers */
 			ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN);
 			if (ret == 1) {
-				ret = process_data(&skb, &hdr);
-				if (ret == NET_RX_DROP)
+				ret = iphc_uncompress_hdr(&skb, &hdr);
+				if (ret < 0)
 					goto drop;
+			} else {
+				return NET_RX_SUCCESS;
 			}
 			break;
 		default:
@@ -515,7 +515,16 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
 		}
 	}
 
-	return NET_RX_SUCCESS;
+	/* Pass IPv6 packet up to next layer */
+	skb->protocol = htons(ETH_P_IPV6);
+	skb->pkt_type = PACKET_HOST;
+	ret = lowpan_give_skb_to_devices(skb, NULL);
+	if (ret < 0)
+		goto drop_skb;
+
+	kfree_skb(skb);
+	return ret;
+
 drop_skb:
 	kfree_skb(skb);
 drop:
-- 
1.9.1

Re: [PATCH][linux-bluetooth 2/3] 6lowpan: Move skb delivery from IPHC.

[Alexander Aring]

On Wed, Sep 10, 2014 at 03:06:07PM +0100, Martin Townsend wrote:
> Passing the skb from 6lowpan up to the higher layers is not a
> function of IPHC.  By moving it out of IPHC we also remove the
> need to support error code returns with NET_RX codes.
> It also makes the lowpan_rcv function more extendable as we
> can support more compression schemes.
> 

I will ack this. But please sperate this patch in two. First renaming
the function namens and then removing deliver callback.

btw. The correct tag is bluetooth not linux-bluetooth, or bluetooth-next.



Also this doesn't fix anything? Then this is for bluetooth-next. I know
this depends on the Patch 1/3. Marcel, do you have any a nice solution
about this, that we can deal with huge fixes in bluetooth and new features
for bluetooth-next. Or simple wait when it's merged?

- Alex

Re: [PATCH][linux-bluetooth 2/3] 6lowpan: Move skb delivery from IPHC.

[Martin Townsend]

Hi Alex,

On 11/09/14 09:18, Alexander Aring wrote:
> On Wed, Sep 10, 2014 at 03:06:07PM +0100, Martin Townsend wrote:
>> Passing the skb from 6lowpan up to the higher layers is not a
>> function of IPHC.  By moving it out of IPHC we also remove the
>> need to support error code returns with NET_RX codes.
>> It also makes the lowpan_rcv function more extendable as we
>> can support more compression schemes.
>>
> I will ack this. But please sperate this patch in two. First renaming
> the function namens and then removing deliver callback.
ok, but should this not be the other way around
moving delivery into receive and then by doing this processs_data naturally becomes IPHC decompress so it can be renamed.
>
> btw. The correct tag is bluetooth not linux-bluetooth, or bluetooth-next.
>
>
>
> Also this doesn't fix anything? Then this is for bluetooth-next. I know
> this depends on the Patch 1/3. Marcel, do you have any a nice solution
> about this, that we can deal with huge fixes in bluetooth and new features
> for bluetooth-next. Or simple wait when it's merged?
I disagree, this with the previous patch fixes error handling in lowpan_rcv.  By moving the skb delivery out of IPHC you automatically fix the nightmare which is returning a mixture of NET_RX codes with error codes.  IPHC now only returns error codes or success.  Delivery is done where is should be in the receive function and can deal with NET_RX codes.
>
> - 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][linux-bluetooth 2/3] 6lowpan: Move skb delivery from IPHC.

[Alexander Aring]

Hi Martin,

On Thu, Sep 11, 2014 at 09:25:56AM +0100, Martin Townsend wrote:
> Hi Alex,
> 
> On 11/09/14 09:18, Alexander Aring wrote:
> > On Wed, Sep 10, 2014 at 03:06:07PM +0100, Martin Townsend wrote:
> >> Passing the skb from 6lowpan up to the higher layers is not a
> >> function of IPHC.  By moving it out of IPHC we also remove the
> >> need to support error code returns with NET_RX codes.
> >> It also makes the lowpan_rcv function more extendable as we
> >> can support more compression schemes.
> >>
> > I will ack this. But please sperate this patch in two. First renaming
> > the function namens and then removing deliver callback.
> ok, but should this not be the other way around
> moving delivery into receive and then by doing this processs_data naturally becomes IPHC decompress so it can be renamed.
> >
> > btw. The correct tag is bluetooth not linux-bluetooth, or bluetooth-next.
> >
> >
> >
> > Also this doesn't fix anything? Then this is for bluetooth-next. I know
> > this depends on the Patch 1/3. Marcel, do you have any a nice solution
> > about this, that we can deal with huge fixes in bluetooth and new features
> > for bluetooth-next. Or simple wait when it's merged?
> I disagree, this with the previous patch fixes error handling in lowpan_rcv.  By moving the skb delivery out of IPHC you automatically fix the nightmare which is returning a mixture of NET_RX codes with error codes.  IPHC now only returns error codes or success.  Delivery is done where is should be in the receive function and can deal with NET_RX codes.

ok. When this is a part of the fix and 1/3 "prepare" the fix, then put
this handling into patch "1/3" to really fix the issue from patch 1/3.

- Alex

Re: [PATCH][linux-bluetooth 2/3] 6lowpan: Move skb delivery from IPHC.

[Martin Townsend]

Hi Alex,

On 11/09/14 10:01, Alexander Aring wrote:
> Hi Martin,
>
> On Thu, Sep 11, 2014 at 09:25:56AM +0100, Martin Townsend wrote:
>> Hi Alex,
>>
>> On 11/09/14 09:18, Alexander Aring wrote:
>>> On Wed, Sep 10, 2014 at 03:06:07PM +0100, Martin Townsend wrote:
>>>> Passing the skb from 6lowpan up to the higher layers is not a
>>>> function of IPHC.  By moving it out of IPHC we also remove the
>>>> need to support error code returns with NET_RX codes.
>>>> It also makes the lowpan_rcv function more extendable as we
>>>> can support more compression schemes.
>>>>
>>> I will ack this. But please sperate this patch in two. First renaming
>>> the function namens and then removing deliver callback.
>> ok, but should this not be the other way around
>> moving delivery into receive and then by doing this processs_data naturally becomes IPHC decompress so it can be renamed.
>>> btw. The correct tag is bluetooth not linux-bluetooth, or bluetooth-next.
>>>
>>>
>>>
>>> Also this doesn't fix anything? Then this is for bluetooth-next. I know
>>> this depends on the Patch 1/3. Marcel, do you have any a nice solution
>>> about this, that we can deal with huge fixes in bluetooth and new features
>>> for bluetooth-next. Or simple wait when it's merged?
>> I disagree, this with the previous patch fixes error handling in lowpan_rcv.  By moving the skb delivery out of IPHC you automatically fix the nightmare which is returning a mixture of NET_RX codes with error codes.  IPHC now only returns error codes or success.  Delivery is done where is should be in the receive function and can deal with NET_RX codes.
> ok. When this is a part of the fix and 1/3 "prepare" the fix, then put
> this handling into patch "1/3" to really fix the issue from patch 1/3.
I'm sorry I don't quite understand.  Are you saying that I should combine patches 1 and 2 into a single patch?

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

Re: [PATCH][linux-bluetooth 2/3] 6lowpan: Move skb delivery from IPHC.

[Alexander Aring]

On Thu, Sep 11, 2014 at 10:33:33AM +0100, Martin Townsend wrote:
> Hi Alex,
> 
> On 11/09/14 10:01, Alexander Aring wrote:
> > Hi Martin,
> >
> > On Thu, Sep 11, 2014 at 09:25:56AM +0100, Martin Townsend wrote:
> >> Hi Alex,
> >>
> >> On 11/09/14 09:18, Alexander Aring wrote:
> >>> On Wed, Sep 10, 2014 at 03:06:07PM +0100, Martin Townsend wrote:
> >>>> Passing the skb from 6lowpan up to the higher layers is not a
> >>>> function of IPHC.  By moving it out of IPHC we also remove the
> >>>> need to support error code returns with NET_RX codes.
> >>>> It also makes the lowpan_rcv function more extendable as we
> >>>> can support more compression schemes.
> >>>>
> >>> I will ack this. But please sperate this patch in two. First renaming
> >>> the function namens and then removing deliver callback.
> >> ok, but should this not be the other way around
> >> moving delivery into receive and then by doing this processs_data naturally becomes IPHC decompress so it can be renamed.
> >>> btw. The correct tag is bluetooth not linux-bluetooth, or bluetooth-next.
> >>>
> >>>
> >>>
> >>> Also this doesn't fix anything? Then this is for bluetooth-next. I know
> >>> this depends on the Patch 1/3. Marcel, do you have any a nice solution
> >>> about this, that we can deal with huge fixes in bluetooth and new features
> >>> for bluetooth-next. Or simple wait when it's merged?
> >> I disagree, this with the previous patch fixes error handling in lowpan_rcv.  By moving the skb delivery out of IPHC you automatically fix the nightmare which is returning a mixture of NET_RX codes with error codes.  IPHC now only returns error codes or success.  Delivery is done where is should be in the receive function and can deal with NET_RX codes.
> > ok. When this is a part of the fix and 1/3 "prepare" the fix, then put
> > this handling into patch "1/3" to really fix the issue from patch 1/3.
> I'm sorry I don't quite understand.  Are you saying that I should combine patches 1 and 2 into a single patch?
> 

So far I understand this is also part of the fix from patch 1. So it's
necessary to have this in one patch, means between patch 1 and 2 it's
still broken. Or?

and remove the renaming. You can do that as cleanup into bluetooth-next.

- Alex

Re: [PATCH][linux-bluetooth 2/3] 6lowpan: Move skb delivery from IPHC.

[Martin Townsend]

Hi Alex,

On 11/09/14 10:53, Alexander Aring wrote:
> On Thu, Sep 11, 2014 at 10:33:33AM +0100, Martin Townsend wrote:
>> Hi Alex,
>>
>> On 11/09/14 10:01, Alexander Aring wrote:
>>> Hi Martin,
>>>
>>> On Thu, Sep 11, 2014 at 09:25:56AM +0100, Martin Townsend wrote:
>>>> Hi Alex,
>>>>
>>>> On 11/09/14 09:18, Alexander Aring wrote:
>>>>> On Wed, Sep 10, 2014 at 03:06:07PM +0100, Martin Townsend wrote:
>>>>>> Passing the skb from 6lowpan up to the higher layers is not a
>>>>>> function of IPHC.  By moving it out of IPHC we also remove the
>>>>>> need to support error code returns with NET_RX codes.
>>>>>> It also makes the lowpan_rcv function more extendable as we
>>>>>> can support more compression schemes.
>>>>>>
>>>>> I will ack this. But please sperate this patch in two. First renaming
>>>>> the function namens and then removing deliver callback.
>>>> ok, but should this not be the other way around
>>>> moving delivery into receive and then by doing this processs_data naturally becomes IPHC decompress so it can be renamed.
>>>>> btw. The correct tag is bluetooth not linux-bluetooth, or bluetooth-next.
>>>>>
>>>>>
>>>>>
>>>>> Also this doesn't fix anything? Then this is for bluetooth-next. I know
>>>>> this depends on the Patch 1/3. Marcel, do you have any a nice solution
>>>>> about this, that we can deal with huge fixes in bluetooth and new features
>>>>> for bluetooth-next. Or simple wait when it's merged?
>>>> I disagree, this with the previous patch fixes error handling in lowpan_rcv.  By moving the skb delivery out of IPHC you automatically fix the nightmare which is returning a mixture of NET_RX codes with error codes.  IPHC now only returns error codes or success.  Delivery is done where is should be in the receive function and can deal with NET_RX codes.
>>> ok. When this is a part of the fix and 1/3 "prepare" the fix, then put
>>> this handling into patch "1/3" to really fix the issue from patch 1/3.
>> I'm sorry I don't quite understand.  Are you saying that I should combine patches 1 and 2 into a single patch?
>>
> So far I understand this is also part of the fix from patch 1. So it's
> necessary to have this in one patch, means between patch 1 and 2 it's
> still broken. Or?
I can merge into 1 patch, makes sense to me.
>
> and remove the renaming. You can do that as cleanup into bluetooth-next.
I would argue that the name must be changed as we are changing what the function does and so the name doesn't match what the function now does.
> - 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

So my plan is to merge patches 1 and 2 into a single fix lowpan_rcv, incorporate your suggestions and send to bluetooth.   Separate patch 3 into a new patch for linux-wpan-next.  I think patch 3 should be to linux-wpan as it is fixing the fact that we don't correctly follow RFC 4944 and support fragmented IPv6 packets that have an uncompressed IPv6 header.  What are you're thoughts on this.

I would also be grateful for any testing by bluetooth developers as I can't physically test the code changes I have made to the bluetooth 6lowpan code.

- Martin.

Re: [PATCH][linux-bluetooth 2/3] 6lowpan: Move skb delivery from IPHC.

[Alexander Aring]

Hi Martin,

On Thu, Sep 11, 2014 at 11:12:09AM +0100, Martin Townsend wrote:
> Hi Alex,
> 
> On 11/09/14 10:53, Alexander Aring wrote:
> > On Thu, Sep 11, 2014 at 10:33:33AM +0100, Martin Townsend wrote:
> >> Hi Alex,
> >>
> >> On 11/09/14 10:01, Alexander Aring wrote:
> >>> Hi Martin,
> >>>
> >>> On Thu, Sep 11, 2014 at 09:25:56AM +0100, Martin Townsend wrote:
> >>>> Hi Alex,
> >>>>
> >>>> On 11/09/14 09:18, Alexander Aring wrote:
> >>>>> On Wed, Sep 10, 2014 at 03:06:07PM +0100, Martin Townsend wrote:
> >>>>>> Passing the skb from 6lowpan up to the higher layers is not a
> >>>>>> function of IPHC.  By moving it out of IPHC we also remove the
> >>>>>> need to support error code returns with NET_RX codes.
> >>>>>> It also makes the lowpan_rcv function more extendable as we
> >>>>>> can support more compression schemes.
> >>>>>>
> >>>>> I will ack this. But please sperate this patch in two. First renaming
> >>>>> the function namens and then removing deliver callback.
> >>>> ok, but should this not be the other way around
> >>>> moving delivery into receive and then by doing this processs_data naturally becomes IPHC decompress so it can be renamed.
> >>>>> btw. The correct tag is bluetooth not linux-bluetooth, or bluetooth-next.
> >>>>>
> >>>>>
> >>>>>
> >>>>> Also this doesn't fix anything? Then this is for bluetooth-next. I know
> >>>>> this depends on the Patch 1/3. Marcel, do you have any a nice solution
> >>>>> about this, that we can deal with huge fixes in bluetooth and new features
> >>>>> for bluetooth-next. Or simple wait when it's merged?
> >>>> I disagree, this with the previous patch fixes error handling in lowpan_rcv.  By moving the skb delivery out of IPHC you automatically fix the nightmare which is returning a mixture of NET_RX codes with error codes.  IPHC now only returns error codes or success.  Delivery is done where is should be in the receive function and can deal with NET_RX codes.
> >>> ok. When this is a part of the fix and 1/3 "prepare" the fix, then put
> >>> this handling into patch "1/3" to really fix the issue from patch 1/3.
> >> I'm sorry I don't quite understand.  Are you saying that I should combine patches 1 and 2 into a single patch?
> >>
> > So far I understand this is also part of the fix from patch 1. So it's
> > necessary to have this in one patch, means between patch 1 and 2 it's
> > still broken. Or?
> I can merge into 1 patch, makes sense to me.
> >

ok.

> > and remove the renaming. You can do that as cleanup into bluetooth-next.
> I would argue that the name must be changed as we are changing what the function does and so the name doesn't match what the function now does.

yea, but code runs fine without that. It's only a very ugly name for it.
This is a cleanup.

> > - 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
> 
> So my plan is to merge patches 1 and 2 into a single fix lowpan_rcv, incorporate your suggestions and send to bluetooth.   Separate patch 3 into a new patch for linux-wpan-next.  I think patch 3 should be to linux-wpan as it is fixing the fact that we don't correctly follow RFC 4944 and support fragmented IPv6 packets that have an uncompressed IPv6 header.  What are you're thoughts on this.
> 

If you like to have it for the current linux kernel and it's of course a
bug fix which we should support, because we support not compressed headers
without fragmentation. Then I can't/will not say make it for -next. So
goahead to make it for wpan. I am fine with that.

> I would also be grateful for any testing by bluetooth developers as I can't physically test the code changes I have made to the bluetooth 6lowpan code.
> 

I know what you mean...

I cc Jukka Rissanen, he implemented the 6lowpan stuff for bluetooth. I also
wanted that he becomes maintainer for 6LOWPAN_GENETIC too to review
the bluetooth side. Jukka what's about that... to be maintainer?

- Alex

Re: [PATCH][linux-bluetooth 2/3] 6lowpan: Move skb delivery from IPHC.

[Jukka Rissanen]

Hi Alex,

On to, 2014-09-11 at 12:25 +0200, Alexander Aring wrote:
> Hi Martin,
> 
> On Thu, Sep 11, 2014 at 11:12:09AM +0100, Martin Townsend wrote:
> > Hi Alex,
> > 

> > I would also be grateful for any testing by bluetooth developers as I can't physically test the code changes I have made to the bluetooth 6lowpan code.
> > 
> 
> I know what you mean...
> 
> I cc Jukka Rissanen, he implemented the 6lowpan stuff for bluetooth. I also
> wanted that he becomes maintainer for 6LOWPAN_GENETIC too to review
> the bluetooth side. Jukka what's about that... to be maintainer?

Sure, I can be the maintainer of the BT 6LoWPAN side. I will send patch
for MAINTAINERS file shortly.


Cheers,
Jukka

Re: [PATCH][linux-bluetooth 2/3] 6lowpan: Move skb delivery from IPHC.

[Marcel Holtmann]

Hi Alex,

>> Passing the skb from 6lowpan up to the higher layers is not a
>> function of IPHC.  By moving it out of IPHC we also remove the
>> need to support error code returns with NET_RX codes.
>> It also makes the lowpan_rcv function more extendable as we
>> can support more compression schemes.
>> 
> 
> I will ack this. But please sperate this patch in two. First renaming
> the function namens and then removing deliver callback.
> 
> btw. The correct tag is bluetooth not linux-bluetooth, or bluetooth-next.
> 
> 
> 
> Also this doesn't fix anything? Then this is for bluetooth-next. I know
> this depends on the Patch 1/3. Marcel, do you have any a nice solution
> about this, that we can deal with huge fixes in bluetooth and new features
> for bluetooth-next. Or simple wait when it's merged?

we need to get it into wireless first and pulled by davem. After that I can ask John to pull wireless tree back into wireless-next.

Regards

Marcel

[PATCH][linux-bluetooth 3/3] 6lowpan: Refactored lowpan_rcv so it's RFC compliant

[Martin Townsend]

Currently we support uncompressed IPv6 headers after performing fragmentation.

Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
---
 include/net/6lowpan.h         | 17 ++++++++++++
 net/ieee802154/6lowpan_rtnl.c | 63 +++++++++++++++++++++++--------------------
 2 files changed, 51 insertions(+), 29 deletions(-)

diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
index d71c062..71b1bf0 100644
--- a/include/net/6lowpan.h
+++ b/include/net/6lowpan.h
@@ -126,11 +126,28 @@
 	 (((a)[6]) == 0xFF) &&	\
 	 (((a)[7]) == 0xFF))
 
+#define lowpan_dispatch_is_nalp(a)	\
+	(((a) & LOWPAN_DISPATCH_MAJOR) == 0x00)
+
+#define lowpan_dispatch_is_mesh(a)	\
+	(((a) & LOWPAN_DISPATCH_MAJOR) == 0x80)
+
+#define lowpan_dispatch_is_broadcast(a)	\
+	((a) == LOWPAN_DISPATCH_BCAST)
+
+#define lowpan_dispatch_is_frag(a)	\
+	(((a) & LOWPAN_DISPATCH_MASK) == LOWPAN_DISPATCH_FRAG1 || \
+	 ((a) & LOWPAN_DISPATCH_MASK) == LOWPAN_DISPATCH_FRAGN)
+
+#define LOWPAN_DISPATCH_MAJOR	0xc0
+#define LOWPAN_DISPATCH_MINOR	0x3f
+
 #define LOWPAN_DISPATCH_IPV6	0x41 /* 01000001 = 65 */
 #define LOWPAN_DISPATCH_HC1	0x42 /* 01000010 = 66 */
 #define LOWPAN_DISPATCH_IPHC	0x60 /* 011xxxxx = ... */
 #define LOWPAN_DISPATCH_FRAG1	0xc0 /* 11000xxx */
 #define LOWPAN_DISPATCH_FRAGN	0xe0 /* 11100xxx */
+#define LOWPAN_DISPATCH_BCAST	0x50 /* 01010000 */
 
 #define LOWPAN_DISPATCH_MASK	0xf8 /* 11111000 */
 
diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
index 4f4b02d..1557ece 100644
--- a/net/ieee802154/6lowpan_rtnl.c
+++ b/net/ieee802154/6lowpan_rtnl.c
@@ -477,41 +477,46 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (ieee802154_hdr_peek_addrs(skb, &hdr) < 0)
 		goto drop_skb;
 
-	/* check that it's our buffer */
+	/* First off if frame is Not A LoWPAN Packet (NALP) then chuck away */
+	if (lowpan_dispatch_is_nalp(skb->data[0]))
+		goto drop_skb;
+
+	/* The 6LoWPAN header stack comprimises of the following (in order)
+	 *   Mesh
+	 *   Broadcast
+	 *   Fragmentation
+	 */
+	if (lowpan_dispatch_is_mesh(skb->data[0]))
+		/* Not supported */
+		goto drop_skb;
+
+	if (lowpan_dispatch_is_broadcast(skb->data[0]))
+		/* Not supported */
+		goto drop_skb;
+
+	if (lowpan_dispatch_is_frag(skb->data[0])) {
+		u8 frag_dispatch = skb->data[0] & 0xe0;
+
+		ret = lowpan_frag_rcv(skb, frag_dispatch);
+		if (ret != 1) {
+			/* more frags to process */
+			return NET_RX_SUCCESS;
+		}
+	}
+
+	/* We should now have an IPv6 Header (Compressed or Uncompressed) */
 	if (skb->data[0] == LOWPAN_DISPATCH_IPV6) {
-		/* Pull off the 1-byte of 6lowpan header. */
+		/* Uncompressed, Pull off the dispatch byte */
 		skb_pull(skb, 1);
-
-		ret = NET_RX_SUCCESS;
 	} else {
-		switch (skb->data[0] & 0xe0) {
-		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
+		if ((skb->data[0] & 0xe0) == LOWPAN_DISPATCH_IPHC) {
+			/* Compressed with IPHC - RFC 6282 */
 			ret = iphc_uncompress_hdr(&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 = iphc_uncompress_hdr(&skb, &hdr);
-				if (ret < 0)
-					goto drop;
-			} else {
-				return NET_RX_SUCCESS;
-			}
-			break;
-		case LOWPAN_DISPATCH_FRAGN:	/* next fragments headers */
-			ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN);
-			if (ret == 1) {
-				ret = iphc_uncompress_hdr(&skb, &hdr);
-				if (ret < 0)
-					goto drop;
-			} else {
-				return NET_RX_SUCCESS;
-			}
-			break;
-		default:
-			break;
+		} else {
+			/* TODO: other compression formats to follow */
+			goto drop_skb;
 		}
 	}
 
-- 
1.9.1

Re: [PATCH][linux-bluetooth 3/3] 6lowpan: Refactored lowpan_rcv so it's RFC compliant

[Alexander Aring]

Hi Martin,

On Wed, Sep 10, 2014 at 03:06:08PM +0100, Martin Townsend wrote:
> Currently we support uncompressed IPv6 headers after performing fragmentation.
> 

This patch is for wpan-next.

> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
> ---
>  include/net/6lowpan.h         | 17 ++++++++++++
>  net/ieee802154/6lowpan_rtnl.c | 63 +++++++++++++++++++++++--------------------
>  2 files changed, 51 insertions(+), 29 deletions(-)
> 
> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
> index d71c062..71b1bf0 100644
> --- a/include/net/6lowpan.h
> +++ b/include/net/6lowpan.h
> @@ -126,11 +126,28 @@
>  	 (((a)[6]) == 0xFF) &&	\
>  	 (((a)[7]) == 0xFF))
>  
> +#define lowpan_dispatch_is_nalp(a)	\
> +	(((a) & LOWPAN_DISPATCH_MAJOR) == 0x00)
> +
> +#define lowpan_dispatch_is_mesh(a)	\
> +	(((a) & LOWPAN_DISPATCH_MAJOR) == 0x80)
> +

don't use the MINOR MAJOR thing here, I can't see that this follow any
structure at RFC4944.

Simple add more mask and values like:

#define lowpan_dispatch_is_mesh(a)	\
	(((a) & LOWPAN_DISPATCH_MESH_MASK) == LOWPAN_DISPATCH_MESH)

Please see the note below that we don't put this stuff into genetic 6lowpan.

> +#define lowpan_dispatch_is_broadcast(a)	\
> +	((a) == LOWPAN_DISPATCH_BCAST)
> +
> +#define lowpan_dispatch_is_frag(a)	\
> +	(((a) & LOWPAN_DISPATCH_MASK) == LOWPAN_DISPATCH_FRAG1 || \
> +	 ((a) & LOWPAN_DISPATCH_MASK) == LOWPAN_DISPATCH_FRAGN)
> +
> +#define LOWPAN_DISPATCH_MAJOR	0xc0
> +#define LOWPAN_DISPATCH_MINOR	0x3f
> +
>  #define LOWPAN_DISPATCH_IPV6	0x41 /* 01000001 = 65 */
>  #define LOWPAN_DISPATCH_HC1	0x42 /* 01000010 = 66 */
>  #define LOWPAN_DISPATCH_IPHC	0x60 /* 011xxxxx = ... */
>  #define LOWPAN_DISPATCH_FRAG1	0xc0 /* 11000xxx */
>  #define LOWPAN_DISPATCH_FRAGN	0xe0 /* 11100xxx */
> +#define LOWPAN_DISPATCH_BCAST	0x50 /* 01010000 */
>  

hehe... yea another big issue is also that much stoff from ieee802154
foo is inside the include/net/6lowpan header. We should not make this
problem bigger. Only IPHC stuff there, which is used by bluetooth and
802154.

For ieee802154 6lowpan, we don't need a global header file.

Simple create one "net/ieee802154/6lowpan_i.h" - underscore means that's
internal header. These defines are only related to 802.15.4, e.g.
fragmentation on bluetooth is done at some mac mechanism (L2CAP, or
something else, that's what 6lowpan bluetooth draft says). So 6lowpan
fragmentation have nothing to do with bluetooth 6lowpan.

>  #define LOWPAN_DISPATCH_MASK	0xf8 /* 11111000 */
>  
> diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
> index 4f4b02d..1557ece 100644
> --- a/net/ieee802154/6lowpan_rtnl.c
> +++ b/net/ieee802154/6lowpan_rtnl.c
> @@ -477,41 +477,46 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
>  	if (ieee802154_hdr_peek_addrs(skb, &hdr) < 0)
>  		goto drop_skb;
>  
> -	/* check that it's our buffer */
> +	/* First off if frame is Not A LoWPAN Packet (NALP) then chuck away */
> +	if (lowpan_dispatch_is_nalp(skb->data[0]))
> +		goto drop_skb;
> +
> +	/* The 6LoWPAN header stack comprimises of the following (in order)
> +	 *   Mesh
> +	 *   Broadcast
> +	 *   Fragmentation
> +	 */
> +	if (lowpan_dispatch_is_mesh(skb->data[0]))

better is:

lowpan_dispatch_is_mesh(*skb_network_header(skb))

the network_header should be pointed to the beginng of 6LoWPAN header.

> +		/* Not supported */
> +		goto drop_skb;
> +
> +	if (lowpan_dispatch_is_broadcast(skb->data[0]))
> +		/* Not supported */
> +		goto drop_skb;
> +
> +	if (lowpan_dispatch_is_frag(skb->data[0])) {
> +		u8 frag_dispatch = skb->data[0] & 0xe0;
> +
> +		ret = lowpan_frag_rcv(skb, frag_dispatch);
> +		if (ret != 1) {
> +			/* more frags to process */
> +			return NET_RX_SUCCESS;
> +		}
> +	}

I know this issue and we should not do that in this way.

Why?

Because this works only for fragmentation with IPHC, for example if we
support mesh or Broadcast or HC1 compression. We should call after
successfully reassembled "means lowpan_frag_rcv returns 1" the lowpan_rcv again.
So this is a recursion and we don't should use recursion to much, but it
should only be one recursion, so I think that's okay. :-)

After successfully reassemble we simple evaluate the DISPATCH value
again, this is also more a bug (because we don't handle LOWPAN_DISPATCH_IPV6
with fragmentation, and this can happen... We already talk about the
issue "don't use IPHC if compressed header size don't fit in a single
frame" and this is handled if we simple calll lowpan_frag_rcv again
after successful reassembling. But I don't will take this as bugfix,
because new support of handling xy, so wpan-next should be okay.

Please separate also this from the "introduce new handling of dispatch
values".

> +
> +	/* We should now have an IPv6 Header (Compressed or Uncompressed) */
>  	if (skb->data[0] == LOWPAN_DISPATCH_IPV6) {
> -		/* Pull off the 1-byte of 6lowpan header. */
> +		/* Uncompressed, Pull off the dispatch byte */
>  		skb_pull(skb, 1);
> -
> -		ret = NET_RX_SUCCESS;
>  	} else {
> -		switch (skb->data[0] & 0xe0) {
> -		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
> +		if ((skb->data[0] & 0xe0) == LOWPAN_DISPATCH_IPHC) {
> +			/* Compressed with IPHC - RFC 6282 */
>  			ret = iphc_uncompress_hdr(&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 = iphc_uncompress_hdr(&skb, &hdr);
> -				if (ret < 0)
> -					goto drop;
> -			} else {
> -				return NET_RX_SUCCESS;
> -			}
> -			break;
> -		case LOWPAN_DISPATCH_FRAGN:	/* next fragments headers */
> -			ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN);
> -			if (ret == 1) {
> -				ret = iphc_uncompress_hdr(&skb, &hdr);
> -				if (ret < 0)
> -					goto drop;
> -			} else {
> -				return NET_RX_SUCCESS;
> -			}
> -			break;
> -		default:
> -			break;
> +		} else {
> +			/* TODO: other compression formats to follow */

don't know why we check at first for some types which we don't support
and drop it and then we have this here and drop also unknown types.

I would like it to check on all RFC4944 types here. And then we know
here that it was some invalid frame.

- Alex

Re: [PATCH][linux-bluetooth 3/3] 6lowpan: Refactored lowpan_rcv so it's RFC compliant

[Alexander Aring]

Hi Martin,

On Thu, Sep 11, 2014 at 10:53:53AM +0200, Alexander Aring wrote:
...
> 
> I know this issue and we should not do that in this way.
> 
> Why?
> 
> Because this works only for fragmentation with IPHC, for example if we
> support mesh or Broadcast or HC1 compression. We should call after
> successfully reassembled "means lowpan_frag_rcv returns 1" the lowpan_rcv again.
> So this is a recursion and we don't should use recursion to much, but it
> should only be one recursion, so I think that's okay. :-)
> 

I reconsider about that, this is not okay. A attacker can send data to
occur this stack overflow...

We need another solution for this. Maybe your current one, but handling
fragmentation at the beginning and then evaulate dispatch values.

- Alex

Re: [PATCH][linux-bluetooth 3/3] 6lowpan: Refactored lowpan_rcv so it's RFC compliant

[Alexander Aring]

On Thu, Sep 11, 2014 at 11:09:50AM +0200, Alexander Aring wrote:
> Hi Martin,
> 
> On Thu, Sep 11, 2014 at 10:53:53AM +0200, Alexander Aring wrote:
> ...
> > 
> > I know this issue and we should not do that in this way.
> > 
> > Why?
> > 
> > Because this works only for fragmentation with IPHC, for example if we
> > support mesh or Broadcast or HC1 compression. We should call after
> > successfully reassembled "means lowpan_frag_rcv returns 1" the lowpan_rcv again.
> > So this is a recursion and we don't should use recursion to much, but it
> > should only be one recursion, so I think that's okay. :-)
> > 
> 
> I reconsider about that, this is not okay. A attacker can send data to
> occur this stack overflow...
> 
> We need another solution for this. Maybe your current one, but handling
> fragmentation at the beginning and then evaulate dispatch values.
> 

I look more in RFC 4944, it seems that mesh and BC0 and MESH always fits
into a single fragmentation... but they don't say anything about max
value and if we have encryption on... I am not sure now if there is a case
where this can happen or not. Simple -> check fragmentation if
fragmentation then goahead until it's reassembled. After reassembled
check for all other dispatch values.

This should be sure that we handle all packets if fragmented or not.

- Alex

Re: [PATCH][linux-bluetooth 3/3] 6lowpan: Refactored lowpan_rcv so it's RFC compliant

[Martin Townsend]

Hi Alex,

On 11/09/14 09:53, Alexander Aring wrote:
> Hi Martin,
>
> On Wed, Sep 10, 2014 at 03:06:08PM +0100, Martin Townsend wrote:
>> Currently we support uncompressed IPv6 headers after performing fragmentation.
>>
> This patch is for wpan-next.
ok, but it depends on the previous patches so how should I handle this?
>
>> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
>> ---
>>  include/net/6lowpan.h         | 17 ++++++++++++
>>  net/ieee802154/6lowpan_rtnl.c | 63 +++++++++++++++++++++++--------------------
>>  2 files changed, 51 insertions(+), 29 deletions(-)
>>
>> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
>> index d71c062..71b1bf0 100644
>> --- a/include/net/6lowpan.h
>> +++ b/include/net/6lowpan.h
>> @@ -126,11 +126,28 @@
>>  	 (((a)[6]) == 0xFF) &&	\
>>  	 (((a)[7]) == 0xFF))
>>  
>> +#define lowpan_dispatch_is_nalp(a)	\
>> +	(((a) & LOWPAN_DISPATCH_MAJOR) == 0x00)
>> +
>> +#define lowpan_dispatch_is_mesh(a)	\
>> +	(((a) & LOWPAN_DISPATCH_MAJOR) == 0x80)
>> +
> don't use the MINOR MAJOR thing here, I can't see that this follow any
> structure at RFC4944.
ok, but the spec hints at it by separating out the first two bits in Figure 2.  Also table 2.1 in "6LoWPAN: The wireless embedded internet" book explicitly states that this is what the coding of the dispatch byte is, which is where I took it from.
>
> Simple add more mask and values like:
>
> #define lowpan_dispatch_is_mesh(a)	\
> 	(((a) & LOWPAN_DISPATCH_MESH_MASK) == LOWPAN_DISPATCH_MESH)
>
> Please see the note below that we don't put this stuff into genetic 6lowpan.
>
>> +#define lowpan_dispatch_is_broadcast(a)	\
>> +	((a) == LOWPAN_DISPATCH_BCAST)
>> +
>> +#define lowpan_dispatch_is_frag(a)	\
>> +	(((a) & LOWPAN_DISPATCH_MASK) == LOWPAN_DISPATCH_FRAG1 || \
>> +	 ((a) & LOWPAN_DISPATCH_MASK) == LOWPAN_DISPATCH_FRAGN)
>> +
>> +#define LOWPAN_DISPATCH_MAJOR	0xc0
>> +#define LOWPAN_DISPATCH_MINOR	0x3f
>> +
>>  #define LOWPAN_DISPATCH_IPV6	0x41 /* 01000001 = 65 */
>>  #define LOWPAN_DISPATCH_HC1	0x42 /* 01000010 = 66 */
>>  #define LOWPAN_DISPATCH_IPHC	0x60 /* 011xxxxx = ... */
>>  #define LOWPAN_DISPATCH_FRAG1	0xc0 /* 11000xxx */
>>  #define LOWPAN_DISPATCH_FRAGN	0xe0 /* 11100xxx */
>> +#define LOWPAN_DISPATCH_BCAST	0x50 /* 01010000 */
>>  
> hehe... yea another big issue is also that much stoff from ieee802154
> foo is inside the include/net/6lowpan header. We should not make this
> problem bigger. Only IPHC stuff there, which is used by bluetooth and
> 802154.
>
> For ieee802154 6lowpan, we don't need a global header file.
>
> Simple create one "net/ieee802154/6lowpan_i.h" - underscore means that's
> internal header. These defines are only related to 802.15.4, e.g.
> fragmentation on bluetooth is done at some mac mechanism (L2CAP, or
> something else, that's what 6lowpan bluetooth draft says). So 6lowpan
> fragmentation have nothing to do with bluetooth 6lowpan.
ok.  isn't this a separate fix/patch though as the problem already exists?
>>  #define LOWPAN_DISPATCH_MASK	0xf8 /* 11111000 */
>>  
>> diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
>> index 4f4b02d..1557ece 100644
>> --- a/net/ieee802154/6lowpan_rtnl.c
>> +++ b/net/ieee802154/6lowpan_rtnl.c
>> @@ -477,41 +477,46 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
>>  	if (ieee802154_hdr_peek_addrs(skb, &hdr) < 0)
>>  		goto drop_skb;
>>  
>> -	/* check that it's our buffer */
>> +	/* First off if frame is Not A LoWPAN Packet (NALP) then chuck away */
>> +	if (lowpan_dispatch_is_nalp(skb->data[0]))
>> +		goto drop_skb;
>> +
>> +	/* The 6LoWPAN header stack comprimises of the following (in order)
>> +	 *   Mesh
>> +	 *   Broadcast
>> +	 *   Fragmentation
>> +	 */
>> +	if (lowpan_dispatch_is_mesh(skb->data[0]))
> better is:
>
> lowpan_dispatch_is_mesh(*skb_network_header(skb))
>
> the network_header should be pointed to the beginng of 6LoWPAN header.
yep, looks a lot better.
>> +		/* Not supported */
>> +		goto drop_skb;
>> +
>> +	if (lowpan_dispatch_is_broadcast(skb->data[0]))
>> +		/* Not supported */
>> +		goto drop_skb;
>> +
>> +	if (lowpan_dispatch_is_frag(skb->data[0])) {
>> +		u8 frag_dispatch = skb->data[0] & 0xe0;
>> +
>> +		ret = lowpan_frag_rcv(skb, frag_dispatch);
>> +		if (ret != 1) {
>> +			/* more frags to process */
>> +			return NET_RX_SUCCESS;
>> +		}
>> +	}
> I know this issue and we should not do that in this way.
>
> Why?
>
> Because this works only for fragmentation with IPHC, for example if we
> support mesh or Broadcast or HC1 compression. We should call after
> successfully reassembled "means lowpan_frag_rcv returns 1" the lowpan_rcv again.
> So this is a recursion and we don't should use recursion to much, but it
> should only be one recursion, so I think that's okay. :-)
The spec says that the headers of the 6LoWPAN frame must fit in the first fragment.  You need to process these headers to get to the fragmentation header, this is why the code is this way.
>
> After successfully reassemble we simple evaluate the DISPATCH value
> again, this is also more a bug (because we don't handle LOWPAN_DISPATCH_IPV6
> with fragmentation, and this can happen... We already talk about the
> issue "don't use IPHC if compressed header size don't fit in a single
> frame" and this is handled if we simple calll lowpan_frag_rcv again
> after successful reassembling. But I don't will take this as bugfix,
> because new support of handling xy, so wpan-next should be okay.
>
> Please separate also this from the "introduce new handling of dispatch
> values".
>
>> +
>> +	/* We should now have an IPv6 Header (Compressed or Uncompressed) */
>>  	if (skb->data[0] == LOWPAN_DISPATCH_IPV6) {
>> -		/* Pull off the 1-byte of 6lowpan header. */
>> +		/* Uncompressed, Pull off the dispatch byte */
>>  		skb_pull(skb, 1);
>> -
>> -		ret = NET_RX_SUCCESS;
>>  	} else {
>> -		switch (skb->data[0] & 0xe0) {
>> -		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
>> +		if ((skb->data[0] & 0xe0) == LOWPAN_DISPATCH_IPHC) {
>> +			/* Compressed with IPHC - RFC 6282 */
>>  			ret = iphc_uncompress_hdr(&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 = iphc_uncompress_hdr(&skb, &hdr);
>> -				if (ret < 0)
>> -					goto drop;
>> -			} else {
>> -				return NET_RX_SUCCESS;
>> -			}
>> -			break;
>> -		case LOWPAN_DISPATCH_FRAGN:	/* next fragments headers */
>> -			ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN);
>> -			if (ret == 1) {
>> -				ret = iphc_uncompress_hdr(&skb, &hdr);
>> -				if (ret < 0)
>> -					goto drop;
>> -			} else {
>> -				return NET_RX_SUCCESS;
>> -			}
>> -			break;
>> -		default:
>> -			break;
>> +		} else {
>> +			/* TODO: other compression formats to follow */
> don't know why we check at first for some types which we don't support
> and drop it and then we have this here and drop also unknown types.
The first part deals with the 6LoWAN header stack.  Once header stack is processed then we decompress the remaining IPv6 headers so here we also throw away frames that we don't support decompression for.  Once your NHC compression change is in I imagine this will be handled :)
> I would like it to check on all RFC4944 types here. And then we know
> here that it was some invalid frame.
>
> - Alex

- Martin.

Re: [PATCH][linux-bluetooth 3/3] 6lowpan: Refactored lowpan_rcv so it's RFC compliant

[Alexander Aring]

On Thu, Sep 11, 2014 at 10:30:46AM +0100, Martin Townsend wrote:
> Hi Alex,
> 
> On 11/09/14 09:53, Alexander Aring wrote:
> > Hi Martin,
> >
> > On Wed, Sep 10, 2014 at 03:06:08PM +0100, Martin Townsend wrote:
> >> Currently we support uncompressed IPv6 headers after performing fragmentation.
> >>
> > This patch is for wpan-next.
> ok, but it depends on the previous patches so how should I handle this?
> >
> >> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
> >> ---
> >>  include/net/6lowpan.h         | 17 ++++++++++++
> >>  net/ieee802154/6lowpan_rtnl.c | 63 +++++++++++++++++++++++--------------------
> >>  2 files changed, 51 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
> >> index d71c062..71b1bf0 100644
> >> --- a/include/net/6lowpan.h
> >> +++ b/include/net/6lowpan.h
> >> @@ -126,11 +126,28 @@
> >>  	 (((a)[6]) == 0xFF) &&	\
> >>  	 (((a)[7]) == 0xFF))
> >>  
> >> +#define lowpan_dispatch_is_nalp(a)	\
> >> +	(((a) & LOWPAN_DISPATCH_MAJOR) == 0x00)
> >> +
> >> +#define lowpan_dispatch_is_mesh(a)	\
> >> +	(((a) & LOWPAN_DISPATCH_MAJOR) == 0x80)
> >> +
> > don't use the MINOR MAJOR thing here, I can't see that this follow any
> > structure at RFC4944.
> ok, but the spec hints at it by separating out the first two bits in Figure 2.  Also table 2.1 in "6LoWPAN: The wireless embedded internet" book explicitly states that this is what the coding of the dispatch byte is, which is where I took it from.

Okay, so MAJOR = '11' is only FRAG thing. Now I can see it and yes you
are right.

Maybe then we should something like that. For example fragmentation handling:

First evaluate the MAJOR -> goes in some function e.g. lowpan_frag_handling
or something like that and then evaluate MINOR here for FRAG1 or FRAGN.

But this is out of scope of this patch. I would accept this patch to
introduce the defines, later we can do it as cleanup.

> >
> > Simple add more mask and values like:
> >
> > #define lowpan_dispatch_is_mesh(a)	\
> > 	(((a) & LOWPAN_DISPATCH_MESH_MASK) == LOWPAN_DISPATCH_MESH)
> >
> > Please see the note below that we don't put this stuff into genetic 6lowpan.
> >
> >> +#define lowpan_dispatch_is_broadcast(a)	\
> >> +	((a) == LOWPAN_DISPATCH_BCAST)
> >> +
> >> +#define lowpan_dispatch_is_frag(a)	\
> >> +	(((a) & LOWPAN_DISPATCH_MASK) == LOWPAN_DISPATCH_FRAG1 || \
> >> +	 ((a) & LOWPAN_DISPATCH_MASK) == LOWPAN_DISPATCH_FRAGN)
> >> +
> >> +#define LOWPAN_DISPATCH_MAJOR	0xc0
> >> +#define LOWPAN_DISPATCH_MINOR	0x3f
> >> +
> >>  #define LOWPAN_DISPATCH_IPV6	0x41 /* 01000001 = 65 */
> >>  #define LOWPAN_DISPATCH_HC1	0x42 /* 01000010 = 66 */
> >>  #define LOWPAN_DISPATCH_IPHC	0x60 /* 011xxxxx = ... */
> >>  #define LOWPAN_DISPATCH_FRAG1	0xc0 /* 11000xxx */
> >>  #define LOWPAN_DISPATCH_FRAGN	0xe0 /* 11100xxx */
> >> +#define LOWPAN_DISPATCH_BCAST	0x50 /* 01010000 */
> >>  
> > hehe... yea another big issue is also that much stoff from ieee802154
> > foo is inside the include/net/6lowpan header. We should not make this
> > problem bigger. Only IPHC stuff there, which is used by bluetooth and
> > 802154.
> >
> > For ieee802154 6lowpan, we don't need a global header file.
> >
> > Simple create one "net/ieee802154/6lowpan_i.h" - underscore means that's
> > internal header. These defines are only related to 802.15.4, e.g.
> > fragmentation on bluetooth is done at some mac mechanism (L2CAP, or
> > something else, that's what 6lowpan bluetooth draft says). So 6lowpan
> > fragmentation have nothing to do with bluetooth 6lowpan.
> ok.  isn't this a separate fix/patch though as the problem already exists?

yea, cleanup. If you like you can do it. Go into bluetooth-next. The
iphc header have also a include of the af_802154 header which is very
very bad. The thing is we need more generic EUI64 defines instead to use
the LONG_ADDRESS (correct word is EXTENDED, not LONG) defines.

EUI64 is more generic which is a synonym for an simple IEEE EUI64
address, used by 802.15.4 and bluetooth. bluetooth need to fill the
ff:fe bits to generate the EUI64 but both can also be named EUI64.

We need handling for the SHORT address, that's not EUI16 or something
else... need some special defines, it's only 802.15.4 relevant.

> >>  #define LOWPAN_DISPATCH_MASK	0xf8 /* 11111000 */
> >>  
> >> diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
> >> index 4f4b02d..1557ece 100644
> >> --- a/net/ieee802154/6lowpan_rtnl.c
> >> +++ b/net/ieee802154/6lowpan_rtnl.c
> >> @@ -477,41 +477,46 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
> >>  	if (ieee802154_hdr_peek_addrs(skb, &hdr) < 0)
> >>  		goto drop_skb;
> >>  
> >> -	/* check that it's our buffer */
> >> +	/* First off if frame is Not A LoWPAN Packet (NALP) then chuck away */
> >> +	if (lowpan_dispatch_is_nalp(skb->data[0]))
> >> +		goto drop_skb;
> >> +
> >> +	/* The 6LoWPAN header stack comprimises of the following (in order)
> >> +	 *   Mesh
> >> +	 *   Broadcast
> >> +	 *   Fragmentation
> >> +	 */
> >> +	if (lowpan_dispatch_is_mesh(skb->data[0]))
> > better is:
> >
> > lowpan_dispatch_is_mesh(*skb_network_header(skb))
> >
> > the network_header should be pointed to the beginng of 6LoWPAN header.
> yep, looks a lot better.
> >> +		/* Not supported */
> >> +		goto drop_skb;
> >> +
> >> +	if (lowpan_dispatch_is_broadcast(skb->data[0]))
> >> +		/* Not supported */
> >> +		goto drop_skb;
> >> +
> >> +	if (lowpan_dispatch_is_frag(skb->data[0])) {
> >> +		u8 frag_dispatch = skb->data[0] & 0xe0;
> >> +
> >> +		ret = lowpan_frag_rcv(skb, frag_dispatch);
> >> +		if (ret != 1) {
> >> +			/* more frags to process */
> >> +			return NET_RX_SUCCESS;
> >> +		}
> >> +	}
> > I know this issue and we should not do that in this way.
> >
> > Why?
> >
> > Because this works only for fragmentation with IPHC, for example if we
> > support mesh or Broadcast or HC1 compression. We should call after
> > successfully reassembled "means lowpan_frag_rcv returns 1" the lowpan_rcv again.
> > So this is a recursion and we don't should use recursion to much, but it
> > should only be one recursion, so I think that's okay. :-)
> The spec says that the headers of the 6LoWPAN frame must fit in the first fragment.  You need to process these headers to get to the fragmentation header, this is why the code is this way.

yes this is for IPHC dispatch values your code works, I don't want to
say that it doesn't work. Because fragmentation is something to
reconstruct the full payload, we should first evaluate the fragmentation
dispatches and then all others. To be sure that we can use fragmentation
on any dispatch value which is not the fragmentation dispatch values.
:-)

Simple move it before nalp check. Maybe somebody fragment something and
the dispatch value after fragmentation is nalp. I know it should catch
the default branch above, but it's a little bit cleaner. I hope you are
in the same opinion.

> >
> > After successfully reassemble we simple evaluate the DISPATCH value
> > again, this is also more a bug (because we don't handle LOWPAN_DISPATCH_IPV6
> > with fragmentation, and this can happen... We already talk about the
> > issue "don't use IPHC if compressed header size don't fit in a single
> > frame" and this is handled if we simple calll lowpan_frag_rcv again
> > after successful reassembling. But I don't will take this as bugfix,
> > because new support of handling xy, so wpan-next should be okay.
> >
> > Please separate also this from the "introduce new handling of dispatch
> > values".
> >
> >> +
> >> +	/* We should now have an IPv6 Header (Compressed or Uncompressed) */
> >>  	if (skb->data[0] == LOWPAN_DISPATCH_IPV6) {
> >> -		/* Pull off the 1-byte of 6lowpan header. */
> >> +		/* Uncompressed, Pull off the dispatch byte */
> >>  		skb_pull(skb, 1);
> >> -
> >> -		ret = NET_RX_SUCCESS;
> >>  	} else {
> >> -		switch (skb->data[0] & 0xe0) {
> >> -		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
> >> +		if ((skb->data[0] & 0xe0) == LOWPAN_DISPATCH_IPHC) {
> >> +			/* Compressed with IPHC - RFC 6282 */
> >>  			ret = iphc_uncompress_hdr(&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 = iphc_uncompress_hdr(&skb, &hdr);
> >> -				if (ret < 0)
> >> -					goto drop;
> >> -			} else {
> >> -				return NET_RX_SUCCESS;
> >> -			}
> >> -			break;
> >> -		case LOWPAN_DISPATCH_FRAGN:	/* next fragments headers */
> >> -			ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN);
> >> -			if (ret == 1) {
> >> -				ret = iphc_uncompress_hdr(&skb, &hdr);
> >> -				if (ret < 0)
> >> -					goto drop;
> >> -			} else {
> >> -				return NET_RX_SUCCESS;
> >> -			}
> >> -			break;
> >> -		default:
> >> -			break;
> >> +		} else {
> >> +			/* TODO: other compression formats to follow */
> > don't know why we check at first for some types which we don't support
> > and drop it and then we have this here and drop also unknown types.
> The first part deals with the 6LoWAN header stack.  Once header stack is processed then we decompress the remaining IPv6 headers so here we also throw away frames that we don't support decompression for.  Once your NHC compression change is in I imagine this will be handled :)

ok. :-)

- Alex

Re: [PATCH][linux-bluetooth 3/3] 6lowpan: Refactored lowpan_rcv so it's RFC compliant

[Martin Townsend]

Hi Alex,

Reposting to everyone this time :)

On 11/09/14 10:50, Alexander Aring wrote:
> On Thu, Sep 11, 2014 at 10:30:46AM +0100, Martin Townsend wrote:
>> Hi Alex,
>>
>> On 11/09/14 09:53, Alexander Aring wrote:
>>> Hi Martin,
>>>
>>> On Wed, Sep 10, 2014 at 03:06:08PM +0100, Martin Townsend wrote:
>>>> Currently we support uncompressed IPv6 headers after performing fragmentation.
>>>>
>>> This patch is for wpan-next.
>> ok, but it depends on the previous patches so how should I handle this?
>>>> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
>>>> ---
>>>>  include/net/6lowpan.h         | 17 ++++++++++++
>>>>  net/ieee802154/6lowpan_rtnl.c | 63 +++++++++++++++++++++++--------------------
>>>>  2 files changed, 51 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
>>>> index d71c062..71b1bf0 100644
>>>> --- a/include/net/6lowpan.h
>>>> +++ b/include/net/6lowpan.h
>>>> @@ -126,11 +126,28 @@
>>>>  	 (((a)[6]) == 0xFF) &&	\
>>>>  	 (((a)[7]) == 0xFF))
>>>>  
>>>> +#define lowpan_dispatch_is_nalp(a)	\
>>>> +	(((a) & LOWPAN_DISPATCH_MAJOR) == 0x00)
>>>> +
>>>> +#define lowpan_dispatch_is_mesh(a)	\
>>>> +	(((a) & LOWPAN_DISPATCH_MAJOR) == 0x80)
>>>> +
>>> don't use the MINOR MAJOR thing here, I can't see that this follow any
>>> structure at RFC4944.
>> ok, but the spec hints at it by separating out the first two bits in Figure 2.  Also table 2.1 in "6LoWPAN: The wireless embedded internet" book explicitly states that this is what the coding of the dispatch byte is, which is where I took it from.
> Okay, so MAJOR = '11' is only FRAG thing. Now I can see it and yes you
> are right.
>
> Maybe then we should something like that. For example fragmentation handling:
>
> First evaluate the MAJOR -> goes in some function e.g. lowpan_frag_handling
> or something like that and then evaluate MINOR here for FRAG1 or FRAGN.
>
> But this is out of scope of this patch. I would accept this patch to
> introduce the defines, later we can do it as cleanup.
>
>>> Simple add more mask and values like:
>>>
>>> #define lowpan_dispatch_is_mesh(a)	\
>>> 	(((a) & LOWPAN_DISPATCH_MESH_MASK) == LOWPAN_DISPATCH_MESH)
>>>
>>> Please see the note below that we don't put this stuff into genetic 6lowpan.
>>>
>>>> +#define lowpan_dispatch_is_broadcast(a)	\
>>>> +	((a) == LOWPAN_DISPATCH_BCAST)
>>>> +
>>>> +#define lowpan_dispatch_is_frag(a)	\
>>>> +	(((a) & LOWPAN_DISPATCH_MASK) == LOWPAN_DISPATCH_FRAG1 || \
>>>> +	 ((a) & LOWPAN_DISPATCH_MASK) == LOWPAN_DISPATCH_FRAGN)
>>>> +
>>>> +#define LOWPAN_DISPATCH_MAJOR	0xc0
>>>> +#define LOWPAN_DISPATCH_MINOR	0x3f
>>>> +
>>>>  #define LOWPAN_DISPATCH_IPV6	0x41 /* 01000001 = 65 */
>>>>  #define LOWPAN_DISPATCH_HC1	0x42 /* 01000010 = 66 */
>>>>  #define LOWPAN_DISPATCH_IPHC	0x60 /* 011xxxxx = ... */
>>>>  #define LOWPAN_DISPATCH_FRAG1	0xc0 /* 11000xxx */
>>>>  #define LOWPAN_DISPATCH_FRAGN	0xe0 /* 11100xxx */
>>>> +#define LOWPAN_DISPATCH_BCAST	0x50 /* 01010000 */
>>>>  
>>> hehe... yea another big issue is also that much stoff from ieee802154
>>> foo is inside the include/net/6lowpan header. We should not make this
>>> problem bigger. Only IPHC stuff there, which is used by bluetooth and
>>> 802154.
>>>
>>> For ieee802154 6lowpan, we don't need a global header file.
>>>
>>> Simple create one "net/ieee802154/6lowpan_i.h" - underscore means that's
>>> internal header. These defines are only related to 802.15.4, e.g.
>>> fragmentation on bluetooth is done at some mac mechanism (L2CAP, or
>>> something else, that's what 6lowpan bluetooth draft says). So 6lowpan
>>> fragmentation have nothing to do with bluetooth 6lowpan.
>> ok.  isn't this a separate fix/patch though as the problem already exists?
> yea, cleanup. If you like you can do it. Go into bluetooth-next. The
> iphc header have also a include of the af_802154 header which is very
> very bad. The thing is we need more generic EUI64 defines instead to use
> the LONG_ADDRESS (correct word is EXTENDED, not LONG) defines.
>
> EUI64 is more generic which is a synonym for an simple IEEE EUI64
> address, used by 802.15.4 and bluetooth. bluetooth need to fill the
> ff:fe bits to generate the EUI64 but both can also be named EUI64.
>
> We need handling for the SHORT address, that's not EUI16 or something
> else... need some special defines, it's only 802.15.4 relevant.
>
>>>>  #define LOWPAN_DISPATCH_MASK	0xf8 /* 11111000 */
>>>>  
>>>> diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
>>>> index 4f4b02d..1557ece 100644
>>>> --- a/net/ieee802154/6lowpan_rtnl.c
>>>> +++ b/net/ieee802154/6lowpan_rtnl.c
>>>> @@ -477,41 +477,46 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
>>>>  	if (ieee802154_hdr_peek_addrs(skb, &hdr) < 0)
>>>>  		goto drop_skb;
>>>>  
>>>> -	/* check that it's our buffer */
>>>> +	/* First off if frame is Not A LoWPAN Packet (NALP) then chuck away */
>>>> +	if (lowpan_dispatch_is_nalp(skb->data[0]))
>>>> +		goto drop_skb;
>>>> +
>>>> +	/* The 6LoWPAN header stack comprimises of the following (in order)
>>>> +	 *   Mesh
>>>> +	 *   Broadcast
>>>> +	 *   Fragmentation
>>>> +	 */
>>>> +	if (lowpan_dispatch_is_mesh(skb->data[0]))
>>> better is:
>>>
>>> lowpan_dispatch_is_mesh(*skb_network_header(skb))
>>>
>>> the network_header should be pointed to the beginng of 6LoWPAN header.
>> yep, looks a lot better.
>>>> +		/* Not supported */
>>>> +		goto drop_skb;
>>>> +
>>>> +	if (lowpan_dispatch_is_broadcast(skb->data[0]))
>>>> +		/* Not supported */
>>>> +		goto drop_skb;
>>>> +
>>>> +	if (lowpan_dispatch_is_frag(skb->data[0])) {
>>>> +		u8 frag_dispatch = skb->data[0] & 0xe0;
>>>> +
>>>> +		ret = lowpan_frag_rcv(skb, frag_dispatch);
>>>> +		if (ret != 1) {
>>>> +			/* more frags to process */
>>>> +			return NET_RX_SUCCESS;
>>>> +		}
>>>> +	}
>>> I know this issue and we should not do that in this way.
>>>
>>> Why?
>>>
>>> Because this works only for fragmentation with IPHC, for example if we
>>> support mesh or Broadcast or HC1 compression. We should call after
>>> successfully reassembled "means lowpan_frag_rcv returns 1" the lowpan_rcv again.
>>> So this is a recursion and we don't should use recursion to much, but it
>>> should only be one recursion, so I think that's okay. :-)
>> The spec says that the headers of the 6LoWPAN frame must fit in the first fragment.  You need to process these headers to get to the fragmentation header, this is why the code is this way.
> yes this is for IPHC dispatch values your code works, I don't want to
> say that it doesn't work. Because fragmentation is something to
> reconstruct the full payload, we should first evaluate the fragmentation
> dispatches and then all others. To be sure that we can use fragmentation
> on any dispatch value which is not the fragmentation dispatch values.
> :-)
>
> Simple move it before nalp check. Maybe somebody fragment something and
> the dispatch value after fragmentation is nalp. I know it should catch
> the default branch above, but it's a little bit cleaner. I hope you are
> in the same opinion.

I think it should stay where it is.
>From RFC 4944
NALP: Specifies that the following bits are not a part of the LoWPAN
encapsulation, and any LoWPAN node that encounters a dispatch
value of 00xxxxxx shall discard the packet. Other non-LoWPAN
protocols that wish to coexist with LoWPAN nodes should include a
byte matching this pattern immediately following the 802.15.4.
header.

The last sentence implies that this NALP code is expected as the first byte following the MAC Header.  If a NALP is encountered after processing the 6LoWPAN header stack then it will be treated as an unknown compression type.


>
>>> After successfully reassemble we simple evaluate the DISPATCH value
>>> again, this is also more a bug (because we don't handle LOWPAN_DISPATCH_IPV6
>>> with fragmentation, and this can happen... We already talk about the
>>> issue "don't use IPHC if compressed header size don't fit in a single
>>> frame" and this is handled if we simple calll lowpan_frag_rcv again
>>> after successful reassembling. But I don't will take this as bugfix,
>>> because new support of handling xy, so wpan-next should be okay.
>>>
>>> Please separate also this from the "introduce new handling of dispatch
>>> values".
>>>
>>>> +
>>>> +	/* We should now have an IPv6 Header (Compressed or Uncompressed) */
>>>>  	if (skb->data[0] == LOWPAN_DISPATCH_IPV6) {
>>>> -		/* Pull off the 1-byte of 6lowpan header. */
>>>> +		/* Uncompressed, Pull off the dispatch byte */
>>>>  		skb_pull(skb, 1);
>>>> -
>>>> -		ret = NET_RX_SUCCESS;
>>>>  	} else {
>>>> -		switch (skb->data[0] & 0xe0) {
>>>> -		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
>>>> +		if ((skb->data[0] & 0xe0) == LOWPAN_DISPATCH_IPHC) {
>>>> +			/* Compressed with IPHC - RFC 6282 */
>>>>  			ret = iphc_uncompress_hdr(&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 = iphc_uncompress_hdr(&skb, &hdr);
>>>> -				if (ret < 0)
>>>> -					goto drop;
>>>> -			} else {
>>>> -				return NET_RX_SUCCESS;
>>>> -			}
>>>> -			break;
>>>> -		case LOWPAN_DISPATCH_FRAGN:	/* next fragments headers */
>>>> -			ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN);
>>>> -			if (ret == 1) {
>>>> -				ret = iphc_uncompress_hdr(&skb, &hdr);
>>>> -				if (ret < 0)
>>>> -					goto drop;
>>>> -			} else {
>>>> -				return NET_RX_SUCCESS;
>>>> -			}
>>>> -			break;
>>>> -		default:
>>>> -			break;
>>>> +		} else {
>>>> +			/* TODO: other compression formats to follow */
>>> don't know why we check at first for some types which we don't support
>>> and drop it and then we have this here and drop also unknown types.
>> The first part deals with the 6LoWAN header stack.  Once header stack is processed then we decompress the remaining IPv6 headers so here we also throw away frames that we don't support decompression for.  Once your NHC compression change is in I imagine this will be handled :)
> ok. :-)
>
> - Alex

- Martin.

Re: [PATCH][linux-bluetooth 3/3] 6lowpan: Refactored lowpan_rcv so it's RFC compliant

[Martin Townsend]

Hi Alex,

Reposting to everyone this time :)

On 11/09/14 10:50, Alexander Aring wrote:
> On Thu, Sep 11, 2014 at 10:30:46AM +0100, Martin Townsend wrote:
>> Hi Alex,
>>
>> On 11/09/14 09:53, Alexander Aring wrote:
>>> Hi Martin,
>>>
>>> On Wed, Sep 10, 2014 at 03:06:08PM +0100, Martin Townsend wrote:
>>>> Currently we support uncompressed IPv6 headers after performing fragmentation.
>>>>
>>> This patch is for wpan-next.
>> ok, but it depends on the previous patches so how should I handle this?
>>>> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
>>>> ---
>>>>  include/net/6lowpan.h         | 17 ++++++++++++
>>>>  net/ieee802154/6lowpan_rtnl.c | 63 +++++++++++++++++++++++--------------------
>>>>  2 files changed, 51 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
>>>> index d71c062..71b1bf0 100644
>>>> --- a/include/net/6lowpan.h
>>>> +++ b/include/net/6lowpan.h
>>>> @@ -126,11 +126,28 @@
>>>>  	 (((a)[6]) == 0xFF) &&	\
>>>>  	 (((a)[7]) == 0xFF))
>>>>  
>>>> +#define lowpan_dispatch_is_nalp(a)	\
>>>> +	(((a) & LOWPAN_DISPATCH_MAJOR) == 0x00)
>>>> +
>>>> +#define lowpan_dispatch_is_mesh(a)	\
>>>> +	(((a) & LOWPAN_DISPATCH_MAJOR) == 0x80)
>>>> +
>>> don't use the MINOR MAJOR thing here, I can't see that this follow any
>>> structure at RFC4944.
>> ok, but the spec hints at it by separating out the first two bits in Figure 2.  Also table 2.1 in "6LoWPAN: The wireless embedded internet" book explicitly states that this is what the coding of the dispatch byte is, which is where I took it from.
> Okay, so MAJOR = '11' is only FRAG thing. Now I can see it and yes you
> are right.
>
> Maybe then we should something like that. For example fragmentation handling:
>
> First evaluate the MAJOR -> goes in some function e.g. lowpan_frag_handling
> or something like that and then evaluate MINOR here for FRAG1 or FRAGN.
>
> But this is out of scope of this patch. I would accept this patch to
> introduce the defines, later we can do it as cleanup.
>
>>> Simple add more mask and values like:
>>>
>>> #define lowpan_dispatch_is_mesh(a)	\
>>> 	(((a) & LOWPAN_DISPATCH_MESH_MASK) == LOWPAN_DISPATCH_MESH)
>>>
>>> Please see the note below that we don't put this stuff into genetic 6lowpan.
>>>
>>>> +#define lowpan_dispatch_is_broadcast(a)	\
>>>> +	((a) == LOWPAN_DISPATCH_BCAST)
>>>> +
>>>> +#define lowpan_dispatch_is_frag(a)	\
>>>> +	(((a) & LOWPAN_DISPATCH_MASK) == LOWPAN_DISPATCH_FRAG1 || \
>>>> +	 ((a) & LOWPAN_DISPATCH_MASK) == LOWPAN_DISPATCH_FRAGN)
>>>> +
>>>> +#define LOWPAN_DISPATCH_MAJOR	0xc0
>>>> +#define LOWPAN_DISPATCH_MINOR	0x3f
>>>> +
>>>>  #define LOWPAN_DISPATCH_IPV6	0x41 /* 01000001 = 65 */
>>>>  #define LOWPAN_DISPATCH_HC1	0x42 /* 01000010 = 66 */
>>>>  #define LOWPAN_DISPATCH_IPHC	0x60 /* 011xxxxx = ... */
>>>>  #define LOWPAN_DISPATCH_FRAG1	0xc0 /* 11000xxx */
>>>>  #define LOWPAN_DISPATCH_FRAGN	0xe0 /* 11100xxx */
>>>> +#define LOWPAN_DISPATCH_BCAST	0x50 /* 01010000 */
>>>>  
>>> hehe... yea another big issue is also that much stoff from ieee802154
>>> foo is inside the include/net/6lowpan header. We should not make this
>>> problem bigger. Only IPHC stuff there, which is used by bluetooth and
>>> 802154.
>>>
>>> For ieee802154 6lowpan, we don't need a global header file.
>>>
>>> Simple create one "net/ieee802154/6lowpan_i.h" - underscore means that's
>>> internal header. These defines are only related to 802.15.4, e.g.
>>> fragmentation on bluetooth is done at some mac mechanism (L2CAP, or
>>> something else, that's what 6lowpan bluetooth draft says). So 6lowpan
>>> fragmentation have nothing to do with bluetooth 6lowpan.
>> ok.  isn't this a separate fix/patch though as the problem already exists?
> yea, cleanup. If you like you can do it. Go into bluetooth-next. The
> iphc header have also a include of the af_802154 header which is very
> very bad. The thing is we need more generic EUI64 defines instead to use
> the LONG_ADDRESS (correct word is EXTENDED, not LONG) defines.
>
> EUI64 is more generic which is a synonym for an simple IEEE EUI64
> address, used by 802.15.4 and bluetooth. bluetooth need to fill the
> ff:fe bits to generate the EUI64 but both can also be named EUI64.
>
> We need handling for the SHORT address, that's not EUI16 or something
> else... need some special defines, it's only 802.15.4 relevant.
>
>>>>  #define LOWPAN_DISPATCH_MASK	0xf8 /* 11111000 */
>>>>  
>>>> diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
>>>> index 4f4b02d..1557ece 100644
>>>> --- a/net/ieee802154/6lowpan_rtnl.c
>>>> +++ b/net/ieee802154/6lowpan_rtnl.c
>>>> @@ -477,41 +477,46 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
>>>>  	if (ieee802154_hdr_peek_addrs(skb, &hdr) < 0)
>>>>  		goto drop_skb;
>>>>  
>>>> -	/* check that it's our buffer */
>>>> +	/* First off if frame is Not A LoWPAN Packet (NALP) then chuck away */
>>>> +	if (lowpan_dispatch_is_nalp(skb->data[0]))
>>>> +		goto drop_skb;
>>>> +
>>>> +	/* The 6LoWPAN header stack comprimises of the following (in order)
>>>> +	 *   Mesh
>>>> +	 *   Broadcast
>>>> +	 *   Fragmentation
>>>> +	 */
>>>> +	if (lowpan_dispatch_is_mesh(skb->data[0]))
>>> better is:
>>>
>>> lowpan_dispatch_is_mesh(*skb_network_header(skb))
>>>
>>> the network_header should be pointed to the beginng of 6LoWPAN header.
>> yep, looks a lot better.
>>>> +		/* Not supported */
>>>> +		goto drop_skb;
>>>> +
>>>> +	if (lowpan_dispatch_is_broadcast(skb->data[0]))
>>>> +		/* Not supported */
>>>> +		goto drop_skb;
>>>> +
>>>> +	if (lowpan_dispatch_is_frag(skb->data[0])) {
>>>> +		u8 frag_dispatch = skb->data[0] & 0xe0;
>>>> +
>>>> +		ret = lowpan_frag_rcv(skb, frag_dispatch);
>>>> +		if (ret != 1) {
>>>> +			/* more frags to process */
>>>> +			return NET_RX_SUCCESS;
>>>> +		}
>>>> +	}
>>> I know this issue and we should not do that in this way.
>>>
>>> Why?
>>>
>>> Because this works only for fragmentation with IPHC, for example if we
>>> support mesh or Broadcast or HC1 compression. We should call after
>>> successfully reassembled "means lowpan_frag_rcv returns 1" the lowpan_rcv again.
>>> So this is a recursion and we don't should use recursion to much, but it
>>> should only be one recursion, so I think that's okay. :-)
>> The spec says that the headers of the 6LoWPAN frame must fit in the first fragment.  You need to process these headers to get to the fragmentation header, this is why the code is this way.
> yes this is for IPHC dispatch values your code works, I don't want to
> say that it doesn't work. Because fragmentation is something to
> reconstruct the full payload, we should first evaluate the fragmentation
> dispatches and then all others. To be sure that we can use fragmentation
> on any dispatch value which is not the fragmentation dispatch values.
> :-)
>
> Simple move it before nalp check. Maybe somebody fragment something and
> the dispatch value after fragmentation is nalp. I know it should catch
> the default branch above, but it's a little bit cleaner. I hope you are
> in the same opinion.

I think it should stay where it is.
 From RFC 4944
NALP: Specifies that the following bits are not a part of the LoWPAN
encapsulation, and any LoWPAN node that encounters a dispatch
value of 00xxxxxx shall discard the packet. Other non-LoWPAN
protocols that wish to coexist with LoWPAN nodes should include a
byte matching this pattern immediately following the 802.15.4.
header.

The last sentence implies that this NALP code is expected as the first byte following the MAC Header.  If a NALP is encountered after processing the 6LoWPAN header stack then it will be treated as an unknown compression type.


>
>>> After successfully reassemble we simple evaluate the DISPATCH value
>>> again, this is also more a bug (because we don't handle LOWPAN_DISPATCH_IPV6
>>> with fragmentation, and this can happen... We already talk about the
>>> issue "don't use IPHC if compressed header size don't fit in a single
>>> frame" and this is handled if we simple calll lowpan_frag_rcv again
>>> after successful reassembling. But I don't will take this as bugfix,
>>> because new support of handling xy, so wpan-next should be okay.
>>>
>>> Please separate also this from the "introduce new handling of dispatch
>>> values".
>>>
>>>> +
>>>> +	/* We should now have an IPv6 Header (Compressed or Uncompressed) */
>>>>  	if (skb->data[0] == LOWPAN_DISPATCH_IPV6) {
>>>> -		/* Pull off the 1-byte of 6lowpan header. */
>>>> +		/* Uncompressed, Pull off the dispatch byte */
>>>>  		skb_pull(skb, 1);
>>>> -
>>>> -		ret = NET_RX_SUCCESS;
>>>>  	} else {
>>>> -		switch (skb->data[0] & 0xe0) {
>>>> -		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
>>>> +		if ((skb->data[0] & 0xe0) == LOWPAN_DISPATCH_IPHC) {
>>>> +			/* Compressed with IPHC - RFC 6282 */
>>>>  			ret = iphc_uncompress_hdr(&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 = iphc_uncompress_hdr(&skb, &hdr);
>>>> -				if (ret < 0)
>>>> -					goto drop;
>>>> -			} else {
>>>> -				return NET_RX_SUCCESS;
>>>> -			}
>>>> -			break;
>>>> -		case LOWPAN_DISPATCH_FRAGN:	/* next fragments headers */
>>>> -			ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN);
>>>> -			if (ret == 1) {
>>>> -				ret = iphc_uncompress_hdr(&skb, &hdr);
>>>> -				if (ret < 0)
>>>> -					goto drop;
>>>> -			} else {
>>>> -				return NET_RX_SUCCESS;
>>>> -			}
>>>> -			break;
>>>> -		default:
>>>> -			break;
>>>> +		} else {
>>>> +			/* TODO: other compression formats to follow */
>>> don't know why we check at first for some types which we don't support
>>> and drop it and then we have this here and drop also unknown types.
>> The first part deals with the 6LoWAN header stack.  Once header stack is processed then we decompress the remaining IPv6 headers so here we also throw away frames that we don't support decompression for.  Once your NHC compression change is in I imagine this will be handled :)
> ok. :-)
>
> - Alex

- Martin.

Re: [PATCH][linux-bluetooth 3/3] 6lowpan: Refactored lowpan_rcv so it's RFC compliant

[Alexander Aring]

On Thu, Sep 11, 2014 at 11:09:26AM +0100, Martin Townsend wrote:
> Hi Alex,
> 
> Reposting to everyone this time :)
> 
ok
...
> >>> I know this issue and we should not do that in this way.
> >>>
> >>> Why?
> >>>
> >>> Because this works only for fragmentation with IPHC, for example if we
> >>> support mesh or Broadcast or HC1 compression. We should call after
> >>> successfully reassembled "means lowpan_frag_rcv returns 1" the lowpan_rcv again.
> >>> So this is a recursion and we don't should use recursion to much, but it
> >>> should only be one recursion, so I think that's okay. :-)
> >> The spec says that the headers of the 6LoWPAN frame must fit in the first fragment.  You need to process these headers to get to the fragmentation header, this is why the code is this way.
> > yes this is for IPHC dispatch values your code works, I don't want to
> > say that it doesn't work. Because fragmentation is something to
> > reconstruct the full payload, we should first evaluate the fragmentation
> > dispatches and then all others. To be sure that we can use fragmentation
> > on any dispatch value which is not the fragmentation dispatch values.
> > :-)
> >
> > Simple move it before nalp check. Maybe somebody fragment something and
> > the dispatch value after fragmentation is nalp. I know it should catch
> > the default branch above, but it's a little bit cleaner. I hope you are
> > in the same opinion.
> 
> I think it should stay where it is.
> From RFC 4944
> NALP: Specifies that the following bits are not a part of the LoWPAN
> encapsulation, and any LoWPAN node that encounters a dispatch
> value of 00xxxxxx shall discard the packet. Other non-LoWPAN
> protocols that wish to coexist with LoWPAN nodes should include a
> byte matching this pattern immediately following the 802.15.4.
> header.
> 
> The last sentence implies that this NALP code is expected as the first byte following the MAC Header.  If a NALP is encountered after processing the 6LoWPAN header stack then it will be treated as an unknown compression type.
> 

yes. But the issue is more a reassembled fragmentet 6LoWPAN packet have
a dispatch value. This dispatch value should not NALP, but maybe
somebody did it and then we should make the same code like what we do
for NALP dispatches without fragmentation.

The same for all other dispatches. The current code makes this with IPHC
and IPv6 dispatch. And I really don't know is that a MESH or BC0 can
also be fragmented because we have some worst cases in mac header,
etc... sizes.

- Alex

Re: [PATCH][linux-bluetooth 3/3] 6lowpan: Refactored lowpan_rcv so it's RFC compliant

[Martin Townsend]

Hi Alex,

On 11/09/14 11:33, Alexander Aring wrote:
> On Thu, Sep 11, 2014 at 11:09:26AM +0100, Martin Townsend wrote:
>> Hi Alex,
>>
>> Reposting to everyone this time :)
>>
> ok
> ...
>>>>> I know this issue and we should not do that in this way.
>>>>>
>>>>> Why?
>>>>>
>>>>> Because this works only for fragmentation with IPHC, for example if we
>>>>> support mesh or Broadcast or HC1 compression. We should call after
>>>>> successfully reassembled "means lowpan_frag_rcv returns 1" the lowpan_rcv again.
>>>>> So this is a recursion and we don't should use recursion to much, but it
>>>>> should only be one recursion, so I think that's okay. :-)
>>>> The spec says that the headers of the 6LoWPAN frame must fit in the first fragment.  You need to process these headers to get to the fragmentation header, this is why the code is this way.
>>> yes this is for IPHC dispatch values your code works, I don't want to
>>> say that it doesn't work. Because fragmentation is something to
>>> reconstruct the full payload, we should first evaluate the fragmentation
>>> dispatches and then all others. To be sure that we can use fragmentation
>>> on any dispatch value which is not the fragmentation dispatch values.
>>> :-)
>>>
>>> Simple move it before nalp check. Maybe somebody fragment something and
>>> the dispatch value after fragmentation is nalp. I know it should catch
>>> the default branch above, but it's a little bit cleaner. I hope you are
>>> in the same opinion.
>> I think it should stay where it is.
>> From RFC 4944
>> NALP: Specifies that the following bits are not a part of the LoWPAN
>> encapsulation, and any LoWPAN node that encounters a dispatch
>> value of 00xxxxxx shall discard the packet. Other non-LoWPAN
>> protocols that wish to coexist with LoWPAN nodes should include a
>> byte matching this pattern immediately following the 802.15.4.
>> header.
>>
>> The last sentence implies that this NALP code is expected as the first byte following the MAC Header.  If a NALP is encountered after processing the 6LoWPAN header stack then it will be treated as an unknown compression type.
>>
> yes. But the issue is more a reassembled fragmentet 6LoWPAN packet have
> a dispatch value. This dispatch value should not NALP, but maybe
> somebody did it and then we should make the same code like what we do
> for NALP dispatches without fragmentation.
A NALP dispatch byte should be the first byte in the MAC payload that allows other protocols to co-exist with 6LoWPAN.  I think Jennet use this to insert their own protocol between 802.15.4 and 6LoWPAN.  So we have to check for this before we do anything.  Having a NALP byte after re-assembly isn't valid and should be treated as an unknown compression type in my opinion.

> The same for all other dispatches. The current code makes this with IPHC
> and IPv6 dispatch. And I really don't know is that a MESH or BC0 can
> also be fragmented because we have some worst cases in mac header,
> etc... sizes.
>
> - Alex

- Martin.

Re: [PATCH][linux-bluetooth 3/3] 6lowpan: Refactored lowpan_rcv so it's RFC compliant

[Alexander Aring]

On Thu, Sep 11, 2014 at 11:45:58AM +0100, Martin Townsend wrote:
> Hi Alex,
> 
> On 11/09/14 11:33, Alexander Aring wrote:
> > On Thu, Sep 11, 2014 at 11:09:26AM +0100, Martin Townsend wrote:
> >> Hi Alex,
> >>
> >> Reposting to everyone this time :)
> >>
> > ok
> > ...
> >>>>> I know this issue and we should not do that in this way.
> >>>>>
> >>>>> Why?
> >>>>>
> >>>>> Because this works only for fragmentation with IPHC, for example if we
> >>>>> support mesh or Broadcast or HC1 compression. We should call after
> >>>>> successfully reassembled "means lowpan_frag_rcv returns 1" the lowpan_rcv again.
> >>>>> So this is a recursion and we don't should use recursion to much, but it
> >>>>> should only be one recursion, so I think that's okay. :-)
> >>>> The spec says that the headers of the 6LoWPAN frame must fit in the first fragment.  You need to process these headers to get to the fragmentation header, this is why the code is this way.
> >>> yes this is for IPHC dispatch values your code works, I don't want to
> >>> say that it doesn't work. Because fragmentation is something to
> >>> reconstruct the full payload, we should first evaluate the fragmentation
> >>> dispatches and then all others. To be sure that we can use fragmentation
> >>> on any dispatch value which is not the fragmentation dispatch values.
> >>> :-)
> >>>
> >>> Simple move it before nalp check. Maybe somebody fragment something and
> >>> the dispatch value after fragmentation is nalp. I know it should catch
> >>> the default branch above, but it's a little bit cleaner. I hope you are
> >>> in the same opinion.
> >> I think it should stay where it is.
> >> From RFC 4944
> >> NALP: Specifies that the following bits are not a part of the LoWPAN
> >> encapsulation, and any LoWPAN node that encounters a dispatch
> >> value of 00xxxxxx shall discard the packet. Other non-LoWPAN
> >> protocols that wish to coexist with LoWPAN nodes should include a
> >> byte matching this pattern immediately following the 802.15.4.
> >> header.
> >>
> >> The last sentence implies that this NALP code is expected as the first byte following the MAC Header.  If a NALP is encountered after processing the 6LoWPAN header stack then it will be treated as an unknown compression type.
> >>
> > yes. But the issue is more a reassembled fragmentet 6LoWPAN packet have
> > a dispatch value. This dispatch value should not NALP, but maybe
> > somebody did it and then we should make the same code like what we do
> > for NALP dispatches without fragmentation.
> A NALP dispatch byte should be the first byte in the MAC payload that allows other protocols to co-exist with 6LoWPAN.  I think Jennet use this to insert their own protocol between 802.15.4 and 6LoWPAN.  So we have to check for this before we do anything.  Having a NALP byte after re-assembly isn't valid and should be treated as an unknown compression type in my opinion.
> 

Okay, I agree with you with the NALP dispatch. What's about the others.

- Alex

Re: [PATCH][linux-bluetooth 3/3] 6lowpan: Refactored lowpan_rcv so it's RFC compliant

[Alexander Aring]

Hi Martin,

On Thu, Sep 11, 2014 at 12:55:05PM +0200, Alexander Aring wrote:
> On Thu, Sep 11, 2014 at 11:45:58AM +0100, Martin Townsend wrote:
> > Hi Alex,
> > 
> > On 11/09/14 11:33, Alexander Aring wrote:
> > > On Thu, Sep 11, 2014 at 11:09:26AM +0100, Martin Townsend wrote:
> > >> Hi Alex,
> > >>
> > >> Reposting to everyone this time :)
> > >>
> > > ok
> > > ...
> > >>>>> I know this issue and we should not do that in this way.
> > >>>>>
> > >>>>> Why?
> > >>>>>
> > >>>>> Because this works only for fragmentation with IPHC, for example if we
> > >>>>> support mesh or Broadcast or HC1 compression. We should call after
> > >>>>> successfully reassembled "means lowpan_frag_rcv returns 1" the lowpan_rcv again.
> > >>>>> So this is a recursion and we don't should use recursion to much, but it
> > >>>>> should only be one recursion, so I think that's okay. :-)
> > >>>> The spec says that the headers of the 6LoWPAN frame must fit in the first fragment.  You need to process these headers to get to the fragmentation header, this is why the code is this way.
> > >>> yes this is for IPHC dispatch values your code works, I don't want to
> > >>> say that it doesn't work. Because fragmentation is something to
> > >>> reconstruct the full payload, we should first evaluate the fragmentation
> > >>> dispatches and then all others. To be sure that we can use fragmentation
> > >>> on any dispatch value which is not the fragmentation dispatch values.
> > >>> :-)
> > >>>
> > >>> Simple move it before nalp check. Maybe somebody fragment something and
> > >>> the dispatch value after fragmentation is nalp. I know it should catch
> > >>> the default branch above, but it's a little bit cleaner. I hope you are
> > >>> in the same opinion.
> > >> I think it should stay where it is.
> > >> From RFC 4944
> > >> NALP: Specifies that the following bits are not a part of the LoWPAN
> > >> encapsulation, and any LoWPAN node that encounters a dispatch
> > >> value of 00xxxxxx shall discard the packet. Other non-LoWPAN
> > >> protocols that wish to coexist with LoWPAN nodes should include a
> > >> byte matching this pattern immediately following the 802.15.4.
> > >> header.
> > >>
> > >> The last sentence implies that this NALP code is expected as the first byte following the MAC Header.  If a NALP is encountered after processing the 6LoWPAN header stack then it will be treated as an unknown compression type.
> > >>
> > > yes. But the issue is more a reassembled fragmentet 6LoWPAN packet have
> > > a dispatch value. This dispatch value should not NALP, but maybe
> > > somebody did it and then we should make the same code like what we do
> > > for NALP dispatches without fragmentation.
> > A NALP dispatch byte should be the first byte in the MAC payload that allows other protocols to co-exist with 6LoWPAN.  I think Jennet use this to insert their own protocol between 802.15.4 and 6LoWPAN.  So we have to check for this before we do anything.  Having a NALP byte after re-assembly isn't valid and should be treated as an unknown compression type in my opinion.
> > 
> 
> Okay, I agree with you with the NALP dispatch. What's about the others.
> 
ok. This is out of interest because we don't support all others than
IPV6 and IPHC. The important thing is, if we don't know the dispatch
after fragmentation -> drop it.

We will think about that issue, if somebody implements that.

- Alex