| Message ID | 20081104135551.916564770@nptl.frec.bull.fr |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Benjamin Thery <benjamin.thery@bull.net> Date: Tue, 04 Nov 2008 14:55:50 +0100 > I think there's a bug in net/ipv6/addrconf.c, addrconf_notify(): > addrconf_dad_run() is not always run when the interface is flagged IF_READY. > Currently it is only run when receiving NETDEV_CHANGE event. Looks like > some (virtual) devices doesn't send this event when becoming up. > > For both NETDEV_UP and NETDEV_CHANGE events, when the interface becomes > ready, run_pending should be set to 1. Patch below. > > 'run_pending = 1' could be moved below the if/else block but it makes > the code less readable. I wonder if we should instead make the virtual devices emit the missing event? -- 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
From: David Miller <davem@davemloft.net> Date: Tue, 04 Nov 2008 14:51:22 -0800 (PST) > From: Benjamin Thery <benjamin.thery@bull.net> > Date: Tue, 04 Nov 2008 14:55:50 +0100 > > > I think there's a bug in net/ipv6/addrconf.c, addrconf_notify(): > > addrconf_dad_run() is not always run when the interface is flagged IF_READY. > > Currently it is only run when receiving NETDEV_CHANGE event. Looks like > > some (virtual) devices doesn't send this event when becoming up. > > > > For both NETDEV_UP and NETDEV_CHANGE events, when the interface becomes > > ready, run_pending should be set to 1. Patch below. > > > > 'run_pending = 1' could be moved below the if/else block but it makes > > the code less readable. > > I wonder if we should instead make the virtual devices emit > the missing event? In any event, for the time being, I'm going to apply Benjamin's patch to fix this problem. It makes the function in question logically consistent in that now everything that can cause IF_READY to become set will also cause run_pending work to run. -- 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
David Miller wrote: > From: David Miller <davem@davemloft.net> > Date: Tue, 04 Nov 2008 14:51:22 -0800 (PST) > >> From: Benjamin Thery <benjamin.thery@bull.net> >> Date: Tue, 04 Nov 2008 14:55:50 +0100 >> >>> I think there's a bug in net/ipv6/addrconf.c, addrconf_notify(): >>> addrconf_dad_run() is not always run when the interface is flagged IF_READY. >>> Currently it is only run when receiving NETDEV_CHANGE event. Looks like >>> some (virtual) devices doesn't send this event when becoming up. >>> >>> For both NETDEV_UP and NETDEV_CHANGE events, when the interface becomes >>> ready, run_pending should be set to 1. Patch below. >>> >>> 'run_pending = 1' could be moved below the if/else block but it makes >>> the code less readable. >> I wonder if we should instead make the virtual devices emit >> the missing event? I don't know. We'll have to identify all the devices that (mis)behave like this. Are all the devices supposed to emit both NETDEV_UP and NETDEV_CHANGE when ifconfig/ip commands set the devices up? But as you state below, it also looked more logical to me to run addrconf_dad_run() on every events that causes the interface to become ready. Benjamin > > In any event, for the time being, I'm going to apply > Benjamin's patch to fix this problem. > > It makes the function in question logically consistent > in that now everything that can cause IF_READY to become > set will also cause run_pending work to run. This is how > >
Index: net-2.6/net/ipv6/addrconf.c =================================================================== --- net-2.6.orig/net/ipv6/addrconf.c +++ net-2.6/net/ipv6/addrconf.c @@ -2483,8 +2483,10 @@ static int addrconf_notify(struct notifi if (!idev && dev->mtu >= IPV6_MIN_MTU) idev = ipv6_add_dev(dev); - if (idev) + if (idev) { idev->if_flags |= IF_READY; + run_pending = 1; + } } else { if (!addrconf_qdisc_ok(dev)) { /* device is still not ready. */
With some net devices types, an IPv6 address configured while the interface was down can stay 'tentative' forever, even after the interface is set up. In some case, pending IPv6 DADs are not executed when the device becomes ready. I observed this while doing some tests with kvm. If I assign an IPv6 address to my interface eth0 (kvm driver rtl8139) when it is still down then the address is flagged tentative (IFA_F_TENTATIVE). Then, I set eth0 up, and to my surprise, the address stays 'tentative', no DAD is executed and the address can't be pinged. I also observed the same behaviour, without kvm, with virtual interfaces types macvlan and veth. Some easy steps to reproduce the issue with macvlan: 1. ip link add link eth0 type macvlan 2. ip -6 addr add 2003::ab32/64 dev macvlan0 3. ip addr show dev macvlan0 ... inet6 2003::ab32/64 scope global tentative ... 4. ip link set macvlan0 up 5. ip addr show dev macvlan0 ... inet6 2003::ab32/64 scope global tentative ... Address is still tentative I think there's a bug in net/ipv6/addrconf.c, addrconf_notify(): addrconf_dad_run() is not always run when the interface is flagged IF_READY. Currently it is only run when receiving NETDEV_CHANGE event. Looks like some (virtual) devices doesn't send this event when becoming up. For both NETDEV_UP and NETDEV_CHANGE events, when the interface becomes ready, run_pending should be set to 1. Patch below. 'run_pending = 1' could be moved below the if/else block but it makes the code less readable. Regards, Benjamin Signed-off-by: Benjamin Thery <benjamin.thery@bull.net> --- net/ipv6/addrconf.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)