On Wed, May 15, 2019 at 7:40 PM Eric Dumazet <eduma...@google.com> wrote:
>
> At ipv6 route dismantle, fib6_drop_pcpu_from() is responsible
> for finding all percpu routes and set their ->from pointer
> to NULL, so that fib6_ref can reach its expected value (1).
>
> The problem right now is that other cpus can still catch the
> route being deleted, since there is no rcu grace period
> between the route deletion and call to fib6_drop_pcpu_from()
>
> This can leak the fib6 and associated resources, since no
> notifier will take care of removing the last reference(s).
>
> I decided to add another boolean (fib6_destroying) instead
> of reusing/renaming exception_bucket_flushed to ease stable backports,
> and properly document the memory barriers used to implement this fix.
>
> This patch has been co-developped with Wei Wang.
>
> Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst
> based routes")
> Signed-off-by: Eric Dumazet <eduma...@google.com>
> Reported-by: syzbot <syzkal...@googlegroups.com>
> Cc: Wei Wang <wei...@google.com>
> Cc: David Ahern <dsah...@gmail.com>
> Cc: Martin Lau <ka...@fb.com>
> ---
Thanks for the fix Eric.
Acked-by: Wei Wang <wei...@google.com>
> include/net/ip6_fib.h | 3 ++-
> net/ipv6/ip6_fib.c | 12 +++++++++---
> net/ipv6/route.c | 7 +++++++
> 3 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
> index
> 40105738e2f6b8e37adac1ff46879ce6c09381b8..525f701653ca69596b941f5f3b4a438d634c4e6c
> 100644
> --- a/include/net/ip6_fib.h
> +++ b/include/net/ip6_fib.h
> @@ -167,7 +167,8 @@ struct fib6_info {
> dst_nocount:1,
> dst_nopolicy:1,
> dst_host:1,
> - unused:3;
> + fib6_destroying:1,
> + unused:2;
>
> struct fib6_nh fib6_nh;
> struct rcu_head rcu;
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index
> 08e0390e001c270ae21013f3fd3ef3bf2a52e039..008421b550c6bfd449665aa5e7ba5505fcabe53d
> 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -904,6 +904,12 @@ static void fib6_drop_pcpu_from(struct fib6_info *f6i,
> {
> int cpu;
>
> + /* Make sure rt6_make_pcpu_route() wont add other percpu routes
> + * while we are cleaning them here.
> + */
> + f6i->fib6_destroying = 1;
> + mb(); /* paired with the cmpxchg() in rt6_make_pcpu_route() */
> +
> /* release the reference to this fib entry from
> * all of its cached pcpu routes
> */
> @@ -927,6 +933,9 @@ static void fib6_purge_rt(struct fib6_info *rt, struct
> fib6_node *fn,
> {
> struct fib6_table *table = rt->fib6_table;
>
> + if (rt->rt6i_pcpu)
> + fib6_drop_pcpu_from(rt, table);
> +
> if (refcount_read(&rt->fib6_ref) != 1) {
> /* This route is used as dummy address holder in some split
> * nodes. It is not leaked, but it still holds other
> resources,
> @@ -948,9 +957,6 @@ static void fib6_purge_rt(struct fib6_info *rt, struct
> fib6_node *fn,
> fn = rcu_dereference_protected(fn->parent,
> lockdep_is_held(&table->tb6_lock));
> }
> -
> - if (rt->rt6i_pcpu)
> - fib6_drop_pcpu_from(rt, table);
> }
> }
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index
> 23a20d62daac29e3252725b8cf95d1d1c2b567c4..27c0cc5d9d30e3689ebe6b8428cd4c586669d808
> 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -1295,6 +1295,13 @@ static struct rt6_info *rt6_make_pcpu_route(struct net
> *net,
> prev = cmpxchg(p, NULL, pcpu_rt);
> BUG_ON(prev);
>
> + if (res->f6i->fib6_destroying) {
> + struct fib6_info *from;
> +
> + from = xchg((__force struct fib6_info **)&pcpu_rt->from,
> NULL);
> + fib6_info_release(from);
> + }
> +
> return pcpu_rt;
> }
>
> --
> 2.21.0.1020.gf2820cf01a-goog
>
