netdev - Re: [RFC PATCH net] IPv6: Fix broken IPv6 routing table after loopback down-up

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140122213446.GD7269@order.stressinduktion.org>
Date:	Wed, 22 Jan 2014 22:34:46 +0100
From:	Hannes Frederic Sowa <hannes@...essinduktion.org>
To:	Sabrina Dubroca <sd@...asysnail.net>
Cc:	netdev@...r.kernel.org, gaofeng@...fujitsu.com
Subject: Re: [RFC PATCH net] IPv6: Fix broken IPv6 routing table after loopback down-up

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html