[SRU][Xenial][Yakkety][Zesty][Artful][PATCH 0/1] bonding: avoid NETDEV_CHANGEMTU event when unregistering slave

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU][Xenial][Yakkety][Zesty][Artful][PATCH 0/1] bonding: avoid NETDEV_CHANGEMTU event when unregistering slave

Joseph Salisbury-3
BugLink: http://bugs.launchpad.net/bugs/1704102

== SRU Justification ==
A stack dump happens when unregistering a netdev.  The specific reproducer
is listed in the bug description.  

This bug is fixed by mainline commit f51048c3e07b68c90b21a77541fc4b208f9244d7 and is a clean cherry pick.  

Commit f51048c3e07b68c90b21a77541fc4b208f9244d7  is in mainline, but not tagged for a specific release as of yet.

== Fix ==
commit f51048c3e07b68c90b21a77541fc4b208f9244d7
Author: WANG Cong <[hidden email]>
Date:   Thu Jul 6 15:01:57 2017 -0700

    bonding: avoid NETDEV_CHANGEMTU event when unregistering slave

== Regression Potential ==
This fix is needed in all Ubuntu releases and needed in the networking subsystem.  

== Test Case ==
A test kernel was built with this patch and tested by the original bug reporter.
The bug reporter states the test kernel resolved the bug.

WANG Cong (1):
  bonding: avoid NETDEV_CHANGEMTU event when unregistering slave

 drivers/net/bonding/bond_main.c | 15 +++++++++------
 include/linux/netdevice.h       |  1 +
 net/core/dev.c                  |  3 ++-
 3 files changed, 12 insertions(+), 7 deletions(-)

--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][Xenial][Yakkety][Zesty][Artful][PATCH 1/1] bonding: avoid NETDEV_CHANGEMTU event when unregistering slave

Joseph Salisbury-3
From: WANG Cong <[hidden email]>

BugLink: http://bugs.launchpad.net/bugs/1704102

As Hongjun/Nicolas summarized in their original patch:

"
When a device changes from one netns to another, it's first unregistered,
then the netns reference is updated and the dev is registered in the new
netns. Thus, when a slave moves to another netns, it is first
unregistered. This triggers a NETDEV_UNREGISTER event which is caught by
the bonding driver. The driver calls bond_release(), which calls
dev_set_mtu() and thus triggers NETDEV_CHANGEMTU (the device is still in
the old netns).
"

This is a very special case, because the device is being unregistered
no one should still care about the NETDEV_CHANGEMTU event triggered
at this point, we can avoid broadcasting this event on this path,
and avoid touching inetdev_event()/addrconf_notify() path.

It requires to export __dev_set_mtu() to bonding driver.

Reported-by: Hongjun Li <[hidden email]>
Reported-by: Nicolas Dichtel <[hidden email]>
Cc: Jay Vosburgh <[hidden email]>
Cc: Veaceslav Falico <[hidden email]>
Cc: Andy Gospodarek <[hidden email]>
Signed-off-by: Cong Wang <[hidden email]>
Signed-off-by: David S. Miller <[hidden email]>
(cherry picked from commit f51048c3e07b68c90b21a77541fc4b208f9244d7)
Signed-off-by: Joseph Salisbury <[hidden email]>
---
 drivers/net/bonding/bond_main.c | 15 +++++++++------
 include/linux/netdevice.h       |  1 +
 net/core/dev.c                  |  3 ++-
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 34481c9..fd92e89 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1814,7 +1814,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
  */
 static int __bond_release_one(struct net_device *bond_dev,
       struct net_device *slave_dev,
-      bool all)
+      bool all, bool unregister)
 {
  struct bonding *bond = netdev_priv(bond_dev);
  struct slave *slave, *oldcurrent;
@@ -1958,7 +1958,10 @@ static int __bond_release_one(struct net_device *bond_dev,
  dev_set_mac_address(slave_dev, &addr);
  }
 
- dev_set_mtu(slave_dev, slave->original_mtu);
+ if (unregister)
+ __dev_set_mtu(slave_dev, slave->original_mtu);
+ else
+ dev_set_mtu(slave_dev, slave->original_mtu);
 
  slave_dev->priv_flags &= ~IFF_BONDING;
 
@@ -1970,7 +1973,7 @@ static int __bond_release_one(struct net_device *bond_dev,
 /* A wrapper used because of ndo_del_link */
 int bond_release(struct net_device *bond_dev, struct net_device *slave_dev)
 {
- return __bond_release_one(bond_dev, slave_dev, false);
+ return __bond_release_one(bond_dev, slave_dev, false, false);
 }
 
 /* First release a slave and then destroy the bond if no more slaves are left.
@@ -1982,7 +1985,7 @@ static int  bond_release_and_destroy(struct net_device *bond_dev,
  struct bonding *bond = netdev_priv(bond_dev);
  int ret;
 
- ret = bond_release(bond_dev, slave_dev);
+ ret = __bond_release_one(bond_dev, slave_dev, false, true);
  if (ret == 0 && !bond_has_slaves(bond)) {
  bond_dev->priv_flags |= IFF_DISABLE_NETPOLL;
  netdev_info(bond_dev, "Destroying bond %s\n",
@@ -3032,7 +3035,7 @@ static int bond_slave_netdev_event(unsigned long event,
  if (bond_dev->type != ARPHRD_ETHER)
  bond_release_and_destroy(bond_dev, slave_dev);
  else
- bond_release(bond_dev, slave_dev);
+ __bond_release_one(bond_dev, slave_dev, false, true);
  break;
  case NETDEV_UP:
  case NETDEV_CHANGE:
@@ -4228,7 +4231,7 @@ static void bond_uninit(struct net_device *bond_dev)
 
  /* Release the bonded slaves */
  bond_for_each_slave(bond, slave, iter)
- __bond_release_one(bond_dev, slave->dev, true);
+ __bond_release_one(bond_dev, slave->dev, true, true);
  netdev_info(bond_dev, "Released all slaves\n");
 
  arr = rtnl_dereference(bond->slave_arr);
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 97456b25..a8d97f0 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3269,6 +3269,7 @@ void __dev_notify_flags(struct net_device *, unsigned int old_flags,
 int dev_change_name(struct net_device *, const char *);
 int dev_set_alias(struct net_device *, const char *, size_t);
 int dev_change_net_namespace(struct net_device *, struct net *, const char *);
+int __dev_set_mtu(struct net_device *, int);
 int dev_set_mtu(struct net_device *, int);
 void dev_set_group(struct net_device *, int);
 int dev_set_mac_address(struct net_device *, struct sockaddr *);
diff --git a/net/core/dev.c b/net/core/dev.c
index 9b58753..8804076 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6535,7 +6535,7 @@ int dev_change_flags(struct net_device *dev, unsigned int flags)
 }
 EXPORT_SYMBOL(dev_change_flags);
 
-static int __dev_set_mtu(struct net_device *dev, int new_mtu)
+int __dev_set_mtu(struct net_device *dev, int new_mtu)
 {
  const struct net_device_ops *ops = dev->netdev_ops;
 
@@ -6545,6 +6545,7 @@ static int __dev_set_mtu(struct net_device *dev, int new_mtu)
  dev->mtu = new_mtu;
  return 0;
 }
+EXPORT_SYMBOL(__dev_set_mtu);
 
 /**
  * dev_set_mtu - Change maximum transfer unit
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK/cmnt: [SRU][Xenial][Yakkety][Zesty][Artful][PATCH 1/1] bonding: avoid NETDEV_CHANGEMTU event when unregistering slave

Stefan Bader-2
On 14.07.2017 22:58, Joseph Salisbury wrote:

> From: WANG Cong <[hidden email]>
>
> BugLink: http://bugs.launchpad.net/bugs/1704102
>
> As Hongjun/Nicolas summarized in their original patch:
>
> "
> When a device changes from one netns to another, it's first unregistered,
> then the netns reference is updated and the dev is registered in the new
> netns. Thus, when a slave moves to another netns, it is first
> unregistered. This triggers a NETDEV_UNREGISTER event which is caught by
> the bonding driver. The driver calls bond_release(), which calls
> dev_set_mtu() and thus triggers NETDEV_CHANGEMTU (the device is still in
> the old netns).
> "
>
> This is a very special case, because the device is being unregistered
> no one should still care about the NETDEV_CHANGEMTU event triggered
> at this point, we can avoid broadcasting this event on this path,
> and avoid touching inetdev_event()/addrconf_notify() path.
>
> It requires to export __dev_set_mtu() to bonding driver.
>
> Reported-by: Hongjun Li <[hidden email]>
> Reported-by: Nicolas Dichtel <[hidden email]>
> Cc: Jay Vosburgh <[hidden email]>
> Cc: Veaceslav Falico <[hidden email]>
> Cc: Andy Gospodarek <[hidden email]>
> Signed-off-by: Cong Wang <[hidden email]>
> Signed-off-by: David S. Miller <[hidden email]>
> (cherry picked from commit f51048c3e07b68c90b21a77541fc4b208f9244d7)
> Signed-off-by: Joseph Salisbury <[hidden email]>
Acked-by: Stefan Bader <[hidden email]>

> ---

Guess! :)

>  drivers/net/bonding/bond_main.c | 15 +++++++++------
>  include/linux/netdevice.h       |  1 +
>  net/core/dev.c                  |  3 ++-
>  3 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 34481c9..fd92e89 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1814,7 +1814,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>   */
>  static int __bond_release_one(struct net_device *bond_dev,
>        struct net_device *slave_dev,
> -      bool all)
> +      bool all, bool unregister)
>  {
>   struct bonding *bond = netdev_priv(bond_dev);
>   struct slave *slave, *oldcurrent;
> @@ -1958,7 +1958,10 @@ static int __bond_release_one(struct net_device *bond_dev,
>   dev_set_mac_address(slave_dev, &addr);
>   }
>  
> - dev_set_mtu(slave_dev, slave->original_mtu);
> + if (unregister)
> + __dev_set_mtu(slave_dev, slave->original_mtu);
> + else
> + dev_set_mtu(slave_dev, slave->original_mtu);
>  
>   slave_dev->priv_flags &= ~IFF_BONDING;
>  
> @@ -1970,7 +1973,7 @@ static int __bond_release_one(struct net_device *bond_dev,
>  /* A wrapper used because of ndo_del_link */
>  int bond_release(struct net_device *bond_dev, struct net_device *slave_dev)
>  {
> - return __bond_release_one(bond_dev, slave_dev, false);
> + return __bond_release_one(bond_dev, slave_dev, false, false);
>  }
>  
>  /* First release a slave and then destroy the bond if no more slaves are left.
> @@ -1982,7 +1985,7 @@ static int  bond_release_and_destroy(struct net_device *bond_dev,
>   struct bonding *bond = netdev_priv(bond_dev);
>   int ret;
>  
> - ret = bond_release(bond_dev, slave_dev);
> + ret = __bond_release_one(bond_dev, slave_dev, false, true);
>   if (ret == 0 && !bond_has_slaves(bond)) {
>   bond_dev->priv_flags |= IFF_DISABLE_NETPOLL;
>   netdev_info(bond_dev, "Destroying bond %s\n",
> @@ -3032,7 +3035,7 @@ static int bond_slave_netdev_event(unsigned long event,
>   if (bond_dev->type != ARPHRD_ETHER)
>   bond_release_and_destroy(bond_dev, slave_dev);
>   else
> - bond_release(bond_dev, slave_dev);
> + __bond_release_one(bond_dev, slave_dev, false, true);
>   break;
>   case NETDEV_UP:
>   case NETDEV_CHANGE:
> @@ -4228,7 +4231,7 @@ static void bond_uninit(struct net_device *bond_dev)
>  
>   /* Release the bonded slaves */
>   bond_for_each_slave(bond, slave, iter)
> - __bond_release_one(bond_dev, slave->dev, true);
> + __bond_release_one(bond_dev, slave->dev, true, true);
>   netdev_info(bond_dev, "Released all slaves\n");
>  
>   arr = rtnl_dereference(bond->slave_arr);
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 97456b25..a8d97f0 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -3269,6 +3269,7 @@ void __dev_notify_flags(struct net_device *, unsigned int old_flags,
>  int dev_change_name(struct net_device *, const char *);
>  int dev_set_alias(struct net_device *, const char *, size_t);
>  int dev_change_net_namespace(struct net_device *, struct net *, const char *);
> +int __dev_set_mtu(struct net_device *, int);
>  int dev_set_mtu(struct net_device *, int);
>  void dev_set_group(struct net_device *, int);
>  int dev_set_mac_address(struct net_device *, struct sockaddr *);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 9b58753..8804076 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6535,7 +6535,7 @@ int dev_change_flags(struct net_device *dev, unsigned int flags)
>  }
>  EXPORT_SYMBOL(dev_change_flags);
>  
> -static int __dev_set_mtu(struct net_device *dev, int new_mtu)
> +int __dev_set_mtu(struct net_device *dev, int new_mtu)
>  {
>   const struct net_device_ops *ops = dev->netdev_ops;
>  
> @@ -6545,6 +6545,7 @@ static int __dev_set_mtu(struct net_device *dev, int new_mtu)
>   dev->mtu = new_mtu;
>   return 0;
>  }
> +EXPORT_SYMBOL(__dev_set_mtu);
>  
>  /**
>   * dev_set_mtu - Change maximum transfer unit
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

ACK / APPLIED[artful]: [SRU][Xenial][Yakkety][Zesty][Artful][PATCH 1/1] bonding: avoid NETDEV_CHANGEMTU event when unregistering slave

Seth Forshee
In reply to this post by Joseph Salisbury-3
On Fri, Jul 14, 2017 at 04:58:52PM -0400, Joseph Salisbury wrote:

> From: WANG Cong <[hidden email]>
>
> BugLink: http://bugs.launchpad.net/bugs/1704102
>
> As Hongjun/Nicolas summarized in their original patch:
>
> "
> When a device changes from one netns to another, it's first unregistered,
> then the netns reference is updated and the dev is registered in the new
> netns. Thus, when a slave moves to another netns, it is first
> unregistered. This triggers a NETDEV_UNREGISTER event which is caught by
> the bonding driver. The driver calls bond_release(), which calls
> dev_set_mtu() and thus triggers NETDEV_CHANGEMTU (the device is still in
> the old netns).
> "
>
> This is a very special case, because the device is being unregistered
> no one should still care about the NETDEV_CHANGEMTU event triggered
> at this point, we can avoid broadcasting this event on this path,
> and avoid touching inetdev_event()/addrconf_notify() path.
>
> It requires to export __dev_set_mtu() to bonding driver.
>
> Reported-by: Hongjun Li <[hidden email]>
> Reported-by: Nicolas Dichtel <[hidden email]>
> Cc: Jay Vosburgh <[hidden email]>
> Cc: Veaceslav Falico <[hidden email]>
> Cc: Andy Gospodarek <[hidden email]>
> Signed-off-by: Cong Wang <[hidden email]>
> Signed-off-by: David S. Miller <[hidden email]>
> (cherry picked from commit f51048c3e07b68c90b21a77541fc4b208f9244d7)
> Signed-off-by: Joseph Salisbury <[hidden email]>

Acked-by: Seth Forshee <[hidden email]>

Applied to artful/master-next and unstable/master, thanks.

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED: [SRU][Xenial][Yakkety][Zesty][Artful][PATCH 0/1] bonding: avoid NETDEV_CHANGEMTU event when unregistering slave

Thadeu Lima de Souza Cascardo-3
In reply to this post by Joseph Salisbury-3
Applied to xenial and zesty master-next branches.

Thanks.
Cascardo.

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team