| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+HUmGgJYeCj-wod=FsWJuZXQN0Kgbwiv+7iY7r+0BCOdqTDvg@mail.gmail.com>
Date: Thu, 3 Oct 2013 14:51:34 -0700
From: Francesco Ruggeri <fruggeri@...stanetworks.com>
To: netdev@...r.kernel.org
Cc: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jiri Pirko <jiri@...nulli.us>,
Alexander Duyck <alexander.h.duyck@...el.com>,
Cong Wang <amwang@...hat.com>,
"Eric W. Biederman" <ebiederm@...ssion.com>
Subject: Re: [PATCH] net: Separate the close_list and the unreg_list
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@...ssion.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@...r.kernel.org>
> Signed-off-by: "Eric W. Biederman" <ebiederm@...ssion.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
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists