[RFC bluetooth-next 0/2] bluetooth: 6lowpan: suggestions for Jukka

[RFC bluetooth-next 0/2] bluetooth: 6lowpan: suggestions for Jukka

[Alexander Aring]

Hi,

this patch series is compile checked only and is a suggestion for Jukka which
has currently some issues with 6lowpan. These issues are page faults and I
suppose something is wrong with the memory management of the skbs.

Jukka, I also add several TODOs, take the patches and take a closer look into
that. Thanks. I mainly copied the parsing mechanism for dispatches from
802.15.4 6LoWPAN.

I hope these patches will solve your issues. If you have questions then simple
ask.

Compile tested only because I still have no btle dongle, but I think there
exists also some simulator, or? :-)

- Alex

Cc: Jukka Rissanen <jukka.rissanen@linux.intel.com>

Alexander Aring (2):
  bluetooth: 6lowpan: avoid endless loop
  bluetooth: 6lowpan: rework receive handling

 net/bluetooth/6lowpan.c | 161 +++++++++++++++++++++++++++++-------------------
 1 file changed, 96 insertions(+), 65 deletions(-)

-- 
2.6.1

[RFC bluetooth-next 1/2] bluetooth: 6lowpan: avoid endless loop

[Alexander Aring]

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

Cc: Jukka Rissanen <jukka.rissanen@linux.intel.com>
Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 net/bluetooth/6lowpan.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index d85af23..d936e7d 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -383,10 +383,8 @@ static int chan_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
 		return -ENOENT;
 
 	err = recv_pkt(skb, dev->netdev, chan);
-	if (err) {
+	if (err)
 		BT_DBG("recv pkt %d", err);
-		err = -EAGAIN;
-	}
 
 	return err;
 }
-- 
2.6.1

Re: [RFC bluetooth-next 1/2] bluetooth: 6lowpan: avoid endless loop

[Marcel Holtmann]

Hi Alex,

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

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.

Regards

Marcel

Re: [RFC bluetooth-next 1/2] bluetooth: 6lowpan: avoid endless loop

[Alexander Aring]

Hi Marcel,

On Mon, Oct 26, 2015 at 05:11:37AM +0900, Marcel Holtmann wrote:
> Hi Alex,
> 
> > 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).
> 
> 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

Re: [RFC bluetooth-next 1/2] bluetooth: 6lowpan: avoid endless loop

[Jukka Rissanen]

Hi Alex,

On su, 2015-10-25 at 21:27 +0100, Alexander Aring wrote:
> Hi Marcel,
> 
> On Mon, Oct 26, 2015 at 05:11:37AM +0900, Marcel Holtmann wrote:
> > Hi Alex,
> > 
> > > 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).
> > 
> > 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.

I was trying to follow how the chan->ops->recv() is called in
l2cap_core.c and the code seems to ignore the actual error value. I
might have missed the actual place thou.

Johan/Marcel, you know what the code should return in channel receive
callback. Does it matter what is the error value returned by
chan->ops->recv()?

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


Cheers,
Jukka

[RFC bluetooth-next 2/2] bluetooth: 6lowpan: rework receive handling

[Alexander Aring]

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 <jukka.rissanen@linux.intel.com>
Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 net/bluetooth/6lowpan.c | 157 +++++++++++++++++++++++++++++-------------------
 1 file changed, 95 insertions(+), 62 deletions(-)

diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index d936e7d..72ccbbf 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -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);
+}
 
-		if (give_skb_to_upper(local_skb, dev)
-				!= NET_RX_SUCCESS) {
-			kfree_skb(local_skb);
-			goto drop;
-		}
+static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
+		    struct l2cap_chan *chan)
+{
+	/* TODO check for reserved, nalp dispatch here? */
+	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 */
+	skb = skb_share_check(skb, GFP_ATOMIC);
+	if (!skb)
+		goto out;
 
-		consume_skb(local_skb);
-		consume_skb(skb);
-	} else {
-		goto drop;
+	/* TODO is this done by bluetooth callback, before? */
+	skb_reset_network_header(skb);
+	/* TODO really necessary? Previous did that in one branch only */
+	skb->dev = dev;
+
+	/* 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;
 	}
 
-	return NET_RX_SUCCESS;
+	return lowpan_invoke_rx_handlers(skb, dev, chan);
 
 drop:
+	kfree_skb(skb);
+out:
 	dev->stats.rx_dropped++;
 	return NET_RX_DROP;
 }
-- 
2.6.1

Re: [RFC bluetooth-next 2/2] bluetooth: 6lowpan: rework receive handling

[Jukka Rissanen]

Hi Alex,

Thanks for the patch, this fixes the UDP compression crash that I am
seeing in my tests. Some comments to the TODOs below.


On la, 2015-10-24 at 16:50 +0200, Alexander Aring wrote:
> 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.

For the .recv callback in patch 1, I think we do not need to do anything
as it really looks like the actual error value is not used anywhere,
only thing important in l2cap_core.c is that there was an error, the
value is ignored.

> 
> I assume the callback itself need to do that.
> 
> Cc: Jukka Rissanen <jukka.rissanen@linux.intel.com>
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>  net/bluetooth/6lowpan.c | 157 +++++++++++++++++++++++++++++-------------------
>  1 file changed, 95 insertions(+), 62 deletions(-)
> 
> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> index d936e7d..72ccbbf 100644
> --- a/net/bluetooth/6lowpan.c
> +++ b/net/bluetooth/6lowpan.c
> @@ -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);
> +}
>  
> -		if (give_skb_to_upper(local_skb, dev)
> -				!= NET_RX_SUCCESS) {
> -			kfree_skb(local_skb);
> -			goto drop;
> -		}
> +static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
> +		    struct l2cap_chan *chan)
> +{
> +	/* TODO check for reserved, nalp dispatch here? */

Yes, NALP (Not A LoWPAN frame) should be checked.


> +	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 */
> +	skb = skb_share_check(skb, GFP_ATOMIC);
> +	if (!skb)
> +		goto out;
>  
> -		consume_skb(local_skb);
> -		consume_skb(skb);
> -	} else {
> -		goto drop;
> +	/* TODO is this done by bluetooth callback, before? */
> +	skb_reset_network_header(skb);

Tried this and commmented the reset away. Unfortunately no packets could
be received any more after that.


> +	/* TODO really necessary? Previous did that in one branch only */
> +	skb->dev = dev;

Commenting skb->dev away causes null pointer access and oops later in
the stack.

[  183.607760] BUG: unable to handle kernel NULL pointer dereference at
0000000000000048
[  183.608007] IP: [<ffffffff81664fc2>] enqueue_to_backlog+0x52/0x220
[  183.608007] PGD bf8e067 PUD 983d067 PMD 0 
[  183.608007] Oops: 0000 [#1] SMP 
[  183.608007] Modules linked in: cmac rfcomm btusb btrtl btbcm btintel
nhc_udp nhc_dest nhc_hop nhc_routing nhc_mobility nhc_ipv6 nhc_fragment
bluetooth_6lowpan 6lowpan nls_utf8 isofs fuse tun ip6t_rpfilter
ip6t_REJECT nf_reject_ipv6 xt_conntrack bnep bluetooth rfkill
ebtable_nat ebtable_broute bridge ebtable_filter ebtables ip6table_nat
nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle
ip6table_security ip6table_raw xt_connmark ip6table_filter ip6_tables
iptable_security iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4
iptable_raw nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iosf_mbi
ppdev joydev pcspkr snd_intel8x0 snd_ac97_codec ac97_bus snd_seq
snd_seq_device snd_pcm snd_timer snd video parport_pc parport i2c_piix4
soundcore ata_generic 8021q garp stp llc mrp serio_raw fjes pata_acpi
e1000
[  183.608007] CPU: 0 PID: 2096 Comm: kworker/u3:2 Not tainted 4.3.0-rc6
+ #5
[  183.608007] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS
VirtualBox 12/01/2006
[  183.608007] Workqueue: hci0 hci_rx_work [bluetooth]
[  183.608007] task: ffff88002e87bb00 ti: ffff88000bca4000 task.ti:
ffff88000bca4000
[  183.608007] RIP: 0010:[<ffffffff81664fc2>]  [<ffffffff81664fc2>]
enqueue_to_backlog+0x52/0x220
[  183.608007] RSP: 0000:ffff88000bca7bc8  EFLAGS: 00010046
[  183.608007] RAX: 0000000000000000 RBX: ffff88003fc17e00 RCX:
0000000000000018
[  183.608007] RDX: 0000000000000001 RSI: 0000000000000000 RDI:
ffff88003fc17ecc
[  183.608007] RBP: ffff88000bca7c00 R08: 00000081bb495e01 R09:
0000000000000000
[  183.608007] R10: ffff880028bda760 R11: ffff88002500cfd8 R12:
0000000000017e00
[  183.608007] R13: ffff88000bca7c18 R14: ffff88003db73440 R15:
0000000000000286
[  183.608007] FS:  0000000000000000(0000) GS:ffff88003fc00000(0000)
knlGS:0000000000000000
[  183.608007] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  183.608007] CR2: 0000000000000048 CR3: 000000000bfa8000 CR4:
00000000000006f0
[  183.608007] Stack:
[  183.608007]  ffff8800251fbc68 ffff88000bca7c00 ffff88001e8b0000
ffff88003db73440
[  183.608007]  ffff88003db73440 ffff88003db73440 ffff880003acadf0
ffff88000bca7c38
[  183.608007]  ffffffff816651d4 0000000000000000 000002ff00000000
000000003dfb3176
[  183.608007] Call Trace:
[  183.608007]  [<ffffffff816651d4>] netif_rx_internal+0x44/0xf0
[  183.608007]  [<ffffffff81665330>] netif_rx_ni+0x20/0x70
[  183.608007]  [<ffffffffa0317f3f>] chan_recv_cb+0x2af/0x2f0
[bluetooth_6lowpan]
[  183.608007]  [<ffffffffa024c5d6>] l2cap_recv_frame+0x2916/0x2a30
[bluetooth]
[  183.608007]  [<ffffffff81651db7>] ? skb_release_data+0xa7/0xd0
[  183.608007]  [<ffffffff81651159>] ? kfree_skbmem+0x59/0x60
[  183.608007]  [<ffffffffa024cf7f>] ? l2cap_recv_acldata+0x4f/0x330
[bluetooth]
[  183.608007]  [<ffffffffa024d14f>] l2cap_recv_acldata+0x21f/0x330
[bluetooth]
[  183.608007]  [<ffffffffa02189a6>] hci_rx_work+0x196/0x3d0 [bluetooth]
[  183.608007]  [<ffffffff810b719e>] process_one_work+0x19e/0x3f0
[  183.608007]  [<ffffffff810b743e>] worker_thread+0x4e/0x450
[  183.608007]  [<ffffffff8177834c>] ? __schedule+0x36c/0x980
[  183.608007]  [<ffffffff810b73f0>] ? process_one_work+0x3f0/0x3f0
[  183.608007]  [<ffffffff810b73f0>] ? process_one_work+0x3f0/0x3f0
[  183.608007]  [<ffffffff810bcda8>] kthread+0xd8/0xf0
[  183.608007]  [<ffffffff810bccd0>] ? kthread_worker_fn+0x160/0x160
[  183.608007]  [<ffffffff8177cd9f>] ret_from_fork+0x3f/0x70
[  183.608007]  [<ffffffff810bccd0>] ? kthread_worker_fn+0x160/0x160
[  183.608007] Code: 89 d5 48 03 1c f5 60 d9 d2 81 9c 58 0f 1f 44 00 00
49 89 c7 fa 66 0f 1f 44 00 00 48 8d bb cc 00 00 00 e8 f2 76 11 00 49 8b
46 20 <48> 8b 40 48 a8 01 74 10 8b 93 c8 00 00 00 8b 05 5e 0e 6d 00 39 
[  183.608007] RIP  [<ffffffff81664fc2>] enqueue_to_backlog+0x52/0x220
[  183.608007]  RSP <ffff88000bca7bc8>
[  183.608007] CR2: 0000000000000048
[  183.608007] ---[ end trace ead20527bbe7e9a0 ]---
[  193.482702] send_mcast_pkt: xmit bt0 to 00:02:72:c6:42:12 type 1 IP
fe80::202:72ff:fec6:4212 chan ffff880025185fb0
[  193.487332] clocksource: timekeeping watchdog: Marking clocksource
'tsc' as unstable because the skew is too large:
[  193.492235] clocksource:                       'acpi_pm' wd_now:
31e1a6 wd_last: 153cbc mask: ffffff
[  193.496173] clocksource:                       'tsc' cs_now:
8863621b45 cs_last: 81b84b089a mask: ffffffffffffffff
[  194.221265] send_mcast_pkt: xmit bt0 to 00:02:72:c6:42:12 type 1 IP
fe80::202:72ff:fec6:4212 chan ffff880025185fb0
[  195.049693] send_mcast_pkt: xmit bt0 to 00:02:72:c6:42:12 type 1 IP
fe80::202:72ff:fec6:4212 chan ffff880025185fb0


> +
> +	/* 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;
>  	}
>  
> -	return NET_RX_SUCCESS;
> +	return lowpan_invoke_rx_handlers(skb, dev, chan);
>  
>  drop:
> +	kfree_skb(skb);
> +out:
>  	dev->stats.rx_dropped++;
>  	return NET_RX_DROP;
>  }


Cheers,
Jukka