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

2018-02-28 Thread David Ahern 
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 
>> ---
>>  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(>rt6i_uncached)) && rt->from))
>> -rt = rt->from;
>> -
>> -rt6_get_cookie_safe(rt, );
>> +rt6_get_cookie_safe(rt->from, );
>>  
>>  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.

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

2018-02-28 Thread Martin KaFai Lau 
On Sun, Feb 25, 2018 at 11:47:28AM -0800, David Ahern wrote:
> Signed-off-by: David Ahern 
> ---
>  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(>rt6i_uncached)) && rt->from))
> - rt = rt->from;
> -
> - rt6_get_cookie_safe(rt, );
> + rt6_get_cookie_safe(rt->from, );
>  
>   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.

>   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()?

> +
>   if (ifa) {
>   if (ifa->idev)
>   in6_dev_put(ifa->idev);
> @@ -1203,7 +1208,7 @@ cleanup_prefix_route(struct inet6_ifaddr *ifp, unsigned 
> long expires, bool del_r
>   else {
>   if (!(rt->rt6i_flags & RTF_EXPIRES))
>   fib6_set_expires(rt, expires);
> - ip6_rt_put(rt);
> + fib6_info_release(rt);
>   }
>   }
>  }
> @@ -2350,8 +2355,7 @@ static struct rt6_info *addrconf_get_prefix_route(const 
> struct in6_addr *pfx,
>   continue;
>   if ((rt->rt6i_flags & noflags) != 0)
>   continue;
> - if (!dst_hold_safe(>dst))
> - rt = NULL;
> + fib6_info_hold(rt);
>   break;
>   }
>  out:
> @@ -2663,7 +2667,7 @@ void addrconf_prefix_rcv(struct net_device *dev, u8 
> *opt, int len, bool sllao)
>   addrconf_prefix_route(>prefix, pinfo->prefix_len,
> dev, expires, flags, GFP_ATOMIC);
>   }
> - ip6_rt_put(rt);
> + fib6_info_release(rt);
>   }
>  
>   /* Try to figure out our local address for this prefix */
> @@ -3330,9 +3334,14 @@ static int fixup_permanent_addr(struct net *net,
>   spin_lock(>lock);
>   prev = ifp->rt;
>   ifp->rt = rt;
> + fib6_info_hold(rt);
>   spin_unlock(>lock);
>  
> - ip6_rt_put(prev);
> + /* 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(prev);
> + fib6_info_release(prev);
>   }
>  
>   if (!(ifp->flags & IFA_F_NOPREFIXROUTE)) {
> @@ -3706,6 +3715,7 @@ static int

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

2018-02-25 Thread David Ahern 
Signed-off-by: David Ahern 
---
 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(>rt6i_uncached)) && rt->from))
-   rt = rt->from;
-
-   rt6_get_cookie_safe(rt, );
+   rt6_get_cookie_safe(rt->from, );
 
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);
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);
+
if (ifa) {
if (ifa->idev)
in6_dev_put(ifa->idev);
@@ -1203,7 +1208,7 @@ cleanup_prefix_route(struct inet6_ifaddr *ifp, unsigned 
long expires, bool del_r
else {
if (!(rt->rt6i_flags & RTF_EXPIRES))
fib6_set_expires(rt, expires);
-   ip6_rt_put(rt);
+   fib6_info_release(rt);
}
}
 }
@@ -2350,8 +2355,7 @@ static struct rt6_info *addrconf_get_prefix_route(const 
struct in6_addr *pfx,
continue;
if ((rt->rt6i_flags & noflags) != 0)
continue;
-   if (!dst_hold_safe(>dst))
-   rt = NULL;
+   fib6_info_hold(rt);
break;
}
 out:
@@ -2663,7 +2667,7 @@ void addrconf_prefix_rcv(struct net_device *dev, u8 *opt, 
int len, bool sllao)
addrconf_prefix_route(>prefix, pinfo->prefix_len,
  dev, expires, flags, GFP_ATOMIC);
}
-   ip6_rt_put(rt);
+   fib6_info_release(rt);
}
 
/* Try to figure out our local address for this prefix */
@@ -3330,9 +3334,14 @@ static int fixup_permanent_addr(struct net *net,
spin_lock(>lock);
prev = ifp->rt;
ifp->rt = rt;
+   fib6_info_hold(rt);
spin_unlock(>lock);
 
-   ip6_rt_put(prev);
+   /* 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(prev);
+   fib6_info_release(prev);
}
 
if (!(ifp->flags & IFA_F_NOPREFIXROUTE)) {
@@ -3706,6 +3715,7 @@ static int addrconf_ifdown(struct net_device *dev, int 
how)
 
rt = ifa->rt;
ifa->rt = NULL;
+   fib6_info_release(rt);
} else {
state = ifa->state;
ifa->state = INET6_IFADDR_STATE_DEAD;
@@ -5600,8 +5610,9 @@ static void __ipv6_ifa_notify(int event, struct