[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)
* 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

[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 refactored so eliminate incorrect return values.

Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
---
 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(-)

diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
index d184df1..05ff67e 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);
+int
+lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
+		    const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
+		    const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
+		    u8 iphc0, u8 iphc1);
 int lowpan_header_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..3888357 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,10 +301,11 @@ err:
 /* TTL uncompression values */
 static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
 
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
-			const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
-			const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
-			u8 iphc0, u8 iphc1, skb_delivery_cb deliver_skb)
+int
+lowpan_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;
@@ -348,7 +318,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 -EINVAL;
 	}
 
 	hdr.version = 6;
@@ -360,7 +330,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 -EINVAL;
 
 		memcpy(&hdr.flow_lbl, &skb->data[0], 3);
 		skb_pull(skb, 3);
@@ -373,7 +343,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 -EINVAL;
 
 		hdr.priority = ((tmp >> 2) & 0x0f);
 		hdr.flow_lbl[0] = ((tmp << 6) & 0xC0) | ((tmp >> 2) & 0x30);
@@ -383,7 +353,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 -EINVAL;
 
 		hdr.flow_lbl[0] = (skb->data[0] & 0x0F) | ((tmp >> 2) & 0x30);
 		memcpy(&hdr.flow_lbl[1], &skb->data[0], 2);
@@ -400,7 +370,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 -EINVAL;
 
 		pr_debug("NH flag is set, next header carried inline: %02x\n",
 			 hdr.nexthdr);
@@ -412,7 +382,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 -EINVAL;
 	}
 
 	/* Extract SAM to the tmp variable */
@@ -431,7 +401,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 
 	/* Check on error of previous branch */
 	if (err)
-		goto drop;
+		return -EINVAL;
 
 	/* Extract DAM to the tmp variable */
 	tmp = ((iphc1 & LOWPAN_IPHC_DAM_11) >> LOWPAN_IPHC_DAM_BIT) & 0x03;
@@ -446,7 +416,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 								tmp);
 
 			if (err)
-				goto drop;
+				return -EINVAL;
 		}
 	} else {
 		err = uncompress_addr(skb, &hdr.daddr, tmp, daddr,
@@ -454,28 +424,26 @@ 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 -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 -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;
+		if (skb_headroom(skb) < sizeof(struct udphdr) + sizeof(hdr)) {
+			int n = sizeof(struct udphdr) + sizeof(hdr);
 
-		skb = new;
+			err = pskb_expand_head(skb, n, 0, GFP_ATOMIC);
+			if (unlikely(err))
+				return err;
+		}
 
 		skb_push(skb, sizeof(struct udphdr));
 		skb_reset_transport_header(skb);
@@ -485,6 +453,13 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 			       (u8 *)&uh, sizeof(uh));
 
 		hdr.nexthdr = UIP_PROTO_UDP;
+	} else {
+		if (skb_headroom(skb) < sizeof(hdr)) {
+			err = pskb_expand_head(skb, sizeof(hdr), 0, GFP_ATOMIC);
+
+			if (unlikely(err))
+				return err;
+		}
 	}
 
 	hdr.payload_len = htons(skb->len);
@@ -499,11 +474,14 @@ 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);
+	skb_push(skb, sizeof(hdr));
+	skb_reset_network_header(skb);
+	skb_copy_to_linear_data(skb, &hdr, sizeof(hdr));
 
-drop:
-	kfree_skb(skb);
-	return -EINVAL;
+	raw_dump_table(__func__, "raw skb data dump before receiving",
+		       skb->data, skb->len);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(lowpan_process_data);
 
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index 206b65c..adfd361 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -230,36 +230,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 -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 -EINVAL;
 
 	if (lowpan_fetch_skb_u8(skb, &iphc0))
-		goto drop;
+		return -EINVAL;
 
 	if (lowpan_fetch_skb_u8(skb, &iphc1))
-		goto drop;
+		return -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,12 +275,8 @@ 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;
+			goto drop_local_skb;
 		}
 
 		dev->stats.rx_bytes += skb->len;
@@ -294,15 +285,40 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
 		kfree_skb(local_skb);
 		kfree_skb(skb);
 	} else {
+		int ret;
+
+		if (skb_cloned(skb)) {
+			struct sk_buff *new;
+			int new_headroom = sizeof(struct ipv6hdr) +
+					   sizeof(struct udphdr);
+
+			new = skb_copy_expand(skb, new_headroom,
+					      skb_tailroom(skb), GFP_ATOMIC);
+			if (!new)
+				return -ENOMEM;
+			consume_skb(skb);
+			skb = new;
+		}
+
 		switch (skb->data[0] & 0xe0) {
 		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
+
+			ret = process_data(skb, dev, chan);
+			if (ret < 0)
+				goto drop;
+
 			local_skb = skb_clone(skb, GFP_ATOMIC);
 			if (!local_skb)
 				goto drop;
 
-			ret = process_data(local_skb, dev, chan);
-			if (ret != NET_RX_SUCCESS)
+			local_skb->protocol = htons(ETH_P_IPV6);
+			local_skb->pkt_type = PACKET_HOST;
+
+			if (give_skb_to_upper(local_skb, dev)
+							!= NET_RX_SUCCESS) {
+				kfree_skb(local_skb);
 				goto drop;
+			}
 
 			dev->stats.rx_bytes += skb->len;
 			dev->stats.rx_packets++;
@@ -316,6 +332,8 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
 
 	return NET_RX_SUCCESS;
 
+drop_local_skb:
+	kfree_skb(local_skb);
 drop:
 	dev->stats.rx_dropped++;
 	kfree_skb(skb);
diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
index 6591d27..c64baa3 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,6 +165,8 @@ static int lowpan_give_skb_to_devices(struct sk_buff *skb,
 		}
 	rcu_read_unlock();
 
+	consume_skb(skb);
+
 	return stat;
 }
 
@@ -176,13 +179,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 -EINVAL;
 
 	if (lowpan_fetch_skb_u8(skb, &iphc0))
-		goto drop;
+		return -EINVAL;
 
 	if (lowpan_fetch_skb_u8(skb, &iphc1))
-		goto drop;
+		return -EINVAL;
 
 	ieee802154_addr_to_sa(&sa, &hdr->source);
 	ieee802154_addr_to_sa(&da, &hdr->dest);
@@ -199,12 +202,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 +476,52 @@ 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 {
+		/* Decompression may use pskb_expand_head, ensure skb unique */
+		if (skb_cloned(skb)) {
+			struct sk_buff *new;
+			int new_headroom = sizeof(struct ipv6hdr) +
+					   sizeof(struct udphdr);
+
+			new = skb_copy_expand(skb, new_headroom,
+					      skb_tailroom(skb), GFP_ATOMIC);
+			if (!new)
+				return -ENOMEM;
+			consume_skb(skb);
+			skb = new;
+		}
+
 		switch (skb->data[0] & 0xe0) {
 		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
 			ret = process_data(skb, &hdr);
-			if (ret == NET_RX_DROP)
-				goto drop;
+			if (ret < 0)
+				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;
+				if (ret < 0)
+					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;
+				if (ret < 0)
+					goto drop_skb;
+			} else if (ret < 0) {
+				goto drop;
+			} else {
+				return NET_RX_SUCCESS;
 			}
 			break;
 		default:
@@ -515,7 +529,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

[Alexander Aring]

Hi Martin,

On Wed, Oct 01, 2014 at 01:10:22PM +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 refactored so eliminate incorrect return values.

I notice this patch and I need some time to take review of this. Also
Jukka should ack these changes, so we don't break anything of 6lowpan bluetooth branch.

Thanks for your work Martin.


Marcel, please wait until this patch will get Acked-by you, Jukka, others who like to give
review notes and me.

It's a terrible issue and we need a longer audit for this one.

- Alex

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] 6lowpan: fix incorrect return values in lowpan_rcv

[Alexander Aring]

Martin,

On Wed, Oct 01, 2014 at 02:42:23PM +0200, Alexander Aring wrote:
> Hi Martin,
> 
> On Wed, Oct 01, 2014 at 01:10:22PM +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 refactored so eliminate incorrect return values.
> 
> I notice this patch and I need some time to take review of this. Also
> Jukka should ack these changes, so we don't break anything of 6lowpan bluetooth branch.
> 
> Thanks for your work Martin.
> 
> 
> Marcel, please wait until this patch will get Acked-by you, Jukka, others who like to give
> review notes and me.
> 
> It's a terrible issue and we need a longer audit for this one.
> 

I have some private workload at the moment, I need the weekend for
reviewing this one. I hope this is ok.

- Alex

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

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

[Alexander Aring]

Hi Martin,

On Wed, Oct 01, 2014 at 01:10:22PM +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.

I think we should split the movement of "passing skb to higher layer"
into a separate patch.

> It also makes the lowpan_rcv function more extendable as we
> can support more compression schemes.
> 
> With the above 2 lowpan_rcv is refactored so eliminate incorrect return values.
> 
> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
> ---
>  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(-)
> 
> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
> index d184df1..05ff67e 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);
> +int
> +lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> +		    const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
> +		    const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
> +		    u8 iphc0, u8 iphc1);
>  int lowpan_header_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..3888357 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,10 +301,11 @@ err:
>  /* TTL uncompression values */
>  static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
>  
> -int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> -			const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
> -			const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
> -			u8 iphc0, u8 iphc1, skb_delivery_cb deliver_skb)
> +int
> +lowpan_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;
> @@ -348,7 +318,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 -EINVAL;
>  	}
>  
>  	hdr.version = 6;
> @@ -360,7 +330,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 -EINVAL;
>  
>  		memcpy(&hdr.flow_lbl, &skb->data[0], 3);
>  		skb_pull(skb, 3);
> @@ -373,7 +343,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 -EINVAL;
>  
>  		hdr.priority = ((tmp >> 2) & 0x0f);
>  		hdr.flow_lbl[0] = ((tmp << 6) & 0xC0) | ((tmp >> 2) & 0x30);
> @@ -383,7 +353,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 -EINVAL;
>  
>  		hdr.flow_lbl[0] = (skb->data[0] & 0x0F) | ((tmp >> 2) & 0x30);
>  		memcpy(&hdr.flow_lbl[1], &skb->data[0], 2);
> @@ -400,7 +370,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 -EINVAL;
>  
>  		pr_debug("NH flag is set, next header carried inline: %02x\n",
>  			 hdr.nexthdr);
> @@ -412,7 +382,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 -EINVAL;
>  	}
>  
>  	/* Extract SAM to the tmp variable */
> @@ -431,7 +401,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>  
>  	/* Check on error of previous branch */
>  	if (err)
> -		goto drop;
> +		return -EINVAL;
>  
>  	/* Extract DAM to the tmp variable */
>  	tmp = ((iphc1 & LOWPAN_IPHC_DAM_11) >> LOWPAN_IPHC_DAM_BIT) & 0x03;
> @@ -446,7 +416,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>  								tmp);
>  
>  			if (err)
> -				goto drop;
> +				return -EINVAL;
>  		}
>  	} else {
>  		err = uncompress_addr(skb, &hdr.daddr, tmp, daddr,
> @@ -454,28 +424,26 @@ 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 -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 -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;
> +		if (skb_headroom(skb) < sizeof(struct udphdr) + sizeof(hdr)) {
> +			int n = sizeof(struct udphdr) + sizeof(hdr);

For this also a separate for "check if we have enough headroom". This
has nothing to do with the errno fix. All patches should follow "keep it
short and simple" not doing too much in a patch, if you can separate it
then separate it. Then we have a better overview while reviewing and a
nice git commit history.

Also this should be "if (skb_headroom(skb) < sizeof(struct udphdr))"
only, without the sizeof(hdr). Why you check also for space for ipv6
header here? This is part of transport header.

>  
> -		skb = new;
> +			err = pskb_expand_head(skb, n, 0, GFP_ATOMIC);
> +			if (unlikely(err))
> +				return err;
> +		}
>  
>  		skb_push(skb, sizeof(struct udphdr));

Here we add data for udphdr only. Then check only for headroom for
udphdr.

>  		skb_reset_transport_header(skb);
> @@ -485,6 +453,13 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>  			       (u8 *)&uh, sizeof(uh));
>  
>  		hdr.nexthdr = UIP_PROTO_UDP;
> +	} else {

Now I see why you make "sizeof(struct udphdr) + sizeof(hdr)" Better
remove this else branch and then simple make:

"if (skb_headroom(skb) < sizeof(hdr))", then we don't need to check this
above. This is a little bit confusing.

> +		if (skb_headroom(skb) < sizeof(hdr)) {
> +			err = pskb_expand_head(skb, sizeof(hdr), 0, GFP_ATOMIC);
> +

remove this whitespace.

> +			if (unlikely(err))
> +				return err;
> +		}
>  	}
>  
>  	hdr.payload_len = htons(skb->len);
> @@ -499,11 +474,14 @@ 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);
> +	skb_push(skb, sizeof(hdr));

Here is the push for sizeof(hdr) above we should _always_ check for
"(skb_headroom(skb) < sizeof(hdr))".


The code is much similar for udphdr and hdr, here.... create a static
function in this file, then we have a generic function for this (this is
useful for other transport header when next header compression layer
comes mainline.

protoype looks like:

int skb_check_and_expand(struct sk_buff *skb, size_t len)

should contain something like:

if (skb_headroom(skb) < len)
	do_expand_stuff(skb....);

return 0;


> +	skb_reset_network_header(skb);
> +	skb_copy_to_linear_data(skb, &hdr, sizeof(hdr));
>  
> -drop:
> -	kfree_skb(skb);
> -	return -EINVAL;
> +	raw_dump_table(__func__, "raw skb data dump before receiving",
> +		       skb->data, skb->len);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(lowpan_process_data);
>  
> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> index 206b65c..adfd361 100644
> --- a/net/bluetooth/6lowpan.c
> +++ b/net/bluetooth/6lowpan.c
> @@ -230,36 +230,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 -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 -EINVAL;
>  
>  	if (lowpan_fetch_skb_u8(skb, &iphc0))
> -		goto drop;
> +		return -EINVAL;
>  
>  	if (lowpan_fetch_skb_u8(skb, &iphc1))
> -		goto drop;
> +		return -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,12 +275,8 @@ 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;
> +			goto drop_local_skb;
>  		}
>  
>  		dev->stats.rx_bytes += skb->len;
> @@ -294,15 +285,40 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
>  		kfree_skb(local_skb);
>  		kfree_skb(skb);
>  	} else {
> +		int ret;
> +
> +		if (skb_cloned(skb)) {

why is this now here? Is this necessary, we don't have such thing
current mainline, if necessary make a separate patch for this or put it
in the right one.

> +			struct sk_buff *new;
> +			int new_headroom = sizeof(struct ipv6hdr) +
> +					   sizeof(struct udphdr);
> +
> +			new = skb_copy_expand(skb, new_headroom,
> +					      skb_tailroom(skb), GFP_ATOMIC);
> +			if (!new)
> +				return -ENOMEM;
> +			consume_skb(skb);
> +			skb = new;
> +		}
> +
>  		switch (skb->data[0] & 0xe0) {
>  		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
> +
> +			ret = process_data(skb, dev, chan);
> +			if (ret < 0)
> +				goto drop;
> +
>  			local_skb = skb_clone(skb, GFP_ATOMIC);
>  			if (!local_skb)
>  				goto drop;
>  

This looks a little bit confusing, I will look at this when you sperate
the patches. Again "move handling to upper layer" and the "check if
expand is necessary". Then I will look at this code for correct
"kfree_skb" and "consume_skb" calling. Hope this is okay, I need to
decrypt this code at first. :-)

Sorry too much changes in one patch, we need to split this one. Also
base it on bluetooth-next, should be complicated to megre this stuff for
bluetooth with -next...

- Alex

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

[Alexander Aring]

Martin,

On Sun, Oct 05, 2014 at 07:50:49PM +0200, Alexander Aring wrote:
...

> if (skb_headroom(skb) < len)
> 	do_expand_stuff(skb....);
> 

Maybe also put the skb_push stuff here, please look yourself what we do also
at udphdr and hdr here. It's should have some similar calling steps.

But please not the skb_reset_stuff here. This is different for network
header and transport header.

> return 0;
> 
> 

- Alex

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

[Alexander Aring]

On Sun, Oct 05, 2014 at 07:50:49PM +0200, Alexander Aring wrote:
> Hi Martin,
> 
> On Wed, Oct 01, 2014 at 01:10:22PM +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.
> 
> I think we should split the movement of "passing skb to higher layer"
> into a separate patch.
> 

and now I realized that we had this also in some other mail, because the
netif call drops the skb, if "passing skb to higher layer" failed...

Maybe try to return 0; then instead an error if failed at this point.
I know I said, just merge these patches... but this is really much changes
here. I only want some clean patches which only fix the error handling
at first without any other changes -> KISS.

Sorry Martin.

- Alex

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

[Martin Townsend]

Hi Alex
On 05/10/14 18:50, Alexander Aring wrote:
> Hi Martin,
>
> On Wed, Oct 01, 2014 at 01:10:22PM +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.
> I think we should split the movement of "passing skb to higher layer"
> into a separate patch.
Wasn't it separated in v1 patch series and you asked me to combine into 
one patch??
>
>> It also makes the lowpan_rcv function more extendable as we
>> can support more compression schemes.
>>
>> With the above 2 lowpan_rcv is refactored so eliminate incorrect return values.
>>
>> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
>> ---
>>   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(-)
>>
>> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
>> index d184df1..05ff67e 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);
>> +int
>> +lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>> +		    const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
>> +		    const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
>> +		    u8 iphc0, u8 iphc1);
>>   int lowpan_header_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..3888357 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,10 +301,11 @@ err:
>>   /* TTL uncompression values */
>>   static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
>>   
>> -int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>> -			const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
>> -			const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
>> -			u8 iphc0, u8 iphc1, skb_delivery_cb deliver_skb)
>> +int
>> +lowpan_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;
>> @@ -348,7 +318,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 -EINVAL;
>>   	}
>>   
>>   	hdr.version = 6;
>> @@ -360,7 +330,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 -EINVAL;
>>   
>>   		memcpy(&hdr.flow_lbl, &skb->data[0], 3);
>>   		skb_pull(skb, 3);
>> @@ -373,7 +343,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 -EINVAL;
>>   
>>   		hdr.priority = ((tmp >> 2) & 0x0f);
>>   		hdr.flow_lbl[0] = ((tmp << 6) & 0xC0) | ((tmp >> 2) & 0x30);
>> @@ -383,7 +353,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 -EINVAL;
>>   
>>   		hdr.flow_lbl[0] = (skb->data[0] & 0x0F) | ((tmp >> 2) & 0x30);
>>   		memcpy(&hdr.flow_lbl[1], &skb->data[0], 2);
>> @@ -400,7 +370,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 -EINVAL;
>>   
>>   		pr_debug("NH flag is set, next header carried inline: %02x\n",
>>   			 hdr.nexthdr);
>> @@ -412,7 +382,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 -EINVAL;
>>   	}
>>   
>>   	/* Extract SAM to the tmp variable */
>> @@ -431,7 +401,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>   
>>   	/* Check on error of previous branch */
>>   	if (err)
>> -		goto drop;
>> +		return -EINVAL;
>>   
>>   	/* Extract DAM to the tmp variable */
>>   	tmp = ((iphc1 & LOWPAN_IPHC_DAM_11) >> LOWPAN_IPHC_DAM_BIT) & 0x03;
>> @@ -446,7 +416,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>   								tmp);
>>   
>>   			if (err)
>> -				goto drop;
>> +				return -EINVAL;
>>   		}
>>   	} else {
>>   		err = uncompress_addr(skb, &hdr.daddr, tmp, daddr,
>> @@ -454,28 +424,26 @@ 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 -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 -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;
>> +		if (skb_headroom(skb) < sizeof(struct udphdr) + sizeof(hdr)) {
>> +			int n = sizeof(struct udphdr) + sizeof(hdr);
> For this also a separate for "check if we have enough headroom". This
> has nothing to do with the errno fix. All patches should follow "keep it
> short and simple" not doing too much in a patch, if you can separate it
> then separate it. Then we have a better overview while reviewing and a
> nice git commit history.
>
> Also this should be "if (skb_headroom(skb) < sizeof(struct udphdr))"
> only, without the sizeof(hdr). Why you check also for space for ipv6
> header here? This is part of transport header.
If you are decompressing UDP header then you also have to decompress the 
IP Header below it.  This would mean that 2 pskb_expand_head calls would 
be needed which could result in 2 data copies which seems a waste, why 
not just do it once?
>>   
>> -		skb = new;
>> +			err = pskb_expand_head(skb, n, 0, GFP_ATOMIC);
>> +			if (unlikely(err))
>> +				return err;
>> +		}
>>   
>>   		skb_push(skb, sizeof(struct udphdr));
> Here we add data for udphdr only. Then check only for headroom for
> udphdr.
>
>>   		skb_reset_transport_header(skb);
>> @@ -485,6 +453,13 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>   			       (u8 *)&uh, sizeof(uh));
>>   
>>   		hdr.nexthdr = UIP_PROTO_UDP;
>> +	} else {
> Now I see why you make "sizeof(struct udphdr) + sizeof(hdr)" Better
> remove this else branch and then simple make:
>
> "if (skb_headroom(skb) < sizeof(hdr))", then we don't need to check this
> above. This is a little bit confusing.
See above, maybe a comment would help things?
>
>> +		if (skb_headroom(skb) < sizeof(hdr)) {
>> +			err = pskb_expand_head(skb, sizeof(hdr), 0, GFP_ATOMIC);
>> +
> remove this whitespace.
ok.
>
>> +			if (unlikely(err))
>> +				return err;
>> +		}
>>   	}
>>   
>>   	hdr.payload_len = htons(skb->len);
>> @@ -499,11 +474,14 @@ 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);
>> +	skb_push(skb, sizeof(hdr));
> Here is the push for sizeof(hdr) above we should _always_ check for
> "(skb_headroom(skb) < sizeof(hdr))".
The intention of the code is to ensure that there is enough headroom 
before this push.  Maybe the following pseudocode helps :)

IF UDP Hdr Compression
     ensure enough room for UDP Hdr and IPv6 Hdr
     make room for UDP Hdr (skb_push)
     decompress UDP Hdr
ELSE
     ensure enough room for IPv6 Hdr

make room for IPv6 Hdr (skb_push)
decompress IPv6 Hdr


>
>
> The code is much similar for udphdr and hdr, here.... create a static
> function in this file, then we have a generic function for this (this is
> useful for other transport header when next header compression layer
> comes mainline.
>
> protoype looks like:
>
> int skb_check_and_expand(struct sk_buff *skb, size_t len)
>
> should contain something like:
>
> if (skb_headroom(skb) < len)
> 	do_expand_stuff(skb....);
>
> return 0;
>
>
>> +	skb_reset_network_header(skb);
>> +	skb_copy_to_linear_data(skb, &hdr, sizeof(hdr));
>>   
>> -drop:
>> -	kfree_skb(skb);
>> -	return -EINVAL;
>> +	raw_dump_table(__func__, "raw skb data dump before receiving",
>> +		       skb->data, skb->len);
>> +
>> +	return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(lowpan_process_data);
>>   
>> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
>> index 206b65c..adfd361 100644
>> --- a/net/bluetooth/6lowpan.c
>> +++ b/net/bluetooth/6lowpan.c
>> @@ -230,36 +230,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 -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 -EINVAL;
>>   
>>   	if (lowpan_fetch_skb_u8(skb, &iphc0))
>> -		goto drop;
>> +		return -EINVAL;
>>   
>>   	if (lowpan_fetch_skb_u8(skb, &iphc1))
>> -		goto drop;
>> +		return -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,12 +275,8 @@ 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;
>> +			goto drop_local_skb;
>>   		}
>>   
>>   		dev->stats.rx_bytes += skb->len;
>> @@ -294,15 +285,40 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
>>   		kfree_skb(local_skb);
>>   		kfree_skb(skb);
>>   	} else {
>> +		int ret;
>> +
>> +		if (skb_cloned(skb)) {
> why is this now here? Is this necessary, we don't have such thing
> current mainline, if necessary make a separate patch for this or put it
> in the right one.
It's necessary as pskb_expand_head must have no references as you are 
potentially reallocating the data part of the skb.   It's position is 
questionable but until other compression schemes are supported it will 
suffice.
>
>> +			struct sk_buff *new;
>> +			int new_headroom = sizeof(struct ipv6hdr) +
>> +					   sizeof(struct udphdr);
>> +
>> +			new = skb_copy_expand(skb, new_headroom,
>> +					      skb_tailroom(skb), GFP_ATOMIC);
>> +			if (!new)
>> +				return -ENOMEM;
>> +			consume_skb(skb);
>> +			skb = new;
>> +		}
>> +
>>   		switch (skb->data[0] & 0xe0) {
>>   		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
>> +
>> +			ret = process_data(skb, dev, chan);
>> +			if (ret < 0)
>> +				goto drop;
>> +
>>   			local_skb = skb_clone(skb, GFP_ATOMIC);
>>   			if (!local_skb)
>>   				goto drop;
>>   
> This looks a little bit confusing, I will look at this when you sperate
> the patches. Again "move handling to upper layer" and the "check if
> expand is necessary". Then I will look at this code for correct
> "kfree_skb" and "consume_skb" calling. Hope this is okay, I need to
> decrypt this code at first. :-)
sorry, I didn't intend to writing confusing code, wait until you see the 
GHC patch :)

>
> Sorry too much changes in one patch, we need to split this one. Also
> base it on bluetooth-next, should be complicated to megre this stuff for
> bluetooth with -next...
>
> - Alex
If I'm going to base it on bluetooth-next then I think the best solution 
is to submit separate patches in stages

1) Checking for headroom and using pskb_expand_head for decompression:
reason for patch: saves on copying the whole structure and there 
potentially might not be a copy especially if the driver allocates a 
larger buffer than needed knowing that it will be decompressed

2) Move delivery out of IPHC.
reason: Ensures return value isn't a mish mash of error codes and 
NET_RX_ codes, allows for better scalability as we don't have a 
requirement for decompression to pass the skb onto the next layer

3) Fix lowpan_rcv
reason: Fix those pesky return value problems


Then I can go on to submit those 2 outstanding patches
4) Refactor lowpan_rcv to handle fragmented uncompressed IPv6 packets.

5) Rename process_data etc..

I think this will help Jukka as my patch currently breaks their transmit 
code and we could see which part causes this. Also I'm really busy at 
the moment writing a QEMU virtual device to model our transceiver so we 
can get a virtual test bed working as well as a couple of other things 
so it would be better for me to concentrate on one patch at a time as 
time is something I don't have a lot of at the moment :)

Would you be happy with this? I know it means the bug fix happens third, 
but I think it will be better in the long run.

- Martin

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

[Alexander Aring]

Hi,

On Sun, Oct 05, 2014 at 10:00:59PM +0100, Martin Townsend wrote:
> Hi Alex
> On 05/10/14 18:50, Alexander Aring wrote:
> >Hi Martin,
> >
> >On Wed, Oct 01, 2014 at 01:10:22PM +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.
> >I think we should split the movement of "passing skb to higher layer"
> >into a separate patch.
> Wasn't it separated in v1 patch series and you asked me to combine into one
> patch??

yea, I know but it's hard to review and we need some "bisectable" git history.

> >
> >>It also makes the lowpan_rcv function more extendable as we
> >>can support more compression schemes.
> >>
> >>With the above 2 lowpan_rcv is refactored so eliminate incorrect return values.
> >>
> >>Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
> >>---
> >>  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(-)
> >>
> >>diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
> >>index d184df1..05ff67e 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);
> >>+int
> >>+lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> >>+		    const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
> >>+		    const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
> >>+		    u8 iphc0, u8 iphc1);
> >>  int lowpan_header_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..3888357 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,10 +301,11 @@ err:
> >>  /* TTL uncompression values */
> >>  static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
> >>-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> >>-			const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
> >>-			const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
> >>-			u8 iphc0, u8 iphc1, skb_delivery_cb deliver_skb)
> >>+int
> >>+lowpan_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;
> >>@@ -348,7 +318,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 -EINVAL;
> >>  	}
> >>  	hdr.version = 6;
> >>@@ -360,7 +330,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 -EINVAL;
> >>  		memcpy(&hdr.flow_lbl, &skb->data[0], 3);
> >>  		skb_pull(skb, 3);
> >>@@ -373,7 +343,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 -EINVAL;
> >>  		hdr.priority = ((tmp >> 2) & 0x0f);
> >>  		hdr.flow_lbl[0] = ((tmp << 6) & 0xC0) | ((tmp >> 2) & 0x30);
> >>@@ -383,7 +353,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 -EINVAL;
> >>  		hdr.flow_lbl[0] = (skb->data[0] & 0x0F) | ((tmp >> 2) & 0x30);
> >>  		memcpy(&hdr.flow_lbl[1], &skb->data[0], 2);
> >>@@ -400,7 +370,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 -EINVAL;
> >>  		pr_debug("NH flag is set, next header carried inline: %02x\n",
> >>  			 hdr.nexthdr);
> >>@@ -412,7 +382,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 -EINVAL;
> >>  	}
> >>  	/* Extract SAM to the tmp variable */
> >>@@ -431,7 +401,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> >>  	/* Check on error of previous branch */
> >>  	if (err)
> >>-		goto drop;
> >>+		return -EINVAL;
> >>  	/* Extract DAM to the tmp variable */
> >>  	tmp = ((iphc1 & LOWPAN_IPHC_DAM_11) >> LOWPAN_IPHC_DAM_BIT) & 0x03;
> >>@@ -446,7 +416,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> >>  								tmp);
> >>  			if (err)
> >>-				goto drop;
> >>+				return -EINVAL;
> >>  		}
> >>  	} else {
> >>  		err = uncompress_addr(skb, &hdr.daddr, tmp, daddr,
> >>@@ -454,28 +424,26 @@ 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 -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 -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;
> >>+		if (skb_headroom(skb) < sizeof(struct udphdr) + sizeof(hdr)) {
> >>+			int n = sizeof(struct udphdr) + sizeof(hdr);
> >For this also a separate for "check if we have enough headroom". This
> >has nothing to do with the errno fix. All patches should follow "keep it
> >short and simple" not doing too much in a patch, if you can separate it
> >then separate it. Then we have a better overview while reviewing and a
> >nice git commit history.
> >
> >Also this should be "if (skb_headroom(skb) < sizeof(struct udphdr))"
> >only, without the sizeof(hdr). Why you check also for space for ipv6
> >header here? This is part of transport header.
> If you are decompressing UDP header then you also have to decompress the IP
> Header below it.  This would mean that 2 pskb_expand_head calls would be
> needed which could result in 2 data copies which seems a waste, why not just
> do it once?
> >>-		skb = new;
> >>+			err = pskb_expand_head(skb, n, 0, GFP_ATOMIC);
> >>+			if (unlikely(err))
> >>+				return err;
> >>+		}
> >>  		skb_push(skb, sizeof(struct udphdr));
> >Here we add data for udphdr only. Then check only for headroom for
> >udphdr.
> >
> >>  		skb_reset_transport_header(skb);
> >>@@ -485,6 +453,13 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> >>  			       (u8 *)&uh, sizeof(uh));
> >>  		hdr.nexthdr = UIP_PROTO_UDP;
> >>+	} else {
> >Now I see why you make "sizeof(struct udphdr) + sizeof(hdr)" Better
> >remove this else branch and then simple make:
> >
> >"if (skb_headroom(skb) < sizeof(hdr))", then we don't need to check this
> >above. This is a little bit confusing.
> See above, maybe a comment would help things?
> >
> >>+		if (skb_headroom(skb) < sizeof(hdr)) {
> >>+			err = pskb_expand_head(skb, sizeof(hdr), 0, GFP_ATOMIC);
> >>+
> >remove this whitespace.
> ok.
> >
> >>+			if (unlikely(err))
> >>+				return err;
> >>+		}
> >>  	}
> >>  	hdr.payload_len = htons(skb->len);
> >>@@ -499,11 +474,14 @@ 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);
> >>+	skb_push(skb, sizeof(hdr));
> >Here is the push for sizeof(hdr) above we should _always_ check for
> >"(skb_headroom(skb) < sizeof(hdr))".
> The intention of the code is to ensure that there is enough headroom before
> this push.  Maybe the following pseudocode helps :)
> 
> IF UDP Hdr Compression
>     ensure enough room for UDP Hdr and IPv6 Hdr
>     make room for UDP Hdr (skb_push)
>     decompress UDP Hdr
> ELSE
>     ensure enough room for IPv6 Hdr
> 
> make room for IPv6 Hdr (skb_push)
> decompress IPv6 Hdr
> 

Yes I know what you doing here, this saves one us one allocation.

I thought about to combine always the length of skb_push with
pskb_expand_head head so it's easier "why" we do this here. But with
your solution we save a allocation.

I will try to add a "length" field for necessary space in next header
compression framework and do this "check and expand" transparent inside
of next header compression framework. I think this should be fine.

Then please leave this code as it is. I am always fine to remove
allocations in hotpaths.
> 
> >
> >
> >The code is much similar for udphdr and hdr, here.... create a static
> >function in this file, then we have a generic function for this (this is
> >useful for other transport header when next header compression layer
> >comes mainline.
> >
> >protoype looks like:
> >
> >int skb_check_and_expand(struct sk_buff *skb, size_t len)
> >
> >should contain something like:
> >
> >if (skb_headroom(skb) < len)
> >	do_expand_stuff(skb....);
> >
> >return 0;
> >
> >
> >>+	skb_reset_network_header(skb);
> >>+	skb_copy_to_linear_data(skb, &hdr, sizeof(hdr));
> >>-drop:
> >>-	kfree_skb(skb);
> >>-	return -EINVAL;
> >>+	raw_dump_table(__func__, "raw skb data dump before receiving",
> >>+		       skb->data, skb->len);
> >>+
> >>+	return 0;
> >>  }
> >>  EXPORT_SYMBOL_GPL(lowpan_process_data);
> >>diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> >>index 206b65c..adfd361 100644
> >>--- a/net/bluetooth/6lowpan.c
> >>+++ b/net/bluetooth/6lowpan.c
> >>@@ -230,36 +230,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 -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 -EINVAL;
> >>  	if (lowpan_fetch_skb_u8(skb, &iphc0))
> >>-		goto drop;
> >>+		return -EINVAL;
> >>  	if (lowpan_fetch_skb_u8(skb, &iphc1))
> >>-		goto drop;
> >>+		return -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,12 +275,8 @@ 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;
> >>+			goto drop_local_skb;
> >>  		}
> >>  		dev->stats.rx_bytes += skb->len;
> >>@@ -294,15 +285,40 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
> >>  		kfree_skb(local_skb);
> >>  		kfree_skb(skb);
> >>  	} else {
> >>+		int ret;
> >>+
> >>+		if (skb_cloned(skb)) {
> >why is this now here? Is this necessary, we don't have such thing
> >current mainline, if necessary make a separate patch for this or put it
> >in the right one.
> It's necessary as pskb_expand_head must have no references as you are
> potentially reallocating the data part of the skb.   It's position is
> questionable but until other compression schemes are supported it will
> suffice.
> >
> >>+			struct sk_buff *new;
> >>+			int new_headroom = sizeof(struct ipv6hdr) +
> >>+					   sizeof(struct udphdr);
> >>+
> >>+			new = skb_copy_expand(skb, new_headroom,
> >>+					      skb_tailroom(skb), GFP_ATOMIC);
> >>+			if (!new)
> >>+				return -ENOMEM;
> >>+			consume_skb(skb);
> >>+			skb = new;
> >>+		}
> >>+
> >>  		switch (skb->data[0] & 0xe0) {
> >>  		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
> >>+
> >>+			ret = process_data(skb, dev, chan);
> >>+			if (ret < 0)
> >>+				goto drop;
> >>+
> >>  			local_skb = skb_clone(skb, GFP_ATOMIC);
> >>  			if (!local_skb)
> >>  				goto drop;
> >This looks a little bit confusing, I will look at this when you sperate
> >the patches. Again "move handling to upper layer" and the "check if
> >expand is necessary". Then I will look at this code for correct
> >"kfree_skb" and "consume_skb" calling. Hope this is okay, I need to
> >decrypt this code at first. :-)
> sorry, I didn't intend to writing confusing code, wait until you see the GHC
> patch :)
> 

nono, please it's only confusing me because there are too much changes.
You didn't write confusing code.

> >
> >Sorry too much changes in one patch, we need to split this one. Also
> >base it on bluetooth-next, should be complicated to megre this stuff for
> >bluetooth with -next...
> >
> >- Alex
> If I'm going to base it on bluetooth-next then I think the best solution is
> to submit separate patches in stages
> 
> 1) Checking for headroom and using pskb_expand_head for decompression:
> reason for patch: saves on copying the whole structure and there potentially
> might not be a copy especially if the driver allocates a larger buffer than
> needed knowing that it will be decompressed
> 
> 2) Move delivery out of IPHC.
> reason: Ensures return value isn't a mish mash of error codes and NET_RX_
> codes, allows for better scalability as we don't have a requirement for
> decompression to pass the skb onto the next layer
> 
> 3) Fix lowpan_rcv
> reason: Fix those pesky return value problems
> 

ack. You can do that.

> 
> Then I can go on to submit those 2 outstanding patches
> 4) Refactor lowpan_rcv to handle fragmented uncompressed IPv6 packets.
> 

I'm very excited about that.

> 5) Rename process_data etc..
> 

ok.

> I think this will help Jukka as my patch currently breaks their transmit
> code and we could see which part causes this. Also I'm really busy at the
> moment writing a QEMU virtual device to model our transceiver so we can get
> a virtual test bed working as well as a couple of other things so it would
> be better for me to concentrate on one patch at a time as time is something
> I don't have a lot of at the moment :)
> 

So it takes some time now, or what do you mean here?

> Would you be happy with this? I know it means the bug fix happens third, but
> I think it will be better in the long run.
> 

I am happy with this and indeed we should fix this issue fast.

- Alex

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

[Martin Townsend]

Hi Marcel,

Are you ok with me breaking this down into separate patches and submitting them individually to bluetooth-next?

- Martin.

On 06/10/14 08:12, Alexander Aring wrote:
> Hi,
>
> On Sun, Oct 05, 2014 at 10:00:59PM +0100, Martin Townsend wrote:
>> Hi Alex
>> On 05/10/14 18:50, Alexander Aring wrote:
>>> Hi Martin,
>>>
>>> On Wed, Oct 01, 2014 at 01:10:22PM +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.
>>> I think we should split the movement of "passing skb to higher layer"
>>> into a separate patch.
>> Wasn't it separated in v1 patch series and you asked me to combine into one
>> patch??
> yea, I know but it's hard to review and we need some "bisectable" git history.
>
>>>> It also makes the lowpan_rcv function more extendable as we
>>>> can support more compression schemes.
>>>>
>>>> With the above 2 lowpan_rcv is refactored so eliminate incorrect return values.
>>>>
>>>> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
>>>> ---
>>>>  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(-)
>>>>
>>>> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
>>>> index d184df1..05ff67e 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);
>>>> +int
>>>> +lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>>> +		    const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
>>>> +		    const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
>>>> +		    u8 iphc0, u8 iphc1);
>>>>  int lowpan_header_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..3888357 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,10 +301,11 @@ err:
>>>>  /* TTL uncompression values */
>>>>  static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
>>>> -int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>>> -			const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
>>>> -			const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
>>>> -			u8 iphc0, u8 iphc1, skb_delivery_cb deliver_skb)
>>>> +int
>>>> +lowpan_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;
>>>> @@ -348,7 +318,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 -EINVAL;
>>>>  	}
>>>>  	hdr.version = 6;
>>>> @@ -360,7 +330,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 -EINVAL;
>>>>  		memcpy(&hdr.flow_lbl, &skb->data[0], 3);
>>>>  		skb_pull(skb, 3);
>>>> @@ -373,7 +343,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 -EINVAL;
>>>>  		hdr.priority = ((tmp >> 2) & 0x0f);
>>>>  		hdr.flow_lbl[0] = ((tmp << 6) & 0xC0) | ((tmp >> 2) & 0x30);
>>>> @@ -383,7 +353,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 -EINVAL;
>>>>  		hdr.flow_lbl[0] = (skb->data[0] & 0x0F) | ((tmp >> 2) & 0x30);
>>>>  		memcpy(&hdr.flow_lbl[1], &skb->data[0], 2);
>>>> @@ -400,7 +370,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 -EINVAL;
>>>>  		pr_debug("NH flag is set, next header carried inline: %02x\n",
>>>>  			 hdr.nexthdr);
>>>> @@ -412,7 +382,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 -EINVAL;
>>>>  	}
>>>>  	/* Extract SAM to the tmp variable */
>>>> @@ -431,7 +401,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>>>  	/* Check on error of previous branch */
>>>>  	if (err)
>>>> -		goto drop;
>>>> +		return -EINVAL;
>>>>  	/* Extract DAM to the tmp variable */
>>>>  	tmp = ((iphc1 & LOWPAN_IPHC_DAM_11) >> LOWPAN_IPHC_DAM_BIT) & 0x03;
>>>> @@ -446,7 +416,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>>>  								tmp);
>>>>  			if (err)
>>>> -				goto drop;
>>>> +				return -EINVAL;
>>>>  		}
>>>>  	} else {
>>>>  		err = uncompress_addr(skb, &hdr.daddr, tmp, daddr,
>>>> @@ -454,28 +424,26 @@ 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 -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 -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;
>>>> +		if (skb_headroom(skb) < sizeof(struct udphdr) + sizeof(hdr)) {
>>>> +			int n = sizeof(struct udphdr) + sizeof(hdr);
>>> For this also a separate for "check if we have enough headroom". This
>>> has nothing to do with the errno fix. All patches should follow "keep it
>>> short and simple" not doing too much in a patch, if you can separate it
>>> then separate it. Then we have a better overview while reviewing and a
>>> nice git commit history.
>>>
>>> Also this should be "if (skb_headroom(skb) < sizeof(struct udphdr))"
>>> only, without the sizeof(hdr). Why you check also for space for ipv6
>>> header here? This is part of transport header.
>> If you are decompressing UDP header then you also have to decompress the IP
>> Header below it.  This would mean that 2 pskb_expand_head calls would be
>> needed which could result in 2 data copies which seems a waste, why not just
>> do it once?
>>>> -		skb = new;
>>>> +			err = pskb_expand_head(skb, n, 0, GFP_ATOMIC);
>>>> +			if (unlikely(err))
>>>> +				return err;
>>>> +		}
>>>>  		skb_push(skb, sizeof(struct udphdr));
>>> Here we add data for udphdr only. Then check only for headroom for
>>> udphdr.
>>>
>>>>  		skb_reset_transport_header(skb);
>>>> @@ -485,6 +453,13 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>>>  			       (u8 *)&uh, sizeof(uh));
>>>>  		hdr.nexthdr = UIP_PROTO_UDP;
>>>> +	} else {
>>> Now I see why you make "sizeof(struct udphdr) + sizeof(hdr)" Better
>>> remove this else branch and then simple make:
>>>
>>> "if (skb_headroom(skb) < sizeof(hdr))", then we don't need to check this
>>> above. This is a little bit confusing.
>> See above, maybe a comment would help things?
>>>> +		if (skb_headroom(skb) < sizeof(hdr)) {
>>>> +			err = pskb_expand_head(skb, sizeof(hdr), 0, GFP_ATOMIC);
>>>> +
>>> remove this whitespace.
>> ok.
>>>> +			if (unlikely(err))
>>>> +				return err;
>>>> +		}
>>>>  	}
>>>>  	hdr.payload_len = htons(skb->len);
>>>> @@ -499,11 +474,14 @@ 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);
>>>> +	skb_push(skb, sizeof(hdr));
>>> Here is the push for sizeof(hdr) above we should _always_ check for
>>> "(skb_headroom(skb) < sizeof(hdr))".
>> The intention of the code is to ensure that there is enough headroom before
>> this push.  Maybe the following pseudocode helps :)
>>
>> IF UDP Hdr Compression
>>     ensure enough room for UDP Hdr and IPv6 Hdr
>>     make room for UDP Hdr (skb_push)
>>     decompress UDP Hdr
>> ELSE
>>     ensure enough room for IPv6 Hdr
>>
>> make room for IPv6 Hdr (skb_push)
>> decompress IPv6 Hdr
>>
> Yes I know what you doing here, this saves one us one allocation.
>
> I thought about to combine always the length of skb_push with
> pskb_expand_head head so it's easier "why" we do this here. But with
> your solution we save a allocation.
>
> I will try to add a "length" field for necessary space in next header
> compression framework and do this "check and expand" transparent inside
> of next header compression framework. I think this should be fine.
>
> Then please leave this code as it is. I am always fine to remove
> allocations in hotpaths.
>>>
>>> The code is much similar for udphdr and hdr, here.... create a static
>>> function in this file, then we have a generic function for this (this is
>>> useful for other transport header when next header compression layer
>>> comes mainline.
>>>
>>> protoype looks like:
>>>
>>> int skb_check_and_expand(struct sk_buff *skb, size_t len)
>>>
>>> should contain something like:
>>>
>>> if (skb_headroom(skb) < len)
>>> 	do_expand_stuff(skb....);
>>>
>>> return 0;
>>>
>>>
>>>> +	skb_reset_network_header(skb);
>>>> +	skb_copy_to_linear_data(skb, &hdr, sizeof(hdr));
>>>> -drop:
>>>> -	kfree_skb(skb);
>>>> -	return -EINVAL;
>>>> +	raw_dump_table(__func__, "raw skb data dump before receiving",
>>>> +		       skb->data, skb->len);
>>>> +
>>>> +	return 0;
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(lowpan_process_data);
>>>> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
>>>> index 206b65c..adfd361 100644
>>>> --- a/net/bluetooth/6lowpan.c
>>>> +++ b/net/bluetooth/6lowpan.c
>>>> @@ -230,36 +230,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 -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 -EINVAL;
>>>>  	if (lowpan_fetch_skb_u8(skb, &iphc0))
>>>> -		goto drop;
>>>> +		return -EINVAL;
>>>>  	if (lowpan_fetch_skb_u8(skb, &iphc1))
>>>> -		goto drop;
>>>> +		return -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,12 +275,8 @@ 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;
>>>> +			goto drop_local_skb;
>>>>  		}
>>>>  		dev->stats.rx_bytes += skb->len;
>>>> @@ -294,15 +285,40 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
>>>>  		kfree_skb(local_skb);
>>>>  		kfree_skb(skb);
>>>>  	} else {
>>>> +		int ret;
>>>> +
>>>> +		if (skb_cloned(skb)) {
>>> why is this now here? Is this necessary, we don't have such thing
>>> current mainline, if necessary make a separate patch for this or put it
>>> in the right one.
>> It's necessary as pskb_expand_head must have no references as you are
>> potentially reallocating the data part of the skb.   It's position is
>> questionable but until other compression schemes are supported it will
>> suffice.
>>>> +			struct sk_buff *new;
>>>> +			int new_headroom = sizeof(struct ipv6hdr) +
>>>> +					   sizeof(struct udphdr);
>>>> +
>>>> +			new = skb_copy_expand(skb, new_headroom,
>>>> +					      skb_tailroom(skb), GFP_ATOMIC);
>>>> +			if (!new)
>>>> +				return -ENOMEM;
>>>> +			consume_skb(skb);
>>>> +			skb = new;
>>>> +		}
>>>> +
>>>>  		switch (skb->data[0] & 0xe0) {
>>>>  		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
>>>> +
>>>> +			ret = process_data(skb, dev, chan);
>>>> +			if (ret < 0)
>>>> +				goto drop;
>>>> +
>>>>  			local_skb = skb_clone(skb, GFP_ATOMIC);
>>>>  			if (!local_skb)
>>>>  				goto drop;
>>> This looks a little bit confusing, I will look at this when you sperate
>>> the patches. Again "move handling to upper layer" and the "check if
>>> expand is necessary". Then I will look at this code for correct
>>> "kfree_skb" and "consume_skb" calling. Hope this is okay, I need to
>>> decrypt this code at first. :-)
>> sorry, I didn't intend to writing confusing code, wait until you see the GHC
>> patch :)
>>
> nono, please it's only confusing me because there are too much changes.
> You didn't write confusing code.
>
>>> Sorry too much changes in one patch, we need to split this one. Also
>>> base it on bluetooth-next, should be complicated to megre this stuff for
>>> bluetooth with -next...
>>>
>>> - Alex
>> If I'm going to base it on bluetooth-next then I think the best solution is
>> to submit separate patches in stages
>>
>> 1) Checking for headroom and using pskb_expand_head for decompression:
>> reason for patch: saves on copying the whole structure and there potentially
>> might not be a copy especially if the driver allocates a larger buffer than
>> needed knowing that it will be decompressed
>>
>> 2) Move delivery out of IPHC.
>> reason: Ensures return value isn't a mish mash of error codes and NET_RX_
>> codes, allows for better scalability as we don't have a requirement for
>> decompression to pass the skb onto the next layer
>>
>> 3) Fix lowpan_rcv
>> reason: Fix those pesky return value problems
>>
> ack. You can do that.
>
>> Then I can go on to submit those 2 outstanding patches
>> 4) Refactor lowpan_rcv to handle fragmented uncompressed IPv6 packets.
>>
> I'm very excited about that.
>
>> 5) Rename process_data etc..
>>
> ok.
>
>> I think this will help Jukka as my patch currently breaks their transmit
>> code and we could see which part causes this. Also I'm really busy at the
>> moment writing a QEMU virtual device to model our transceiver so we can get
>> a virtual test bed working as well as a couple of other things so it would
>> be better for me to concentrate on one patch at a time as time is something
>> I don't have a lot of at the moment :)
>>
> So it takes some time now, or what do you mean here?
>
>> Would you be happy with this? I know it means the bug fix happens third, but
>> I think it will be better in the long run.
>>
> I am happy with this and indeed we should fix this issue fast.
>
> - Alex

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

[Martin Townsend]

Hi Alex,

On 06/10/14 08:12, Alexander Aring wrote:
> Hi,
>
> On Sun, Oct 05, 2014 at 10:00:59PM +0100, Martin Townsend wrote:
>> Hi Alex
>> On 05/10/14 18:50, Alexander Aring wrote:
>>> Hi Martin,
>>>
>>> On Wed, Oct 01, 2014 at 01:10:22PM +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.
>>> I think we should split the movement of "passing skb to higher layer"
>>> into a separate patch.
>> Wasn't it separated in v1 patch series and you asked me to combine into one
>> patch??
> yea, I know but it's hard to review and we need some "bisectable" git history.
>
>>>> It also makes the lowpan_rcv function more extendable as we
>>>> can support more compression schemes.
>>>>
>>>> With the above 2 lowpan_rcv is refactored so eliminate incorrect return values.
>>>>
>>>> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
>>>> ---
>>>>  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(-)
>>>>
>>>> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
>>>> index d184df1..05ff67e 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);
>>>> +int
>>>> +lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>>> +		    const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
>>>> +		    const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
>>>> +		    u8 iphc0, u8 iphc1);
>>>>  int lowpan_header_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..3888357 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,10 +301,11 @@ err:
>>>>  /* TTL uncompression values */
>>>>  static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
>>>> -int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>>> -			const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
>>>> -			const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
>>>> -			u8 iphc0, u8 iphc1, skb_delivery_cb deliver_skb)
>>>> +int
>>>> +lowpan_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;
>>>> @@ -348,7 +318,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 -EINVAL;
>>>>  	}
>>>>  	hdr.version = 6;
>>>> @@ -360,7 +330,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 -EINVAL;
>>>>  		memcpy(&hdr.flow_lbl, &skb->data[0], 3);
>>>>  		skb_pull(skb, 3);
>>>> @@ -373,7 +343,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 -EINVAL;
>>>>  		hdr.priority = ((tmp >> 2) & 0x0f);
>>>>  		hdr.flow_lbl[0] = ((tmp << 6) & 0xC0) | ((tmp >> 2) & 0x30);
>>>> @@ -383,7 +353,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 -EINVAL;
>>>>  		hdr.flow_lbl[0] = (skb->data[0] & 0x0F) | ((tmp >> 2) & 0x30);
>>>>  		memcpy(&hdr.flow_lbl[1], &skb->data[0], 2);
>>>> @@ -400,7 +370,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 -EINVAL;
>>>>  		pr_debug("NH flag is set, next header carried inline: %02x\n",
>>>>  			 hdr.nexthdr);
>>>> @@ -412,7 +382,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 -EINVAL;
>>>>  	}
>>>>  	/* Extract SAM to the tmp variable */
>>>> @@ -431,7 +401,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>>>  	/* Check on error of previous branch */
>>>>  	if (err)
>>>> -		goto drop;
>>>> +		return -EINVAL;
>>>>  	/* Extract DAM to the tmp variable */
>>>>  	tmp = ((iphc1 & LOWPAN_IPHC_DAM_11) >> LOWPAN_IPHC_DAM_BIT) & 0x03;
>>>> @@ -446,7 +416,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>>>  								tmp);
>>>>  			if (err)
>>>> -				goto drop;
>>>> +				return -EINVAL;
>>>>  		}
>>>>  	} else {
>>>>  		err = uncompress_addr(skb, &hdr.daddr, tmp, daddr,
>>>> @@ -454,28 +424,26 @@ 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 -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 -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;
>>>> +		if (skb_headroom(skb) < sizeof(struct udphdr) + sizeof(hdr)) {
>>>> +			int n = sizeof(struct udphdr) + sizeof(hdr);
>>> For this also a separate for "check if we have enough headroom". This
>>> has nothing to do with the errno fix. All patches should follow "keep it
>>> short and simple" not doing too much in a patch, if you can separate it
>>> then separate it. Then we have a better overview while reviewing and a
>>> nice git commit history.
>>>
>>> Also this should be "if (skb_headroom(skb) < sizeof(struct udphdr))"
>>> only, without the sizeof(hdr). Why you check also for space for ipv6
>>> header here? This is part of transport header.
>> If you are decompressing UDP header then you also have to decompress the IP
>> Header below it.  This would mean that 2 pskb_expand_head calls would be
>> needed which could result in 2 data copies which seems a waste, why not just
>> do it once?
>>>> -		skb = new;
>>>> +			err = pskb_expand_head(skb, n, 0, GFP_ATOMIC);
>>>> +			if (unlikely(err))
>>>> +				return err;
>>>> +		}
>>>>  		skb_push(skb, sizeof(struct udphdr));
>>> Here we add data for udphdr only. Then check only for headroom for
>>> udphdr.
>>>
>>>>  		skb_reset_transport_header(skb);
>>>> @@ -485,6 +453,13 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>>>>  			       (u8 *)&uh, sizeof(uh));
>>>>  		hdr.nexthdr = UIP_PROTO_UDP;
>>>> +	} else {
>>> Now I see why you make "sizeof(struct udphdr) + sizeof(hdr)" Better
>>> remove this else branch and then simple make:
>>>
>>> "if (skb_headroom(skb) < sizeof(hdr))", then we don't need to check this
>>> above. This is a little bit confusing.
>> See above, maybe a comment would help things?
>>>> +		if (skb_headroom(skb) < sizeof(hdr)) {
>>>> +			err = pskb_expand_head(skb, sizeof(hdr), 0, GFP_ATOMIC);
>>>> +
>>> remove this whitespace.
>> ok.
>>>> +			if (unlikely(err))
>>>> +				return err;
>>>> +		}
>>>>  	}
>>>>  	hdr.payload_len = htons(skb->len);
>>>> @@ -499,11 +474,14 @@ 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);
>>>> +	skb_push(skb, sizeof(hdr));
>>> Here is the push for sizeof(hdr) above we should _always_ check for
>>> "(skb_headroom(skb) < sizeof(hdr))".
>> The intention of the code is to ensure that there is enough headroom before
>> this push.  Maybe the following pseudocode helps :)
>>
>> IF UDP Hdr Compression
>>     ensure enough room for UDP Hdr and IPv6 Hdr
>>     make room for UDP Hdr (skb_push)
>>     decompress UDP Hdr
>> ELSE
>>     ensure enough room for IPv6 Hdr
>>
>> make room for IPv6 Hdr (skb_push)
>> decompress IPv6 Hdr
>>
> Yes I know what you doing here, this saves one us one allocation.
>
> I thought about to combine always the length of skb_push with
> pskb_expand_head head so it's easier "why" we do this here. But with
> your solution we save a allocation.
>
> I will try to add a "length" field for necessary space in next header
> compression framework and do this "check and expand" transparent inside
> of next header compression framework. I think this should be fine.
>
> Then please leave this code as it is. I am always fine to remove
> allocations in hotpaths.
>>>
>>> The code is much similar for udphdr and hdr, here.... create a static
>>> function in this file, then we have a generic function for this (this is
>>> useful for other transport header when next header compression layer
>>> comes mainline.
>>>
>>> protoype looks like:
>>>
>>> int skb_check_and_expand(struct sk_buff *skb, size_t len)
>>>
>>> should contain something like:
>>>
>>> if (skb_headroom(skb) < len)
>>> 	do_expand_stuff(skb....);
>>>
>>> return 0;
>>>
>>>
>>>> +	skb_reset_network_header(skb);
>>>> +	skb_copy_to_linear_data(skb, &hdr, sizeof(hdr));
>>>> -drop:
>>>> -	kfree_skb(skb);
>>>> -	return -EINVAL;
>>>> +	raw_dump_table(__func__, "raw skb data dump before receiving",
>>>> +		       skb->data, skb->len);
>>>> +
>>>> +	return 0;
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(lowpan_process_data);
>>>> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
>>>> index 206b65c..adfd361 100644
>>>> --- a/net/bluetooth/6lowpan.c
>>>> +++ b/net/bluetooth/6lowpan.c
>>>> @@ -230,36 +230,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 -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 -EINVAL;
>>>>  	if (lowpan_fetch_skb_u8(skb, &iphc0))
>>>> -		goto drop;
>>>> +		return -EINVAL;
>>>>  	if (lowpan_fetch_skb_u8(skb, &iphc1))
>>>> -		goto drop;
>>>> +		return -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,12 +275,8 @@ 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;
>>>> +			goto drop_local_skb;
>>>>  		}
>>>>  		dev->stats.rx_bytes += skb->len;
>>>> @@ -294,15 +285,40 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
>>>>  		kfree_skb(local_skb);
>>>>  		kfree_skb(skb);
>>>>  	} else {
>>>> +		int ret;
>>>> +
>>>> +		if (skb_cloned(skb)) {
>>> why is this now here? Is this necessary, we don't have such thing
>>> current mainline, if necessary make a separate patch for this or put it
>>> in the right one.
>> It's necessary as pskb_expand_head must have no references as you are
>> potentially reallocating the data part of the skb.   It's position is
>> questionable but until other compression schemes are supported it will
>> suffice.
>>>> +			struct sk_buff *new;
>>>> +			int new_headroom = sizeof(struct ipv6hdr) +
>>>> +					   sizeof(struct udphdr);
>>>> +
>>>> +			new = skb_copy_expand(skb, new_headroom,
>>>> +					      skb_tailroom(skb), GFP_ATOMIC);
>>>> +			if (!new)
>>>> +				return -ENOMEM;
>>>> +			consume_skb(skb);
>>>> +			skb = new;
>>>> +		}
>>>> +
>>>>  		switch (skb->data[0] & 0xe0) {
>>>>  		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
>>>> +
>>>> +			ret = process_data(skb, dev, chan);
>>>> +			if (ret < 0)
>>>> +				goto drop;
>>>> +
>>>>  			local_skb = skb_clone(skb, GFP_ATOMIC);
>>>>  			if (!local_skb)
>>>>  				goto drop;
>>> This looks a little bit confusing, I will look at this when you sperate
>>> the patches. Again "move handling to upper layer" and the "check if
>>> expand is necessary". Then I will look at this code for correct
>>> "kfree_skb" and "consume_skb" calling. Hope this is okay, I need to
>>> decrypt this code at first. :-)
>> sorry, I didn't intend to writing confusing code, wait until you see the GHC
>> patch :)
>>
> nono, please it's only confusing me because there are too much changes.
> You didn't write confusing code.
>
>>> Sorry too much changes in one patch, we need to split this one. Also
>>> base it on bluetooth-next, should be complicated to megre this stuff for
>>> bluetooth with -next...
>>>
>>> - Alex
>> If I'm going to base it on bluetooth-next then I think the best solution is
>> to submit separate patches in stages
>>
>> 1) Checking for headroom and using pskb_expand_head for decompression:
>> reason for patch: saves on copying the whole structure and there potentially
>> might not be a copy especially if the driver allocates a larger buffer than
>> needed knowing that it will be decompressed
>>
>> 2) Move delivery out of IPHC.
>> reason: Ensures return value isn't a mish mash of error codes and NET_RX_
>> codes, allows for better scalability as we don't have a requirement for
>> decompression to pass the skb onto the next layer
>>
>> 3) Fix lowpan_rcv
>> reason: Fix those pesky return value problems
>>
> ack. You can do that.
>
>> Then I can go on to submit those 2 outstanding patches
>> 4) Refactor lowpan_rcv to handle fragmented uncompressed IPv6 packets.
>>
> I'm very excited about that.
>
>> 5) Rename process_data etc..
>>
> ok.
>
>> I think this will help Jukka as my patch currently breaks their transmit
>> code and we could see which part causes this. Also I'm really busy at the
>> moment writing a QEMU virtual device to model our transceiver so we can get
>> a virtual test bed working as well as a couple of other things so it would
>> be better for me to concentrate on one patch at a time as time is something
>> I don't have a lot of at the moment :)
>>
> So it takes some time now, or what do you mean here?
I mean I'm very busy over the next few weeks so submitting them one patch at a time will make my life a lot easier.  I'll wait for Marcel to acknowledge that he's ok with this and I'll start preparing the first patch.
>
>> Would you be happy with this? I know it means the bug fix happens third, but
>> I think it will be better in the long run.
>>
> I am happy with this and indeed we should fix this issue fast.
>
> - 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

[Marcel Holtmann]

Hi Martin,

> Are you ok with me breaking this down into separate patches and submitting them individually to bluetooth-next?

yes, just break it down into smaller logical patches.

Regards

Marcel

[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