| Message ID | 20170705155725.15469-1-nicolas.dichtel@6wind.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Nicolas Dichtel <nicolas.dichtel@6wind.com> Date: Wed, 5 Jul 2017 17:57:25 +0200 > From: Hongjun Li <hongjun.li@6wind.com> > > When the netdev event NETDEV_CHANGEMTU is triggered, the inet[6]dev may be > created even if the corresponding device is down. This may lead to a leak > in the procfs when the device is unregistered, and finally trigger a > backtrace: ... > 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). > > Signed-off-by: Hongjun Li <hongjun.li@6wind.com> > Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> I'm still not convinced about this. We have lots of code which iterates ipv6 idevs, and then has a check for IFF_UP. So having an idev attached to a down interface is not a bug nor illegal. In fact, addrconf_cleanup() walks all of the init_net idevs and calls addrconf_ifdown() with how=1 regardless of IFF_UP or not. This entire area is quite a mess. Can you show exactly why the procfs state isn't cleaned up for these devices moving between namespaces? Maybe that is the real bug and a better place to fix this. Thanks.
On Sat, Jul 8, 2017 at 3:02 AM, David Miller <davem@davemloft.net> wrote: > From: Nicolas Dichtel <nicolas.dichtel@6wind.com> > Date: Wed, 5 Jul 2017 17:57:25 +0200 > >> From: Hongjun Li <hongjun.li@6wind.com> >> >> When the netdev event NETDEV_CHANGEMTU is triggered, the inet[6]dev may be >> created even if the corresponding device is down. This may lead to a leak >> in the procfs when the device is unregistered, and finally trigger a >> backtrace: > ... >> 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). >> >> Signed-off-by: Hongjun Li <hongjun.li@6wind.com> >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> > > I'm still not convinced about this. > > We have lots of code which iterates ipv6 idevs, and then has a > check for IFF_UP. > > So having an idev attached to a down interface is not a bug nor > illegal. > > In fact, addrconf_cleanup() walks all of the init_net idevs and > calls addrconf_ifdown() with how=1 regardless of IFF_UP or not. > > This entire area is quite a mess. +1. I fixed a nasty bug with how=1 for loopback before... > > Can you show exactly why the procfs state isn't cleaned up for > these devices moving between namespaces? Maybe that is the real > bug and a better place to fix this. > It is because the ipv6_add_dev() adds these proc files back after NETDEV_UNREGISTER event.
Le 08/07/2017 à 20:44, Cong Wang a écrit : > On Sat, Jul 8, 2017 at 3:02 AM, David Miller <davem@davemloft.net> wrote: [snip] >> Can you show exactly why the procfs state isn't cleaned up for >> these devices moving between namespaces? Maybe that is the real >> bug and a better place to fix this. >> > > It is because the ipv6_add_dev() adds these proc files back after > NETDEV_UNREGISTER event. > Yep, that was the initial problem. Regards, Nicolas
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c index df14815a3b8c..fe94f48854d3 100644 --- a/net/ipv4/devinet.c +++ b/net/ipv4/devinet.c @@ -1424,7 +1424,8 @@ static int inetdev_event(struct notifier_block *this, unsigned long event, IN_DEV_CONF_SET(in_dev, NOXFRM, 1); IN_DEV_CONF_SET(in_dev, NOPOLICY, 1); } - } else if (event == NETDEV_CHANGEMTU) { + } else if (event == NETDEV_CHANGEMTU && + dev->flags & IFF_UP) { /* Re-enabling IP */ if (inetdev_valid_mtu(dev->mtu)) in_dev = inetdev_init(dev); diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index 1d2dbace42ff..034e74a309a0 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -3395,6 +3395,9 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event, break; } + if (!(dev->flags & IFF_UP)) + break; + /* allocate new idev */ idev = ipv6_add_dev(dev); if (IS_ERR(idev))