[PATCH] net: remove the unnecessary list_del

[PATCH] net: remove the unnecessary list_del

[Gao feng]

These lists are used by unregister_netdevice_many,
they are local variables,will not be seen by others.
there is no need to delete them.

Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
---
 drivers/net/macvlan.c | 1 -
 net/core/dev.c        | 1 -
 net/core/rtnetlink.c  | 1 -
 net/mac80211/iface.c  | 1 -
 4 files changed, 4 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index defcd8a..9b728f1 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -945,7 +945,6 @@ static int macvlan_device_event(struct notifier_block *unused,
 		list_for_each_entry_safe(vlan, next, &port->vlans, list)
 			vlan->dev->rtnl_link_ops->dellink(vlan->dev, &list_kill);
 		unregister_netdevice_many(&list_kill);
-		list_del(&list_kill);
 		break;
 	case NETDEV_PRE_TYPE_CHANGE:
 		/* Forbid underlaying device to change its type. */
diff --git a/net/core/dev.c b/net/core/dev.c
index 18d8b5a..7c5e9d2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6172,7 +6172,6 @@ static void __net_exit default_device_exit_batch(struct list_head *net_list)
 		}
 	}
 	unregister_netdevice_many(&dev_kill_list);
-	list_del(&dev_kill_list);
 	rtnl_unlock();
 }
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d8aa20f..8e35210 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1613,7 +1613,6 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
 
 	ops->dellink(dev, &list_kill);
 	unregister_netdevice_many(&list_kill);
-	list_del(&list_kill);
 	return 0;
 }
 
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 2c059e5..a799629 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1639,7 +1639,6 @@ void ieee80211_remove_interfaces(struct ieee80211_local *local)
 	}
 	mutex_unlock(&local->iflist_mtx);
 	unregister_netdevice_many(&unreg_list);
-	list_del(&unreg_list);
 
 	list_for_each_entry_safe(sdata, tmp, &wdev_list, list) {
 		list_del(&sdata->list);
-- 
1.7.11.7

Re: [PATCH] net: remove the unnecessary list_del

[David Miller]

From: Gao feng <gaofeng@cn.fujitsu.com>
Date: Mon, 25 Feb 2013 12:41:56 +0800

> These lists are used by unregister_netdevice_many,
> they are local variables,will not be seen by others.
> there is no need to delete them.
> 
> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>

What about the devices on the list?  The ones at the front and the
rear of the list will have list pointers that point into no longer
valid stack frame locations.

Re: [PATCH] net: remove the unnecessary list_del

[Gao feng]

On 2013/02/25 12:54, David Miller wrote:
> From: Gao feng <gaofeng@cn.fujitsu.com>
> Date: Mon, 25 Feb 2013 12:41:56 +0800
> 
>> These lists are used by unregister_netdevice_many,
>> they are local variables,will not be seen by others.
>> there is no need to delete them.
>>
>> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
> 
> What about the devices on the list?  The ones at the front and the
> rear of the list will have list pointers that point into no longer
> valid stack frame locations.

These lists are only used by unregister_netdevice_many,
we don't access these lists again after unregister_netdevice_many,
so I think it doesn't have invalid stack frame locations problems.

Re: [PATCH] net: remove the unnecessary list_del

[David Miller]

From: Gao feng <gaofeng@cn.fujitsu.com>
Date: Mon, 25 Feb 2013 13:03:04 +0800

> On 2013/02/25 12:54, David Miller wrote:
>> From: Gao feng <gaofeng@cn.fujitsu.com>
>> Date: Mon, 25 Feb 2013 12:41:56 +0800
>> 
>>> These lists are used by unregister_netdevice_many,
>>> they are local variables,will not be seen by others.
>>> there is no need to delete them.
>>>
>>> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
>> 
>> What about the devices on the list?  The ones at the front and the
>> rear of the list will have list pointers that point into no longer
>> valid stack frame locations.
> 
> These lists are only used by unregister_netdevice_many,
> we don't access these lists again after unregister_netdevice_many,
> so I think it doesn't have invalid stack frame locations problems.

I do not see unregister_netdevice_many() list_del()'ing the devices
on the list, therefore those netdevice objects have pointers into
the stale stack frame.

You cannot make this change.

Re: [PATCH] net: remove the unnecessary list_del

[Gao feng]

On 2013/02/25 13:39, David Miller wrote:
> From: Gao feng <gaofeng@cn.fujitsu.com>
> Date: Mon, 25 Feb 2013 13:03:04 +0800
> 
>> On 2013/02/25 12:54, David Miller wrote:
>>> From: Gao feng <gaofeng@cn.fujitsu.com>
>>> Date: Mon, 25 Feb 2013 12:41:56 +0800
>>>
>>>> These lists are used by unregister_netdevice_many,
>>>> they are local variables,will not be seen by others.
>>>> there is no need to delete them.
>>>>
>>>> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
>>>
>>> What about the devices on the list?  The ones at the front and the
>>> rear of the list will have list pointers that point into no longer
>>> valid stack frame locations.
>>
>> These lists are only used by unregister_netdevice_many,
>> we don't access these lists again after unregister_netdevice_many,
>> so I think it doesn't have invalid stack frame locations problems.
> 
> I do not see unregister_netdevice_many() list_del()'ing the devices
> on the list, therefore those netdevice objects have pointers into
> the stale stack frame.
> 

Yes, I agree that this will make the netdevice objects have pointers
into the stale stack frame.

I check the codes,and found there are tons of codes that don't call
list_del after unregister_netdevice_many(). such as mroute_clean_tables,
ip6_tnl_destroy_tunnels...

I think it's better to make sure netdevice objects have valid pointers
by calling list_del in unregister_netdevice_many.

> You cannot make this change.
> 

Will send another patch.

Thanks

Re: [PATCH] net: remove the unnecessary list_del

[David Miller]

From: Gao feng <gaofeng@cn.fujitsu.com>
Date: Mon, 25 Feb 2013 16:01:24 +0800

> I think it's better to make sure netdevice objects have valid pointers
> by calling list_del in unregister_netdevice_many.

Please, for the sake of understanding, read the commit below.  And
please make such necessary research in the future and reference any
such commits in your proposed patch, as well as explaining why those
commits in the past were wrong and why you're reverting of them is
correct.

Just saying "this isn't necessary" is a very disappointing explanation
for something as very non-trivial as this is.

Thank you.

commit ceaaec98ad99859ac90ac6863ad0a6cd075d8e0e
Author: Eric Dumazet <eric.dumazet@gmail.com>
Date:   Thu Feb 17 22:59:19 2011 +0000

    net: deinit automatic LIST_HEAD
    
    commit 9b5e383c11b08784 (net: Introduce
    unregister_netdevice_many()) left an active LIST_HEAD() in
    rollback_registered(), with possible memory corruption.
    
    Even if device is freed without touching its unreg_list (and therefore
    touching the previous memory location holding LISTE_HEAD(single), better
    close the bug for good, since its really subtle.
    
    (Same fix for default_device_exit_batch() for completeness)
    
    Reported-by: Michal Hocko <mhocko@suse.cz>
    Tested-by: Michal Hocko <mhocko@suse.cz>
    Reported-by: Eric W. Biderman <ebiderman@xmission.com>
    Tested-by: Eric W. Biderman <ebiderman@xmission.com>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
    Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
    CC: Ingo Molnar <mingo@elte.hu>
    CC: Octavian Purdila <opurdila@ixiacom.com>
    CC: stable <stable@kernel.org> [.33+]
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/core/dev.c b/net/core/dev.c
index a18c164..8ae6631 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5066,6 +5066,7 @@ static void rollback_registered(struct net_device *dev)
 
 	list_add(&dev->unreg_list, &single);
 	rollback_registered_many(&single);
+	list_del(&single);
 }
 
 unsigned long netdev_fix_features(unsigned long features, const char *name)
@@ -6219,6 +6220,7 @@ static void __net_exit default_device_exit_batch(struct list_head *net_list)
 		}
 	}
 	unregister_netdevice_many(&dev_kill_list);
+	list_del(&dev_kill_list);
 	rtnl_unlock();
 }
 

Re: [PATCH] net: remove the unnecessary list_del

[Gao feng]

On 2013/02/25 16:12, David Miller wrote:
> From: Gao feng <gaofeng@cn.fujitsu.com>
> Date: Mon, 25 Feb 2013 16:01:24 +0800
> 
>> I think it's better to make sure netdevice objects have valid pointers
>> by calling list_del in unregister_netdevice_many.
> 
> Please, for the sake of understanding, read the commit below.  And
> please make such necessary research in the future and reference any
> such commits in your proposed patch, as well as explaining why those
> commits in the past were wrong and why you're reverting of them is
> correct.
> 
> Just saying "this isn't necessary" is a very disappointing explanation
> for something as very non-trivial as this is.

Get it, sorry for the noise :(
Will do more research before making patch.

Thanks, the commit below is very useful.

> 
> Thank you.
> 
> commit ceaaec98ad99859ac90ac6863ad0a6cd075d8e0e
> Author: Eric Dumazet <eric.dumazet@gmail.com>
> Date:   Thu Feb 17 22:59:19 2011 +0000
> 
>     net: deinit automatic LIST_HEAD
>     
>     commit 9b5e383c11b08784 (net: Introduce
>     unregister_netdevice_many()) left an active LIST_HEAD() in
>     rollback_registered(), with possible memory corruption.
>     
>     Even if device is freed without touching its unreg_list (and therefore
>     touching the previous memory location holding LISTE_HEAD(single), better
>     close the bug for good, since its really subtle.
>     
>     (Same fix for default_device_exit_batch() for completeness)
>     
>     Reported-by: Michal Hocko <mhocko@suse.cz>
>     Tested-by: Michal Hocko <mhocko@suse.cz>
>     Reported-by: Eric W. Biderman <ebiderman@xmission.com>
>     Tested-by: Eric W. Biderman <ebiderman@xmission.com>
>     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>     Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>     CC: Ingo Molnar <mingo@elte.hu>
>     CC: Octavian Purdila <opurdila@ixiacom.com>
>     CC: stable <stable@kernel.org> [.33+]
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index a18c164..8ae6631 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5066,6 +5066,7 @@ static void rollback_registered(struct net_device *dev)
>  
>  	list_add(&dev->unreg_list, &single);
>  	rollback_registered_many(&single);
> +	list_del(&single);
>  }
>  
>  unsigned long netdev_fix_features(unsigned long features, const char *name)
> @@ -6219,6 +6220,7 @@ static void __net_exit default_device_exit_batch(struct list_head *net_list)
>  		}
>  	}
>  	unregister_netdevice_many(&dev_kill_list);
> +	list_del(&dev_kill_list);
>  	rtnl_unlock();
>  }
>  
> --
> 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
>