[PATCH net] ipv6: prevent possible fib6 leaks

[PATCH net] ipv6: prevent possible fib6 leaks

[Eric Dumazet]

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 <edumazet@google.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
Cc: Wei Wang <weiwan@google.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Martin Lau <kafai@fb.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

Re: [PATCH net] ipv6: prevent possible fib6 leaks

[Wei Wang]

On Wed, May 15, 2019 at 7:40 PM Eric Dumazet <edumazet@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 <edumazet@google.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Cc: Wei Wang <weiwan@google.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Martin Lau <kafai@fb.com>
> ---
Thanks for the fix Eric.

Acked-by: Wei Wang <weiwan@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
>

Re: [PATCH net] ipv6: prevent possible fib6 leaks

[Martin Lau]

On Wed, May 15, 2019 at 07:39:52PM -0700, Eric Dumazet 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.
Nice fix!

Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [PATCH net] ipv6: prevent possible fib6 leaks

[David Ahern]

On 5/15/19 8:39 PM, Eric Dumazet 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 <edumazet@google.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Cc: Wei Wang <weiwan@google.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Martin Lau <kafai@fb.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(-)
> 

Reviewed-by: David Ahern <dsahern@gmail.com>

Thanks, Eric. Attaching 'from' to rt6_info makes v6 much more
complicated than ipv4.

Re: [PATCH net] ipv6: prevent possible fib6 leaks

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Wed, 15 May 2019 19:39:52 -0700

> 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 <edumazet@google.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>

Applied and queued up for -stable.