netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH net-next] net: Separate the close_list and the unreg_list
@ 2013-10-05 13:18 Francesco Ruggeri
  2013-10-06  2:13 ` Eric W. Biederman
  2013-10-06  2:26 ` [PATCH] net: Separate the close_list and the unreg_list v2 Eric W. Biederman
  0 siblings, 2 replies; 8+ messages in thread
From: Francesco Ruggeri @ 2013-10-05 13:18 UTC (permalink / raw)
  To: fruggeri, netdev, ebiederm

Hi Eric,
I am running some more extensive tests with this patch and I ran into the crash below.

It may be caused by

@@ -1301,7 +1301,7 @@ int dev_close(struct net_device *dev)
 	if (dev->flags & IFF_UP) {
 		LIST_HEAD(single);
 
-		list_add(&dev->unreg_list, &single);
+		list_add(&dev->close_list, &single);
 		dev_close_many(&single);
 		list_del(&single);
 	}

dev_close_many expects net_devices to be linked by unreg_list.
Let me know what you think.

Thanks,
Francesco

<1>BUG: unable to handle kernel NULL pointer dereference at 0000000000000190
<1>IP: [<ffffffff8142c709>] packet_notifier+0x1e/0x163
<4>PGD 94f815067 PUD 0 
<4>Oops: 0000 [#1] SMP 
<4>CPU 1 
<4>Modules linked in: dummy loop tulip veth xt_tcpudp iptable_filter nfsd lockd nfs_acl auth_rpcgss exportfs sunrpc cpufreq_ondemand acpi_cpufreq freq_table mperf macvlan dm_mirror dm_region_hash dm_log uinput ip6table_filter ip6_tables bonding kvm_intel kvm fuse xt_multiport iptable_nat ip_tables nf_nat x_tables nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 tun 8021q coretemp hwmon crc32c_intel ghash_clmulni_intel sg joydev iTCO_wdt iTCO_vendor_support pcspkr i2c_i801 microcode i2c_core ioatdma dca raid1 aesni_intel cryptd aes_x86_64 aes_generic isci libsas scsi_transport_sas ehci_hcd igb wmi dm_mod [last unloaded: scsi_wait_scan]
<4>
<4>Pid: 14572, comm: Ira Not tainted 3.4.43-1479218.2011fruggeriArora.1.fc14.x86_64 #1 Supermicro X9DRT/X9DRT
<4>RIP: 0010:[<ffffffff8142c709>]  [<ffffffff8142c709>] packet_notifier+0x1e/0x163
<4>RSP: 0018:ffff881006c1f668  EFLAGS: 00210292
<4>RAX: 0000000000000000 RBX: ffff88108dec1810 RCX: ffffffff81a6df10
<4>RDX: ffff88108dec1810 RSI: 0000000000000009 RDI: ffffffff81a6df10
<4>RBP: ffff881006c1f698 R08: 0000000000000000 R09: 0000000000000003
<4>R10: 0000000000000003 R11: 0000000000000000 R12: ffff88108dec1810
<4>R13: 00000000fffffff3 R14: ffffffffa0207050 R15: 0000000000000009
<4>FS:  0000000000000000(0000) GS:ffff88103fc20000(0063) knlGS:00000000f5630b00
<4>CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
<4>CR2: 0000000000000190 CR3: 0000000b405bb000 CR4: 00000000000007e0
<4>DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
<4>DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
<4>Process Ira (pid: 14572, threadinfo ffff881006c1e000, task ffff88094f8d8da0)
<4>Stack:
<4> ffff881006c1f698 ffff88108dec1810 0000000000000009 00000000fffffff3
<4> ffffffffa0207050 ffffffffa02f2060 ffff881006c1f6d8 ffffffff814513a5
<4> ffffea000a276500 0000000000000000 0000000000000009 ffff88108dec1810
<4>Call Trace:
<4> [<ffffffff814513a5>] notifier_call_chain+0x32/0x5e
<4> [<ffffffff8104ec06>] raw_notifier_call_chain+0xf/0x11
<4> [<ffffffff813848f6>] call_netdevice_notifiers+0x45/0x4a
<4> [<ffffffff8138493d>] __dev_close_many+0x42/0xbc
<4> [<ffffffff81384a67>] dev_close_many+0x6e/0xf8
<4> [<ffffffff81384b2b>] dev_close+0x3a/0x4d
<4> [<ffffffff81386b54>] dev_change_net_namespace+0xa4/0x1a0
<4> [<ffffffff8139249e>] do_setlink+0x77/0x78b
<4> [<ffffffff810cb555>] ? pmd_offset+0x14/0x3b
<4> [<ffffffff8144e0b9>] ? _raw_spin_lock+0x9/0xb
<4> [<ffffffff810f8317>] ? full_name_hash+0x19/0x5c
<4> [<ffffffff8138661c>] ? dev_name_hash.clone.50+0x24/0x3c
<4> [<ffffffff81393a9b>] rtnl_newlink+0x24d/0x49a
<4> [<ffffffff8139390d>] ? rtnl_newlink+0xbf/0x49a
<4> [<ffffffff8144e0e9>] ? _raw_spin_unlock_irqrestore+0x12/0x14
<4> [<ffffffff810e7c3b>] ? virt_to_head_page+0x9/0x2c
<4> [<ffffffff8139364f>] rtnetlink_rcv_msg+0x24c/0x262
<4> [<ffffffff81393403>] ? __rtnl_unlock+0x12/0x12
<4> [<ffffffff813a7e44>] netlink_rcv_skb+0x40/0x8c
<4> [<ffffffff81392d80>] rtnetlink_rcv+0x21/0x28
<4> [<ffffffff813a795b>] netlink_unicast+0xec/0x155
<4> [<ffffffff813a7c4c>] netlink_sendmsg+0x288/0x2a6
<4> [<ffffffff8137414a>] __sock_sendmsg+0x6e/0x7a
<4> [<ffffffff81374a29>] sock_sendmsg+0xa3/0xbc
<4> [<ffffffff810b195a>] ? generic_file_aio_write+0xa8/0xb8
<4> [<ffffffff811665b9>] ? ext4_file_write+0x1eb/0x240
<4> [<ffffffff810f1247>] ? fget_light+0x33/0x83
<4> [<ffffffff81374aaa>] ? sockfd_lookup_light+0x1b/0x53
<4> [<ffffffff81375f7b>] sys_sendto+0x12a/0x16c
<4> [<ffffffff810ef8a3>] ? fsnotify_modify+0x5d/0x65
<4> [<ffffffff81375fcc>] sys_send+0xf/0x11
<4> [<ffffffff8139cc9c>] compat_sys_socketcall+0xd7/0x19b
<4> [<ffffffff81455bd6>] sysenter_dispatch+0x7/0x21
<4>Code: ff ff 80 8b 48 04 00 00 01 5b 5b c9 c3 55 48 89 e5 41 57 49 89 f7 41 56 41 55 41 54 49 89 d4 53 48 83 ec 08 48 8b 82 48 04 00 00 <4c> 8b b0 90 01 00 00 e9 06 01 00 00 4c 8b ab 58 04 00 00 4d 85 
<1>RIP  [<ffffffff8142c709>] packet_notifier+0x1e/0x163
<4> RSP <ffff881006c1f668>
<4>CR2: 0000000000000190

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH net-next] net: Separate the close_list and the unreg_list
@ 2013-10-06 17:13 Francesco Ruggeri
  2013-10-10  0:22 ` Francesco Ruggeri
  0 siblings, 1 reply; 8+ messages in thread
From: Francesco Ruggeri @ 2013-10-06 17:13 UTC (permalink / raw)
  To: Eric W. Biederman; +Cc: netdev

>
> Can you verify this incremental change fixes it for you?
>

Sure, I will.
Thanks,

Francesco

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] net: Separate the close_list and the unreg_list
@ 2013-10-03 21:51 Francesco Ruggeri
  2013-10-03 21:53 ` David Miller
  0 siblings, 1 reply; 8+ messages in thread
From: Francesco Ruggeri @ 2013-10-03 21:51 UTC (permalink / raw)
  To: netdev
  Cc: David S. Miller, Eric Dumazet, Jiri Pirko, Alexander Duyck,
	Cong Wang, Eric W. Biederman

This patch from Eric may have fallen through the cracks.
At the time he submitted it I did verify it in some failures I was
seeing (I have not run extensive tests).

Francesco


On Mon, Sep 16, 2013 at 4:57 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> Separate the unreg_list and the close_list in dev_close_many preventing
> dev_close_many from permuting the unreg_list.  The permutations of the
> unreg_list have resulted in cases where the loopback device is accessed
> it has been freed in code such as dst_ifdown.  Resulting in subtle
> memory corruption.
>
> This is the second bug from sharing the storage between the close_list
> and the unreg_list.  The issues that crop up with sharing are apparently
> too subtle to show up in normal testing or usage, so let's forget about
> being clever and use two separate lists.
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>
> Frencesco if you could test this patch and verify it fixes some of your
> issues that would be great.  I don't expect this fixes the fundamental
> problem you were seeing but it should solve some related issues.
>
>  include/linux/netdevice.h |    1 +
>  net/core/dev.c            |   25 +++++++++++++------------
>  net/sched/sch_generic.c   |    6 +++---
>  3 files changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 0741a1e..7858bfc 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1106,6 +1106,7 @@ struct net_device {
>         struct list_head        dev_list;
>         struct list_head        napi_list;
>         struct list_head        unreg_list;
> +       struct list_head        close_list;
>         struct list_head        upper_dev_list; /* List of upper devices */
>
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index a3d8d44..5d702fe 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1307,7 +1307,7 @@ static int __dev_close_many(struct list_head *head)
>         ASSERT_RTNL();
>         might_sleep();
>
> -       list_for_each_entry(dev, head, unreg_list) {
> +       list_for_each_entry(dev, head, close_list) {
>                 call_netdevice_notifiers(NETDEV_GOING_DOWN, dev);
>
>                 clear_bit(__LINK_STATE_START, &dev->state);
> @@ -1323,7 +1323,7 @@ static int __dev_close_many(struct list_head *head)
>
>         dev_deactivate_many(head);
>
> -       list_for_each_entry(dev, head, unreg_list) {
> +       list_for_each_entry(dev, head, close_list) {
>                 const struct net_device_ops *ops = dev->netdev_ops;
>
>                 /*
> @@ -1351,7 +1351,7 @@ static int __dev_close(struct net_device *dev)
>         /* Temporarily disable netpoll until the interface is down */
>         netpoll_rx_disable(dev);
>
> -       list_add(&dev->unreg_list, &single);
> +       list_add(&dev->close_list, &single);
>         retval = __dev_close_many(&single);
>         list_del(&single);
>
> @@ -1362,21 +1362,21 @@ static int __dev_close(struct net_device *dev)
>  static int dev_close_many(struct list_head *head)
>  {
>         struct net_device *dev, *tmp;
> -       LIST_HEAD(tmp_list);
> +       LIST_HEAD(many);
>
> -       list_for_each_entry_safe(dev, tmp, head, unreg_list)
> -               if (!(dev->flags & IFF_UP))
> -                       list_move(&dev->unreg_list, &tmp_list);
> +       /* rollback_registered_many needs the original unmodified list */
> +       list_for_each_entry(dev, head, unreg_list)
> +               if (dev->flags & IFF_UP)
> +                       list_add_tail(&dev->close_list, &many);
>
> -       __dev_close_many(head);
> +       __dev_close_many(&many);
>
> -       list_for_each_entry(dev, head, unreg_list) {
> +       list_for_each_entry_safe(dev, tmp, &many, close_list) {
>                 rtmsg_ifinfo(RTM_NEWLINK, dev, IFF_UP|IFF_RUNNING);
>                 call_netdevice_notifiers(NETDEV_DOWN, dev);
> +               list_del_init(&dev->close_list);
>         }
>
> -       /* rollback_registered_many needs the complete original list */
> -       list_splice(&tmp_list, head);
>         return 0;
>  }
>
> @@ -1397,7 +1397,7 @@ int dev_close(struct net_device *dev)
>                 /* Block netpoll rx while the interface is going down */
>                 netpoll_rx_disable(dev);
>
> -               list_add(&dev->unreg_list, &single);
> +               list_add(&dev->close_list, &single);
>                 dev_close_many(&single);
>                 list_del(&single);
>
> @@ -5823,6 +5823,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
>
>         INIT_LIST_HEAD(&dev->napi_list);
>         INIT_LIST_HEAD(&dev->unreg_list);
> +       INIT_LIST_HEAD(&dev->close_list);
>         INIT_LIST_HEAD(&dev->link_watch_list);
>         INIT_LIST_HEAD(&dev->upper_dev_list);
>         dev->priv_flags = IFF_XMIT_DST_RELEASE;
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 4626cef..dee62f4 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -818,7 +818,7 @@ void dev_deactivate_many(struct list_head *head)
>         struct net_device *dev;
>         bool sync_needed = false;
>
> -       list_for_each_entry(dev, head, unreg_list) {
> +       list_for_each_entry(dev, head, close_list) {
>                 netdev_for_each_tx_queue(dev, dev_deactivate_queue,
>                                          &noop_qdisc);
>                 if (dev_ingress_queue(dev))
> @@ -837,7 +837,7 @@ void dev_deactivate_many(struct list_head *head)
>                 synchronize_net();
>
>         /* Wait for outstanding qdisc_run calls. */
> -       list_for_each_entry(dev, head, unreg_list)
> +       list_for_each_entry(dev, head, close_list)
>                 while (some_qdisc_is_busy(dev))
>                         yield();
>  }
> @@ -846,7 +846,7 @@ void dev_deactivate(struct net_device *dev)
>  {
>         LIST_HEAD(single);
>
> -       list_add(&dev->unreg_list, &single);
> +       list_add(&dev->close_list, &single);
>         dev_deactivate_many(&single);
>         list_del(&single);
>  }
> --
> 1.7.5.4
>

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-10-10  0:23 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-05 13:18 [PATCH net-next] net: Separate the close_list and the unreg_list Francesco Ruggeri
2013-10-06  2:13 ` Eric W. Biederman
2013-10-06  2:26 ` [PATCH] net: Separate the close_list and the unreg_list v2 Eric W. Biederman
2013-10-07 19:22   ` David Miller
2013-10-07 22:45     ` Eric W. Biederman
  -- strict thread matches above, loose matches on Subject: below --
2013-10-06 17:13 [PATCH net-next] net: Separate the close_list and the unreg_list Francesco Ruggeri
2013-10-10  0:22 ` Francesco Ruggeri
2013-10-03 21:51 [PATCH] " Francesco Ruggeri
2013-10-03 21:53 ` David Miller
2013-10-04  9:34   ` [PATCH net-next] " Eric W. Biederman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).