[PATCH v4 bluetooth] Fix lowpan_rcv

[PATCH v4 bluetooth] Fix lowpan_rcv

[Martin Townsend]

This series aims to 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

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

Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
---

v1 -> v2:
* combined first two patches into one.
* split out renaming of process_data into a new patch
* re-use err in lowpan_process_data and remove err_ret local variable.
* removed third patch as applies to linux-wpan

v2 -> v3: (Updated from Alexander Aring's Review) 
* re-use err hadn't been included in patch.
* fixed some 'goto drop' cases in lowpan_rcv that should be 'goto drop skb'
* handle error codes when returning from lowpan_frag_rcv.
* Refactored lowpan_give_skb_to_devices so it consumes or free's skb and then
  only returns NET_RX status codes.

v3 -> v4: (Updated from Jukka Rissanen and Alexander Aring's Reviews)
* lowpan_process_data and process_data return skb or ERR_PTR, no need for 
  skb_inout
* prefer consume_skb on non error conditions.
* removed spurious setting of ret in lowpan_rcv

Martin Townsend (1):
  6lowpan: fix incorrect return values in lowpan_rcv

 include/net/6lowpan.h         |  4 +--
 net/6lowpan/iphc.c            | 73 ++++++++++++++++++-------------------------
 net/bluetooth/6lowpan.c       | 26 +++++++++------
 net/ieee802154/6lowpan_rtnl.c | 44 +++++++++++++++-----------
 4 files changed, 75 insertions(+), 72 deletions(-)

-- 
1.9.1

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

[Martin Townsend]

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

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.

With the above 2 lowpan_rcv is refacored so eliminate incorrect return values.

Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
---
 include/net/6lowpan.h         |  9 +++--
 net/6lowpan/iphc.c            | 88 +++++++++++++++++--------------------------
 net/bluetooth/6lowpan.c       | 38 ++++++++++---------
 net/ieee802154/6lowpan_rtnl.c | 61 ++++++++++++++++--------------
 4 files changed, 94 insertions(+), 102 deletions(-)

diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
index d184df1..c28fadb 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, 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);
+struct sk_buff *
+lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
+		    const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
+		    const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
+		    u8 iphc0, u8 iphc1);
 int lowpan_header_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 142eef5..6ac7765 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -171,37 +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);
-	kfree_skb(skb);
-
-	if (!new)
-		return -ENOMEM;
-
-	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);
-
-	kfree_skb(new);
-
-	return stat;
-}
-
 /* Uncompress function for multicast destination address,
  * when M bit is set.
  */
@@ -332,14 +301,16 @@ err:
 /* TTL uncompression values */
 static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
 
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
-			const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
-			const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
-			u8 iphc0, u8 iphc1, skb_delivery_cb deliver_skb)
+struct sk_buff *
+lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
+		    const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
+		    const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
+		    u8 iphc0, u8 iphc1)
 {
 	struct ipv6hdr hdr = {};
 	u8 tmp, num_context = 0;
 	int err;
+	struct sk_buff *new;
 
 	raw_dump_table(__func__, "raw skb data dump uncompressed",
 		       skb->data, skb->len);
@@ -348,7 +319,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 	if (iphc1 & LOWPAN_IPHC_CID) {
 		pr_debug("CID flag is set, increase header with one\n");
 		if (lowpan_fetch_skb(skb, &num_context, sizeof(num_context)))
-			goto drop;
+			return ERR_PTR(-EINVAL);
 	}
 
 	hdr.version = 6;
@@ -360,7 +331,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 	 */
 	case 0: /* 00b */
 		if (lowpan_fetch_skb(skb, &tmp, sizeof(tmp)))
-			goto drop;
+			return ERR_PTR(-EINVAL);
 
 		memcpy(&hdr.flow_lbl, &skb->data[0], 3);
 		skb_pull(skb, 3);
@@ -373,7 +344,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 	 */
 	case 2: /* 10b */
 		if (lowpan_fetch_skb(skb, &tmp, sizeof(tmp)))
-			goto drop;
+			return ERR_PTR(-EINVAL);
 
 		hdr.priority = ((tmp >> 2) & 0x0f);
 		hdr.flow_lbl[0] = ((tmp << 6) & 0xC0) | ((tmp >> 2) & 0x30);
@@ -383,7 +354,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 	 */
 	case 1: /* 01b */
 		if (lowpan_fetch_skb(skb, &tmp, sizeof(tmp)))
-			goto drop;
+			return ERR_PTR(-EINVAL);
 
 		hdr.flow_lbl[0] = (skb->data[0] & 0x0F) | ((tmp >> 2) & 0x30);
 		memcpy(&hdr.flow_lbl[1], &skb->data[0], 2);
@@ -400,7 +371,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 	if ((iphc0 & LOWPAN_IPHC_NH_C) == 0) {
 		/* Next header is carried inline */
 		if (lowpan_fetch_skb(skb, &hdr.nexthdr, sizeof(hdr.nexthdr)))
-			goto drop;
+			return ERR_PTR(-EINVAL);
 
 		pr_debug("NH flag is set, next header carried inline: %02x\n",
 			 hdr.nexthdr);
@@ -412,7 +383,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 	} else {
 		if (lowpan_fetch_skb(skb, &hdr.hop_limit,
 				     sizeof(hdr.hop_limit)))
-			goto drop;
+			return ERR_PTR(-EINVAL);
 	}
 
 	/* Extract SAM to the tmp variable */
@@ -431,7 +402,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 
 	/* Check on error of previous branch */
 	if (err)
-		goto drop;
+		return ERR_PTR(-EINVAL);
 
 	/* Extract DAM to the tmp variable */
 	tmp = ((iphc1 & LOWPAN_IPHC_DAM_11) >> LOWPAN_IPHC_DAM_BIT) & 0x03;
@@ -446,7 +417,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 								tmp);
 
 			if (err)
-				goto drop;
+				return ERR_PTR(-EINVAL);
 		}
 	} else {
 		err = uncompress_addr(skb, &hdr.daddr, tmp, daddr,
@@ -454,27 +425,25 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 		pr_debug("dest: stateless compression mode %d dest %pI6c\n",
 			 tmp, &hdr.daddr);
 		if (err)
-			goto drop;
+			return ERR_PTR(-EINVAL);
 	}
 
 	/* UDP data uncompression */
 	if (iphc0 & LOWPAN_IPHC_NH_C) {
 		struct udphdr uh;
-		struct sk_buff *new;
 
 		if (uncompress_udp_header(skb, &uh))
-			goto drop;
+			return ERR_PTR(-EINVAL);
 
 		/* replace the compressed UDP head by the uncompressed UDP
 		 * header
 		 */
 		new = skb_copy_expand(skb, sizeof(struct udphdr),
 				      skb_tailroom(skb), GFP_ATOMIC);
-		kfree_skb(skb);
-
 		if (!new)
-			return -ENOMEM;
+			return ERR_PTR(-ENOMEM);
 
+		consume_skb(skb);
 		skb = new;
 
 		skb_push(skb, sizeof(struct udphdr));
@@ -499,11 +468,24 @@ int lowpan_process_data(struct sk_buff *skb, 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. */
+	new = skb_copy_expand(skb, sizeof(hdr), skb_tailroom(skb),
+			      GFP_ATOMIC);
+	if (!new)
+		return ERR_PTR(-ENOMEM);
 
-drop:
-	kfree_skb(skb);
-	return -EINVAL;
+	consume_skb(skb);
+	skb = new;
+
+	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 skb;
 }
 EXPORT_SYMBOL_GPL(lowpan_process_data);
 
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index 206b65c..a44938b 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -215,8 +215,9 @@ 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,
-			struct l2cap_chan *chan)
+static struct sk_buff *
+process_data(struct sk_buff *skb, struct net_device *netdev,
+	     struct l2cap_chan *chan)
 {
 	const u8 *saddr, *daddr;
 	u8 iphc0, iphc1;
@@ -230,36 +231,31 @@ static int process_data(struct sk_buff *skb, struct net_device *netdev,
 	peer = peer_lookup_chan(dev, chan);
 	read_unlock_irqrestore(&devices_lock, flags);
 	if (!peer)
-		goto drop;
+		return ERR_PTR(-EINVAL);
 
 	saddr = peer->eui64_addr;
 	daddr = dev->netdev->dev_addr;
 
 	/* at least two bytes will be used for the encoding */
 	if (skb->len < 2)
-		goto drop;
+		return ERR_PTR(-EINVAL);
 
 	if (lowpan_fetch_skb_u8(skb, &iphc0))
-		goto drop;
+		return ERR_PTR(-EINVAL);
 
 	if (lowpan_fetch_skb_u8(skb, &iphc1))
-		goto drop;
+		return ERR_PTR(-EINVAL);
 
 	return lowpan_process_data(skb, netdev,
 				   saddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
 				   daddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
-				   iphc0, iphc1, give_skb_to_upper);
-
-drop:
-	kfree_skb(skb);
-	return -EINVAL;
+				   iphc0, iphc1);
 }
 
 static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
 		    struct l2cap_chan *chan)
 {
 	struct sk_buff *local_skb;
-	int ret;
 
 	if (!netif_running(dev))
 		goto drop;
@@ -280,9 +276,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;
@@ -300,9 +293,20 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
 			if (!local_skb)
 				goto drop;
 
-			ret = process_data(local_skb, dev, chan);
-			if (ret != NET_RX_SUCCESS)
+			skb = process_data(local_skb, dev, chan);
+			if (IS_ERR(skb)) {
+				kfree_skb(local_skb);
 				goto drop;
+			}
+
+			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 6591d27..a0804df 100644
--- a/net/ieee802154/6lowpan_rtnl.c
+++ b/net/ieee802154/6lowpan_rtnl.c
@@ -155,8 +155,9 @@ static int lowpan_give_skb_to_devices(struct sk_buff *skb,
 		if (lowpan_dev_info(entry->ldev)->real_dev == skb->dev) {
 			skb_cp = skb_copy(skb, GFP_ATOMIC);
 			if (!skb_cp) {
-				stat = -ENOMEM;
-				break;
+				kfree_skb(skb);
+				rcu_read_unlock();
+				return NET_RX_DROP;
 			}
 
 			skb_cp->dev = entry->ldev;
@@ -164,10 +165,13 @@ static int lowpan_give_skb_to_devices(struct sk_buff *skb,
 		}
 	rcu_read_unlock();
 
+	consume_skb(skb);
+
 	return stat;
 }
 
-static int process_data(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
+static struct sk_buff *
+process_data(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
 {
 	u8 iphc0, iphc1;
 	struct ieee802154_addr_sa sa, da;
@@ -176,13 +180,13 @@ static int process_data(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
 	raw_dump_table(__func__, "raw skb data dump", skb->data, skb->len);
 	/* at least two bytes will be used for the encoding */
 	if (skb->len < 2)
-		goto drop;
+		return ERR_PTR(-EINVAL);
 
 	if (lowpan_fetch_skb_u8(skb, &iphc0))
-		goto drop;
+		return ERR_PTR(-EINVAL);
 
 	if (lowpan_fetch_skb_u8(skb, &iphc1))
-		goto drop;
+		return ERR_PTR(-EINVAL);
 
 	ieee802154_addr_to_sa(&sa, &hdr->source);
 	ieee802154_addr_to_sa(&da, &hdr->dest);
@@ -199,12 +203,7 @@ static int process_data(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
 
 	return lowpan_process_data(skb, skb->dev, sap, sa.addr_type,
 				   IEEE802154_ADDR_LEN, dap, da.addr_type,
-				   IEEE802154_ADDR_LEN, iphc0, iphc1,
-				   lowpan_give_skb_to_devices);
-
-drop:
-	kfree_skb(skb);
-	return -EINVAL;
+				   IEEE802154_ADDR_LEN, iphc0, iphc1);
 }
 
 static int lowpan_set_address(struct net_device *dev, void *p)
@@ -478,36 +477,38 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
 
 	/* check that it's our buffer */
 	if (skb->data[0] == LOWPAN_DISPATCH_IPV6) {
-		skb->protocol = htons(ETH_P_IPV6);
-		skb->pkt_type = PACKET_HOST;
-
 		/* Pull off the 1-byte of 6lowpan header. */
 		skb_pull(skb, 1);
 
-		ret = lowpan_give_skb_to_devices(skb, NULL);
-		if (ret == NET_RX_DROP)
-			goto drop;
 	} else {
 		switch (skb->data[0] & 0xe0) {
 		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
-			ret = process_data(skb, &hdr);
-			if (ret == NET_RX_DROP)
-				goto drop;
+			skb = process_data(skb, &hdr);
+			if (IS_ERR(skb))
+				goto drop_skb;
 			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)
-					goto drop;
+				skb = process_data(skb, &hdr);
+				if (IS_ERR(skb))
+					goto drop_skb;
+			} else 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)
-					goto drop;
+				skb = process_data(skb, &hdr);
+				if (IS_ERR(skb))
+					goto drop_skb;
+			} else if (ret < 0) {
+				goto drop;
+			} else {
+				return NET_RX_SUCCESS;
 			}
 			break;
 		default:
@@ -515,7 +516,11 @@ 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;
+	return lowpan_give_skb_to_devices(skb, NULL);
+
 drop_skb:
 	kfree_skb(skb);
 drop:
-- 
1.9.1

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

[Martin Townsend]

Just noticed a typo in the commit msg, refacored != refactored.  I'll fix in v5 after further comments.

- Martin.

On 16/09/14 12:01, Martin Townsend wrote:
> Currently there are a number of error paths in the lowpan_rcv function that
> free the skb before returning, the patch simplifies the receive path by
> ensuring that the skb is only freed from this function.
>
> 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.
>
> With the above 2 lowpan_rcv is refacored so eliminate incorrect return values.
>
> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
> ---
>  include/net/6lowpan.h         |  9 +++--
>  net/6lowpan/iphc.c            | 88 +++++++++++++++++--------------------------
>  net/bluetooth/6lowpan.c       | 38 ++++++++++---------
>  net/ieee802154/6lowpan_rtnl.c | 61 ++++++++++++++++--------------
>  4 files changed, 94 insertions(+), 102 deletions(-)
>
> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
> index d184df1..c28fadb 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, 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);
> +struct sk_buff *
> +lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> +		    const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
> +		    const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
> +		    u8 iphc0, u8 iphc1);
>  int lowpan_header_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 142eef5..6ac7765 100644
> --- a/net/6lowpan/iphc.c
> +++ b/net/6lowpan/iphc.c
> @@ -171,37 +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);
> -	kfree_skb(skb);
> -
> -	if (!new)
> -		return -ENOMEM;
> -
> -	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);
> -
> -	kfree_skb(new);
> -
> -	return stat;
> -}
> -
>  /* Uncompress function for multicast destination address,
>   * when M bit is set.
>   */
> @@ -332,14 +301,16 @@ err:
>  /* TTL uncompression values */
>  static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
>  
> -int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> -			const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
> -			const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
> -			u8 iphc0, u8 iphc1, skb_delivery_cb deliver_skb)
> +struct sk_buff *
> +lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> +		    const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
> +		    const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
> +		    u8 iphc0, u8 iphc1)
>  {
>  	struct ipv6hdr hdr = {};
>  	u8 tmp, num_context = 0;
>  	int err;
> +	struct sk_buff *new;
>  
>  	raw_dump_table(__func__, "raw skb data dump uncompressed",
>  		       skb->data, skb->len);
> @@ -348,7 +319,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>  	if (iphc1 & LOWPAN_IPHC_CID) {
>  		pr_debug("CID flag is set, increase header with one\n");
>  		if (lowpan_fetch_skb(skb, &num_context, sizeof(num_context)))
> -			goto drop;
> +			return ERR_PTR(-EINVAL);
>  	}
>  
>  	hdr.version = 6;
> @@ -360,7 +331,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>  	 */
>  	case 0: /* 00b */
>  		if (lowpan_fetch_skb(skb, &tmp, sizeof(tmp)))
> -			goto drop;
> +			return ERR_PTR(-EINVAL);
>  
>  		memcpy(&hdr.flow_lbl, &skb->data[0], 3);
>  		skb_pull(skb, 3);
> @@ -373,7 +344,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>  	 */
>  	case 2: /* 10b */
>  		if (lowpan_fetch_skb(skb, &tmp, sizeof(tmp)))
> -			goto drop;
> +			return ERR_PTR(-EINVAL);
>  
>  		hdr.priority = ((tmp >> 2) & 0x0f);
>  		hdr.flow_lbl[0] = ((tmp << 6) & 0xC0) | ((tmp >> 2) & 0x30);
> @@ -383,7 +354,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>  	 */
>  	case 1: /* 01b */
>  		if (lowpan_fetch_skb(skb, &tmp, sizeof(tmp)))
> -			goto drop;
> +			return ERR_PTR(-EINVAL);
>  
>  		hdr.flow_lbl[0] = (skb->data[0] & 0x0F) | ((tmp >> 2) & 0x30);
>  		memcpy(&hdr.flow_lbl[1], &skb->data[0], 2);
> @@ -400,7 +371,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>  	if ((iphc0 & LOWPAN_IPHC_NH_C) == 0) {
>  		/* Next header is carried inline */
>  		if (lowpan_fetch_skb(skb, &hdr.nexthdr, sizeof(hdr.nexthdr)))
> -			goto drop;
> +			return ERR_PTR(-EINVAL);
>  
>  		pr_debug("NH flag is set, next header carried inline: %02x\n",
>  			 hdr.nexthdr);
> @@ -412,7 +383,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>  	} else {
>  		if (lowpan_fetch_skb(skb, &hdr.hop_limit,
>  				     sizeof(hdr.hop_limit)))
> -			goto drop;
> +			return ERR_PTR(-EINVAL);
>  	}
>  
>  	/* Extract SAM to the tmp variable */
> @@ -431,7 +402,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>  
>  	/* Check on error of previous branch */
>  	if (err)
> -		goto drop;
> +		return ERR_PTR(-EINVAL);
>  
>  	/* Extract DAM to the tmp variable */
>  	tmp = ((iphc1 & LOWPAN_IPHC_DAM_11) >> LOWPAN_IPHC_DAM_BIT) & 0x03;
> @@ -446,7 +417,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>  								tmp);
>  
>  			if (err)
> -				goto drop;
> +				return ERR_PTR(-EINVAL);
>  		}
>  	} else {
>  		err = uncompress_addr(skb, &hdr.daddr, tmp, daddr,
> @@ -454,27 +425,25 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>  		pr_debug("dest: stateless compression mode %d dest %pI6c\n",
>  			 tmp, &hdr.daddr);
>  		if (err)
> -			goto drop;
> +			return ERR_PTR(-EINVAL);
>  	}
>  
>  	/* UDP data uncompression */
>  	if (iphc0 & LOWPAN_IPHC_NH_C) {
>  		struct udphdr uh;
> -		struct sk_buff *new;
>  
>  		if (uncompress_udp_header(skb, &uh))
> -			goto drop;
> +			return ERR_PTR(-EINVAL);
>  
>  		/* replace the compressed UDP head by the uncompressed UDP
>  		 * header
>  		 */
>  		new = skb_copy_expand(skb, sizeof(struct udphdr),
>  				      skb_tailroom(skb), GFP_ATOMIC);
> -		kfree_skb(skb);
> -
>  		if (!new)
> -			return -ENOMEM;
> +			return ERR_PTR(-ENOMEM);
>  
> +		consume_skb(skb);
>  		skb = new;
>  
>  		skb_push(skb, sizeof(struct udphdr));
> @@ -499,11 +468,24 @@ int lowpan_process_data(struct sk_buff *skb, 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. */
> +	new = skb_copy_expand(skb, sizeof(hdr), skb_tailroom(skb),
> +			      GFP_ATOMIC);
> +	if (!new)
> +		return ERR_PTR(-ENOMEM);
>  
> -drop:
> -	kfree_skb(skb);
> -	return -EINVAL;
> +	consume_skb(skb);
> +	skb = new;
> +
> +	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 skb;
>  }
>  EXPORT_SYMBOL_GPL(lowpan_process_data);
>  
> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> index 206b65c..a44938b 100644
> --- a/net/bluetooth/6lowpan.c
> +++ b/net/bluetooth/6lowpan.c
> @@ -215,8 +215,9 @@ 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,
> -			struct l2cap_chan *chan)
> +static struct sk_buff *
> +process_data(struct sk_buff *skb, struct net_device *netdev,
> +	     struct l2cap_chan *chan)
>  {
>  	const u8 *saddr, *daddr;
>  	u8 iphc0, iphc1;
> @@ -230,36 +231,31 @@ static int process_data(struct sk_buff *skb, struct net_device *netdev,
>  	peer = peer_lookup_chan(dev, chan);
>  	read_unlock_irqrestore(&devices_lock, flags);
>  	if (!peer)
> -		goto drop;
> +		return ERR_PTR(-EINVAL);
>  
>  	saddr = peer->eui64_addr;
>  	daddr = dev->netdev->dev_addr;
>  
>  	/* at least two bytes will be used for the encoding */
>  	if (skb->len < 2)
> -		goto drop;
> +		return ERR_PTR(-EINVAL);
>  
>  	if (lowpan_fetch_skb_u8(skb, &iphc0))
> -		goto drop;
> +		return ERR_PTR(-EINVAL);
>  
>  	if (lowpan_fetch_skb_u8(skb, &iphc1))
> -		goto drop;
> +		return ERR_PTR(-EINVAL);
>  
>  	return lowpan_process_data(skb, netdev,
>  				   saddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
>  				   daddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
> -				   iphc0, iphc1, give_skb_to_upper);
> -
> -drop:
> -	kfree_skb(skb);
> -	return -EINVAL;
> +				   iphc0, iphc1);
>  }
>  
>  static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
>  		    struct l2cap_chan *chan)
>  {
>  	struct sk_buff *local_skb;
> -	int ret;
>  
>  	if (!netif_running(dev))
>  		goto drop;
> @@ -280,9 +276,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;
> @@ -300,9 +293,20 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
>  			if (!local_skb)
>  				goto drop;
>  
> -			ret = process_data(local_skb, dev, chan);
> -			if (ret != NET_RX_SUCCESS)
> +			skb = process_data(local_skb, dev, chan);
> +			if (IS_ERR(skb)) {
> +				kfree_skb(local_skb);
>  				goto drop;
> +			}
> +
> +			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 6591d27..a0804df 100644
> --- a/net/ieee802154/6lowpan_rtnl.c
> +++ b/net/ieee802154/6lowpan_rtnl.c
> @@ -155,8 +155,9 @@ static int lowpan_give_skb_to_devices(struct sk_buff *skb,
>  		if (lowpan_dev_info(entry->ldev)->real_dev == skb->dev) {
>  			skb_cp = skb_copy(skb, GFP_ATOMIC);
>  			if (!skb_cp) {
> -				stat = -ENOMEM;
> -				break;
> +				kfree_skb(skb);
> +				rcu_read_unlock();
> +				return NET_RX_DROP;
>  			}
>  
>  			skb_cp->dev = entry->ldev;
> @@ -164,10 +165,13 @@ static int lowpan_give_skb_to_devices(struct sk_buff *skb,
>  		}
>  	rcu_read_unlock();
>  
> +	consume_skb(skb);
> +
>  	return stat;
>  }
>  
> -static int process_data(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
> +static struct sk_buff *
> +process_data(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
>  {
>  	u8 iphc0, iphc1;
>  	struct ieee802154_addr_sa sa, da;
> @@ -176,13 +180,13 @@ static int process_data(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
>  	raw_dump_table(__func__, "raw skb data dump", skb->data, skb->len);
>  	/* at least two bytes will be used for the encoding */
>  	if (skb->len < 2)
> -		goto drop;
> +		return ERR_PTR(-EINVAL);
>  
>  	if (lowpan_fetch_skb_u8(skb, &iphc0))
> -		goto drop;
> +		return ERR_PTR(-EINVAL);
>  
>  	if (lowpan_fetch_skb_u8(skb, &iphc1))
> -		goto drop;
> +		return ERR_PTR(-EINVAL);
>  
>  	ieee802154_addr_to_sa(&sa, &hdr->source);
>  	ieee802154_addr_to_sa(&da, &hdr->dest);
> @@ -199,12 +203,7 @@ static int process_data(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
>  
>  	return lowpan_process_data(skb, skb->dev, sap, sa.addr_type,
>  				   IEEE802154_ADDR_LEN, dap, da.addr_type,
> -				   IEEE802154_ADDR_LEN, iphc0, iphc1,
> -				   lowpan_give_skb_to_devices);
> -
> -drop:
> -	kfree_skb(skb);
> -	return -EINVAL;
> +				   IEEE802154_ADDR_LEN, iphc0, iphc1);
>  }
>  
>  static int lowpan_set_address(struct net_device *dev, void *p)
> @@ -478,36 +477,38 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
>  
>  	/* check that it's our buffer */
>  	if (skb->data[0] == LOWPAN_DISPATCH_IPV6) {
> -		skb->protocol = htons(ETH_P_IPV6);
> -		skb->pkt_type = PACKET_HOST;
> -
>  		/* Pull off the 1-byte of 6lowpan header. */
>  		skb_pull(skb, 1);
>  
> -		ret = lowpan_give_skb_to_devices(skb, NULL);
> -		if (ret == NET_RX_DROP)
> -			goto drop;
>  	} else {
>  		switch (skb->data[0] & 0xe0) {
>  		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
> -			ret = process_data(skb, &hdr);
> -			if (ret == NET_RX_DROP)
> -				goto drop;
> +			skb = process_data(skb, &hdr);
> +			if (IS_ERR(skb))
> +				goto drop_skb;
>  			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)
> -					goto drop;
> +				skb = process_data(skb, &hdr);
> +				if (IS_ERR(skb))
> +					goto drop_skb;
> +			} else 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)
> -					goto drop;
> +				skb = process_data(skb, &hdr);
> +				if (IS_ERR(skb))
> +					goto drop_skb;
> +			} else if (ret < 0) {
> +				goto drop;
> +			} else {
> +				return NET_RX_SUCCESS;
>  			}
>  			break;
>  		default:
> @@ -515,7 +516,11 @@ 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;
> +	return lowpan_give_skb_to_devices(skb, NULL);
> +
>  drop_skb:
>  	kfree_skb(skb);
>  drop:

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

[Alexander Aring]

On Tue, Sep 16, 2014 at 12:01:59PM +0100, Martin Townsend wrote:
> Currently there are a number of error paths in the lowpan_rcv function that
> free the skb before returning, the patch simplifies the receive path by
> ensuring that the skb is only freed from this function.
> 
> 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.
> 
> With the above 2 lowpan_rcv is refacored so eliminate incorrect return values.
> 
> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
> ---
>  include/net/6lowpan.h         |  9 +++--
>  net/6lowpan/iphc.c            | 88 +++++++++++++++++--------------------------
>  net/bluetooth/6lowpan.c       | 38 ++++++++++---------
>  net/ieee802154/6lowpan_rtnl.c | 61 ++++++++++++++++--------------
>  4 files changed, 94 insertions(+), 102 deletions(-)
> 
> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
> index d184df1..c28fadb 100644
> --- a/include/net/6lowpan.h
> +++ b/include/net/6lowpan.h
...
> -static int process_data(struct sk_buff *skb, struct net_device *netdev,
> -			struct l2cap_chan *chan)
> +static struct sk_buff *
> +process_data(struct sk_buff *skb, struct net_device *netdev,
> +	     struct l2cap_chan *chan)
>  {
>  	const u8 *saddr, *daddr;
>  	u8 iphc0, iphc1;
> @@ -230,36 +231,31 @@ static int process_data(struct sk_buff *skb, struct net_device *netdev,
>  	peer = peer_lookup_chan(dev, chan);
>  	read_unlock_irqrestore(&devices_lock, flags);
>  	if (!peer)
> -		goto drop;
> +		return ERR_PTR(-EINVAL);
>  
>  	saddr = peer->eui64_addr;
>  	daddr = dev->netdev->dev_addr;
>  
>  	/* at least two bytes will be used for the encoding */
>  	if (skb->len < 2)
> -		goto drop;
> +		return ERR_PTR(-EINVAL);
>  
>  	if (lowpan_fetch_skb_u8(skb, &iphc0))
> -		goto drop;
> +		return ERR_PTR(-EINVAL);
>  
>  	if (lowpan_fetch_skb_u8(skb, &iphc1))
> -		goto drop;
> +		return ERR_PTR(-EINVAL);
>  
>  	return lowpan_process_data(skb, netdev,
>  				   saddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
>  				   daddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
> -				   iphc0, iphc1, give_skb_to_upper);
> -
> -drop:
> -	kfree_skb(skb);
> -	return -EINVAL;
> +				   iphc0, iphc1);
>  }
>  
>  static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
>  		    struct l2cap_chan *chan)
>  {
>  	struct sk_buff *local_skb;
> -	int ret;
>  
>  	if (!netif_running(dev))
>  		goto drop;
> @@ -280,9 +276,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;
> @@ -300,9 +293,20 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
>  			if (!local_skb)
>  				goto drop;
>  
> -			ret = process_data(local_skb, dev, chan);
> -			if (ret != NET_RX_SUCCESS)
> +			skb = process_data(local_skb, dev, chan);
> +			if (IS_ERR(skb)) {
> +				kfree_skb(local_skb);
>  				goto drop;
> +			}
> +
> +			local_skb->protocol = htons(ETH_P_IPV6);
> +			local_skb->pkt_type = PACKET_HOST;

this is the wrong skb here, the new one is skb. I don't know maybe there
is some optimization to call skb = process_data(skb, ...);

> +
> +			if (give_skb_to_upper(local_skb, dev)

also here local_skb should be skb, or?

- Alex

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

[Martin Townsend]

Hi Alex,
On 16/09/14 12:36, Alexander Aring wrote:
> On Tue, Sep 16, 2014 at 12:01:59PM +0100, Martin Townsend wrote:
>> Currently there are a number of error paths in the lowpan_rcv function that
>> free the skb before returning, the patch simplifies the receive path by
>> ensuring that the skb is only freed from this function.
>>
>> 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.
>>
>> With the above 2 lowpan_rcv is refacored so eliminate incorrect return values.
>>
>> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
>> ---
>>  include/net/6lowpan.h         |  9 +++--
>>  net/6lowpan/iphc.c            | 88 +++++++++++++++++--------------------------
>>  net/bluetooth/6lowpan.c       | 38 ++++++++++---------
>>  net/ieee802154/6lowpan_rtnl.c | 61 ++++++++++++++++--------------
>>  4 files changed, 94 insertions(+), 102 deletions(-)
>>
>> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
>> index d184df1..c28fadb 100644
>> --- a/include/net/6lowpan.h
>> +++ b/include/net/6lowpan.h
> ...
>> -static int process_data(struct sk_buff *skb, struct net_device *netdev,
>> -			struct l2cap_chan *chan)
>> +static struct sk_buff *
>> +process_data(struct sk_buff *skb, struct net_device *netdev,
>> +	     struct l2cap_chan *chan)
>>  {
>>  	const u8 *saddr, *daddr;
>>  	u8 iphc0, iphc1;
>> @@ -230,36 +231,31 @@ static int process_data(struct sk_buff *skb, struct net_device *netdev,
>>  	peer = peer_lookup_chan(dev, chan);
>>  	read_unlock_irqrestore(&devices_lock, flags);
>>  	if (!peer)
>> -		goto drop;
>> +		return ERR_PTR(-EINVAL);
>>  
>>  	saddr = peer->eui64_addr;
>>  	daddr = dev->netdev->dev_addr;
>>  
>>  	/* at least two bytes will be used for the encoding */
>>  	if (skb->len < 2)
>> -		goto drop;
>> +		return ERR_PTR(-EINVAL);
>>  
>>  	if (lowpan_fetch_skb_u8(skb, &iphc0))
>> -		goto drop;
>> +		return ERR_PTR(-EINVAL);
>>  
>>  	if (lowpan_fetch_skb_u8(skb, &iphc1))
>> -		goto drop;
>> +		return ERR_PTR(-EINVAL);
>>  
>>  	return lowpan_process_data(skb, netdev,
>>  				   saddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
>>  				   daddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
>> -				   iphc0, iphc1, give_skb_to_upper);
>> -
>> -drop:
>> -	kfree_skb(skb);
>> -	return -EINVAL;
>> +				   iphc0, iphc1);
>>  }
>>  
>>  static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
>>  		    struct l2cap_chan *chan)
>>  {
>>  	struct sk_buff *local_skb;
>> -	int ret;
>>  
>>  	if (!netif_running(dev))
>>  		goto drop;
>> @@ -280,9 +276,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;
>> @@ -300,9 +293,20 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
>>  			if (!local_skb)
>>  				goto drop;
>>  
>> -			ret = process_data(local_skb, dev, chan);
>> -			if (ret != NET_RX_SUCCESS)
>> +			skb = process_data(local_skb, dev, chan);
>> +			if (IS_ERR(skb)) {
>> +				kfree_skb(local_skb);
>>  				goto drop;
>> +			}
>> +
>> +			local_skb->protocol = htons(ETH_P_IPV6);
>> +			local_skb->pkt_type = PACKET_HOST;
> this is the wrong skb here, the new one is skb. I don't know maybe there
> is some optimization to call skb = process_data(skb, ...);
yes you are right, my mistake, I'll fix in next series.
>
>> +
>> +			if (give_skb_to_upper(local_skb, dev)
> also here local_skb should be skb, or?
>
> - Alex

- Martin.

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

[Alexander Aring]

On Tue, Sep 16, 2014 at 12:39:11PM +0100, Martin Townsend wrote:
> Hi Alex,
> On 16/09/14 12:36, Alexander Aring wrote:
> > On Tue, Sep 16, 2014 at 12:01:59PM +0100, Martin Townsend wrote:
> >> Currently there are a number of error paths in the lowpan_rcv function that
> >> free the skb before returning, the patch simplifies the receive path by
> >> ensuring that the skb is only freed from this function.
> >>
> >> 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.
> >>
> >> With the above 2 lowpan_rcv is refacored so eliminate incorrect return values.
> >>
> >> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
> >> ---
> >>  include/net/6lowpan.h         |  9 +++--
> >>  net/6lowpan/iphc.c            | 88 +++++++++++++++++--------------------------
> >>  net/bluetooth/6lowpan.c       | 38 ++++++++++---------
> >>  net/ieee802154/6lowpan_rtnl.c | 61 ++++++++++++++++--------------
> >>  4 files changed, 94 insertions(+), 102 deletions(-)
> >>
> >> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
> >> index d184df1..c28fadb 100644
> >> --- a/include/net/6lowpan.h
> >> +++ b/include/net/6lowpan.h
> > ...
> >> -static int process_data(struct sk_buff *skb, struct net_device *netdev,
> >> -			struct l2cap_chan *chan)
> >> +static struct sk_buff *
> >> +process_data(struct sk_buff *skb, struct net_device *netdev,
> >> +	     struct l2cap_chan *chan)
> >>  {
> >>  	const u8 *saddr, *daddr;
> >>  	u8 iphc0, iphc1;
> >> @@ -230,36 +231,31 @@ static int process_data(struct sk_buff *skb, struct net_device *netdev,
> >>  	peer = peer_lookup_chan(dev, chan);
> >>  	read_unlock_irqrestore(&devices_lock, flags);
> >>  	if (!peer)
> >> -		goto drop;
> >> +		return ERR_PTR(-EINVAL);
> >>  
> >>  	saddr = peer->eui64_addr;
> >>  	daddr = dev->netdev->dev_addr;
> >>  
> >>  	/* at least two bytes will be used for the encoding */
> >>  	if (skb->len < 2)
> >> -		goto drop;
> >> +		return ERR_PTR(-EINVAL);
> >>  
> >>  	if (lowpan_fetch_skb_u8(skb, &iphc0))
> >> -		goto drop;
> >> +		return ERR_PTR(-EINVAL);
> >>  
> >>  	if (lowpan_fetch_skb_u8(skb, &iphc1))
> >> -		goto drop;
> >> +		return ERR_PTR(-EINVAL);
> >>  
> >>  	return lowpan_process_data(skb, netdev,
> >>  				   saddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
> >>  				   daddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
> >> -				   iphc0, iphc1, give_skb_to_upper);
> >> -
> >> -drop:
> >> -	kfree_skb(skb);
> >> -	return -EINVAL;
> >> +				   iphc0, iphc1);
> >>  }
> >>  
> >>  static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
> >>  		    struct l2cap_chan *chan)
> >>  {
> >>  	struct sk_buff *local_skb;
> >> -	int ret;
> >>  
> >>  	if (!netif_running(dev))
> >>  		goto drop;
> >> @@ -280,9 +276,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;
> >> @@ -300,9 +293,20 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
> >>  			if (!local_skb)
> >>  				goto drop;
> >>  
> >> -			ret = process_data(local_skb, dev, chan);
> >> -			if (ret != NET_RX_SUCCESS)
> >> +			skb = process_data(local_skb, dev, chan);
> >> +			if (IS_ERR(skb)) {
> >> +				kfree_skb(local_skb);
> >>  				goto drop;
> >> +			}
> >> +
> >> +			local_skb->protocol = htons(ETH_P_IPV6);
> >> +			local_skb->pkt_type = PACKET_HOST;
> > this is the wrong skb here, the new one is skb. I don't know maybe there
> > is some optimization to call skb = process_data(skb, ...);

and this also smells like side effects for me, because we have the
local_skb which is sometimes freed inside of lowpan_process_data and
returning skb. Then we don't know which we should kfree_skb now, the skb
or local_skb now. Need to thing more about this to offer some solution,
somebody agree here with me?

- Alex

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

[Alexander Aring]

On Tue, Sep 16, 2014 at 01:47:59PM +0200, Alexander Aring wrote:
> On Tue, Sep 16, 2014 at 12:39:11PM +0100, Martin Townsend wrote:
> > Hi Alex,
> > On 16/09/14 12:36, Alexander Aring wrote:
> > > On Tue, Sep 16, 2014 at 12:01:59PM +0100, Martin Townsend wrote:
...
> 
> and this also smells like side effects for me, because we have the
> local_skb which is sometimes freed inside of lowpan_process_data and
> returning skb. Then we don't know which we should kfree_skb now, the skb
> or local_skb now. Need to thing more about this to offer some solution,
> somebody agree here with me?
> 

I mean sometimes we do this *skb = *new and skb is the parameter and before we
did a consume_skb(skb); then local_skb is already freed after this and
returning an errno and we make kfree_skb(local_skb) will crash something,
I suppose.

- Alex

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

[Alexander Aring]

On Tue, Sep 16, 2014 at 01:53:57PM +0200, Alexander Aring wrote:
> On Tue, Sep 16, 2014 at 01:47:59PM +0200, Alexander Aring wrote:
> > On Tue, Sep 16, 2014 at 12:39:11PM +0100, Martin Townsend wrote:
> > > Hi Alex,
> > > On 16/09/14 12:36, Alexander Aring wrote:
> > > > On Tue, Sep 16, 2014 at 12:01:59PM +0100, Martin Townsend wrote:
> ...
> > 
> > and this also smells like side effects for me, because we have the
> > local_skb which is sometimes freed inside of lowpan_process_data and
> > returning skb. Then we don't know which we should kfree_skb now, the skb
> > or local_skb now. Need to thing more about this to offer some solution,
> > somebody agree here with me?
> > 
> 
> I mean sometimes we do this *skb = *new and skb is the parameter and before we
> did a consume_skb(skb); then local_skb is already freed after this and
> returning an errno and we make kfree_skb(local_skb) will crash something,
> I suppose.

I meant skb = new for the expand skb thing. And we can't never free
kfree_skb(skb) here if (IS_ERR(skb) is true, but we can't decide if
we need a kfree_skb(local_skb) or not, because we do a
consume_skb($SKB_FROM_PARAMTER) in lowpan_process_data.

- Alex

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

[Alexander Aring]

On Tue, Sep 16, 2014 at 02:02:47PM +0200, Alexander Aring wrote:
> On Tue, Sep 16, 2014 at 01:53:57PM +0200, Alexander Aring wrote:
> > On Tue, Sep 16, 2014 at 01:47:59PM +0200, Alexander Aring wrote:
> > > On Tue, Sep 16, 2014 at 12:39:11PM +0100, Martin Townsend wrote:
> > > > Hi Alex,
> > > > On 16/09/14 12:36, Alexander Aring wrote:
> > > > > On Tue, Sep 16, 2014 at 12:01:59PM +0100, Martin Townsend wrote:
> > ...
> > > 
> > > and this also smells like side effects for me, because we have the
> > > local_skb which is sometimes freed inside of lowpan_process_data and
> > > returning skb. Then we don't know which we should kfree_skb now, the skb
> > > or local_skb now. Need to thing more about this to offer some solution,
> > > somebody agree here with me?
> > > 
> > 
> > I mean sometimes we do this *skb = *new and skb is the parameter and before we
> > did a consume_skb(skb); then local_skb is already freed after this and
> > returning an errno and we make kfree_skb(local_skb) will crash something,
> > I suppose.
> 
> I meant skb = new for the expand skb thing. And we can't never free
> kfree_skb(skb) here if (IS_ERR(skb) is true, but we can't decide if
> we need a kfree_skb(local_skb) or not, because we do a
> consume_skb($SKB_FROM_PARAMTER) in lowpan_process_data.
> 

This all comes now in, because the ERR_PTR conversion. So we have two
choices:

 - drop the ERR_PTR convertsion and make old behaviour
 - handle consume_skb/kfree_skb inside lowpan_process_data

- Alex

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

[Martin Townsend]

Hi Alex,

On 16/09/14 13:18, Alexander Aring wrote:
> On Tue, Sep 16, 2014 at 02:02:47PM +0200, Alexander Aring wrote:
>> On Tue, Sep 16, 2014 at 01:53:57PM +0200, Alexander Aring wrote:
>>> On Tue, Sep 16, 2014 at 01:47:59PM +0200, Alexander Aring wrote:
>>>> On Tue, Sep 16, 2014 at 12:39:11PM +0100, Martin Townsend wrote:
>>>>> Hi Alex,
>>>>> On 16/09/14 12:36, Alexander Aring wrote:
>>>>>> On Tue, Sep 16, 2014 at 12:01:59PM +0100, Martin Townsend wrote:
>>> ...
>>>> and this also smells like side effects for me, because we have the
>>>> local_skb which is sometimes freed inside of lowpan_process_data and
>>>> returning skb. Then we don't know which we should kfree_skb now, the skb
>>>> or local_skb now. Need to thing more about this to offer some solution,
>>>> somebody agree here with me?
>>>>
>>> I mean sometimes we do this *skb = *new and skb is the parameter and before we
>>> did a consume_skb(skb); then local_skb is already freed after this and
>>> returning an errno and we make kfree_skb(local_skb) will crash something,
>>> I suppose.
>> I meant skb = new for the expand skb thing. And we can't never free
>> kfree_skb(skb) here if (IS_ERR(skb) is true, but we can't decide if
>> we need a kfree_skb(local_skb) or not, because we do a
>> consume_skb($SKB_FROM_PARAMTER) in lowpan_process_data.
>>
> This all comes now in, because the ERR_PTR conversion. So we have two
> choices:
>
>  - drop the ERR_PTR convertsion and make old behaviour
>  - handle consume_skb/kfree_skb inside lowpan_process_data
>
> - Alex
>
How about a label for drop_local_skb?

		switch (skb->data[0] & 0xe0) {
		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
			local_skb = skb_clone(skb, GFP_ATOMIC);
			if (!local_skb)
				goto drop;

			local_skb = process_data(local_skb, dev, chan);
			if (IS_ERR(local_skb))
				goto drop_local_skb;

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

			kfree_skb(skb);
			break;
		default:
			break;
		}
	}

	return NET_RX_SUCCESS;

drop_local_skb:
	kfree_skb(local_skb);
drop:
	dev->stats.rx_dropped++;
	kfree_skb(skb);
	return NET_RX_DROP;
}


- Martin.

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

[Alexander Aring]

On Tue, Sep 16, 2014 at 01:26:19PM +0100, Martin Townsend wrote:
> Hi Alex,
> 
> On 16/09/14 13:18, Alexander Aring wrote:
> > On Tue, Sep 16, 2014 at 02:02:47PM +0200, Alexander Aring wrote:
> >> On Tue, Sep 16, 2014 at 01:53:57PM +0200, Alexander Aring wrote:
> >>> On Tue, Sep 16, 2014 at 01:47:59PM +0200, Alexander Aring wrote:
> >>>> On Tue, Sep 16, 2014 at 12:39:11PM +0100, Martin Townsend wrote:
> >>>>> Hi Alex,
> >>>>> On 16/09/14 12:36, Alexander Aring wrote:
> >>>>>> On Tue, Sep 16, 2014 at 12:01:59PM +0100, Martin Townsend wrote:
> >>> ...
> >>>> and this also smells like side effects for me, because we have the
> >>>> local_skb which is sometimes freed inside of lowpan_process_data and
> >>>> returning skb. Then we don't know which we should kfree_skb now, the skb
> >>>> or local_skb now. Need to thing more about this to offer some solution,
> >>>> somebody agree here with me?
> >>>>
> >>> I mean sometimes we do this *skb = *new and skb is the parameter and before we
> >>> did a consume_skb(skb); then local_skb is already freed after this and
> >>> returning an errno and we make kfree_skb(local_skb) will crash something,
> >>> I suppose.
> >> I meant skb = new for the expand skb thing. And we can't never free
> >> kfree_skb(skb) here if (IS_ERR(skb) is true, but we can't decide if
> >> we need a kfree_skb(local_skb) or not, because we do a
> >> consume_skb($SKB_FROM_PARAMTER) in lowpan_process_data.
> >>
> > This all comes now in, because the ERR_PTR conversion. So we have two
> > choices:
> >
> >  - drop the ERR_PTR convertsion and make old behaviour
> >  - handle consume_skb/kfree_skb inside lowpan_process_data
> >
> > - Alex
> >
> How about a label for drop_local_skb?
> 
> 		switch (skb->data[0] & 0xe0) {
> 		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
> 			local_skb = skb_clone(skb, GFP_ATOMIC);
> 			if (!local_skb)
> 				goto drop;
> 
> 			local_skb = process_data(local_skb, dev, chan);
> 			if (IS_ERR(local_skb))
> 				goto drop_local_skb;
> 
> 			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++;
> 
> 			kfree_skb(skb);
> 			break;
> 		default:
> 			break;
> 		}
> 	}
> 
> 	return NET_RX_SUCCESS;
> 
> drop_local_skb:
> 	kfree_skb(local_skb);

no this can't work, when IS_ERR(local_skb) is true, local_skb is an
invalid pointer some "((void *) -errno)", you can rescue it with if
(!IS_ERR(local_skb)), but... I don't know it looks complicated. :-)

What I mean is in lowpan_process_data you have a paramater skb and a skb
as return value.

Sometimes we need a consume_skb($PARAMETER_SKB), because we make the
copy_expand. After this the $PARAMETER_SKB is invalid and we have the
$RETURN_SKB as our new skb.

We don't know here if we need a kfree_skb($PARAMETER_SKB) or not because
we don't know if we did a consume_skb($PARAMETER_SKB). I think the error
handling need to be in lowpan_process_data again or make something which
handle this case.


I hope it was understandable what I mean here.

- Alex

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

[Martin Townsend]

On 16/09/14 13:34, Alexander Aring wrote:
> On Tue, Sep 16, 2014 at 01:26:19PM +0100, Martin Townsend wrote:
>> Hi Alex,
>>
>> On 16/09/14 13:18, Alexander Aring wrote:
>>> On Tue, Sep 16, 2014 at 02:02:47PM +0200, Alexander Aring wrote:
>>>> On Tue, Sep 16, 2014 at 01:53:57PM +0200, Alexander Aring wrote:
>>>>> On Tue, Sep 16, 2014 at 01:47:59PM +0200, Alexander Aring wrote:
>>>>>> On Tue, Sep 16, 2014 at 12:39:11PM +0100, Martin Townsend wrote:
>>>>>>> Hi Alex,
>>>>>>> On 16/09/14 12:36, Alexander Aring wrote:
>>>>>>>> On Tue, Sep 16, 2014 at 12:01:59PM +0100, Martin Townsend wrote:
>>>>> ...
>>>>>> and this also smells like side effects for me, because we have the
>>>>>> local_skb which is sometimes freed inside of lowpan_process_data and
>>>>>> returning skb. Then we don't know which we should kfree_skb now, the skb
>>>>>> or local_skb now. Need to thing more about this to offer some solution,
>>>>>> somebody agree here with me?
>>>>>>
>>>>> I mean sometimes we do this *skb = *new and skb is the parameter and before we
>>>>> did a consume_skb(skb); then local_skb is already freed after this and
>>>>> returning an errno and we make kfree_skb(local_skb) will crash something,
>>>>> I suppose.
>>>> I meant skb = new for the expand skb thing. And we can't never free
>>>> kfree_skb(skb) here if (IS_ERR(skb) is true, but we can't decide if
>>>> we need a kfree_skb(local_skb) or not, because we do a
>>>> consume_skb($SKB_FROM_PARAMTER) in lowpan_process_data.
>>>>
>>> This all comes now in, because the ERR_PTR conversion. So we have two
>>> choices:
>>>
>>>  - drop the ERR_PTR convertsion and make old behaviour
>>>  - handle consume_skb/kfree_skb inside lowpan_process_data
>>>
>>> - Alex
>>>
>> How about a label for drop_local_skb?
>>
>> 		switch (skb->data[0] & 0xe0) {
>> 		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
>> 			local_skb = skb_clone(skb, GFP_ATOMIC);
>> 			if (!local_skb)
>> 				goto drop;
>>
>> 			local_skb = process_data(local_skb, dev, chan);
>> 			if (IS_ERR(local_skb))
>> 				goto drop_local_skb;
>>
>> 			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++;
>>
>> 			kfree_skb(skb);
>> 			break;
>> 		default:
>> 			break;
>> 		}
>> 	}
>>
>> 	return NET_RX_SUCCESS;
>>
>> drop_local_skb:
>> 	kfree_skb(local_skb);
> no this can't work, when IS_ERR(local_skb) is true, local_skb is an
> invalid pointer some "((void *) -errno)", you can rescue it with if
> (!IS_ERR(local_skb)), but... I don't know it looks complicated. :-)
>
> What I mean is in lowpan_process_data you have a paramater skb and a skb
> as return value.
>
> Sometimes we need a consume_skb($PARAMETER_SKB), because we make the
> copy_expand. After this the $PARAMETER_SKB is invalid and we have the
> $RETURN_SKB as our new skb.
>
> We don't know here if we need a kfree_skb($PARAMETER_SKB) or not because
> we don't know if we did a consume_skb($PARAMETER_SKB). I think the error
> handling need to be in lowpan_process_data again or make something which
> handle this case.
>
>
> I hope it was understandable what I mean here.
>
> - Alex

Yes I see the problem now, maybe it's better to revert back to skb_inout, less chance of introducing bugs and then we have a well defined return value.

- Martin.

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

[Alexander Aring]

Hi Martin,

On Tue, Sep 16, 2014 at 01:40:24PM +0100, Martin Townsend wrote:
...
> 
> Yes I see the problem now, maybe it's better to revert back to skb_inout, less chance of introducing bugs and then we have a well defined return value.
> 

No problem, for me it's okay, if this is okay for Jukka, we can change
it later to a better behaviour. Jukka please answer what you think about this.

I also did a small c example because this now:

char *foo(char *buf)
{
        char *new;

        if (some_error)
                return NULL;

        if (some_error)
                return NULL;

        new = expand(buf, 23);
        if (!new)
                return NULL;

        free(buf);
        buf = new;

	/* buf is now different than the parameter buf */
        if (some_error)
                return NULL;
             
        return buf;
}             
              
int main(int argc, const char *argv[])
{             
        char *local_buf = malloc(42);
        char *buf;
             
        buf = foo(local_buf);
        if (!buf) {
                /* BUG */
                /* we don't know if local_buf is still valid. */
                free(local_buf);
        }        
                 
        return 0;
}

I think if you do buf = foo(buf) you can rescue it but this doesn't
look like a clean solution for me.

- Alex

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

[Jukka Rissanen]

Hi Alex,

On ti, 2014-09-16 at 14:48 +0200, Alexander Aring wrote:
> Hi Martin,
> 
> On Tue, Sep 16, 2014 at 01:40:24PM +0100, Martin Townsend wrote:
> ...
> > 
> > Yes I see the problem now, maybe it's better to revert back to skb_inout, less chance of introducing bugs and then we have a well defined return value.
> > 
> 
> No problem, for me it's okay, if this is okay for Jukka, we can change
> it later to a better behaviour. Jukka please answer what you think about this.
> 

What about doing things like this in your example?

> I also did a small c example because this now:
> 
> char *foo(char *buf)
> {
>         char *new;
> 
>         if (some_error)
>                 return NULL;

In this case you should probably not return NULL but something like
-EINVAL

if (some_error) {
	free(buf);
	return -EINVAL;
}

> 
>         if (some_error)
>                 return NULL;

Ditto

> 
>         new = expand(buf, 23);
>         if (!new)
>                 return NULL;

if (!new) {
	free(buf);
	return -ENOMEM;
}

> 
>         free(buf);
>         buf = new;
> 
> 	/* buf is now different than the parameter buf */
>         if (some_error)
>                 return NULL;

if (some_error) {
	free(buf);
	return -EFOOBAR;
}

>              
>         return buf;
> }             
>               
> int main(int argc, const char *argv[])
> {             
>         char *local_buf = malloc(42);
>         char *buf;
>              
>         buf = foo(local_buf);
>         if (!buf) {
>                 /* BUG */
>                 /* we don't know if local_buf is still valid. */
>                 free(local_buf);
>         }        

if (IS_ERR_OR_NULL(buf)) {
	fail();
} else
	free(buf);

>                  
>         return 0;
> }
> 
> I think if you do buf = foo(buf) you can rescue it but this doesn't
> look like a clean solution for me.
> 
> - Alex


In this simplified example, the subroutine frees the buf which does not
look nice I have to admit.



Cheers,
Jukka

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

[Alexander Aring]

Hi Jukka,

On Tue, Sep 16, 2014 at 04:20:19PM +0300, Jukka Rissanen wrote:
> Hi Alex,
> 
> On ti, 2014-09-16 at 14:48 +0200, Alexander Aring wrote:
> > Hi Martin,
> > 
> > On Tue, Sep 16, 2014 at 01:40:24PM +0100, Martin Townsend wrote:
> > ...
> > > 
> > > Yes I see the problem now, maybe it's better to revert back to skb_inout, less chance of introducing bugs and then we have a well defined return value.
> > > 
> > 
> > No problem, for me it's okay, if this is okay for Jukka, we can change
> > it later to a better behaviour. Jukka please answer what you think about this.
> > 
> 
> What about doing things like this in your example?
> 

ehm yes, the example is only there to describe the current situation.

> > I also did a small c example because this now:
> > 
> > char *foo(char *buf)
> > {
> >         char *new;
> > 
> >         if (some_error)
> >                 return NULL;
> 
> In this case you should probably not return NULL but something like
> -EINVAL
> 
> if (some_error) {
> 	free(buf);
> 	return -EINVAL;
> }

yes, that's the second choice, let do consume_skb/kfree_skb inside
lowpan_process_data function.

> 
> > 
> >         if (some_error)
> >                 return NULL;
> 
> Ditto
> 
> > 
> >         new = expand(buf, 23);
> >         if (!new)
> >                 return NULL;
> 
> if (!new) {
> 	free(buf);
> 	return -ENOMEM;
> }
> 
> > 
> >         free(buf);
> >         buf = new;
> > 
> > 	/* buf is now different than the parameter buf */
> >         if (some_error)
> >                 return NULL;
> 
> if (some_error) {
> 	free(buf);
> 	return -EFOOBAR;
> }
> 
> >              
> >         return buf;
> > }             
> >               
> > int main(int argc, const char *argv[])
> > {             
> >         char *local_buf = malloc(42);
> >         char *buf;
> >              
> >         buf = foo(local_buf);
> >         if (!buf) {
> >                 /* BUG */
> >                 /* we don't know if local_buf is still valid. */
> >                 free(local_buf);
> >         }        
> 
> if (IS_ERR_OR_NULL(buf)) {
> 	fail();
> } else
> 	free(buf);
> 
> >                  
> >         return 0;
> > }
> > 
> > I think if you do buf = foo(buf) you can rescue it but this doesn't
> > look like a clean solution for me.
> > 
> > - Alex
> 
> 
> In this simplified example, the subroutine frees the buf which does not
> look nice I have to admit.
> 

I am also fine with this solution. Make something I will review it and
look if we run into trouble.

In my last mails stands, that we have two choices:

- make the skb_inout thingy
- handle error freeing into lowpan_process_data function.

You described the last one now. :-)

- Alex

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

[Jukka Rissanen]

Hi Alex,

On ti, 2014-09-16 at 15:32 +0200, Alexander Aring wrote:
> Hi Jukka,
> 
> On Tue, Sep 16, 2014 at 04:20:19PM +0300, Jukka Rissanen wrote:
> > Hi Alex,
> > 
> > On ti, 2014-09-16 at 14:48 +0200, Alexander Aring wrote:
> > > Hi Martin,
> > > 
> > > On Tue, Sep 16, 2014 at 01:40:24PM +0100, Martin Townsend wrote:
> > > ...
> > > > 
> > > > Yes I see the problem now, maybe it's better to revert back to skb_inout, less chance of introducing bugs and then we have a well defined return value.
> > > > 
> > > 
> > > No problem, for me it's okay, if this is okay for Jukka, we can change
> > > it later to a better behaviour. Jukka please answer what you think about this.
> > > 
> > 
> > What about doing things like this in your example?
> > 
> 
> ehm yes, the example is only there to describe the current situation.
> 
> > > I also did a small c example because this now:
> > > 
> > > char *foo(char *buf)
> > > {
> > >         char *new;
> > > 
> > >         if (some_error)
> > >                 return NULL;
> > 
> > In this case you should probably not return NULL but something like
> > -EINVAL
> > 
> > if (some_error) {
> > 	free(buf);
> > 	return -EINVAL;
> > }
> 
> yes, that's the second choice, let do consume_skb/kfree_skb inside
> lowpan_process_data function.
> 
> > 
> > > 
> > >         if (some_error)
> > >                 return NULL;
> > 
> > Ditto
> > 
> > > 
> > >         new = expand(buf, 23);
> > >         if (!new)
> > >                 return NULL;
> > 
> > if (!new) {
> > 	free(buf);
> > 	return -ENOMEM;
> > }
> > 
> > > 
> > >         free(buf);
> > >         buf = new;
> > > 
> > > 	/* buf is now different than the parameter buf */
> > >         if (some_error)
> > >                 return NULL;
> > 
> > if (some_error) {
> > 	free(buf);
> > 	return -EFOOBAR;
> > }
> > 
> > >              
> > >         return buf;
> > > }             
> > >               
> > > int main(int argc, const char *argv[])
> > > {             
> > >         char *local_buf = malloc(42);
> > >         char *buf;
> > >              
> > >         buf = foo(local_buf);
> > >         if (!buf) {
> > >                 /* BUG */
> > >                 /* we don't know if local_buf is still valid. */
> > >                 free(local_buf);
> > >         }        
> > 
> > if (IS_ERR_OR_NULL(buf)) {
> > 	fail();
> > } else
> > 	free(buf);
> > 
> > >                  
> > >         return 0;
> > > }
> > > 
> > > I think if you do buf = foo(buf) you can rescue it but this doesn't
> > > look like a clean solution for me.
> > > 
> > > - Alex
> > 
> > 
> > In this simplified example, the subroutine frees the buf which does not
> > look nice I have to admit.
> > 
> 
> I am also fine with this solution. Make something I will review it and
> look if we run into trouble.
> 
> In my last mails stands, that we have two choices:
> 
> - make the skb_inout thingy
> - handle error freeing into lowpan_process_data function.
> 
> You described the last one now. :-)

Great, your example clarified the issue nicely :)

I would vote for option 2) but if it makes the code too ugly then 1) is
ok too.


Cheers,
Jukka

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

[Alexander Aring]

Hi Jukka and Martin,

On Tue, Sep 16, 2014 at 04:52:50PM +0300, Jukka Rissanen wrote:
...
> Great, your example clarified the issue nicely :)
> 
> I would vote for option 2) but if it makes the code too ugly then 1) is
> ok too.
> 
I begin to have the feeling like there is a reason because there are
different indicators for consume_skb, kfree_skb. Error or not error,
because it's hard to implement it in some way to make a correct handling
without using a pointer of pointer. A pointer of pointer always means also
a unnecessary dereferencing (netdev people doesn't like unnecessary
dereferencing stuff, takes too much time).

That's why I vote also for option 2)... but we can also clarify this on
the netdev mailinglist and ask other networking kernel hackers.

- Alex

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

[Martin Townsend]

I would like to keep freeing skb's out of process_data as process_data will become something like iphc_decompress_hdr and it would be good if that's all it did.  Otherwise I feel we are going to put a constraint on all future header decompression routines in that they must free the skb on error.  I think it would be better to defer this so on error you might want to try something else with the skb, maybe not but at least the option is there.
So how about

        struct sk_buff * ret_skb;
        switch (skb->data[0] & 0xe0) {
        case LOWPAN_DISPATCH_IPHC:    /* ipv6 datagram */
            ret_skb = process_data(skb, &hdr);
            if (IS_ERR(ret_skb))
                goto drop_skb;
            else
                skb = ret_skb;
            break;

I know we currently have 3 calls to process_data so it will look fairly ugly in this patch but in my next patch to fix lowpan_rcv to handle uncompressed IPv6 packets that are fragmented there will only be one call to process_data so it won't look so bad.  You could even wrap it in a macro but I'm not a fan of this as they can obfuscate the code a bit.

Thoughts?

- Martin.



On 16/09/14 15:05, Alexander Aring wrote:
> Hi Jukka and Martin,
>
> On Tue, Sep 16, 2014 at 04:52:50PM +0300, Jukka Rissanen wrote:
> ...
>> Great, your example clarified the issue nicely :)
>>
>> I would vote for option 2) but if it makes the code too ugly then 1) is
>> ok too.
>>
> I begin to have the feeling like there is a reason because there are
> different indicators for consume_skb, kfree_skb. Error or not error,
> because it's hard to implement it in some way to make a correct handling
> without using a pointer of pointer. A pointer of pointer always means also
> a unnecessary dereferencing (netdev people doesn't like unnecessary
> dereferencing stuff, takes too much time).
>
> That's why I vote also for option 2)... but we can also clarify this on
> the netdev mailinglist and ask other networking kernel hackers.
>
> - 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 v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv

[Alexander Aring]

Hi Martin,

On Tue, Sep 16, 2014 at 03:44:43PM +0100, Martin Townsend wrote:
> I would like to keep freeing skb's out of process_data as process_data will become something like iphc_decompress_hdr and it would be good if that's all it did.  Otherwise I feel we are going to put a constraint on all future header decompression routines in that they must free the skb on error.  I think it would be better to defer this so on error you might want to try something else with the skb, maybe not but at least the option is there.
> So how about
> 
>         struct sk_buff * ret_skb;
>         switch (skb->data[0] & 0xe0) {
>         case LOWPAN_DISPATCH_IPHC:    /* ipv6 datagram */
>             ret_skb = process_data(skb, &hdr);
>             if (IS_ERR(ret_skb))
>                 goto drop_skb;
>             else
>                 skb = ret_skb;
>             break;
> 
> I know we currently have 3 calls to process_data so it will look fairly ugly in this patch but in my next patch to fix lowpan_rcv to handle uncompressed IPv6 packets that are fragmented there will only be one call to process_data so it won't look so bad.  You could even wrap it in a macro but I'm not a fan of this as they can obfuscate the code a bit.
> 
> Thoughts?
> 

sorry, I can't follow how this solve the issue if the "parameter skb" is
already consumed or not. If process_data returns a error before
parameter consume, then we should run kfree_skb(parameter_skb), if it's
afterwards we should do nothing. Point is we don't know that there. I
suppose if we do consume_skb and refcount reach 0 the parameter_skb
becomes a dangling pointer.

- Alex

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

[Martin Townsend]

Hi Alex,

On 16/09/14 18:38, Alexander Aring wrote:
> Hi Martin,
>
> On Tue, Sep 16, 2014 at 03:44:43PM +0100, Martin Townsend wrote:
>> I would like to keep freeing skb's out of process_data as process_data will become something like iphc_decompress_hdr and it would be good if that's all it did.  Otherwise I feel we are going to put a constraint on all future header decompression routines in that they must free the skb on error.  I think it would be better to defer this so on error you might want to try something else with the skb, maybe not but at least the option is there.
>> So how about
>>
>>          struct sk_buff * ret_skb;
>>          switch (skb->data[0] & 0xe0) {
>>          case LOWPAN_DISPATCH_IPHC:    /* ipv6 datagram */
>>              ret_skb = process_data(skb, &hdr);
>>              if (IS_ERR(ret_skb))
>>                  goto drop_skb;
>>              else
>>                  skb = ret_skb;
>>              break;
>>
>> I know we currently have 3 calls to process_data so it will look fairly ugly in this patch but in my next patch to fix lowpan_rcv to handle uncompressed IPv6 packets that are fragmented there will only be one call to process_data so it won't look so bad.  You could even wrap it in a macro but I'm not a fan of this as they can obfuscate the code a bit.
>>
>> Thoughts?
>>
> sorry, I can't follow how this solve the issue if the "parameter skb" is
> already consumed or not. If process_data returns a error before
> parameter consume, then we should run kfree_skb(parameter_skb), if it's
> afterwards we should do nothing. Point is we don't know that there. I
> suppose if we do consume_skb and refcount reach 0 the parameter_skb
> becomes a dangling pointer.
>
> - Alex

process_data never consumes the skb, it may copy_expand and then consume 
the old one so it will either return an error or an skb that contains 
the uncompressed ipv6 header.  By calling process_data using a different 
sk_buff pointer (ret_skb) that the parameter we can check this for an 
error and if so goto drop_skb which will kfree_skb(skb) which is fine as 
skb is still valid.   if ret_skb is good and we assign to skb and carry 
on to the function that  passes the skb up the stack, 
lowpan_give_skb_to_devices, which deals with either consuming or kfreeing.

Or am I missing something?

- Martin.

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

[Alexander Aring]

On Tue, Sep 16, 2014 at 07:57:27PM +0100, Martin Townsend wrote:
> Hi Alex,
> 
> On 16/09/14 18:38, Alexander Aring wrote:
> >Hi Martin,
> >
> >On Tue, Sep 16, 2014 at 03:44:43PM +0100, Martin Townsend wrote:
> >>I would like to keep freeing skb's out of process_data as process_data will become something like iphc_decompress_hdr and it would be good if that's all it did.  Otherwise I feel we are going to put a constraint on all future header decompression routines in that they must free the skb on error.  I think it would be better to defer this so on error you might want to try something else with the skb, maybe not but at least the option is there.
> >>So how about
> >>
> >>         struct sk_buff * ret_skb;
> >>         switch (skb->data[0] & 0xe0) {
> >>         case LOWPAN_DISPATCH_IPHC:    /* ipv6 datagram */
> >>             ret_skb = process_data(skb, &hdr);
> >>             if (IS_ERR(ret_skb))
> >>                 goto drop_skb;
> >>             else
> >>                 skb = ret_skb;
> >>             break;
> >>
> >>I know we currently have 3 calls to process_data so it will look fairly ugly in this patch but in my next patch to fix lowpan_rcv to handle uncompressed IPv6 packets that are fragmented there will only be one call to process_data so it won't look so bad.  You could even wrap it in a macro but I'm not a fan of this as they can obfuscate the code a bit.
> >>
> >>Thoughts?
> >>
> >sorry, I can't follow how this solve the issue if the "parameter skb" is
> >already consumed or not. If process_data returns a error before
> >parameter consume, then we should run kfree_skb(parameter_skb), if it's
> >afterwards we should do nothing. Point is we don't know that there. I
> >suppose if we do consume_skb and refcount reach 0 the parameter_skb
> >becomes a dangling pointer.
> >
> >- Alex
> 
> process_data never consumes the skb, it may copy_expand and then consume the
> old one so it will either return an error or an skb that contains the
> uncompressed ipv6 header.  By calling process_data using a different sk_buff
> pointer (ret_skb) that the parameter we can check this for an error and if
> so goto drop_skb which will kfree_skb(skb) which is fine as skb is still

are you sure it's still valid? I don't get it. :-(

> valid.   if ret_skb is good and we assign to skb and carry on to the
> function that  passes the skb up the stack, lowpan_give_skb_to_devices,
> which deals with either consuming or kfreeing.
> 
> Or am I missing something?
> 

I make another c example, hopeful more correct than the last one:

char *foo(char *skb)
{
        char *new;

        if (some_error_before_consume)
                return ERR_PTR(-EINVAL); /* here we need to do a free(skb) */

        /* UDP expand */
        new = expand(skb, 16);
        if (!new)
                return ERR_PTR(-ENOMEM);
        consume(skb); /* parameter skb becomes dangling pointer */
        skb = new; /* doesn't rescue it, it is different than skb from caller function
                      at this point, the skb_inout had rescue it, because it was a pointer
                      of pointer */

        /* IPv6 expand */
        new = expand(skb, 40);
        if (!new) /* some error after a consume(skb), will crash at drop_skb label */
                return ERR_PTR(-ENOMEM);
        consume(skb);
        skb = new;

        return skb;
}

int main(int argc, const char *argv[])
{
        char *local_buf = malloc(42);
        char *skb;

        local_skb = foo(skb);
        if (IS_ERR(local_skb))
                goto drop_skb;
        else
                skb = local_skb; /* ??? */

        return NET_RX_SUCCESS;

drop_skb:
        free(skb); /* dangling pointer will be freed if foo called consume(skb)
                      it's correct when foo returned on some_error_before_consume
                      condition. */
drop:
        return NET_RX_DROP;
}

I don't know what "skb = local_skb" did now there.

- Alex

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

[Martin Townsend]

Hi Alex,

On 16/09/14 20:37, Alexander Aring wrote:
> On Tue, Sep 16, 2014 at 07:57:27PM +0100, Martin Townsend wrote:
>> Hi Alex,
>>
>> On 16/09/14 18:38, Alexander Aring wrote:
>>> Hi Martin,
>>>
>>> On Tue, Sep 16, 2014 at 03:44:43PM +0100, Martin Townsend wrote:
>>>> I would like to keep freeing skb's out of process_data as process_data will become something like iphc_decompress_hdr and it would be good if that's all it did.  Otherwise I feel we are going to put a constraint on all future header decompression routines in that they must free the skb on error.  I think it would be better to defer this so on error you might want to try something else with the skb, maybe not but at least the option is there.
>>>> So how about
>>>>
>>>>          struct sk_buff * ret_skb;
>>>>          switch (skb->data[0] & 0xe0) {
>>>>          case LOWPAN_DISPATCH_IPHC:    /* ipv6 datagram */
>>>>              ret_skb = process_data(skb, &hdr);
>>>>              if (IS_ERR(ret_skb))
>>>>                  goto drop_skb;
>>>>              else
>>>>                  skb = ret_skb;
>>>>              break;
>>>>
>>>> I know we currently have 3 calls to process_data so it will look fairly ugly in this patch but in my next patch to fix lowpan_rcv to handle uncompressed IPv6 packets that are fragmented there will only be one call to process_data so it won't look so bad.  You could even wrap it in a macro but I'm not a fan of this as they can obfuscate the code a bit.
>>>>
>>>> Thoughts?
>>>>
>>> sorry, I can't follow how this solve the issue if the "parameter skb" is
>>> already consumed or not. If process_data returns a error before
>>> parameter consume, then we should run kfree_skb(parameter_skb), if it's
>>> afterwards we should do nothing. Point is we don't know that there. I
>>> suppose if we do consume_skb and refcount reach 0 the parameter_skb
>>> becomes a dangling pointer.
>>>
>>> - Alex
>> process_data never consumes the skb, it may copy_expand and then consume the
>> old one so it will either return an error or an skb that contains the
>> uncompressed ipv6 header.  By calling process_data using a different sk_buff
>> pointer (ret_skb) that the parameter we can check this for an error and if
>> so goto drop_skb which will kfree_skb(skb) which is fine as skb is still
> are you sure it's still valid? I don't get it. :-(
>
>> valid.   if ret_skb is good and we assign to skb and carry on to the
>> function that  passes the skb up the stack, lowpan_give_skb_to_devices,
>> which deals with either consuming or kfreeing.
>>
>> Or am I missing something?
>>
> I make another c example, hopeful more correct than the last one:
>
> char *foo(char *skb)
> {
>          char *new;
>
>          if (some_error_before_consume)
>                  return ERR_PTR(-EINVAL); /* here we need to do a free(skb) */
>
>          /* UDP expand */
>          new = expand(skb, 16);
>          if (!new)
>                  return ERR_PTR(-ENOMEM);
>          consume(skb); /* parameter skb becomes dangling pointer */
>          skb = new; /* doesn't rescue it, it is different than skb from caller function
>                        at this point, the skb_inout had rescue it, because it was a pointer
>                        of pointer */
>
>          /* IPv6 expand */
>          new = expand(skb, 40);
>          if (!new) /* some error after a consume(skb), will crash at drop_skb label */
>                  return ERR_PTR(-ENOMEM);
>          consume(skb);
>          skb = new;
>
>          return skb;
> }
I see the problem now, once the skb has been copied and then an error 
occurs you have to return the error and the skb has been lost.  Would 
using the skb->cb to store decompress status get around this problem?
> int main(int argc, const char *argv[])
> {
>          char *local_buf = malloc(42);
>          char *skb;
>
>          local_skb = foo(skb);
>          if (IS_ERR(local_skb))
>                  goto drop_skb;
>          else
>                  skb = local_skb; /* ??? */
>
>          return NET_RX_SUCCESS;
>
> drop_skb:
>          free(skb); /* dangling pointer will be freed if foo called consume(skb)
>                        it's correct when foo returned on some_error_before_consume
>                        condition. */
> drop:
>          return NET_RX_DROP;
> }
>
> I don't know what "skb = local_skb" did now there.
>
> - 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 v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv

[Alexander Aring]

On Tue, Sep 16, 2014 at 08:53:06PM +0100, Martin Townsend wrote:
...
> >I make another c example, hopeful more correct than the last one:
> >
> >char *foo(char *skb)
> >{
> >         char *new;
> >
> >         if (some_error_before_consume)
> >                 return ERR_PTR(-EINVAL); /* here we need to do a free(skb) */
> >
> >         /* UDP expand */
> >         new = expand(skb, 16);
s/16/8 , argl, doesn't matter was only an example. :-)

> >         if (!new)
> >                 return ERR_PTR(-ENOMEM);
> >         consume(skb); /* parameter skb becomes dangling pointer */
> >         skb = new; /* doesn't rescue it, it is different than skb from caller function
> >                       at this point, the skb_inout had rescue it, because it was a pointer
> >                       of pointer */
> >
> >         /* IPv6 expand */
> >         new = expand(skb, 40);
> >         if (!new) /* some error after a consume(skb), will crash at drop_skb label */
> >                 return ERR_PTR(-ENOMEM);
> >         consume(skb);
> >         skb = new;
> >
> >         return skb;
> >}
> I see the problem now, once the skb has been copied and then an error occurs
> you have to return the error and the skb has been lost.  Would using the
> skb->cb to store decompress status get around this problem?

mhhh, complicated... on 802.15.4 6LoWPAN we use the control block
information for fragmentation information. I don't know right now if we
get trouble when we add the "uncompression on the fly when FRAG1 was
received".

What exactly do you mean with "decompress status"?

- Alex

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

[Martin Townsend]

Hi Alex,

On 16/09/14 21:07, Alexander Aring wrote:
> On Tue, Sep 16, 2014 at 08:53:06PM +0100, Martin Townsend wrote:
> ...
>>> I make another c example, hopeful more correct than the last one:
>>>
>>> char *foo(char *skb)
>>> {
>>>          char *new;
>>>
>>>          if (some_error_before_consume)
>>>                  return ERR_PTR(-EINVAL); /* here we need to do a free(skb) */
>>>
>>>          /* UDP expand */
>>>          new = expand(skb, 16);
> s/16/8 , argl, doesn't matter was only an example. :-)
>
>>>          if (!new)
>>>                  return ERR_PTR(-ENOMEM);
>>>          consume(skb); /* parameter skb becomes dangling pointer */
>>>          skb = new; /* doesn't rescue it, it is different than skb from caller function
>>>                        at this point, the skb_inout had rescue it, because it was a pointer
>>>                        of pointer */
>>>
>>>          /* IPv6 expand */
>>>          new = expand(skb, 40);
>>>          if (!new) /* some error after a consume(skb), will crash at drop_skb label */
>>>                  return ERR_PTR(-ENOMEM);
>>>          consume(skb);
>>>          skb = new;
>>>
>>>          return skb;
>>> }
>> I see the problem now, once the skb has been copied and then an error occurs
>> you have to return the error and the skb has been lost.  Would using the
>> skb->cb to store decompress status get around this problem?
> mhhh, complicated... on 802.15.4 6LoWPAN we use the control block
> information for fragmentation information. I don't know right now if we
> get trouble when we add the "uncompression on the fly when FRAG1 was
> received".
>
> What exactly do you mean with "decompress status"?
An integer that either contains an error code or 0 that process_data 
would set as process_data is now IPHC decompression.
> - Alex
- Martin.

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

[Alexander Aring]

On Tue, Sep 16, 2014 at 09:19:58PM +0100, Martin Townsend wrote:
> Hi Alex,
> 
> On 16/09/14 21:07, Alexander Aring wrote:
> >On Tue, Sep 16, 2014 at 08:53:06PM +0100, Martin Townsend wrote:
> >...
> >>>I make another c example, hopeful more correct than the last one:
> >>>
> >>>char *foo(char *skb)
> >>>{
> >>>         char *new;
> >>>
> >>>         if (some_error_before_consume)
> >>>                 return ERR_PTR(-EINVAL); /* here we need to do a free(skb) */
> >>>
> >>>         /* UDP expand */
> >>>         new = expand(skb, 16);
> >s/16/8 , argl, doesn't matter was only an example. :-)
> >
> >>>         if (!new)
> >>>                 return ERR_PTR(-ENOMEM);
> >>>         consume(skb); /* parameter skb becomes dangling pointer */
> >>>         skb = new; /* doesn't rescue it, it is different than skb from caller function
> >>>                       at this point, the skb_inout had rescue it, because it was a pointer
> >>>                       of pointer */
> >>>
> >>>         /* IPv6 expand */
> >>>         new = expand(skb, 40);
> >>>         if (!new) /* some error after a consume(skb), will crash at drop_skb label */
> >>>                 return ERR_PTR(-ENOMEM);
> >>>         consume(skb);
> >>>         skb = new;
> >>>
> >>>         return skb;
> >>>}
> >>I see the problem now, once the skb has been copied and then an error occurs
> >>you have to return the error and the skb has been lost.  Would using the
> >>skb->cb to store decompress status get around this problem?
> >mhhh, complicated... on 802.15.4 6LoWPAN we use the control block
> >information for fragmentation information. I don't know right now if we
> >get trouble when we add the "uncompression on the fly when FRAG1 was
> >received".
> >
> >What exactly do you mean with "decompress status"?
> An integer that either contains an error code or 0 that process_data would
> set as process_data is now IPHC decompression.

sounds for me that this basically could work, but not easy to maintain.
The control block is "normally" for store information across callbacks
which used the same sk buff.

Maybe simple do that what Jukka said, to handle the kfree_skb inside of
lowpan_process_data. What's wrong now with that?

- Alex

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

[Alexander Aring]

On Tue, Sep 16, 2014 at 10:30:32PM +0200, Alexander Aring wrote:
...
> 
> Maybe simple do that what Jukka said, to handle the kfree_skb inside of
> lowpan_process_data. What's wrong now with that?
> 

ping. Really bad issue, what's about the state to fix it?

- Alex

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

[Martin Townsend]

Hi Alex,

I'm not keen on handling kfree_skb inside of lowpan_process_data for reasons already stated in a previous email.  I'll have a rethink to see if there's another way of handling this.  One idea I'm investigating is pskb_expand_head, this doesn't create a new sk_buff but adds room to the head and/or tail which seems perfect for decompression.  There must only be a reference count of 1, so we would need to make a copy if this is the case before passing to decompression, I can't think why there would be more than 1 reference as the packet will only have passed through the 802.15.4 layer and only 6LoWPAN can decompress it.   If it's cloned for monitoring then this would be a problem.
Any thoughts on this?

- Martin.

On 25/09/14 06:55, Alexander Aring wrote:
> On Tue, Sep 16, 2014 at 10:30:32PM +0200, Alexander Aring wrote:
> ...
>> Maybe simple do that what Jukka said, to handle the kfree_skb inside of
>> lowpan_process_data. What's wrong now with that?
>>
> ping. Really bad issue, what's about the state to fix it?
>
> - 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 v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv

[Alexander Aring]

On Thu, Sep 25, 2014 at 08:25:33AM +0100, Martin Townsend wrote:
> Hi Alex,
> 
> I'm not keen on handling kfree_skb inside of lowpan_process_data for reasons already stated in a previous email.  I'll have a rethink to see if there's another way of handling this.  One idea I'm investigating is pskb_expand_head, this doesn't create a new sk_buff but adds room to the head and/or tail which seems perfect for decompression.  There must only be a reference count of 1, so we would need to make a copy if this is the case before passing to decompression, I can't think why there would be more than 1 reference as the packet will only have passed through the 802.15.4 layer and only 6LoWPAN can decompress it.   If it's cloned for monitoring then this would be a problem.
> Any thoughts on this?
> 

For me it doesn't matter if kfree_skb is inside of lowpan_process_data
or not. Go ahead and send fixes for this, I will only review if there
are still issues which are not solved by your patch.

Don't know what you exactly mean with "monitoring" now.

- Alex

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

[Alexander Aring]

Martin,

On Thu, Sep 25, 2014 at 09:31:43AM +0200, Alexander Aring wrote:
> On Thu, Sep 25, 2014 at 08:25:33AM +0100, Martin Townsend wrote:
> > Hi Alex,
> > 
> > I'm not keen on handling kfree_skb inside of lowpan_process_data for reasons already stated in a previous email.  I'll have a rethink to see if there's another way of handling this.  One idea I'm investigating is pskb_expand_head, this doesn't create a new sk_buff but adds room to the head and/or tail which seems perfect for decompression.  There must only be a reference count of 1, so we would need to make a copy if this is the case before passing to decompression, I can't think why there would be more than 1 reference as the packet will only have passed through the 802.15.4 layer and only 6LoWPAN can decompress it.   If it's cloned for monitoring then this would be a problem.
> > Any thoughts on this?
> > 
> 
> For me it doesn't matter if kfree_skb is inside of lowpan_process_data
> or not. Go ahead and send fixes for this, I will only review if there
> are still issues which are not solved by your patch.
> 
> Don't know what you exactly mean with "monitoring" now.
> 
For me it's only important to know that you still working on this issue.
If you still working on it, all seems to be fine.

Thanks, for your effort.

- Alex

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

[Martin Townsend]

Hi Alex,

Another idea that just occured to me is to use the control buffer (cb) 
in the skb to store the (de)compress_status variable.  This should be 
possible as it is only valid for this layer.  Then process_data (or 
iphc_header_decompress as I like to think of it now) will only ever 
return an skb pointer; either the same one passed in (no copy_expand) or 
more likely a new one (as we will skb_copy_expand to make room for 
decompression).  On failure process_data or functions called by this set 
the decompress status variable in skb->cb for lowpan_rcv to check.

- Martin.

On 16/09/14 18:38, Alexander Aring wrote:
> Hi Martin,
>
> On Tue, Sep 16, 2014 at 03:44:43PM +0100, Martin Townsend wrote:
>> I would like to keep freeing skb's out of process_data as process_data will become something like iphc_decompress_hdr and it would be good if that's all it did.  Otherwise I feel we are going to put a constraint on all future header decompression routines in that they must free the skb on error.  I think it would be better to defer this so on error you might want to try something else with the skb, maybe not but at least the option is there.
>> So how about
>>
>>          struct sk_buff * ret_skb;
>>          switch (skb->data[0] & 0xe0) {
>>          case LOWPAN_DISPATCH_IPHC:    /* ipv6 datagram */
>>              ret_skb = process_data(skb, &hdr);
>>              if (IS_ERR(ret_skb))
>>                  goto drop_skb;
>>              else
>>                  skb = ret_skb;
>>              break;
>>
>> I know we currently have 3 calls to process_data so it will look fairly ugly in this patch but in my next patch to fix lowpan_rcv to handle uncompressed IPv6 packets that are fragmented there will only be one call to process_data so it won't look so bad.  You could even wrap it in a macro but I'm not a fan of this as they can obfuscate the code a bit.
>>
>> Thoughts?
>>
> sorry, I can't follow how this solve the issue if the "parameter skb" is
> already consumed or not. If process_data returns a error before
> parameter consume, then we should run kfree_skb(parameter_skb), if it's
> afterwards we should do nothing. Point is we don't know that there. I
> suppose if we do consume_skb and refcount reach 0 the parameter_skb
> becomes a dangling pointer.
>
> - 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

[PATCH v4 bluetooth] Fix lowpan_rcv

[Martin Townsend]

This series aims to 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

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

Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
---

v1 -> v2:
* combined first two patches into one.
* split out renaming of process_data into a new patch
* re-use err in lowpan_process_data and remove err_ret local variable.
* removed third patch as applies to linux-wpan

v2 -> v3: (Updated from Alexander Aring's Review) 
* re-use err hadn't been included in patch.
* fixed some 'goto drop' cases in lowpan_rcv that should be 'goto drop skb'
* handle error codes when returning from lowpan_frag_rcv.
* Refactored lowpan_give_skb_to_devices so it consumes or free's skb and then
  only returns NET_RX status codes.

v3 -> v4: (Updated from Jukka Rissanen and Alexander Aring's Reviews)
* Removed use of double skb pointer in decompress routines in favour of using
  pskb_expand_head to obtain more headroom.  Requires an uncloned skb so the
  calling functions now ensure this.
* prefer consume_skb on non error conditions.
* removed spurious setting of ret in lowpan_rcv

Martin Townsend (1):
  6lowpan: fix incorrect return values in lowpan_rcv

 include/net/6lowpan.h         |  9 +++--
 net/6lowpan/iphc.c            | 92 ++++++++++++++++---------------------------
 net/bluetooth/6lowpan.c       | 52 ++++++++++++++++--------
 net/ieee802154/6lowpan_rtnl.c | 66 ++++++++++++++++++++-----------
 4 files changed, 117 insertions(+), 102 deletions(-)

-- 
1.9.1

Re: [PATCH v4 bluetooth] Fix lowpan_rcv

[Jukka Rissanen]

Hi Martin,

just in case add me to cc: next time so I do not miss these 6lowpan
patches.

On ke, 2014-10-01 at 13:10 +0100, Martin Townsend wrote:
> This series aims to 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
> 
> 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

FYI, I tried this with latest bluetooth-next tree and got following oops
when trying to connect to peer bt 6lowpan device. The system probably
crashed before any packet was delivered to peer as I did not see
anything on the other end of the connection.


BUG: unable to handle kernel NULL pointer dereference at 000000a0
IP: [<c17712c3>] __netif_receive_skb_core+0xa3/0x7b0
*pde = 00000000 
Oops: 0000 [#1] PREEMPT SMP 
Modules linked in: bluetooth_6lowpan 6lowpan rfcomm bnep ecb btusb
bluetooth nfc rfkill snd_intel8x0 ohci_pci snd_ac97_codec ac97_bus
parport_pc parport
CPU: 0 PID: 353 Comm: systemd-network Not tainted 3.17.0-rc1-bt6lowpan
#1
Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox
12/01/2006
task: ccb695e0 ti: cc88e000 task.ti: cc88e000
EIP: 0060:[<c17712c3>] EFLAGS: 00010282 CPU: 0
EIP is at __netif_receive_skb_core+0xa3/0x7b0
EAX: cd964900 EBX: cd964900 ECX: 00000000 EDX: 00000000
ESI: cd964900 EDI: 00000001 EBP: cf00bf4c ESP: cf00bf18
 DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
CR0: 8005003b CR2: d0cd6040 CR3: 0d057000 CR4: 000006d0
Stack:
 00000002 00000000 00000000 c1771279 cf73e698 cf00bf00 00000000 00000001
 cd964900 00000046 cd964900 00000000 cf73e698 cf00bf5c c17719eb cd964900
 00000000 cf00bf80 c177300f cf73e664 cf73e670 00000040 cf73e5d4 cf73e698
Call Trace:
 [<c1771279>] ? __netif_receive_skb_core+0x59/0x7b0
 [<c17719eb>] __netif_receive_skb+0x1b/0x70
 [<c177300f>] process_backlog+0x9f/0x140
 [<c1772e48>] net_rx_action+0x128/0x250
 [<c104fd84>] __do_softirq+0xd4/0x300
 [<c104fcb0>] ? __local_bh_enable_ip+0xf0/0xf0
 [<c10049fc>] do_softirq_own_stack+0x2c/0x40
 <IRQ> 
 [<c1050136>] irq_exit+0x86/0xb0
 [<c188bd98>] smp_apic_timer_interrupt+0x38/0x50
 [<c188b6ce>] apic_timer_interrupt+0x32/0x38
Code: 00 31 c9 c7 44 24 04 00 00 00 00 c7 04 24 02 00 00 00 31 d2 b8 14
42 c0 c1 e8 0a 1f 92 ff c7 45 e8 01 00 00 00 8b 45 ec 8b 50 14 <8b> 92
a0 00 00 00 89 50 74 b8 f8 7c ad c1 e8 1a f7 c5 ff 8b 5d
EIP: [<c17712c3>] __netif_receive_skb_core+0xa3/0x7b0 SS:ESP
0068:cf00bf18
CR2: 00000000000000a0
---[ end trace 704a740b1671072d ]---
Kernel panic - not syncing: Fatal exception in interrupt
Kernel Offset: 0x0 from 0xc1000000 (relocation range:
0xc0000000-0xd07effff)
---[ end Kernel panic - not syncing: Fatal exception in interrupt



Cheers,
Jukka

Re: [PATCH v4 bluetooth] Fix lowpan_rcv

[Jukka Rissanen]

Hi Martin,

just in case add me to cc: next time so I do not miss these 6lowpan
patches.

On ke, 2014-10-01 at 13:10 +0100, Martin Townsend wrote:
> This series aims to 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
> 
> 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

FYI, I tried this with latest bluetooth-next tree and got following oops
when trying to connect to peer bt 6lowpan device. The system probably
crashed before any packet was delivered to peer as I did not see
anything on the other end of the connection.


BUG: unable to handle kernel NULL pointer dereference at 000000a0
IP: [<c17712c3>] __netif_receive_skb_core+0xa3/0x7b0
*pde = 00000000 
Oops: 0000 [#1] PREEMPT SMP 
Modules linked in: bluetooth_6lowpan 6lowpan rfcomm bnep ecb btusb
bluetooth nfc rfkill snd_intel8x0 ohci_pci snd_ac97_codec ac97_bus
parport_pc parport
CPU: 0 PID: 353 Comm: systemd-network Not tainted 3.17.0-rc1-bt6lowpan
#1
Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox
12/01/2006
task: ccb695e0 ti: cc88e000 task.ti: cc88e000
EIP: 0060:[<c17712c3>] EFLAGS: 00010282 CPU: 0
EIP is at __netif_receive_skb_core+0xa3/0x7b0
EAX: cd964900 EBX: cd964900 ECX: 00000000 EDX: 00000000
ESI: cd964900 EDI: 00000001 EBP: cf00bf4c ESP: cf00bf18
 DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
CR0: 8005003b CR2: d0cd6040 CR3: 0d057000 CR4: 000006d0
Stack:
 00000002 00000000 00000000 c1771279 cf73e698 cf00bf00 00000000 00000001
 cd964900 00000046 cd964900 00000000 cf73e698 cf00bf5c c17719eb cd964900
 00000000 cf00bf80 c177300f cf73e664 cf73e670 00000040 cf73e5d4 cf73e698
Call Trace:
 [<c1771279>] ? __netif_receive_skb_core+0x59/0x7b0
 [<c17719eb>] __netif_receive_skb+0x1b/0x70
 [<c177300f>] process_backlog+0x9f/0x140
 [<c1772e48>] net_rx_action+0x128/0x250
 [<c104fd84>] __do_softirq+0xd4/0x300
 [<c104fcb0>] ? __local_bh_enable_ip+0xf0/0xf0
 [<c10049fc>] do_softirq_own_stack+0x2c/0x40
 <IRQ> 
 [<c1050136>] irq_exit+0x86/0xb0
 [<c188bd98>] smp_apic_timer_interrupt+0x38/0x50
 [<c188b6ce>] apic_timer_interrupt+0x32/0x38
Code: 00 31 c9 c7 44 24 04 00 00 00 00 c7 04 24 02 00 00 00 31 d2 b8 14
42 c0 c1 e8 0a 1f 92 ff c7 45 e8 01 00 00 00 8b 45 ec 8b 50 14 <8b> 92
a0 00 00 00 89 50 74 b8 f8 7c ad c1 e8 1a f7 c5 ff 8b 5d
EIP: [<c17712c3>] __netif_receive_skb_core+0xa3/0x7b0 SS:ESP
0068:cf00bf18
CR2: 00000000000000a0
---[ end trace 704a740b1671072d ]---
Kernel panic - not syncing: Fatal exception in interrupt
Kernel Offset: 0x0 from 0xc1000000 (relocation range:
0xc0000000-0xd07effff)
---[ end Kernel panic - not syncing: Fatal exception in interrupt



Cheers,
Jukka

Re: [PATCH v4 bluetooth] Fix lowpan_rcv

[Martin Townsend]

Hi Jukka,

I spotted one thing in recv_pkt when skb_copy_expand fails it should goto drop but this is not what you are seeing.  This also exists in 6lowpan_rtnl.c so I will fix this in both for the next patch.

I would be interested to know if the 802.15.4 wireless guys are seeing this to narrow this down.  I don't see this but then I had to manually add the code into my bluetooth tree from ours due to differences so I could have well screwed something up.

- Martin.

On 01/10/14 15:47, Jukka Rissanen wrote:
> Hi Martin,
>
> just in case add me to cc: next time so I do not miss these 6lowpan
> patches.
>
> On ke, 2014-10-01 at 13:10 +0100, Martin Townsend wrote:
>> This series aims to 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
>>
>> 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
> FYI, I tried this with latest bluetooth-next tree and got following oops
> when trying to connect to peer bt 6lowpan device. The system probably
> crashed before any packet was delivered to peer as I did not see
> anything on the other end of the connection.
>
>
> BUG: unable to handle kernel NULL pointer dereference at 000000a0
> IP: [<c17712c3>] __netif_receive_skb_core+0xa3/0x7b0
> *pde = 00000000 
> Oops: 0000 [#1] PREEMPT SMP 
> Modules linked in: bluetooth_6lowpan 6lowpan rfcomm bnep ecb btusb
> bluetooth nfc rfkill snd_intel8x0 ohci_pci snd_ac97_codec ac97_bus
> parport_pc parport
> CPU: 0 PID: 353 Comm: systemd-network Not tainted 3.17.0-rc1-bt6lowpan
> #1
> Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox
> 12/01/2006
> task: ccb695e0 ti: cc88e000 task.ti: cc88e000
> EIP: 0060:[<c17712c3>] EFLAGS: 00010282 CPU: 0
> EIP is at __netif_receive_skb_core+0xa3/0x7b0
> EAX: cd964900 EBX: cd964900 ECX: 00000000 EDX: 00000000
> ESI: cd964900 EDI: 00000001 EBP: cf00bf4c ESP: cf00bf18
>  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
> CR0: 8005003b CR2: d0cd6040 CR3: 0d057000 CR4: 000006d0
> Stack:
>  00000002 00000000 00000000 c1771279 cf73e698 cf00bf00 00000000 00000001
>  cd964900 00000046 cd964900 00000000 cf73e698 cf00bf5c c17719eb cd964900
>  00000000 cf00bf80 c177300f cf73e664 cf73e670 00000040 cf73e5d4 cf73e698
> Call Trace:
>  [<c1771279>] ? __netif_receive_skb_core+0x59/0x7b0
>  [<c17719eb>] __netif_receive_skb+0x1b/0x70
>  [<c177300f>] process_backlog+0x9f/0x140
>  [<c1772e48>] net_rx_action+0x128/0x250
>  [<c104fd84>] __do_softirq+0xd4/0x300
>  [<c104fcb0>] ? __local_bh_enable_ip+0xf0/0xf0
>  [<c10049fc>] do_softirq_own_stack+0x2c/0x40
>  <IRQ> 
>  [<c1050136>] irq_exit+0x86/0xb0
>  [<c188bd98>] smp_apic_timer_interrupt+0x38/0x50
>  [<c188b6ce>] apic_timer_interrupt+0x32/0x38
> Code: 00 31 c9 c7 44 24 04 00 00 00 00 c7 04 24 02 00 00 00 31 d2 b8 14
> 42 c0 c1 e8 0a 1f 92 ff c7 45 e8 01 00 00 00 8b 45 ec 8b 50 14 <8b> 92
> a0 00 00 00 89 50 74 b8 f8 7c ad c1 e8 1a f7 c5 ff 8b 5d
> EIP: [<c17712c3>] __netif_receive_skb_core+0xa3/0x7b0 SS:ESP
> 0068:cf00bf18
> CR2: 00000000000000a0
> ---[ end trace 704a740b1671072d ]---
> Kernel panic - not syncing: Fatal exception in interrupt
> Kernel Offset: 0x0 from 0xc1000000 (relocation range:
> 0xc0000000-0xd07effff)
> ---[ end Kernel panic - not syncing: Fatal exception in interrupt
>
>
>
> Cheers,
> Jukka
>
>
>
> --
> 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 v4 bluetooth] Fix lowpan_rcv

[Jukka Rissanen]

Hi Martin,

On ke, 2014-10-01 at 16:24 +0100, Martin Townsend wrote:
> Hi Jukka,
> 
> I spotted one thing in recv_pkt when skb_copy_expand fails it should goto drop but this is not what you are seeing.  This also exists in 6lowpan_rtnl.c so I will fix this in both for the next patch.
> 
> I would be interested to know if the 802.15.4 wireless guys are seeing this to narrow this down.  I don't see this but then I had to manually add the code into my bluetooth tree from ours due to differences so I could have well screwed something up.
> 
> - Martin.
> 
> On 01/10/14 15:47, Jukka Rissanen wrote:
> > Hi Martin,
> >
> > just in case add me to cc: next time so I do not miss these 6lowpan
> > patches.
> >
> > On ke, 2014-10-01 at 13:10 +0100, Martin Townsend wrote:
> >> This series aims to 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
> >>
> >> 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
> > FYI, I tried this with latest bluetooth-next tree and got following oops
> > when trying to connect to peer bt 6lowpan device. The system probably
> > crashed before any packet was delivered to peer as I did not see
> > anything on the other end of the connection.
> >
> >
> > BUG: unable to handle kernel NULL pointer dereference at 000000a0
> > IP: [<c17712c3>] __netif_receive_skb_core+0xa3/0x7b0
> > *pde = 00000000 
> > Oops: 0000 [#1] PREEMPT SMP 
> > Modules linked in: bluetooth_6lowpan 6lowpan rfcomm bnep ecb btusb
> > bluetooth nfc rfkill snd_intel8x0 ohci_pci snd_ac97_codec ac97_bus
> > parport_pc parport
> > CPU: 0 PID: 353 Comm: systemd-network Not tainted 3.17.0-rc1-bt6lowpan
> > #1
> > Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox
> > 12/01/2006
> > task: ccb695e0 ti: cc88e000 task.ti: cc88e000
> > EIP: 0060:[<c17712c3>] EFLAGS: 00010282 CPU: 0
> > EIP is at __netif_receive_skb_core+0xa3/0x7b0
> > EAX: cd964900 EBX: cd964900 ECX: 00000000 EDX: 00000000
> > ESI: cd964900 EDI: 00000001 EBP: cf00bf4c ESP: cf00bf18
> >  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
> > CR0: 8005003b CR2: d0cd6040 CR3: 0d057000 CR4: 000006d0
> > Stack:
> >  00000002 00000000 00000000 c1771279 cf73e698 cf00bf00 00000000 00000001
> >  cd964900 00000046 cd964900 00000000 cf73e698 cf00bf5c c17719eb cd964900
> >  00000000 cf00bf80 c177300f cf73e664 cf73e670 00000040 cf73e5d4 cf73e698
> > Call Trace:
> >  [<c1771279>] ? __netif_receive_skb_core+0x59/0x7b0

I investigated this a bit more by adding some debug prints into
__netif_receive_skb_core() and the reason for the oops is that the
skb->dev is NULL.

The culprit seems to be this removal from skb_deliver() function:

-	new->dev = dev;

And then dev is null in bluetooth side. You seem to set it in ieee802154
side so the oops is not shown there.


> >  [<c17719eb>] __netif_receive_skb+0x1b/0x70
> >  [<c177300f>] process_backlog+0x9f/0x140
> >  [<c1772e48>] net_rx_action+0x128/0x250
> >  [<c104fd84>] __do_softirq+0xd4/0x300
> >  [<c104fcb0>] ? __local_bh_enable_ip+0xf0/0xf0
> >  [<c10049fc>] do_softirq_own_stack+0x2c/0x40
> >  <IRQ> 
> >  [<c1050136>] irq_exit+0x86/0xb0
> >  [<c188bd98>] smp_apic_timer_interrupt+0x38/0x50
> >  [<c188b6ce>] apic_timer_interrupt+0x32/0x38
> > Code: 00 31 c9 c7 44 24 04 00 00 00 00 c7 04 24 02 00 00 00 31 d2 b8 14
> > 42 c0 c1 e8 0a 1f 92 ff c7 45 e8 01 00 00 00 8b 45 ec 8b 50 14 <8b> 92
> > a0 00 00 00 89 50 74 b8 f8 7c ad c1 e8 1a f7 c5 ff 8b 5d
> > EIP: [<c17712c3>] __netif_receive_skb_core+0xa3/0x7b0 SS:ESP
> > 0068:cf00bf18
> > CR2: 00000000000000a0
> > ---[ end trace 704a740b1671072d ]---
> > Kernel panic - not syncing: Fatal exception in interrupt
> > Kernel Offset: 0x0 from 0xc1000000 (relocation range:
> > 0xc0000000-0xd07effff)
> > ---[ end Kernel panic - not syncing: Fatal exception in interrupt
> >
> >
> >

Cheers,
Jukka

Re: [PATCH v4 bluetooth] Fix lowpan_rcv

[Martin Townsend]

Hi Jukka,

Thanks for investigating this. 

If you set the dev in your receive function does the oops go away,  BTW this is where the pkt_type is set to PACKET_HOST, for us this was causing problems, I don't know if this is the same for you guys.

Here's the code:

            local_skb->protocol = htons(ETH_P_IPV6);
>>>     local_skb->dev = dev;
            local_skb->pkt_type = PACKET_HOST;

            if (give_skb_to_upper(local_skb, dev)
                            != NET_RX_SUCCESS) {
                kfree_skb(local_skb);

- Martin.

On 02/10/14 12:28, Jukka Rissanen wrote:
> Hi Martin,
>
> On ke, 2014-10-01 at 16:24 +0100, Martin Townsend wrote:
>> Hi Jukka,
>>
>> I spotted one thing in recv_pkt when skb_copy_expand fails it should goto drop but this is not what you are seeing.  This also exists in 6lowpan_rtnl.c so I will fix this in both for the next patch.
>>
>> I would be interested to know if the 802.15.4 wireless guys are seeing this to narrow this down.  I don't see this but then I had to manually add the code into my bluetooth tree from ours due to differences so I could have well screwed something up.
>>
>> - Martin.
>>
>> On 01/10/14 15:47, Jukka Rissanen wrote:
>>> Hi Martin,
>>>
>>> just in case add me to cc: next time so I do not miss these 6lowpan
>>> patches.
>>>
>>> On ke, 2014-10-01 at 13:10 +0100, Martin Townsend wrote:
>>>> This series aims to 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
>>>>
>>>> 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
>>> FYI, I tried this with latest bluetooth-next tree and got following oops
>>> when trying to connect to peer bt 6lowpan device. The system probably
>>> crashed before any packet was delivered to peer as I did not see
>>> anything on the other end of the connection.
>>>
>>>
>>> BUG: unable to handle kernel NULL pointer dereference at 000000a0
>>> IP: [<c17712c3>] __netif_receive_skb_core+0xa3/0x7b0
>>> *pde = 00000000 
>>> Oops: 0000 [#1] PREEMPT SMP 
>>> Modules linked in: bluetooth_6lowpan 6lowpan rfcomm bnep ecb btusb
>>> bluetooth nfc rfkill snd_intel8x0 ohci_pci snd_ac97_codec ac97_bus
>>> parport_pc parport
>>> CPU: 0 PID: 353 Comm: systemd-network Not tainted 3.17.0-rc1-bt6lowpan
>>> #1
>>> Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox
>>> 12/01/2006
>>> task: ccb695e0 ti: cc88e000 task.ti: cc88e000
>>> EIP: 0060:[<c17712c3>] EFLAGS: 00010282 CPU: 0
>>> EIP is at __netif_receive_skb_core+0xa3/0x7b0
>>> EAX: cd964900 EBX: cd964900 ECX: 00000000 EDX: 00000000
>>> ESI: cd964900 EDI: 00000001 EBP: cf00bf4c ESP: cf00bf18
>>>  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
>>> CR0: 8005003b CR2: d0cd6040 CR3: 0d057000 CR4: 000006d0
>>> Stack:
>>>  00000002 00000000 00000000 c1771279 cf73e698 cf00bf00 00000000 00000001
>>>  cd964900 00000046 cd964900 00000000 cf73e698 cf00bf5c c17719eb cd964900
>>>  00000000 cf00bf80 c177300f cf73e664 cf73e670 00000040 cf73e5d4 cf73e698
>>> Call Trace:
>>>  [<c1771279>] ? __netif_receive_skb_core+0x59/0x7b0
> I investigated this a bit more by adding some debug prints into
> __netif_receive_skb_core() and the reason for the oops is that the
> skb->dev is NULL.
>
> The culprit seems to be this removal from skb_deliver() function:
>
> -	new->dev = dev;
>
> And then dev is null in bluetooth side. You seem to set it in ieee802154
> side so the oops is not shown there.
>
>
>>>  [<c17719eb>] __netif_receive_skb+0x1b/0x70
>>>  [<c177300f>] process_backlog+0x9f/0x140
>>>  [<c1772e48>] net_rx_action+0x128/0x250
>>>  [<c104fd84>] __do_softirq+0xd4/0x300
>>>  [<c104fcb0>] ? __local_bh_enable_ip+0xf0/0xf0
>>>  [<c10049fc>] do_softirq_own_stack+0x2c/0x40
>>>  <IRQ> 
>>>  [<c1050136>] irq_exit+0x86/0xb0
>>>  [<c188bd98>] smp_apic_timer_interrupt+0x38/0x50
>>>  [<c188b6ce>] apic_timer_interrupt+0x32/0x38
>>> Code: 00 31 c9 c7 44 24 04 00 00 00 00 c7 04 24 02 00 00 00 31 d2 b8 14
>>> 42 c0 c1 e8 0a 1f 92 ff c7 45 e8 01 00 00 00 8b 45 ec 8b 50 14 <8b> 92
>>> a0 00 00 00 89 50 74 b8 f8 7c ad c1 e8 1a f7 c5 ff 8b 5d
>>> EIP: [<c17712c3>] __netif_receive_skb_core+0xa3/0x7b0 SS:ESP
>>> 0068:cf00bf18
>>> CR2: 00000000000000a0
>>> ---[ end trace 704a740b1671072d ]---
>>> Kernel panic - not syncing: Fatal exception in interrupt
>>> Kernel Offset: 0x0 from 0xc1000000 (relocation range:
>>> 0xc0000000-0xd07effff)
>>> ---[ end Kernel panic - not syncing: Fatal exception in interrupt
>>>
>>>
>>>
> Cheers,
> Jukka
>
>
> --
> 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 v4 bluetooth] Fix lowpan_rcv

[Jukka Rissanen]

Hi Martin,

On to, 2014-10-02 at 13:16 +0100, Martin Townsend wrote:
> Hi Jukka,
> 
> Thanks for investigating this. 
> 
> If you set the dev in your receive function does the oops go away,  BTW this is where the pkt_type is set to PACKET_HOST, for us this was causing problems, I don't know if this is the same for you guys.
> 
> Here's the code:
> 
>             local_skb->protocol = htons(ETH_P_IPV6);
> >>>     local_skb->dev = dev;
>             local_skb->pkt_type = PACKET_HOST;
> 
>             if (give_skb_to_upper(local_skb, dev)
>                             != NET_RX_SUCCESS) {
>                 kfree_skb(local_skb);
> 
> - Martin.

So I applied this patch

diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index e4f5ce5..aa64f91 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -323,6 +323,7 @@ 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;
+               local_skb->dev = dev;
 
                if (give_skb_to_upper(local_skb, dev) != NET_RX_SUCCESS)
{
                        goto drop_local_skb;
@@ -362,6 +363,7 @@ 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;
+                       local_skb->dev = dev;
 
                        if (give_skb_to_upper(local_skb, dev)
                                                        !=
NET_RX_SUCCESS) {


and I do not see the earlier null pointer oops any more.


Unfortunately a new error is seen:

[  340.676353] inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage.
[  340.676353] kworker/u3:1/370 [HC0[0]:SC0[0]:HE1:SE1] takes:
[  340.676353]  (&(&list->lock)->rlock#6){+.?...}, at: [<f8319d4c>]
hci_send_acl+0xac/0x290 [bluetooth]
[  340.676353] {IN-SOFTIRQ-W} state was registered at:
[  340.676353]   [<c10915a3>] __lock_acquire+0x6d3/0x1d20
[  340.676353]   [<c109325d>] lock_acquire+0x9d/0x140
[  340.676353]   [<c1889c25>] _raw_spin_lock+0x45/0x80
[  340.676353]   [<f8319d4c>] hci_send_acl+0xac/0x290 [bluetooth]
[  340.676353]   [<f833abf0>] l2cap_do_send+0x60/0x100 [bluetooth]
[  340.676353]   [<f833e7c0>] l2cap_chan_send+0x7f0/0x10e0 [bluetooth]
[  340.676353]   [<f850691e>] send_pkt+0x4e/0xa0 [bluetooth_6lowpan]
[  340.676353]   [<f8506d20>] bt_xmit+0x3b0/0x770 [bluetooth_6lowpan]
[  340.676353]   [<c17742f4>] dev_hard_start_xmit+0x344/0x670
[  340.676353]   [<c17749ad>] __dev_queue_xmit+0x38d/0x680
[  340.676353]   [<c1774caf>] dev_queue_xmit+0xf/0x20
[  340.676353]   [<c177b8b0>] neigh_connected_output+0x130/0x1a0
[  340.676353]   [<c1812a63>] ip6_finish_output2+0x173/0x8c0
[  340.676353]   [<c18182db>] ip6_finish_output+0x7b/0x1b0
[  340.676353]   [<c18184a7>] ip6_output+0x97/0x2a0
...

I have to investigate this more what your new code is doing as I did not
see this error earlier before your patch.

So I have very latest bluetooth-next (commit b57d4471fd18) + your v4
patch + above patch that sets the dev pointer, and this combination
triggers the above inconsistent softirq usage error.


Cheers,
Jukka

Re: [PATCH v4 bluetooth] Fix lowpan_rcv

[Martin Townsend]

Hi Jukka,

Very bizarre that the patch causes a locking issue in the Tx path.  Is 
it a side effect from not cloning the skb before calling process_data.  
I couldn't see why a clone was being made so maybe I'm missing something 
here.

- Martin.


On 02/10/14 14:55, Jukka Rissanen wrote:
> Hi Martin,
>
> On to, 2014-10-02 at 13:16 +0100, Martin Townsend wrote:
>> Hi Jukka,
>>
>> Thanks for investigating this.
>>
>> If you set the dev in your receive function does the oops go away,  BTW this is where the pkt_type is set to PACKET_HOST, for us this was causing problems, I don't know if this is the same for you guys.
>>
>> Here's the code:
>>
>>              local_skb->protocol = htons(ETH_P_IPV6);
>>>>>      local_skb->dev = dev;
>>              local_skb->pkt_type = PACKET_HOST;
>>
>>              if (give_skb_to_upper(local_skb, dev)
>>                              != NET_RX_SUCCESS) {
>>                  kfree_skb(local_skb);
>>
>> - Martin.
> So I applied this patch
>
> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> index e4f5ce5..aa64f91 100644
> --- a/net/bluetooth/6lowpan.c
> +++ b/net/bluetooth/6lowpan.c
> @@ -323,6 +323,7 @@ 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;
> +               local_skb->dev = dev;
>   
>                  if (give_skb_to_upper(local_skb, dev) != NET_RX_SUCCESS)
> {
>                          goto drop_local_skb;
> @@ -362,6 +363,7 @@ 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;
> +                       local_skb->dev = dev;
>   
>                          if (give_skb_to_upper(local_skb, dev)
>                                                          !=
> NET_RX_SUCCESS) {
>
>
> and I do not see the earlier null pointer oops any more.
>
>
> Unfortunately a new error is seen:
>
> [  340.676353] inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage.
> [  340.676353] kworker/u3:1/370 [HC0[0]:SC0[0]:HE1:SE1] takes:
> [  340.676353]  (&(&list->lock)->rlock#6){+.?...}, at: [<f8319d4c>]
> hci_send_acl+0xac/0x290 [bluetooth]
> [  340.676353] {IN-SOFTIRQ-W} state was registered at:
> [  340.676353]   [<c10915a3>] __lock_acquire+0x6d3/0x1d20
> [  340.676353]   [<c109325d>] lock_acquire+0x9d/0x140
> [  340.676353]   [<c1889c25>] _raw_spin_lock+0x45/0x80
> [  340.676353]   [<f8319d4c>] hci_send_acl+0xac/0x290 [bluetooth]
> [  340.676353]   [<f833abf0>] l2cap_do_send+0x60/0x100 [bluetooth]
> [  340.676353]   [<f833e7c0>] l2cap_chan_send+0x7f0/0x10e0 [bluetooth]
> [  340.676353]   [<f850691e>] send_pkt+0x4e/0xa0 [bluetooth_6lowpan]
> [  340.676353]   [<f8506d20>] bt_xmit+0x3b0/0x770 [bluetooth_6lowpan]
> [  340.676353]   [<c17742f4>] dev_hard_start_xmit+0x344/0x670
> [  340.676353]   [<c17749ad>] __dev_queue_xmit+0x38d/0x680
> [  340.676353]   [<c1774caf>] dev_queue_xmit+0xf/0x20
> [  340.676353]   [<c177b8b0>] neigh_connected_output+0x130/0x1a0
> [  340.676353]   [<c1812a63>] ip6_finish_output2+0x173/0x8c0
> [  340.676353]   [<c18182db>] ip6_finish_output+0x7b/0x1b0
> [  340.676353]   [<c18184a7>] ip6_output+0x97/0x2a0
> ...
>
> I have to investigate this more what your new code is doing as I did not
> see this error earlier before your patch.
>
> So I have very latest bluetooth-next (commit b57d4471fd18) + your v4
> patch + above patch that sets the dev pointer, and this combination
> triggers the above inconsistent softirq usage error.
>
>
> Cheers,
> Jukka
>
>