netdev - Re: [PATCH RFC net-next 18/20] net/ipv6: separate handling of FIB entries from dst based routes

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: <da887eb5-3866-fd8d-54c9-30b881778b2b@gmail.com>
Date:   Wed, 28 Feb 2018 13:10:38 -0700
From:   David Ahern <dsahern@...il.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, idosch@...sch.org,
        roopa@...ulusnetworks.com, eric.dumazet@...il.com,
        weiwan@...gle.com, yoshfuji@...ux-ipv6.org
Subject: Re: [PATCH RFC net-next 18/20] net/ipv6: separate handling of FIB
 entries from dst based routes

On 2/28/18 11:44 AM, Martin KaFai Lau wrote:
> On Sun, Feb 25, 2018 at 11:47:28AM -0800, David Ahern wrote:
>> Signed-off-by: David Ahern <dsahern@...il.com>
>> ---
>>  include/net/ip6_fib.h   |   4 +-
>>  include/net/ip6_route.h |   3 +-
>>  net/ipv6/addrconf.c     |  31 ++++++---
>>  net/ipv6/anycast.c      |   7 +-
>>  net/ipv6/ip6_fib.c      |  50 +++++++++------
>>  net/ipv6/ip6_output.c   |   3 +-
>>  net/ipv6/ndisc.c        |   6 +-
>>  net/ipv6/route.c        | 167 +++++++++++++++++-------------------------------
>>  8 files changed, 121 insertions(+), 150 deletions(-)
>>
>> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
>> index 70978deac538..ff16e3d571a2 100644
>> --- a/include/net/ip6_fib.h
>> +++ b/include/net/ip6_fib.h
>> @@ -315,9 +315,7 @@ static inline u32 rt6_get_cookie(const struct rt6_info *rt)
>>  
>>  	if (rt->rt6i_flags & RTF_PCPU ||
>>  	    (unlikely(!list_empty(&rt->rt6i_uncached)) && rt->from))
>> -		rt = rt->from;
>> -
>> -	rt6_get_cookie_safe(rt, &cookie);
>> +		rt6_get_cookie_safe(rt->from, &cookie);
>>  
>>  	return cookie;
>>  }
>> diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
>> index 24c78fb6ac36..fcda09a58193 100644
>> --- a/include/net/ip6_route.h
>> +++ b/include/net/ip6_route.h
>> @@ -113,8 +113,7 @@ static inline int ip6_route_get_saddr(struct net *net, struct rt6_info *rt,
>>  				      unsigned int prefs,
>>  				      struct in6_addr *saddr)
>>  {
>> -	struct inet6_dev *idev =
>> -			rt ? ip6_dst_idev((struct dst_entry *)rt) : NULL;
>> +	struct inet6_dev *idev = rt ? rt->rt6i_idev : NULL;
>>  	int err = 0;
>>  
>>  	if (rt && rt->rt6i_prefsrc.plen)
>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>> index 2a032b932922..4dd7b4e9de4c 100644
>> --- a/net/ipv6/addrconf.c
>> +++ b/net/ipv6/addrconf.c
>> @@ -927,7 +927,7 @@ void inet6_ifa_finish_destroy(struct inet6_ifaddr *ifp)
>>  		pr_warn("Freeing alive inet6 address %p\n", ifp);
>>  		return;
>>  	}
>> -	ip6_rt_put(ifp->rt);
>> +	fib6_info_release(ifp->rt);
>>  
>>  	kfree_rcu(ifp, rcu);
>>  }
>> @@ -1080,6 +1080,7 @@ ipv6_add_addr(struct inet6_dev *idev, const struct in6_addr *addr,
>>  	ifa->cstamp = ifa->tstamp = jiffies;
>>  	ifa->tokenized = false;
>>  
>> +	fib6_info_hold(rt);
> Did fib6_info_alloc() already bump the refcnt?  Why
> another fib6_info_hold() is needed?  Comment would be
> useful here.

The alloc does set it to 1; the extra bump here is because of the
ifa->rt assignment.

Without the additional reference, deleting the route would cause rt to
be freed, yet ifa has a reference to it. I did not want to wade into
changes to the accounting for this set, though I do think it needs to be
revisited.

I'll take another look at whether this is really needed.

> 
>>  	ifa->rt = rt;
>>  
>>  	ifa->idev = idev;
>> @@ -1114,8 +1115,12 @@ ipv6_add_addr(struct inet6_dev *idev, const struct in6_addr *addr,
>>  	inet6addr_notifier_call_chain(NETDEV_UP, ifa);
>>  out:
>>  	if (unlikely(err < 0)) {
>> -		if (rt)
>> -			ip6_rt_put(rt);
>> +		/* one release for the hold taken when rt is set in ifa
>> +		 * and a second release for the hold taken on rt create
>> +		 */
>> +		fib6_info_release(rt);
>> +		fib6_info_release(rt);
> The extra release corresponds to the above fib6_info_hold()?

yes.