On Sat, Oct 24, 2015 at 09:20:00PM +0300, Julian Anastasov wrote: > When fib_netdev_event calls fib_disable_ip on NETDEV_DOWN event > we should not delete the local routes if the local address > is still present. The confusion comes from the fact that both > fib_netdev_event and fib_inetaddr_event use the NETDEV_DOWN > constant. Fix it by returning back the variable 'force'. > > Steps to reproduce: > modprobe dummy > ifconfig dummy0 192.168.168.1 up > ip route list table local | grep dummy | grep host > local 192.168.168.1 dev dummy0 proto kernel scope host src 192.168.168.1 I tested this before and after your patch and I don't see a different output. Was I supposed to see something different?
> Second fix I would prefer you move these two fixes into 2 separate patches as it isn't totally clear which hunks fix each of these issues. > is for fib_sync_up: when nexthop is part of multipath > route we should clear the LINKDOWN flag when link goes UP > or when first address is added. This is needed because we always > set LINKDOWN flag when DEAD flag is set but now the nexthop > is not dead anymore. Examples when LINKDOWN bit can be forgotten: > > - link goes down (LINKDOWN is set), then link goes UP and device > shows carrier OK but LINKDOWN remains set > > - last address is deleted (LINKDOWN is set), then address is > added and device shows carrier OK but LINKDOWN remains set Are you seeing this with iproute2 (or other tools) or are you just seeing this by monitoring netlink messages/looking at a netlink cache you have built inside an application? I have seen a problem similar to what you have reported with netlink caches and have a fix I can give you if you would like to try it. It is a slightly larger structural change, but it appears to cover covers a few more cases than this fix does. > > Fixes: 8a3d03166f19 ("net: track link-status of ipv4 nexthops") > Signed-off-by: Julian Anastasov <j...@ssi.bg> > --- > include/net/ip_fib.h | 2 +- > net/ipv4/fib_frontend.c | 13 +++++++------ > net/ipv4/fib_semantics.c | 18 +++++++++++++++--- > 3 files changed, 23 insertions(+), 10 deletions(-) > > diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h > index 727d6e9..654aec1 100644 > --- a/include/net/ip_fib.h > +++ b/include/net/ip_fib.h > @@ -317,7 +317,7 @@ void fib_flush_external(struct net *net); > > /* Exported by fib_semantics.c */ > int ip_fib_check_default(__be32 gw, struct net_device *dev); > -int fib_sync_down_dev(struct net_device *dev, unsigned long event); > +int fib_sync_down_dev(struct net_device *dev, unsigned long event, int > force); > int fib_sync_down_addr(struct net *net, __be32 local); > int fib_sync_up(struct net_device *dev, unsigned int nh_flags); > void fib_select_multipath(struct fib_result *res); > diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c > index 690bcbc..4826a22 100644 > --- a/net/ipv4/fib_frontend.c > +++ b/net/ipv4/fib_frontend.c > @@ -1110,9 +1110,10 @@ static void nl_fib_lookup_exit(struct net *net) > net->ipv4.fibnl = NULL; > } > > -static void fib_disable_ip(struct net_device *dev, unsigned long event) > +static void fib_disable_ip(struct net_device *dev, unsigned long event, > + int force) > { > - if (fib_sync_down_dev(dev, event)) > + if (fib_sync_down_dev(dev, event, force)) > fib_flush(dev_net(dev)); > rt_cache_flush(dev_net(dev)); > arp_ifdown(dev); > @@ -1140,7 +1141,7 @@ static int fib_inetaddr_event(struct notifier_block > *this, unsigned long event, > /* Last address was deleted from this interface. > * Disable IP. > */ > - fib_disable_ip(dev, event); > + fib_disable_ip(dev, event, 1); > } else { > rt_cache_flush(dev_net(dev)); > } > @@ -1157,7 +1158,7 @@ static int fib_netdev_event(struct notifier_block > *this, unsigned long event, vo > unsigned int flags; > > if (event == NETDEV_UNREGISTER) { > - fib_disable_ip(dev, event); > + fib_disable_ip(dev, event, 2); > rt_flush_dev(dev); > return NOTIFY_DONE; > } > @@ -1178,14 +1179,14 @@ static int fib_netdev_event(struct notifier_block > *this, unsigned long event, vo > rt_cache_flush(net); > break; > case NETDEV_DOWN: > - fib_disable_ip(dev, event); > + fib_disable_ip(dev, event, 0); > break; > case NETDEV_CHANGE: > flags = dev_get_flags(dev); > if (flags & (IFF_RUNNING | IFF_LOWER_UP)) > fib_sync_up(dev, RTNH_F_LINKDOWN); > else > - fib_sync_down_dev(dev, event); > + fib_sync_down_dev(dev, event, 0); > /* fall through */ > case NETDEV_CHANGEMTU: > rt_cache_flush(net); > diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c > index 064bd3c..f657418 100644 > --- a/net/ipv4/fib_semantics.c > +++ b/net/ipv4/fib_semantics.c > @@ -1281,7 +1281,13 @@ int fib_sync_down_addr(struct net *net, __be32 local) > return ret; > } > > -int fib_sync_down_dev(struct net_device *dev, unsigned long event) > +/* Event force Flags Description > + * NETDEV_CHANGE 0 LINKDOWN Carrier OFF, not for scope host > + * NETDEV_DOWN 0 LINKDOWN|DEAD Link down, not for scope host > + * NETDEV_DOWN 1 LINKDOWN|DEAD Last address removed > + * NETDEV_UNREGISTER 2 LINKDOWN|DEAD Device removed > + */ > +int fib_sync_down_dev(struct net_device *dev, unsigned long event, int force) > { > int ret = 0; > int scope = RT_SCOPE_NOWHERE; > @@ -1290,8 +1296,7 @@ int fib_sync_down_dev(struct net_device *dev, unsigned > long event) > struct hlist_head *head = &fib_info_devhash[hash]; > struct fib_nh *nh; > > - if (event == NETDEV_UNREGISTER || > - event == NETDEV_DOWN) > + if (force) > scope = -1; > > hlist_for_each_entry(nh, head, nh_hash) { > @@ -1440,6 +1445,13 @@ int fib_sync_up(struct net_device *dev, unsigned int > nh_flags) > if (!(dev->flags & IFF_UP)) > return 0; > > + if (nh_flags & RTNH_F_DEAD) { > + unsigned int flags = dev_get_flags(dev); > + > + if (flags & (IFF_RUNNING | IFF_LOWER_UP)) > + nh_flags |= RTNH_F_LINKDOWN; > + } > + > prev_fi = NULL; > hash = fib_devindex_hashfn(dev->ifindex); > head = &fib_info_devhash[hash]; > -- > 1.9.3 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html