From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hannes Frederic Sowa Subject: Re: [RFC PATCH net] IPv6: Fix broken IPv6 routing table after loopback down-up Date: Wed, 22 Jan 2014 22:34:46 +0100 Message-ID: <20140122213446.GD7269@order.stressinduktion.org> References: <1390404908-3914-1-git-send-email-sd@queasysnail.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: netdev@vger.kernel.org, gaofeng@cn.fujitsu.com To: Sabrina Dubroca Return-path: Received: from order.stressinduktion.org ([87.106.68.36]:58916 "EHLO order.stressinduktion.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752748AbaAVVes (ORCPT ); Wed, 22 Jan 2014 16:34:48 -0500 Content-Disposition: inline In-Reply-To: <1390404908-3914-1-git-send-email-sd@queasysnail.net> Sender: netdev-owner@vger.kernel.org List-ID: On Wed, Jan 22, 2014 at 04:35:08PM +0100, Sabrina Dubroca wrote: > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > index 4b6b720..b2eb653 100644 > --- a/net/ipv6/addrconf.c > +++ b/net/ipv6/addrconf.c > @@ -2578,6 +2578,23 @@ static void sit_add_v4_addrs(struct inet6_dev *idev) > } > #endif > > +struct rt6_info *addrconf_dst_alloc_once(struct inet6_dev *idev, > + const struct in6_addr *addr, > + bool anycast) > +{ > + struct net *net = dev_net(idev->dev); > + struct rt6_info *rt = rt6_lookup(net, addr, NULL, 0, 0); We should use addrconf_get_prefix_route here. Eric's comment applies here, too, we do a dst_hold in addrconf_get_prefix_route and thus must decrease the reference counter with ip6_rt_put then, too. > + if (rt == NULL) > + return addrconf_dst_alloc(idev, addr, anycast); > + else if (!(rt->rt6i_flags & RTF_NONEXTHOP && > + ((anycast && rt->rt6i_flags & RTF_ANYCAST) || > + (!anycast && rt->rt6i_flags & RTF_LOCAL)))) > + return addrconf_dst_alloc(idev, addr, anycast); The necessary flags can be given to addrconf_get_prefix_route, then. > + return ERR_PTR(-EEXIST); > +} > + > static void init_loopback(struct net_device *dev) > { > struct inet6_dev *idev; > @@ -2611,10 +2628,7 @@ static void init_loopback(struct net_device *dev) > if (sp_ifa->flags & (IFA_F_DADFAILED | IFA_F_TENTATIVE)) > continue; > > - if (sp_ifa->rt) > - continue; We should try to clean sp_ifa->rt on ifdown so we don't have dangling pointer to it if it is already in dst gc queue and obsolete. So even if we leave this code in there we would never hit the continue (it seems we have to decrease the reference counter in addrconf_ifdown before setting sp_ifa->rt to NULL). > - > - sp_rt = addrconf_dst_alloc(idev, &sp_ifa->addr, false); > + sp_rt = addrconf_dst_alloc_once(idev, &sp_ifa->addr, false); > > /* Failure cases are ignored */ > if (!IS_ERR(sp_rt)) { I am fine if we get this fix in for 3.14 because it solves a real problem. I'll already work on the full route preserving logic on ifdown/up and would build this on this patch then. I currently wonder if we need the relookup logic at all as we currently remove the prefix routes in all cases (in rt6_ifdown, which iterates over all routing tables). So we always have a obsolete dst which we just need to clean up in addrconf_ifdown and just allocate the prefix route in init_loopback in all cases. We just have to steal some code from inet6_rtm_newaddr to calculate the appropriate flags from the inet6_ifaddr's ifa_flags (e.g. RTF_EXPIRES). Thanks a lot, Hannes