This patch aims to reduce the amount of copying in the receive path when
decompressing by checking for headroom and calling pskb_expand_head if required.
Another benefit of this is that the skb pointer passed to the decompression
routine will be the same skb after decompression. This will be a step towards
freeing the skb within the receive function and not within all the error paths
of the decompression routine.
Bluetooth and 802.15.4 have been changed to ensure that only one reference
to the skb exists before calling the decompression routine as this is a
requirement of pskb_expand_head.
v1 -> v2
Use const int for new headroom size.
Remove blank line.
Use skb_unshare instead of skb_copy_expand.
Use consume_skb.
v2 -> v3
Remove cases where we are trying to free a NULL skb.
Martin Townsend (1):
6lowpan: Use pskb_expand_head in IPHC decompression.
net/6lowpan/iphc.c | 51 +++++++++++++++++++++----------------------
net/bluetooth/6lowpan.c | 12 ++++++----
net/ieee802154/6lowpan_rtnl.c | 5 +++++
3 files changed, 38 insertions(+), 30 deletions(-)
--
1.9.1
On Wed, Oct 08, 2014 at 08:35:45AM +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 <[email protected]>
Acked-by: Alexander Aring <[email protected]>
Hi Alex,
On 08/10/14 09:06, Alexander Aring wrote:
> Hi Martin,
>
> On Wed, Oct 08, 2014 at 09:54:58AM +0200, Alexander Aring wrote:
> ...
>> here we lost the skb reference from parameter if failed and I don't see
>> that skb_unshare free it on failing (maybe I am wrong here).
>>
> I will do this step by step.
>
> 1. call skb_unshare
>
> 2. check if cloned
>
> 3. call skb_copy
>
> 4. call __alloc_skb
>
> 5. __alloc_skb return NULL
>
> 6. assign to temp nskb;
>
> 7. parameter skb will be freed. kfree_skb(skb).
>
> 8. skb = nskb;
>
> 9. return skb;
>
> So I see that kfree_skb(skb) is always called also on failure.
Yes this caught me out too :)
> So your code seems to be correct. This is a lack of documentation.
>
> - Alex
So is v3 patch good to go?
- Martin.
Hi Martin,
On Wed, Oct 08, 2014 at 09:54:58AM +0200, Alexander Aring wrote:
...
>
> here we lost the skb reference from parameter if failed and I don't see
> that skb_unshare free it on failing (maybe I am wrong here).
>
I will do this step by step.
1. call skb_unshare
2. check if cloned
3. call skb_copy
4. call __alloc_skb
5. __alloc_skb return NULL
6. assign to temp nskb;
7. parameter skb will be freed. kfree_skb(skb).
8. skb = nskb;
9. return skb;
So I see that kfree_skb(skb) is always called also on failure.
So your code seems to be correct. This is a lack of documentation.
- Alex
Hi Martin,
On Wed, Oct 08, 2014 at 08:35:45AM +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 <[email protected]>
> ---
> net/6lowpan/iphc.c | 51 +++++++++++++++++++++----------------------
> net/bluetooth/6lowpan.c | 12 ++++++----
> net/ieee802154/6lowpan_rtnl.c | 5 +++++
> 3 files changed, 38 insertions(+), 30 deletions(-)
>
> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
> index 142eef5..853b4b8 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));
>
Just a hint, don't do that in this patch or send a patch series.
I would in a future move:
skb_push(skb, sizeof(struct ipv6hdr));
skb_reset_network_header(skb);
skb_copy_to_linear_data(skb, hdr, sizeof(struct ipv6hdr));
into the end of iphc decompression, because we do the same thing for
transport header in iphc decompression.
> - 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;
This we should leave here. Then bluetooth/802.15.4 can set different
things here.
...
> hdr.payload_len = htons(skb->len);
> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> index c2e0d14..bd85edf 100644
> --- a/net/bluetooth/6lowpan.c
> +++ b/net/bluetooth/6lowpan.c
> @@ -343,13 +343,17 @@ 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 */
> + skb = skb_unshare(skb, GFP_ATOMIC);
> + if (!skb) {
> + dev->stats.rx_dropped++;
> + return NET_RX_DROP;
> + }
> +
What I meant here in my previous review notes is that kfree_skb(skb);
was correct but in this case we can't do it, we need a temp variable.
skb = skb_unshare(skb, GFP_ATOMIC);
here we lost the skb reference from parameter if failed and I don't see
that skb_unshare free it on failing (maybe I am wrong here).
foo(any name for a tmp skb name, I see a lot of nskb names for a tmp skb
name. Maybe this is a usual name convention) = skb_unshare(skb, GFP_ATOMIC);
Then we don't lost the "skb" parameter reference and can free it with
kfree_skb(skb).
If all is fine we can run a skb = foo.
I think that skb_unshare and the buffer is NOT shared then (skb == foo),
but I am not sure about this. If it's NOT the same then we have some
memory leaking here.
- Alex
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 <[email protected]>
---
net/6lowpan/iphc.c | 51 +++++++++++++++++++++----------------------
net/bluetooth/6lowpan.c | 12 ++++++----
net/ieee802154/6lowpan_rtnl.c | 5 +++++
3 files changed, 38 insertions(+), 30 deletions(-)
diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 142eef5..853b4b8 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);
+ consume_skb(skb);
return stat;
}
@@ -460,7 +452,7 @@ 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;
+ const int needed = sizeof(struct udphdr) + sizeof(hdr);
if (uncompress_udp_header(skb, &uh))
goto drop;
@@ -468,14 +460,13 @@ 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) < needed) {
+ err = pskb_expand_head(skb, needed, 0, GFP_ATOMIC);
+ if (unlikely(err)) {
+ kfree_skb(skb);
+ return err;
+ }
+ }
skb_push(skb, sizeof(struct udphdr));
skb_reset_transport_header(skb);
@@ -485,6 +476,14 @@ 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);
+ 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..bd85edf 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -343,13 +343,17 @@ 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 */
+ skb = skb_unshare(skb, GFP_ATOMIC);
+ if (!skb) {
+ dev->stats.rx_dropped++;
+ return NET_RX_DROP;
+ }
+
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..68287db 100644
--- a/net/ieee802154/6lowpan_rtnl.c
+++ b/net/ieee802154/6lowpan_rtnl.c
@@ -537,6 +537,11 @@ 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 */
+ skb = skb_unshare(skb, GFP_ATOMIC);
+ if (!skb)
+ goto drop;
+
switch (skb->data[0] & 0xe0) {
case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
ret = process_data(skb, &hdr);
--
1.9.1