bluetooth: 6lowpan: suggestions for Jukka

When -EAGAIN as return value for receive handling will do a retry of parsing I can trigger a endless loop when iphc decompression e.g. returns an errno because some missing function "-ENOTSUPP" or something else. Somebody from outside can trigger an endless loop when sending a an IPHC header which triggers this behaviour.

NOTE: This really depends only if -EAGAIN means "try again to call the receive handler with the skb". Sometimes we also drop (and kfree) the skb, I think something is broken there... depends on the error branch. When receiving failed simple free skb and return errno (which is not -EAGAIN).

Hi Alex,

Quoted 6 lines

When -EAGAIN as return value for receive handling will do a retry of parsing I can trigger a endless loop when iphc decompression e.g. returns an errno because some missing function "-ENOTSUPP" or something else. Somebody from outside can trigger an endless loop when sending a an IPHC header which triggers this behaviour.

Quoted 5 lines

NOTE: This really depends only if -EAGAIN means "try again to call the receive handler with the skb". Sometimes we also drop (and kfree) the skb, I think something is broken there... depends on the error branch. When receiving failed simple free skb and return errno (which is not -EAGAIN).

I am lost on this comment, you need to explain this more and might actually want to put a comment in the code on this.

Hi Marcel,

Quoted 8 lines · Marcel Holtmann

Hi Alex,

Quoted 6 lines

When -EAGAIN as return value for receive handling will do a retry of parsing I can trigger a endless loop when iphc decompression e.g. returns an errno because some missing function "-ENOTSUPP" or something else. Somebody from outside can trigger an endless loop when sending a an IPHC header which triggers this behaviour.

Quoted 8 lines

Quoted 5 lines

NOTE: This really depends only if -EAGAIN means "try again to call the receive handler with the skb". Sometimes we also drop (and kfree) the skb, I think something is broken there... depends on the error branch. When receiving failed simple free skb and return errno (which is not -EAGAIN).

I am lost on this comment, you need to explain this more and might actually want to put a comment in the code on this.

I am sorry, what I meant there is "How exactly does the '.recv' of 'l2cap_ops'" handle an -EAGAIN errno. If this means the callback will be called again with the same parameters (What I am thinking of, when I return -EAGAIN) then we have an endless loop which can be triggered from the outside by sending an invalid/not supported IPHC 6LoWPAN header. Because the function "chan_recv_cb" (which is ".recv") will call recv_pkt (which return -EAGAIN on failure) and this function will call ret = iphc_decompress(local_skb, dev, chan), which parse the IPHC Header (can occur errno on invalid frames), then the complete calling chain will return -EAGAIN at ".recv" callback of l2cap_ops". The IPHC header comming from the "outside", everybody can manipulate this data.

Is it more clear now what I try to explained here?

First look on ".recv" callback handling in "net/bluetooth/l2cap_core.c" a return of -EAGAIN, will not try to call the ".recv" callback with the same parameters again.

Anyway, we should not use -EAGAIN here, or? Simple return the errno from function which returns it.

- Alex

This patch reworks the receive handling of bluetooth 6lowpan. I introduce the same mechanism like we do in 802.15.4 6LoWPAN. Small change is that I changed RX_QUEUED to RX_QUEUE, because it's not queued, returning RX_QUEUE means the skb will be queued.

About the TODOs:

I added several TODOs which I am not sure how to handle it right now. E.g. "skb->dev = dev", if this is really necessary, the current implementation does it in IPHC and not in IPv6 dispatch value.

About memory management here:

This is currently wrong somehow, you can see that at the first call of "skb_share_check", if this fails then the skb will be freed by kfree_skb. But the previous error checks doesn't kfree_skb the skb. This is a different handling for the same thing.

Jukka, please look how the ".recv" callback of "l2cap_ops" is working. Who will free the skb? The ".recv" callback of "l2cap_ops" if returning errno, or the callback itself need to do that. If it's when returning errno, then you can't use skb_share_check/skb_unshare here.

I assume the callback itself need to do that.

Cc Jukka Rissanen <[email protected]>

Signed-off-by Alexander Aring <[email protected]>

---
 net/bluetooth/6lowpan.c | 157 +++++++++++++++++++++++++++++-------------------
 1 file changed, 95 insertions(+), 62 deletions(-)

net/bluetooth/6lowpan.c +71 -50

index d936e7d..72ccbbf 100644
@@ -29,6 +29,11 @@
 
 #define VERSION "0.1"
 
+typedef unsigned __bitwise__ lowpan_rx_result;
+#define RX_CONTINUE		((__force lowpan_rx_result) 0u)
+#define RX_DROP_UNUSABLE	((__force lowpan_rx_result) 1u)
+#define RX_QUEUE		((__force lowpan_rx_result) 3u)
+
 static struct dentry *lowpan_enable_debugfs;
 static struct dentry *lowpan_control_debugfs;
 

@@ -257,13 +262,38 @@ static struct lowpan_dev *lookup_dev(struct l2cap_conn *conn)
 
 static int give_skb_to_upper(struct sk_buff *skb, struct net_device *dev)
 {
-	struct sk_buff *skb_cp;
+	skb->protocol = htons(ETH_P_IPV6);
+	skb->pkt_type = PACKET_HOST;
+
+	dev->stats.rx_bytes += skb->len;
+	dev->stats.rx_packets++;
+
+	return netif_rx(skb);
+}
 
-	skb_cp = skb_copy(skb, GFP_ATOMIC);
-	if (!skb_cp)
+static int lowpan_rx_handlers_result(struct sk_buff *skb,
+				     struct net_device *dev,
+				     lowpan_rx_result res)
+{
+	switch (res) {
+	case RX_CONTINUE:
+		/* nobody cared about this packet */
+		net_warn_ratelimited("%s: received unknown dispatch\n",
+				     __func__);
+
+		/* fall-through */
+	case RX_DROP_UNUSABLE:
+		kfree_skb(skb);
 		return NET_RX_DROP;
+	case RX_QUEUE:
+		return give_skb_to_upper(skb, dev);
+	default:
+		/* should never occur */
+		WARN_ON_ONCE(1);
+		break;
+	}
 
-	return netif_rx(skb_cp);
+	return NET_RX_DROP;
 }
 
 static int iphc_decompress(struct sk_buff *skb, struct net_device *netdev,

@@ -287,82 +317,85 @@ static int iphc_decompress(struct sk_buff *skb, struct net_device *netdev,
 	return lowpan_header_decompress(skb, netdev, daddr, saddr);
 }
 
-static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
-		    struct l2cap_chan *chan)
+static lowpan_rx_result lowpan_rx_h_iphc(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;
-
-	if (dev->type != ARPHRD_6LOWPAN || !skb->len)
-		goto drop;
-
-	skb_reset_network_header(skb);
+	if (!lowpan_is_iphc(*skb_network_header(skb)))
+		return RX_CONTINUE;
 
-	skb = skb_share_check(skb, GFP_ATOMIC);
-	if (!skb)
-		goto drop;
-
-	/* check that it's our buffer */
-	if (lowpan_is_ipv6(*skb_network_header(skb))) {
-		/* Copy the packet so that the IPv6 header is
-		 * properly aligned.
-		 */
-		local_skb = skb_copy_expand(skb, NET_SKB_PAD - 1,
-					    skb_tailroom(skb), GFP_ATOMIC);
-		if (!local_skb)
-			goto drop;
+	ret = iphc_decompress(skb, dev, chan);
+	if (ret < 0)
+		return RX_DROP_UNUSABLE;
 
-		local_skb->protocol = htons(ETH_P_IPV6);
-		local_skb->pkt_type = PACKET_HOST;
+	return RX_QUEUE;
+}
 
-		skb_set_transport_header(local_skb, sizeof(struct ipv6hdr));
+static lowpan_rx_result lowpan_rx_h_ipv6(struct sk_buff *skb,
+					 struct net_device *dev,
+					 struct l2cap_chan *chan)
+{
+	if (!lowpan_is_ipv6(*skb_network_header(skb)))
+		return RX_CONTINUE;
 
-		if (give_skb_to_upper(local_skb, dev) != NET_RX_SUCCESS) {
-			kfree_skb(local_skb);
-			goto drop;
-		}
+	/* Pull off the 1-byte of 6lowpan header. */
+	skb_pull(skb, 1);
+	return RX_QUEUE;
+}
 
-		dev->stats.rx_bytes += skb->len;
-		dev->stats.rx_packets++;
+static int lowpan_invoke_rx_handlers(struct sk_buff *skb,
+				     struct net_device *dev,
+				     struct l2cap_chan *chan)
+{
+	lowpan_rx_result res;
 
-		consume_skb(local_skb);
-		consume_skb(skb);
-	} else if (lowpan_is_iphc(*skb_network_header(skb))) {
-		local_skb = skb_clone(skb, GFP_ATOMIC);
-		if (!local_skb)
-			goto drop;
+#define CALL_RXH(rxh)			\
+	do {				\
+		res = rxh(skb, dev, chan); \
+		if (res != RX_CONTINUE)	\
+			goto rxh_next;	\
+	} while (0)
 
-		ret = iphc_decompress(local_skb, dev, chan);
-		if (ret < 0) {
-			kfree_skb(local_skb);
-			goto drop;
-		}
+	/* likely at first */
+	CALL_RXH(lowpan_rx_h_iphc);
+	CALL_RXH(lowpan_rx_h_ipv6);
 
-		local_skb->protocol = htons(ETH_P_IPV6);
-		local_skb->pkt_type = PACKET_HOST;
-		local_skb->dev = dev;
+rxh_next:
+	return lowpan_rx_handlers_result(skb, dev, res);
+}

net/bluetooth/6lowpan.c +4 -2

+	if (!netif_running(dev) ||
+	    dev->type != ARPHRD_6LOWPAN || !skb->len)
+		goto drop;
 
-		dev->stats.rx_bytes += skb->len;
-		dev->stats.rx_packets++;
+	/* we will manipulate the skb struct */

net/bluetooth/6lowpan.c +6 -0

+
+	/* iphc will manipulate the skb buffer, unshare it */
+	if (lowpan_is_iphc(*skb_network_header(skb))) {
+		skb = skb_unshare(skb, GFP_ATOMIC);
+		if (!skb)
+			goto out;
 	}