[PATCH net] macvlan: fix leak in macvlan_handle_frame

[PATCH net] macvlan: fix leak in macvlan_handle_frame

[Sabrina Dubroca]

Reset pskb in macvlan_handle_frame in case skb_share_check returned a
clone.

Fixes: 8a4eb5734e8d ("net: introduce rx_handler results and logic around that")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macvlan.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 86f6c6292c27..06c8bfeaccd6 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -415,6 +415,7 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
 		skb = ip_check_defrag(dev_net(skb->dev), skb, IP_DEFRAG_MACVLAN);
 		if (!skb)
 			return RX_HANDLER_CONSUMED;
+		*pskb = skb;
 		eth = eth_hdr(skb);
 		macvlan_forward_source(skb, port, eth->h_source);
 		src = macvlan_hash_lookup(port, eth->h_source);
@@ -456,6 +457,7 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
 		goto out;
 	}
 
+	*pskb = skb;
 	skb->dev = dev;
 	skb->pkt_type = PACKET_HOST;
 
-- 
2.6.2

Re: [PATCH net] macvlan: fix leak in macvlan_handle_frame

[David Miller]

From: Sabrina Dubroca <sd@queasysnail.net>
Date: Mon, 16 Nov 2015 22:54:20 +0100

> Reset pskb in macvlan_handle_frame in case skb_share_check returned a
> clone.
> 
> Fixes: 8a4eb5734e8d ("net: introduce rx_handler results and logic around that")
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Thanks for finding all of these bugs which seem to have the same
signature.

I always find pass by reference handling of SKBs to be extremely error
prone, and it's a very inefficient calling convention anyways.

I wondered if we could do better with the interface for rx_handler,
for example returning an error-pointer for the 'new_skb', but nothing
I came up with could provide the same semantics we have now.

Sigh...

Re: [PATCH net] macvlan: fix leak in macvlan_handle_frame

[David Miller]

From: Sabrina Dubroca <sd@queasysnail.net>
Date: Mon, 16 Nov 2015 22:54:20 +0100

> Reset pskb in macvlan_handle_frame in case skb_share_check returned a
> clone.
> 
> Fixes: 8a4eb5734e8d ("net: introduce rx_handler results and logic around that")
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Applied.

Re: [PATCH net] macvlan: fix leak in macvlan_handle_frame

[Paul Gortmaker]

On Tue, Nov 17, 2015 at 2:40 PM, David Miller <davem@davemloft.net> wrote:
> From: Sabrina Dubroca <sd@queasysnail.net>
> Date: Mon, 16 Nov 2015 22:54:20 +0100
>
>> Reset pskb in macvlan_handle_frame in case skb_share_check returned a
>> clone.
>>
>> Fixes: 8a4eb5734e8d ("net: introduce rx_handler results and logic around that")
>> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
>
> Applied.

Just as a report, using my sbc8641d (powerpc dual core 7448) and as of this
commit it fails to NFS boot (according to git bisect).  Unclear to me why this
would trigger it.  The fail looks like:

IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready
fsl-gianfar f8024000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
IP-Config: Unable to set interface netmask (-22)

Board uses 1st gen FSL gianfar hardware.

Bisect log follows. Will look into it more as time permits.

Paul.
--

paul@yow-lpgnfs-02:~/git/linux-head$ git bisect log
git bisect start 'net' 'drivers/net'
# good: [6a13feb9c82803e2b815eca72fa7a9f5561d7861] Linux 4.3
git bisect good 6a13feb9c82803e2b815eca72fa7a9f5561d7861
# bad: [31ade3b83e1821da5fbb2f11b5b3d4ab2ec39db8] Linux 4.4-rc3
git bisect bad 31ade3b83e1821da5fbb2f11b5b3d4ab2ec39db8
# good: [991659674288dba28c2f5a3d1a0133ef4d20824a] Merge tag
'wireless-drivers-next-for-davem-2015-10-09' of
git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next
git bisect good 991659674288dba28c2f5a3d1a0133ef4d20824a
# good: [d59542ddcb547871b1913c34c31a6d207627db02] Merge tag
'wireless-drivers-next-for-davem-2015-10-27' of
git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next
git bisect good d59542ddcb547871b1913c34c31a6d207627db02
# good: [41ecf1404b34d9975eb97f5005d9e4274eaeb76a] Merge tag
'for-linus-4.4-rc0-tag' of
git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip
git bisect good 41ecf1404b34d9975eb97f5005d9e4274eaeb76a
# good: [2df4ee78d042ee3d17cbebd51e31b300286549dc] Merge
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net
git bisect good 2df4ee78d042ee3d17cbebd51e31b300286549dc
# good: [00ee5927177792a6e139d50b6b7564d35705556a] net: fix
__netdev_update_features return on ndo_set_features failure
git bisect good 00ee5927177792a6e139d50b6b7564d35705556a
# bad: [a3a116e04cc6a94d595ead4e956ab1bc1d2f4746] af_unix: take
receive queue lock while appending new skb
git bisect bad a3a116e04cc6a94d595ead4e956ab1bc1d2f4746
# good: [f1a454a37618b819f2528ccd234f77a02b3a6016] ipg: Remove ipg driver
git bisect good f1a454a37618b819f2528ccd234f77a02b3a6016
# bad: [e639b8d8a7a728f0b05ef2df6cb6b45dc3d4e556] macvlan: fix leak in
macvlan_handle_frame
git bisect bad e639b8d8a7a728f0b05ef2df6cb6b45dc3d4e556
# good: [28f9ee22bcdd84726dbf6267d0b58f254166b900] vlan: Do not put
vlan headers back on bridge and macvlan ports
git bisect good 28f9ee22bcdd84726dbf6267d0b58f254166b900
# good: [cf554ada0be7077906aa9a17faf151ff66e3cb8e] ipvlan: fix leak in
ipvlan_rcv_frame
git bisect good cf554ada0be7077906aa9a17faf151ff66e3cb8e
# good: [a534dc529853c69e94994aa47c1d80a03ce2c11d] ipvlan: fix use
after free of skb
git bisect good a534dc529853c69e94994aa47c1d80a03ce2c11d
# first bad commit: [e639b8d8a7a728f0b05ef2df6cb6b45dc3d4e556]
macvlan: fix leak in macvlan_handle_frame
paul@yow-lpgnfs-02:~/git/linux-head$



> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] macvlan: fix leak in macvlan_handle_frame

[Paul Gortmaker]

On Sun, Dec 6, 2015 at 6:35 PM, Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
> On Tue, Nov 17, 2015 at 2:40 PM, David Miller <davem@davemloft.net> wrote:
>> From: Sabrina Dubroca <sd@queasysnail.net>
>> Date: Mon, 16 Nov 2015 22:54:20 +0100
>>
>>> Reset pskb in macvlan_handle_frame in case skb_share_check returned a
>>> clone.
>>>
>>> Fixes: 8a4eb5734e8d ("net: introduce rx_handler results and logic around that")
>>> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
>>
>> Applied.
>
> Just as a report, using my sbc8641d (powerpc dual core 7448) and as of this
> commit it fails to NFS boot (according to git bisect).  Unclear to me why this
> would trigger it.  The fail looks like:

Apologies; turns out I pooched the bisect while trying to race a dying laptop
battery.  It is a couple commits after this, v4.3-11564-g321beec5047a that
changes PHY handling based on whether it has an IRQ or not (which makes
a lot more sense).  Reverting that on v4.4-rc4 makes it work and confirms
I've got the bisect right this time.

I'll follow up that patch once I investigate a bit further as to whether it just
uncovers a board issue (broken PHY irq line or similar) that previously just
went unseen.

Sorry again for the noise.
Paul.
--

>
> IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready
> fsl-gianfar f8024000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
> IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> IP-Config: Unable to set interface netmask (-22)
>
> Board uses 1st gen FSL gianfar hardware.
>
> Bisect log follows. Will look into it more as time permits.
>
> Paul.
> --
>
> paul@yow-lpgnfs-02:~/git/linux-head$ git bisect log
> git bisect start 'net' 'drivers/net'
> # good: [6a13feb9c82803e2b815eca72fa7a9f5561d7861] Linux 4.3
> git bisect good 6a13feb9c82803e2b815eca72fa7a9f5561d7861
> # bad: [31ade3b83e1821da5fbb2f11b5b3d4ab2ec39db8] Linux 4.4-rc3
> git bisect bad 31ade3b83e1821da5fbb2f11b5b3d4ab2ec39db8
> # good: [991659674288dba28c2f5a3d1a0133ef4d20824a] Merge tag
> 'wireless-drivers-next-for-davem-2015-10-09' of
> git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next
> git bisect good 991659674288dba28c2f5a3d1a0133ef4d20824a
> # good: [d59542ddcb547871b1913c34c31a6d207627db02] Merge tag
> 'wireless-drivers-next-for-davem-2015-10-27' of
> git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next
> git bisect good d59542ddcb547871b1913c34c31a6d207627db02
> # good: [41ecf1404b34d9975eb97f5005d9e4274eaeb76a] Merge tag
> 'for-linus-4.4-rc0-tag' of
> git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip
> git bisect good 41ecf1404b34d9975eb97f5005d9e4274eaeb76a
> # good: [2df4ee78d042ee3d17cbebd51e31b300286549dc] Merge
> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net
> git bisect good 2df4ee78d042ee3d17cbebd51e31b300286549dc
> # good: [00ee5927177792a6e139d50b6b7564d35705556a] net: fix
> __netdev_update_features return on ndo_set_features failure
> git bisect good 00ee5927177792a6e139d50b6b7564d35705556a
> # bad: [a3a116e04cc6a94d595ead4e956ab1bc1d2f4746] af_unix: take
> receive queue lock while appending new skb
> git bisect bad a3a116e04cc6a94d595ead4e956ab1bc1d2f4746
> # good: [f1a454a37618b819f2528ccd234f77a02b3a6016] ipg: Remove ipg driver
> git bisect good f1a454a37618b819f2528ccd234f77a02b3a6016
> # bad: [e639b8d8a7a728f0b05ef2df6cb6b45dc3d4e556] macvlan: fix leak in
> macvlan_handle_frame
> git bisect bad e639b8d8a7a728f0b05ef2df6cb6b45dc3d4e556
> # good: [28f9ee22bcdd84726dbf6267d0b58f254166b900] vlan: Do not put
> vlan headers back on bridge and macvlan ports
> git bisect good 28f9ee22bcdd84726dbf6267d0b58f254166b900
> # good: [cf554ada0be7077906aa9a17faf151ff66e3cb8e] ipvlan: fix leak in
> ipvlan_rcv_frame
> git bisect good cf554ada0be7077906aa9a17faf151ff66e3cb8e
> # good: [a534dc529853c69e94994aa47c1d80a03ce2c11d] ipvlan: fix use
> after free of skb
> git bisect good a534dc529853c69e94994aa47c1d80a03ce2c11d
> # first bad commit: [e639b8d8a7a728f0b05ef2df6cb6b45dc3d4e556]
> macvlan: fix leak in macvlan_handle_frame
> paul@yow-lpgnfs-02:~/git/linux-head$
>
>
>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html