Return-Path: Date: Mon, 6 Oct 2014 09:12:28 +0200 From: Alexander Aring To: Martin Townsend Cc: Martin Townsend , linux-zigbee-devel@lists.sourceforge.net, linux-bluetooth@vger.kernel.org, linux-wpan@vger.kernel.org, marcel@holtmann.org, werner@almesberger.net Subject: Re: [PATCH v4 bluetooth] 6lowpan: fix incorrect return values in lowpan_rcv Message-ID: <20141006071225.GA10024@omega> References: <1412165422-31063-1-git-send-email-martin.townsend@xsilon.com> <1412165422-31063-2-git-send-email-martin.townsend@xsilon.com> <20141005175049.GA15923@omega> <5431B18B.4030603@xsilon.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 In-Reply-To: <5431B18B.4030603@xsilon.com> List-ID: 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 > >>--- > >> 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