Hi Alex,
On 07/10/14 17:14, Alexander Aring wrote:
Hi Martin,
On Tue, Oct 07, 2014 at 04:33:57PM +0100, Martin Townsend wrote:
Currently there are potentially 2 skb_copy_expand calls in IPHC
decompression. This patch replaces this with one call to
pskb_expand_head. It also checks to see if there is enough headroom
first to ensure it's only done if necessary.
As pskb_expand_head must only have one reference the calling code
now ensures this.
Signed-off-by: Martin Townsend <martin.townsend@xxxxxxxxxx>
---
net/6lowpan/iphc.c | 53 ++++++++++++++++++++++---------------------
net/bluetooth/6lowpan.c | 19 ++++++++++++----
net/ieee802154/6lowpan_rtnl.c | 13 +++++++++++
3 files changed, 55 insertions(+), 30 deletions(-)
diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 142eef5..fa8f348 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -174,30 +174,22 @@ static int uncompress_context_based_src_addr(struct sk_buff *skb,
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));
+ skb_push(skb, sizeof(struct ipv6hdr));
+ skb_reset_network_header(skb);
+ skb_copy_to_linear_data(skb, hdr, sizeof(struct ipv6hdr));
- new->protocol = htons(ETH_P_IPV6);
- new->pkt_type = PACKET_HOST;
- new->dev = dev;
+ skb->protocol = htons(ETH_P_IPV6);
+ skb->pkt_type = PACKET_HOST;
+ skb->dev = dev;
raw_dump_table(__func__, "raw skb data dump before receiving",
- new->data, new->len);
+ skb->data, skb->len);
- stat = deliver_skb(new, dev);
+ stat = deliver_skb(skb, dev);
- kfree_skb(new);
+ kfree_skb(skb);
I know what before but "consume_skb"
agreed.
return stat;
}
@@ -460,7 +452,6 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
/* UDP data uncompression */
if (iphc0 & LOWPAN_IPHC_NH_C) {
struct udphdr uh;
- struct sk_buff *new;
if (uncompress_udp_header(skb, &uh))
goto drop;
@@ -468,14 +459,15 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
/* 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;
-
- skb = new;
+ if (skb_headroom(skb) < sizeof(struct udphdr) + sizeof(hdr)) {
+ int n = sizeof(struct udphdr) + sizeof(hdr);
+
+ err = pskb_expand_head(skb, n, 0, GFP_ATOMIC);
replace n with constant value "sizeof(struct udphdr) + sizeof(hdr)".
ok, I was only trying to make the code more readable here instead of
having the calculation split over 2 lines. Actually what is the coding
style for this case? or could I use a #define and then I could use it in
the if statement above and well as the pskb_expand_head?
+ if (unlikely(err)) {
+ kfree_skb(skb);
+ return err;
+ }
+ }
skb_push(skb, sizeof(struct udphdr));
skb_reset_transport_header(skb);
@@ -485,6 +477,15 @@ 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);
+
remove whitespace
ok
+ if (unlikely(err)) {
+ kfree_skb(skb);
+ return err;
+ }
+ }
}
hdr.payload_len = htons(skb->len);
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index c2e0d14..ab51e16 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -343,13 +343,24 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
kfree_skb(local_skb);
kfree_skb(skb);
} else {
+ /* Decompression may use pskb_expand_head, ensure skb unique */
+ if (skb_cloned(skb)) {
marker
+ 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)
+ goto drop;
+ consume_skb(skb);
+ skb = new;
+ }
+
switch (skb->data[0] & 0xe0) {
case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
- local_skb = skb_clone(skb, GFP_ATOMIC);
- if (!local_skb)
- goto drop;
- ret = process_data(local_skb, dev, chan);
+ ret = process_data(skb, dev, chan);
if (ret != NET_RX_SUCCESS)
goto drop;
diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
index c7e07b8..2ccea84 100644
--- a/net/ieee802154/6lowpan_rtnl.c
+++ b/net/ieee802154/6lowpan_rtnl.c
@@ -537,6 +537,19 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
if (ret == NET_RX_DROP)
goto drop;
} else {
+ /* Decompression may use pskb_expand_head, ensure skb unique */
+ if (skb_cloned(skb)) {
At this point:
First:
this SHOULD be always true in this function, but it doesn't. This is a
complicated issue inside of mac802154. I will ack this, but for the
rework we need to remove this again. Means: Currently I am fine with
this change.
That's the intention, once you're rework is in place this can be removed.
We _should_ do it inside the deliver function for multiple interface,
normally we need something like this at mac802154 layer [0]. It's simple
complete broken at current mac802154 mainline state. We set the skb->dev
for only one skb struct and we need to clone it before for each interface...
then we can touch skb->dev again. So multiple interface is... better not use
that. :-)
Funny at monitor rx function somebody implement a skb_clone before.
Second:
I don't know why this is here, before evaluate the dispatch. If we
receive LOWPAN_DISPATCH_FRAGN we don't need headroom for ipv6hdr/udphdr.
I think we only need this headroom for uncompression and this is for the
LOWPAN_DISPATCH_IPHC case. But above, see "marker" you do the same
thing again. I mean here we always add room for sizeof(struct ipv6hdr)
+ sizeof(struct udphdr), doesn't matter if next header compression is set.
And above at "marker" we do the same thing. Also do here always a ... +
sizeof(struct udphdr) doesn't fit together with the comming next header
compression framework, which needs to evaluate the nhc id to get the
right next header size which is necessary for the headroom.
I can only imaging why you doing this here, because it is necessary for
doing the FRAG1 uncompression on the fly stuff, maybe this is some reason
why this is here. But it's not necessary for FRAGN.
As it's temporary code is it ok to leave it?
+ 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)
+ goto drop_skb;
+ consume_skb(skb);
+ skb = new;
+ }
switch (skb->data[0] & 0xe0) {
case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
ret = process_data(skb, &hdr);
- Alex
[0] https://github.com/linux-wpan/linux-wpan-next/blob/wpan_rework_rfc/net/mac802154/rx.c#L2
- Martin.
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
