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 <[email protected]>
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
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 <[email protected]>
> Signed-off-by: Alexander Aring <[email protected]>
> ---
> 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
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
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
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
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(-)
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
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 <[email protected]>
Signed-off-by: Alexander Aring <[email protected]>
---
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