[PATCH bluetooth-next] ieee802154: 6lowpan: fix memory leak

[PATCH bluetooth-next] ieee802154: 6lowpan: fix memory leak

[Alexander Aring]

Looking at current situation of memory management in 6lowpan receive
function I detected some invalid handling. After calling
lowpan_invoke_rx_handlers we will do a kfree_skb and then NET_RX_DROP on
error handling. We don't do this before, also on
skb_share_check/skb_unshare which might manipulate the reference
counters.

After running some 'grep -r "dev_add_pack" net/' to look how others
packet-layer receive callbacks works I detected that every subsystem do
a kfree_skb, then NET_RX_DROP without calling skb functions which
might manipulate the skb reference counters. This is the reason why we
should do the same here like all others subsystems. I didn't find any
documentation how the packet-layer receive callbacks handle NET_RX_DROP
return values either.

This patch will add a kfree_skb, then NET_RX_DROP handling for the
"trivial checks", in case of skb_share_check/skb_unshare the kfree_skb
call will be done inside these functions.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
References how other filesystem handle the callback:

[0] http://lxr.free-electrons.com/source/net/ipx/af_ipx.c#L1638
[1] http://lxr.free-electrons.com/source/net/can/af_can.c#L709
[2] http://lxr.free-electrons.com/source/net/phonet/af_phonet.c#L371

 net/ieee802154/6lowpan/rx.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/ieee802154/6lowpan/rx.c b/net/ieee802154/6lowpan/rx.c
index 403f171..ef185dd 100644
--- a/net/ieee802154/6lowpan/rx.c
+++ b/net/ieee802154/6lowpan/rx.c
@@ -284,16 +284,16 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *wdev,
 	if (wdev->type != ARPHRD_IEEE802154 ||
 	    skb->pkt_type == PACKET_OTHERHOST ||
 	    !lowpan_rx_h_check(skb))
-		return NET_RX_DROP;
+		goto drop;
 
 	ldev = wdev->ieee802154_ptr->lowpan_dev;
 	if (!ldev || !netif_running(ldev))
-		return NET_RX_DROP;
+		goto drop;
 
 	/* Replacing skb->dev and followed rx handlers will manipulate skb. */
 	skb = skb_share_check(skb, GFP_ATOMIC);
 	if (!skb)
-		return NET_RX_DROP;
+		goto out;
 	skb->dev = ldev;
 
 	/* When receive frag1 it's likely that we manipulate the buffer.
@@ -304,10 +304,15 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *wdev,
 	    lowpan_is_iphc(*skb_network_header(skb))) {
 		skb = skb_unshare(skb, GFP_ATOMIC);
 		if (!skb)
-			return NET_RX_DROP;
+			goto out;
 	}
 
 	return lowpan_invoke_rx_handlers(skb);
+
+drop:
+	kfree_skb(skb);
+out:
+	return NET_RX_DROP;
 }
 
 static struct packet_type lowpan_packet_type = {
-- 
2.6.1

Re: [PATCH bluetooth-next] ieee802154: 6lowpan: fix memory leak

[Alexander Aring]

On Thu, Oct 22, 2015 at 12:11:27PM +0200, Alexander Aring wrote:
> Looking at current situation of memory management in 6lowpan receive
> function I detected some invalid handling. After calling
> lowpan_invoke_rx_handlers we will do a kfree_skb and then NET_RX_DROP on
> error handling. We don't do this before, also on
> skb_share_check/skb_unshare which might manipulate the reference
> counters.
> 
> After running some 'grep -r "dev_add_pack" net/' to look how others
> packet-layer receive callbacks works I detected that every subsystem do
> a kfree_skb, then NET_RX_DROP without calling skb functions which
> might manipulate the skb reference counters. This is the reason why we
> should do the same here like all others subsystems. I didn't find any
> documentation how the packet-layer receive callbacks handle NET_RX_DROP
> return values either.
> 
> This patch will add a kfree_skb, then NET_RX_DROP handling for the
> "trivial checks", in case of skb_share_check/skb_unshare the kfree_skb
> call will be done inside these functions.
> 
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
> References how other filesystem handle the callback:

s/filesystem/subsystems/

- Alex

Re: [PATCH bluetooth-next] ieee802154: 6lowpan: fix memory leak

[Marcel Holtmann]

Hi Alex,

> Looking at current situation of memory management in 6lowpan receive
> function I detected some invalid handling. After calling
> lowpan_invoke_rx_handlers we will do a kfree_skb and then NET_RX_DROP on
> error handling. We don't do this before, also on
> skb_share_check/skb_unshare which might manipulate the reference
> counters.
> 
> After running some 'grep -r "dev_add_pack" net/' to look how others
> packet-layer receive callbacks works I detected that every subsystem do
> a kfree_skb, then NET_RX_DROP without calling skb functions which
> might manipulate the skb reference counters. This is the reason why we
> should do the same here like all others subsystems. I didn't find any
> documentation how the packet-layer receive callbacks handle NET_RX_DROP
> return values either.
> 
> This patch will add a kfree_skb, then NET_RX_DROP handling for the
> "trivial checks", in case of skb_share_check/skb_unshare the kfree_skb
> call will be done inside these functions.
> 
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
> References how other filesystem handle the callback:
> 
> [0] http://lxr.free-electrons.com/source/net/ipx/af_ipx.c#L1638
> [1] http://lxr.free-electrons.com/source/net/can/af_can.c#L709
> [2] http://lxr.free-electrons.com/source/net/phonet/af_phonet.c#L371
> 
> net/ieee802154/6lowpan/rx.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel