Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1292253 > unrolled thread

Re: [PATCH 4.3] vrf: Fix memory leak on registration failure in vrf_newlink()

Started byDavid Ahern <dsa@cumulusnetworks.com>
First post2015-12-15 16:20 +0100
Last post2015-12-15 16:50 +0100
Articles 7 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 4.3] vrf: Fix memory leak on registration failure in  vrf_newlink() David Ahern <dsa@cumulusnetworks.com> - 2015-12-15 16:20 +0100
    Re: [PATCH 4.3] vrf: Fix memory leak on registration failure in  vrf_newlink() Ben Hutchings <ben@decadent.org.uk> - 2015-12-15 16:30 +0100
    [PATCH 4.3 2/2] vrf: fix double free and memory corruption on  register_netdevice failure Nikolay Aleksandrov <nikolay@cumulusnetworks.com> - 2015-12-15 16:40 +0100
      Re: [PATCH 4.3 2/2] vrf: fix double free and memory corruption on  register_netdevice failure David Ahern <dsa@cumulusnetworks.com> - 2015-12-15 17:00 +0100
      Re: [PATCH 4.3 2/2] vrf: fix double free and memory corruption on  register_netdevice failure Ben Hutchings <ben@decadent.org.uk> - 2015-12-15 18:10 +0100
    [PATCH 4.3 1/2] Revert "vrf: fix double free and memory corruption  on register_netdevice failure" Ben Hutchings <ben@decadent.org.uk> - 2015-12-15 16:40 +0100
      Re: [PATCH 4.3 1/2] Revert "vrf: fix double free and memory  corruption on register_netdevice failure" David Ahern <dsa@cumulusnetworks.com> - 2015-12-15 16:50 +0100

#1292253 — Re: [PATCH 4.3] vrf: Fix memory leak on registration failure in vrf_newlink()

FromDavid Ahern <dsa@cumulusnetworks.com>
Date2015-12-15 16:20 +0100
SubjectRe: [PATCH 4.3] vrf: Fix memory leak on registration failure in vrf_newlink()
Message-ID<qFZyq-5QW-19@gated-at.bofh.it>
On 12/15/15 8:12 AM, Ben Hutchings wrote:
> @@ -598,7 +599,10 @@ static int vrf_newlink(struct net *src_net, struct net_device *dev,
>
>   	rcu_assign_pointer(dev->vrf_ptr, vrf_ptr);
>
> -	return register_netdev(dev);
> +	err = register_netdev(dev);
> +	if (err)
> +		kfree(vrf_ptr);
> +	return err;
>   }
>
>   static size_t vrf_nl_getsize(const struct net_device *dev)
>

The rcu_assign_pointer should only be done if the register_netdev succeeded.

Thanks for creating the patch.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1292260

FromBen Hutchings <ben@decadent.org.uk>
Date2015-12-15 16:30 +0100
Message-ID<qFZI5-5X4-11@gated-at.bofh.it>
In reply to#1292253

[Multipart message — attachments visible in raw view] — view raw

On Tue, 2015-12-15 at 08:15 -0700, David Ahern wrote:
> On 12/15/15 8:12 AM, Ben Hutchings wrote:
> > @@ -598,7 +599,10 @@ static int vrf_newlink(struct net *src_net, struct net_device *dev,
> > 
> >   	rcu_assign_pointer(dev->vrf_ptr, vrf_ptr);
> > 
> > -	return register_netdev(dev);
> > +	err = register_netdev(dev);
> > +	if (err)
> > +		kfree(vrf_ptr);
> > +	return err;
> >   }
> > 
> >   static size_t vrf_nl_getsize(const struct net_device *dev)
> > 
> 
> The rcu_assign_pointer should only be done if the register_netdev succeeded.

Oh, yes I see.  I though that no other task could access an
unregistered device, but register_netdev() can still fail after listing
the device.

Wait, it's worse, this is calling register_netdev() which will deadlock
as we already hold the RTNL lock.

Ben.

> Thanks for creating the patch.
> 
-- 
Ben Hutchings
Logic doesn't apply to the real world. - Marvin Minsky

[toc] | [prev] | [next] | [standalone]

#1292263 — [PATCH 4.3 2/2] vrf: fix double free and memory corruption on register_netdevice failure

FromNikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date2015-12-15 16:40 +0100
Subject[PATCH 4.3 2/2] vrf: fix double free and memory corruption on register_netdevice failure
Message-ID<qFZRM-64B-5@gated-at.bofh.it>
In reply to#1292253

[Multipart message — attachments visible in raw view] — view raw

commit 7f109f7cc37108cba7243bc832988525b0d85909 upstream.

When vrf's ->newlink is called, if register_netdevice() fails then it
does free_netdev(), but that's also done by rtnl_newlink() so a second
free happens and memory gets corrupted, to reproduce execute the
following line a couple of times (1 - 5 usually is enough):
$ for i in `seq 1 5`; do ip link add vrf: type vrf table 1; done;
This works because we fail in register_netdevice() because of the wrong
name "vrf:".

And here's a trace of one crash:
[   28.792157] ------------[ cut here ]------------
[   28.792407] kernel BUG at fs/namei.c:246!
[   28.792608] invalid opcode: 0000 [#1] SMP
[   28.793240] Modules linked in: vrf nfsd auth_rpcgss oid_registry
nfs_acl nfs lockd grace sunrpc crct10dif_pclmul crc32_pclmul
crc32c_intel qxl drm_kms_helper ttm drm aesni_intel aes_x86_64 psmouse
glue_helper lrw evdev gf128mul i2c_piix4 ablk_helper cryptd ppdev
parport_pc parport serio_raw pcspkr virtio_balloon virtio_console
i2c_core acpi_cpufreq button 9pnet_virtio 9p 9pnet fscache ipv6 autofs4
ext4 crc16 mbcache jbd2 virtio_blk virtio_net sg sr_mod cdrom
ata_generic ehci_pci uhci_hcd ehci_hcd e1000 usbcore usb_common ata_piix
libata virtio_pci virtio_ring virtio scsi_mod floppy
[   28.796016] CPU: 0 PID: 1148 Comm: ld-linux-x86-64 Not tainted
4.4.0-rc1+ #24
[   28.796016] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.8.1-20150318_183358- 04/01/2014
[   28.796016] task: ffff8800352561c0 ti: ffff88003592c000 task.ti:
ffff88003592c000
[   28.796016] RIP: 0010:[<ffffffff812187b3>]  [<ffffffff812187b3>]
putname+0x43/0x60
[   28.796016] RSP: 0018:ffff88003592fe88  EFLAGS: 00010246
[   28.796016] RAX: 0000000000000000 RBX: ffff8800352561c0 RCX:
0000000000000001
[   28.796016] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
ffff88003784f000
[   28.796016] RBP: ffff88003592ff08 R08: 0000000000000001 R09:
0000000000000000
[   28.796016] R10: 0000000000000000 R11: 0000000000000001 R12:
0000000000000000
[   28.796016] R13: 000000000000047c R14: ffff88003784f000 R15:
ffff8800358c4a00
[   28.796016] FS:  0000000000000000(0000) GS:ffff88003fc00000(0000)
knlGS:0000000000000000
[   28.796016] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   28.796016] CR2: 00007ffd583bc2d9 CR3: 0000000035a99000 CR4:
00000000000406f0
[   28.796016] Stack:
[   28.796016]  ffffffff8121045d ffffffff812102d3 ffff8800352561c0
ffff880035a91660
[   28.796016]  ffff8800008a9880 0000000000000000 ffffffff81a49940
00ffffff81218684
[   28.796016]  ffff8800352561c0 000000000000047c 0000000000000000
ffff880035b36d80
[   28.796016] Call Trace:
[   28.796016]  [<ffffffff8121045d>] ?
do_execveat_common.isra.34+0x74d/0x930
[   28.796016]  [<ffffffff812102d3>] ?
do_execveat_common.isra.34+0x5c3/0x930
[   28.796016]  [<ffffffff8121066c>] do_execve+0x2c/0x30
[   28.796016]  [<ffffffff810939a0>]
call_usermodehelper_exec_async+0xf0/0x140
[   28.796016]  [<ffffffff810938b0>] ? umh_complete+0x40/0x40
[   28.796016]  [<ffffffff815cb1af>] ret_from_fork+0x3f/0x70
[   28.796016] Code: 48 8d 47 1c 48 89 e5 53 48 8b 37 48 89 fb 48 39 c6
74 1a 48 8b 3d 7e e9 8f 00 e8 49 fa fc ff 48 89 df e8 f1 01 fd ff 5b 5d
f3 c3 <0f> 0b 48 89 fe 48 8b 3d 61 e9 8f 00 e8 2c fa fc ff 5b 5d eb e9
[   28.796016] RIP  [<ffffffff812187b3>] putname+0x43/0x60
[   28.796016]  RSP <ffff88003592fe88>

Fixes: 193125dbd8eb ("net: Introduce VRF device driver")
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Acked-by: David Ahern <dsa@cumulusnetworks.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
[bwh: For 4.3, retain the kfree() on failure]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/net/vrf.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
index 488c6f5..374feba 100644
--- a/drivers/net/vrf.c
+++ b/drivers/net/vrf.c
@@ -608,7 +608,6 @@ static int vrf_newlink(struct net *src_net, struct net_device *dev,
 
 out_fail:
 	kfree(vrf_ptr);
-	free_netdev(dev);
 	return err;
 }

[toc] | [prev] | [next] | [standalone]

#1292305 — Re: [PATCH 4.3 2/2] vrf: fix double free and memory corruption on register_netdevice failure

FromDavid Ahern <dsa@cumulusnetworks.com>
Date2015-12-15 17:00 +0100
SubjectRe: [PATCH 4.3 2/2] vrf: fix double free and memory corruption on register_netdevice failure
Message-ID<qG0b8-6bS-23@gated-at.bofh.it>
In reply to#1292263
On 12/15/15 8:32 AM, Nikolay Aleksandrov wrote:
> diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
> index 488c6f5..374feba 100644
> --- a/drivers/net/vrf.c
> +++ b/drivers/net/vrf.c
> @@ -608,7 +608,6 @@ static int vrf_newlink(struct net *src_net, struct net_device *dev,
>
>   out_fail:
>   	kfree(vrf_ptr);
> -	free_netdev(dev);
>   	return err;
>   }
>

Acked-by: David Ahern <dsa@cumulusnetworks.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1292357 — Re: [PATCH 4.3 2/2] vrf: fix double free and memory corruption on register_netdevice failure

FromBen Hutchings <ben@decadent.org.uk>
Date2015-12-15 18:10 +0100
SubjectRe: [PATCH 4.3 2/2] vrf: fix double free and memory corruption on register_netdevice failure
Message-ID<qG1gS-76S-19@gated-at.bofh.it>
In reply to#1292263

[Multipart message — attachments visible in raw view] — view raw

Sorry, this was from me; I didn't mean to forge Nikolay's address.

Ben.

On Tue, 2015-12-15 at 15:32 +0000, Nikolay Aleksandrov wrote:
> commit 7f109f7cc37108cba7243bc832988525b0d85909 upstream.
> 
> When vrf's ->newlink is called, if register_netdevice() fails then it
> does free_netdev(), but that's also done by rtnl_newlink() so a
> second
> free happens and memory gets corrupted, to reproduce execute the
> following line a couple of times (1 - 5 usually is enough):
> $ for i in `seq 1 5`; do ip link add vrf: type vrf table 1; done;
> This works because we fail in register_netdevice() because of the
> wrong
> name "vrf:".
> 
> And here's a trace of one crash:
> [   28.792157] ------------[ cut here ]------------
> [   28.792407] kernel BUG at fs/namei.c:246!
> [   28.792608] invalid opcode: 0000 [#1] SMP
> [   28.793240] Modules linked in: vrf nfsd auth_rpcgss oid_registry
> nfs_acl nfs lockd grace sunrpc crct10dif_pclmul crc32_pclmul
> crc32c_intel qxl drm_kms_helper ttm drm aesni_intel aes_x86_64
> psmouse
> glue_helper lrw evdev gf128mul i2c_piix4 ablk_helper cryptd ppdev
> parport_pc parport serio_raw pcspkr virtio_balloon virtio_console
> i2c_core acpi_cpufreq button 9pnet_virtio 9p 9pnet fscache ipv6
> autofs4
> ext4 crc16 mbcache jbd2 virtio_blk virtio_net sg sr_mod cdrom
> ata_generic ehci_pci uhci_hcd ehci_hcd e1000 usbcore usb_common
> ata_piix
> libata virtio_pci virtio_ring virtio scsi_mod floppy
> [   28.796016] CPU: 0 PID: 1148 Comm: ld-linux-x86-64 Not tainted
> 4.4.0-rc1+ #24
> [   28.796016] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS 1.8.1-20150318_183358- 04/01/2014
> [   28.796016] task: ffff8800352561c0 ti: ffff88003592c000 task.ti:
> ffff88003592c000
> [   28.796016] RIP: 0010:[<ffffffff812187b3>]  [<ffffffff812187b3>]
> putname+0x43/0x60
> [   28.796016] RSP: 0018:ffff88003592fe88  EFLAGS: 00010246
> [   28.796016] RAX: 0000000000000000 RBX: ffff8800352561c0 RCX:
> 0000000000000001
> [   28.796016] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
> ffff88003784f000
> [   28.796016] RBP: ffff88003592ff08 R08: 0000000000000001 R09:
> 0000000000000000
> [   28.796016] R10: 0000000000000000 R11: 0000000000000001 R12:
> 0000000000000000
> [   28.796016] R13: 000000000000047c R14: ffff88003784f000 R15:
> ffff8800358c4a00
> [   28.796016] FS:  0000000000000000(0000) GS:ffff88003fc00000(0000)
> knlGS:0000000000000000
> [   28.796016] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   28.796016] CR2: 00007ffd583bc2d9 CR3: 0000000035a99000 CR4:
> 00000000000406f0
> [   28.796016] Stack:
> [   28.796016]  ffffffff8121045d ffffffff812102d3 ffff8800352561c0
> ffff880035a91660
> [   28.796016]  ffff8800008a9880 0000000000000000 ffffffff81a49940
> 00ffffff81218684
> [   28.796016]  ffff8800352561c0 000000000000047c 0000000000000000
> ffff880035b36d80
> [   28.796016] Call Trace:
> [   28.796016]  [<ffffffff8121045d>] ?
> do_execveat_common.isra.34+0x74d/0x930
> [   28.796016]  [<ffffffff812102d3>] ?
> do_execveat_common.isra.34+0x5c3/0x930
> [   28.796016]  [<ffffffff8121066c>] do_execve+0x2c/0x30
> [   28.796016]  [<ffffffff810939a0>]
> call_usermodehelper_exec_async+0xf0/0x140
> [   28.796016]  [<ffffffff810938b0>] ? umh_complete+0x40/0x40
> [   28.796016]  [<ffffffff815cb1af>] ret_from_fork+0x3f/0x70
> [   28.796016] Code: 48 8d 47 1c 48 89 e5 53 48 8b 37 48 89 fb 48 39
> c6
> 74 1a 48 8b 3d 7e e9 8f 00 e8 49 fa fc ff 48 89 df e8 f1 01 fd ff 5b
> 5d
> f3 c3 <0f> 0b 48 89 fe 48 8b 3d 61 e9 8f 00 e8 2c fa fc ff 5b 5d eb
> e9
> [   28.796016] RIP  [<ffffffff812187b3>] putname+0x43/0x60
> [   28.796016]  RSP <ffff88003592fe88>
> 
> Fixes: 193125dbd8eb ("net: Introduce VRF device driver")
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> Acked-by: David Ahern <dsa@cumulusnetworks.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> [bwh: For 4.3, retain the kfree() on failure]
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
>  drivers/net/vrf.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
> index 488c6f5..374feba 100644
> --- a/drivers/net/vrf.c
> +++ b/drivers/net/vrf.c
> @@ -608,7 +608,6 @@ static int vrf_newlink(struct net *src_net,
> struct net_device *dev,
>  
>  out_fail:
>  	kfree(vrf_ptr);
> -	free_netdev(dev);
>  	return err;
>  }
>  
-- 
Ben Hutchings
Logic doesn't apply to the real world. - Marvin Minsky

[toc] | [prev] | [next] | [standalone]

#1292270 — [PATCH 4.3 1/2] Revert "vrf: fix double free and memory corruption on register_netdevice failure"

FromBen Hutchings <ben@decadent.org.uk>
Date2015-12-15 16:40 +0100
Subject[PATCH 4.3 1/2] Revert "vrf: fix double free and memory corruption on register_netdevice failure"
Message-ID<qFZRM-64B-25@gated-at.bofh.it>
In reply to#1292253

[Multipart message — attachments visible in raw view] — view raw

This reverts commit b3abad339f8e268bb261e5844ab68b18a7797c29, which
was an attempt to backport commit 7f109f7cc37108cba7243bc832988525b0d85909
upstream.  The backport introduced a deadlock and other bugs.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/net/vrf.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
index c9e309c..488c6f5 100644
--- a/drivers/net/vrf.c
+++ b/drivers/net/vrf.c
@@ -581,6 +581,7 @@ static int vrf_newlink(struct net *src_net, struct net_device *dev,
 {
 	struct net_vrf *vrf = netdev_priv(dev);
 	struct net_vrf_dev *vrf_ptr;
+	int err;
 
 	if (!data || !data[IFLA_VRF_TABLE])
 		return -EINVAL;
@@ -589,16 +590,26 @@ static int vrf_newlink(struct net *src_net, struct net_device *dev,
 
 	dev->priv_flags |= IFF_VRF_MASTER;
 
+	err = -ENOMEM;
 	vrf_ptr = kmalloc(sizeof(*dev->vrf_ptr), GFP_KERNEL);
 	if (!vrf_ptr)
-		return -ENOMEM;
+		goto out_fail;
 
 	vrf_ptr->ifindex = dev->ifindex;
 	vrf_ptr->tb_id = vrf->tb_id;
 
+	err = register_netdevice(dev);
+	if (err < 0)
+		goto out_fail;
+
 	rcu_assign_pointer(dev->vrf_ptr, vrf_ptr);
 
-	return register_netdev(dev);
+	return 0;
+
+out_fail:
+	kfree(vrf_ptr);
+	free_netdev(dev);
+	return err;
 }
 
 static size_t vrf_nl_getsize(const struct net_device *dev)

[toc] | [prev] | [next] | [standalone]

#1292286 — Re: [PATCH 4.3 1/2] Revert "vrf: fix double free and memory corruption on register_netdevice failure"

FromDavid Ahern <dsa@cumulusnetworks.com>
Date2015-12-15 16:50 +0100
SubjectRe: [PATCH 4.3 1/2] Revert "vrf: fix double free and memory corruption on register_netdevice failure"
Message-ID<qG01s-68o-23@gated-at.bofh.it>
In reply to#1292270
On 12/15/15 8:31 AM, Ben Hutchings wrote:
> This reverts commit b3abad339f8e268bb261e5844ab68b18a7797c29, which
> was an attempt to backport commit 7f109f7cc37108cba7243bc832988525b0d85909
> upstream.  The backport introduced a deadlock and other bugs.
>
> Signed-off-by: Ben Hutchings<ben@decadent.org.uk>
> ---

Best course of action -- revert the old and do over.

Acked-by: David Ahern <dsa@cumulusnetworks.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web