[PATCH net-next 0/3] ipv6: fib6_ref conversion to refcount_t

[PATCH net-next 0/3] ipv6: fib6_ref conversion to refcount_t

[Eric Dumazet]

We are chasing use-after-free in IPv6 that could have their origin
in fib6_ref 0 -> 1 transitions.

This patch series should help finding the root causes if these
illegal transitions ever happen.

Eric Dumazet (3):
  ipv6: fib6_info_destroy_rcu() cleanup
  ipv6: broadly use fib6_info_hold() helper
  ipv6: convert fib6_ref to refcount_t

 include/net/ip6_fib.h |  8 ++++----
 net/ipv6/ip6_fib.c    | 25 +++++++++++--------------
 net/ipv6/route.c      |  2 +-
 3 files changed, 16 insertions(+), 19 deletions(-)

-- 
2.21.0.593.g511ec345e18-goog

[PATCH net-next 1/3] ipv6: fib6_info_destroy_rcu() cleanup

[Eric Dumazet]

We do not need to clear f6i->rt6i_exception_bucket right before
freeing f6i.

Note that f6i->rt6i_exception_bucket is properly protected by
f6i->exception_bucket_flushed being set to one in rt6_flush_exceptions()
under the protection of rt6_exception_lock.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Wei Wang <weiwan@google.com>
---
 net/ipv6/ip6_fib.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index b47e15df9769ccb2919e6ec0de5edd93f2ae518f..55193859152969794dab3df02637217a7f21016f 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -175,10 +175,7 @@ void fib6_info_destroy_rcu(struct rcu_head *head)
 	WARN_ON(f6i->fib6_node);
 
 	bucket = rcu_dereference_protected(f6i->rt6i_exception_bucket, 1);
-	if (bucket) {
-		f6i->rt6i_exception_bucket = NULL;
-		kfree(bucket);
-	}
+	kfree(bucket);
 
 	if (f6i->rt6i_pcpu) {
 		int cpu;
-- 
2.21.0.593.g511ec345e18-goog

[PATCH net-next 2/3] ipv6: broadly use fib6_info_hold() helper

[Eric Dumazet]

Instead of using atomic_inc(), prefer fib6_info_hold()
so that upcoming refcount_t conversion is simpler.

Only fib6_info_alloc() is using atomic_set() since we
just allocated a new object.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Wei Wang <weiwan@google.com>
---
 net/ipv6/ip6_fib.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 55193859152969794dab3df02637217a7f21016f..a5e83593e0e45c2762eca85d04757f9d8e118e0f 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -162,7 +162,7 @@ struct fib6_info *fib6_info_alloc(gfp_t gfp_flags)
 	}
 
 	INIT_LIST_HEAD(&f6i->fib6_siblings);
-	atomic_inc(&f6i->fib6_ref);
+	atomic_set(&f6i->fib6_ref, 1);
 
 	return f6i;
 }
@@ -846,8 +846,8 @@ static struct fib6_node *fib6_add_1(struct net *net,
 
 		RCU_INIT_POINTER(in->parent, pn);
 		in->leaf = fn->leaf;
-		atomic_inc(&rcu_dereference_protected(in->leaf,
-				lockdep_is_held(&table->tb6_lock))->fib6_ref);
+		fib6_info_hold(rcu_dereference_protected(in->leaf,
+				lockdep_is_held(&table->tb6_lock)));
 
 		/* update parent pointer */
 		if (dir)
@@ -942,7 +942,7 @@ static void fib6_purge_rt(struct fib6_info *rt, struct fib6_node *fn,
 			struct fib6_info *new_leaf;
 			if (!(fn->fn_flags & RTN_RTINFO) && leaf == rt) {
 				new_leaf = fib6_find_prefix(net, table, fn);
-				atomic_inc(&new_leaf->fib6_ref);
+				fib6_info_hold(new_leaf);
 
 				rcu_assign_pointer(fn->leaf, new_leaf);
 				fib6_info_release(rt);
@@ -1108,7 +1108,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
 			return err;
 
 		rcu_assign_pointer(rt->fib6_next, iter);
-		atomic_inc(&rt->fib6_ref);
+		fib6_info_hold(rt);
 		rcu_assign_pointer(rt->fib6_node, fn);
 		rcu_assign_pointer(*ins, rt);
 		if (!info->skip_notify)
@@ -1136,7 +1136,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
 		if (err)
 			return err;
 
-		atomic_inc(&rt->fib6_ref);
+		fib6_info_hold(rt);
 		rcu_assign_pointer(rt->fib6_node, fn);
 		rt->fib6_next = iter->fib6_next;
 		rcu_assign_pointer(*ins, rt);
@@ -1278,7 +1278,7 @@ int fib6_add(struct fib6_node *root, struct fib6_info *rt,
 			if (!sfn)
 				goto failure;
 
-			atomic_inc(&info->nl_net->ipv6.fib6_null_entry->fib6_ref);
+			fib6_info_hold(info->nl_net->ipv6.fib6_null_entry);
 			rcu_assign_pointer(sfn->leaf,
 					   info->nl_net->ipv6.fib6_null_entry);
 			sfn->fn_flags = RTN_ROOT;
@@ -1321,7 +1321,7 @@ int fib6_add(struct fib6_node *root, struct fib6_info *rt,
 				rcu_assign_pointer(fn->leaf,
 					    info->nl_net->ipv6.fib6_null_entry);
 			} else {
-				atomic_inc(&rt->fib6_ref);
+				fib6_info_hold(rt);
 				rcu_assign_pointer(fn->leaf, rt);
 			}
 		}
-- 
2.21.0.593.g511ec345e18-goog

[PATCH net-next 3/3] ipv6: convert fib6_ref to refcount_t

[Eric Dumazet]

We suspect some issues involving fib6_ref 0 -> 1 transitions might
cause strange syzbot reports.

Lets convert fib6_ref to refcount_t to catch them earlier.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Wei Wang <weiwan@google.com>
---
 include/net/ip6_fib.h | 8 ++++----
 net/ipv6/ip6_fib.c    | 6 +++---
 net/ipv6/route.c      | 2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 6b7557b71c8caef7785dd0c8c52dfb92cf4e8c52..355a47bfc452aa150f537522dfb77ffa50bcb9ec 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -146,7 +146,7 @@ struct fib6_info {
 	struct list_head		fib6_siblings;
 	unsigned int			fib6_nsiblings;
 
-	atomic_t			fib6_ref;
+	refcount_t			fib6_ref;
 	unsigned long			expires;
 	struct dst_metrics		*fib6_metrics;
 #define fib6_pmtu		fib6_metrics->metrics[RTAX_MTU-1]
@@ -284,17 +284,17 @@ void fib6_info_destroy_rcu(struct rcu_head *head);
 
 static inline void fib6_info_hold(struct fib6_info *f6i)
 {
-	atomic_inc(&f6i->fib6_ref);
+	refcount_inc(&f6i->fib6_ref);
 }
 
 static inline bool fib6_info_hold_safe(struct fib6_info *f6i)
 {
-	return atomic_inc_not_zero(&f6i->fib6_ref);
+	return refcount_inc_not_zero(&f6i->fib6_ref);
 }
 
 static inline void fib6_info_release(struct fib6_info *f6i)
 {
-	if (f6i && atomic_dec_and_test(&f6i->fib6_ref))
+	if (f6i && refcount_dec_and_test(&f6i->fib6_ref))
 		call_rcu(&f6i->rcu, fib6_info_destroy_rcu);
 }
 
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index a5e83593e0e45c2762eca85d04757f9d8e118e0f..a8919c217cc214821e039786492ed284552cb0b4 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -162,7 +162,7 @@ struct fib6_info *fib6_info_alloc(gfp_t gfp_flags)
 	}
 
 	INIT_LIST_HEAD(&f6i->fib6_siblings);
-	atomic_set(&f6i->fib6_ref, 1);
+	refcount_set(&f6i->fib6_ref, 1);
 
 	return f6i;
 }
@@ -929,7 +929,7 @@ static void fib6_purge_rt(struct fib6_info *rt, struct fib6_node *fn,
 {
 	struct fib6_table *table = rt->fib6_table;
 
-	if (atomic_read(&rt->fib6_ref) != 1) {
+	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,
 		 * which must be released in time. So, scan ascendant nodes
@@ -2311,7 +2311,7 @@ static int ipv6_route_seq_show(struct seq_file *seq, void *v)
 
 	dev = rt->fib6_nh.fib_nh_dev;
 	seq_printf(seq, " %08x %08x %08x %08x %8s\n",
-		   rt->fib6_metric, atomic_read(&rt->fib6_ref), 0,
+		   rt->fib6_metric, refcount_read(&rt->fib6_ref), 0,
 		   flags, dev ? dev->name : "");
 	iter->w.leaf = NULL;
 	return 0;
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index e8c73b7782cdc5a2c4ca5ef50aca4c8d8556c7be..7ce14a4c3d89955d6e6468bb3cdac5efa394839b 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -296,7 +296,7 @@ static const struct fib6_info fib6_null_entry_template = {
 	.fib6_flags	= (RTF_REJECT | RTF_NONEXTHOP),
 	.fib6_protocol  = RTPROT_KERNEL,
 	.fib6_metric	= ~(u32)0,
-	.fib6_ref	= ATOMIC_INIT(1),
+	.fib6_ref	= REFCOUNT_INIT(1),
 	.fib6_type	= RTN_UNREACHABLE,
 	.fib6_metrics	= (struct dst_metrics *)&dst_default_metrics,
 };
-- 
2.21.0.593.g511ec345e18-goog

Re: [PATCH net-next 1/3] ipv6: fib6_info_destroy_rcu() cleanup

[Wei Wang]

On Mon, Apr 22, 2019 at 6:35 PM Eric Dumazet <edumazet@google.com> wrote:
>
> We do not need to clear f6i->rt6i_exception_bucket right before
> freeing f6i.
>
> Note that f6i->rt6i_exception_bucket is properly protected by
> f6i->exception_bucket_flushed being set to one in rt6_flush_exceptions()
> under the protection of rt6_exception_lock.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Wei Wang <weiwan@google.com>
> ---

Acked-by: Wei Wang <weiwan@google.com>

>  net/ipv6/ip6_fib.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index b47e15df9769ccb2919e6ec0de5edd93f2ae518f..55193859152969794dab3df02637217a7f21016f 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -175,10 +175,7 @@ void fib6_info_destroy_rcu(struct rcu_head *head)
>         WARN_ON(f6i->fib6_node);
>
>         bucket = rcu_dereference_protected(f6i->rt6i_exception_bucket, 1);
> -       if (bucket) {
> -               f6i->rt6i_exception_bucket = NULL;
> -               kfree(bucket);
> -       }
> +       kfree(bucket);
>
>         if (f6i->rt6i_pcpu) {
>                 int cpu;
> --
> 2.21.0.593.g511ec345e18-goog
>

Re: [PATCH net-next 2/3] ipv6: broadly use fib6_info_hold() helper

[Wei Wang]

On Mon, Apr 22, 2019 at 6:35 PM Eric Dumazet <edumazet@google.com> wrote:
>
> Instead of using atomic_inc(), prefer fib6_info_hold()
> so that upcoming refcount_t conversion is simpler.
>
> Only fib6_info_alloc() is using atomic_set() since we
> just allocated a new object.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Wei Wang <weiwan@google.com>
> ---

Acked-by: Wei Wang <weiwan@google.com>

>  net/ipv6/ip6_fib.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index 55193859152969794dab3df02637217a7f21016f..a5e83593e0e45c2762eca85d04757f9d8e118e0f 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -162,7 +162,7 @@ struct fib6_info *fib6_info_alloc(gfp_t gfp_flags)
>         }
>
>         INIT_LIST_HEAD(&f6i->fib6_siblings);
> -       atomic_inc(&f6i->fib6_ref);
> +       atomic_set(&f6i->fib6_ref, 1);
>
>         return f6i;
>  }
> @@ -846,8 +846,8 @@ static struct fib6_node *fib6_add_1(struct net *net,
>
>                 RCU_INIT_POINTER(in->parent, pn);
>                 in->leaf = fn->leaf;
> -               atomic_inc(&rcu_dereference_protected(in->leaf,
> -                               lockdep_is_held(&table->tb6_lock))->fib6_ref);
> +               fib6_info_hold(rcu_dereference_protected(in->leaf,
> +                               lockdep_is_held(&table->tb6_lock)));
>
>                 /* update parent pointer */
>                 if (dir)
> @@ -942,7 +942,7 @@ static void fib6_purge_rt(struct fib6_info *rt, struct fib6_node *fn,
>                         struct fib6_info *new_leaf;
>                         if (!(fn->fn_flags & RTN_RTINFO) && leaf == rt) {
>                                 new_leaf = fib6_find_prefix(net, table, fn);
> -                               atomic_inc(&new_leaf->fib6_ref);
> +                               fib6_info_hold(new_leaf);
>
>                                 rcu_assign_pointer(fn->leaf, new_leaf);
>                                 fib6_info_release(rt);
> @@ -1108,7 +1108,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
>                         return err;
>
>                 rcu_assign_pointer(rt->fib6_next, iter);
> -               atomic_inc(&rt->fib6_ref);
> +               fib6_info_hold(rt);
>                 rcu_assign_pointer(rt->fib6_node, fn);
>                 rcu_assign_pointer(*ins, rt);
>                 if (!info->skip_notify)
> @@ -1136,7 +1136,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
>                 if (err)
>                         return err;
>
> -               atomic_inc(&rt->fib6_ref);
> +               fib6_info_hold(rt);
>                 rcu_assign_pointer(rt->fib6_node, fn);
>                 rt->fib6_next = iter->fib6_next;
>                 rcu_assign_pointer(*ins, rt);
> @@ -1278,7 +1278,7 @@ int fib6_add(struct fib6_node *root, struct fib6_info *rt,
>                         if (!sfn)
>                                 goto failure;
>
> -                       atomic_inc(&info->nl_net->ipv6.fib6_null_entry->fib6_ref);
> +                       fib6_info_hold(info->nl_net->ipv6.fib6_null_entry);
>                         rcu_assign_pointer(sfn->leaf,
>                                            info->nl_net->ipv6.fib6_null_entry);
>                         sfn->fn_flags = RTN_ROOT;
> @@ -1321,7 +1321,7 @@ int fib6_add(struct fib6_node *root, struct fib6_info *rt,
>                                 rcu_assign_pointer(fn->leaf,
>                                             info->nl_net->ipv6.fib6_null_entry);
>                         } else {
> -                               atomic_inc(&rt->fib6_ref);
> +                               fib6_info_hold(rt);
>                                 rcu_assign_pointer(fn->leaf, rt);
>                         }
>                 }
> --
> 2.21.0.593.g511ec345e18-goog
>

Re: [PATCH net-next 3/3] ipv6: convert fib6_ref to refcount_t

[Wei Wang]

On Mon, Apr 22, 2019 at 6:35 PM Eric Dumazet <edumazet@google.com> wrote:
>
> We suspect some issues involving fib6_ref 0 -> 1 transitions might
> cause strange syzbot reports.
>
> Lets convert fib6_ref to refcount_t to catch them earlier.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Wei Wang <weiwan@google.com>
> ---
Acked-by: Wei Wang <weiwan@google.com>

Thanks Eric.

>  include/net/ip6_fib.h | 8 ++++----
>  net/ipv6/ip6_fib.c    | 6 +++---
>  net/ipv6/route.c      | 2 +-
>  3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
> index 6b7557b71c8caef7785dd0c8c52dfb92cf4e8c52..355a47bfc452aa150f537522dfb77ffa50bcb9ec 100644
> --- a/include/net/ip6_fib.h
> +++ b/include/net/ip6_fib.h
> @@ -146,7 +146,7 @@ struct fib6_info {
>         struct list_head                fib6_siblings;
>         unsigned int                    fib6_nsiblings;
>
> -       atomic_t                        fib6_ref;
> +       refcount_t                      fib6_ref;
>         unsigned long                   expires;
>         struct dst_metrics              *fib6_metrics;
>  #define fib6_pmtu              fib6_metrics->metrics[RTAX_MTU-1]
> @@ -284,17 +284,17 @@ void fib6_info_destroy_rcu(struct rcu_head *head);
>
>  static inline void fib6_info_hold(struct fib6_info *f6i)
>  {
> -       atomic_inc(&f6i->fib6_ref);
> +       refcount_inc(&f6i->fib6_ref);
>  }
>
>  static inline bool fib6_info_hold_safe(struct fib6_info *f6i)
>  {
> -       return atomic_inc_not_zero(&f6i->fib6_ref);
> +       return refcount_inc_not_zero(&f6i->fib6_ref);
>  }
>
>  static inline void fib6_info_release(struct fib6_info *f6i)
>  {
> -       if (f6i && atomic_dec_and_test(&f6i->fib6_ref))
> +       if (f6i && refcount_dec_and_test(&f6i->fib6_ref))
>                 call_rcu(&f6i->rcu, fib6_info_destroy_rcu);
>  }
>
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index a5e83593e0e45c2762eca85d04757f9d8e118e0f..a8919c217cc214821e039786492ed284552cb0b4 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -162,7 +162,7 @@ struct fib6_info *fib6_info_alloc(gfp_t gfp_flags)
>         }
>
>         INIT_LIST_HEAD(&f6i->fib6_siblings);
> -       atomic_set(&f6i->fib6_ref, 1);
> +       refcount_set(&f6i->fib6_ref, 1);
>
>         return f6i;
>  }
> @@ -929,7 +929,7 @@ static void fib6_purge_rt(struct fib6_info *rt, struct fib6_node *fn,
>  {
>         struct fib6_table *table = rt->fib6_table;
>
> -       if (atomic_read(&rt->fib6_ref) != 1) {
> +       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,
>                  * which must be released in time. So, scan ascendant nodes
> @@ -2311,7 +2311,7 @@ static int ipv6_route_seq_show(struct seq_file *seq, void *v)
>
>         dev = rt->fib6_nh.fib_nh_dev;
>         seq_printf(seq, " %08x %08x %08x %08x %8s\n",
> -                  rt->fib6_metric, atomic_read(&rt->fib6_ref), 0,
> +                  rt->fib6_metric, refcount_read(&rt->fib6_ref), 0,
>                    flags, dev ? dev->name : "");
>         iter->w.leaf = NULL;
>         return 0;
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index e8c73b7782cdc5a2c4ca5ef50aca4c8d8556c7be..7ce14a4c3d89955d6e6468bb3cdac5efa394839b 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -296,7 +296,7 @@ static const struct fib6_info fib6_null_entry_template = {
>         .fib6_flags     = (RTF_REJECT | RTF_NONEXTHOP),
>         .fib6_protocol  = RTPROT_KERNEL,
>         .fib6_metric    = ~(u32)0,
> -       .fib6_ref       = ATOMIC_INIT(1),
> +       .fib6_ref       = REFCOUNT_INIT(1),
>         .fib6_type      = RTN_UNREACHABLE,
>         .fib6_metrics   = (struct dst_metrics *)&dst_default_metrics,
>  };
> --
> 2.21.0.593.g511ec345e18-goog
>

Re: [PATCH net-next 1/3] ipv6: fib6_info_destroy_rcu() cleanup

[David Ahern]

On 4/22/19 7:35 PM, Eric Dumazet wrote:
> We do not need to clear f6i->rt6i_exception_bucket right before
> freeing f6i.
> 
> Note that f6i->rt6i_exception_bucket is properly protected by
> f6i->exception_bucket_flushed being set to one in rt6_flush_exceptions()
> under the protection of rt6_exception_lock.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Wei Wang <weiwan@google.com>
> ---
>  net/ipv6/ip6_fib.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 

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

Re: [PATCH net-next 2/3] ipv6: broadly use fib6_info_hold() helper

[David Ahern]

On 4/22/19 7:35 PM, Eric Dumazet wrote:
> Instead of using atomic_inc(), prefer fib6_info_hold()
> so that upcoming refcount_t conversion is simpler.
> 
> Only fib6_info_alloc() is using atomic_set() since we
> just allocated a new object.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Wei Wang <weiwan@google.com>
> ---
>  net/ipv6/ip6_fib.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 

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

Re: [PATCH net-next 3/3] ipv6: convert fib6_ref to refcount_t

[David Ahern]

On 4/22/19 7:35 PM, Eric Dumazet wrote:
> We suspect some issues involving fib6_ref 0 -> 1 transitions might
> cause strange syzbot reports.
> 
> Lets convert fib6_ref to refcount_t to catch them earlier.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Wei Wang <weiwan@google.com>
> ---
>  include/net/ip6_fib.h | 8 ++++----
>  net/ipv6/ip6_fib.c    | 6 +++---
>  net/ipv6/route.c      | 2 +-
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 

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

Re: [PATCH net-next 1/3] ipv6: fib6_info_destroy_rcu() cleanup

[Eric Dumazet]

On Tue, Apr 23, 2019 at 9:33 AM David Ahern <dsahern@gmail.com> wrote:
>
> On 4/22/19 7:35 PM, Eric Dumazet wrote:
> > We do not need to clear f6i->rt6i_exception_bucket right before
> > freeing f6i.
> >
> > Note that f6i->rt6i_exception_bucket is properly protected by
> > f6i->exception_bucket_flushed being set to one in rt6_flush_exceptions()
> > under the protection of rt6_exception_lock.
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Wei Wang <weiwan@google.com>
> > ---
> >  net/ipv6/ip6_fib.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
>
> Reviewed-by: David Ahern <dsahern@gmail.com>

Thanks David

I believe we have a problem with f6i->rt6i_pcpu rules :

It seems concurrent lookups can happily use it while
the call_rcu() is ongoing before fib6_info_destroy_rcu() is finally called.

We lack proper RCU rules here. Concurrent lookups should be warned
that the destroy operation is ongoing.

One solution could look at fib6_ref value : If it is 0, then we are
not allowed to mess with rt6i_pcpu.

This is why I was looking at this function :/

BUG: KASAN: user-memory-access in atomic_read
include/asm-generic/atomic-instrumented.h:26 [inline]
BUG: KASAN: user-memory-access in atomic_fetch_add_unless
include/linux/atomic-fallback.h:1086 [inline]
BUG: KASAN: user-memory-access in atomic_add_unless
include/linux/atomic-fallback.h:1111 [inline]
BUG: KASAN: user-memory-access in atomic_inc_not_zero
include/linux/atomic-fallback.h:1127 [inline]
BUG: KASAN: user-memory-access in dst_hold_safe include/net/dst.h:308 [inline]
BUG: KASAN: user-memory-access in ip6_hold_safe+0xb3/0x3a0 net/ipv6/route.c:1023
Read of size 4 at addr 00000000000191e0 by task syz-executor.0/28086

CPU: 0 PID: 28086 Comm: syz-executor.0 Not tainted 5.1.0-rc5+ #176
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x172/0x1f0 lib/dump_stack.c:113
 kasan_report.cold+0x5/0x40 mm/kasan/report.c:321
 check_memory_region_inline mm/kasan/generic.c:185 [inline]
 check_memory_region+0x123/0x190 mm/kasan/generic.c:191
 kasan_check_read+0x11/0x20 mm/kasan/common.c:102
 atomic_read include/asm-generic/atomic-instrumented.h:26 [inline]
 atomic_fetch_add_unless include/linux/atomic-fallback.h:1086 [inline]
 atomic_add_unless include/linux/atomic-fallback.h:1111 [inline]
 atomic_inc_not_zero include/linux/atomic-fallback.h:1127 [inline]
 dst_hold_safe include/net/dst.h:308 [inline]
 ip6_hold_safe+0xb3/0x3a0 net/ipv6/route.c:1023
 rt6_get_pcpu_route net/ipv6/route.c:1245 [inline]
 ip6_pol_route+0x348/0x1040 net/ipv6/route.c:1906
 ip6_pol_route_output+0x54/0x70 net/ipv6/route.c:2082
 fib6_rule_lookup+0x269/0x560 net/ipv6/fib6_rules.c:122
 ip6_route_output_flags+0x2c4/0x350 net/ipv6/route.c:2111
 ip6_route_output include/net/ip6_route.h:88 [inline]
 icmpv6_xrlim_allow net/ipv6/icmp.c:213 [inline]
 icmp6_send+0x1150/0x21f0 net/ipv6/icmp.c:541
 icmpv6_send+0xee/0x230 net/ipv6/ip6_icmp.c:43
 __udp6_lib_rcv+0x1ad1/0x2cc0 net/ipv6/udp.c:939
 udpv6_rcv+0x22/0x30 net/ipv6/udp.c:1028
 ip6_protocol_deliver_rcu+0x303/0x16c0 net/ipv6/ip6_input.c:394
 ip6_input_finish+0x84/0x170 net/ipv6/ip6_input.c:434
 NF_HOOK include/linux/netfilter.h:289 [inline]
 NF_HOOK include/linux/netfilter.h:283 [inline]
 ip6_input+0xe4/0x3f0 net/ipv6/ip6_input.c:443
 dst_input include/net/dst.h:450 [inline]
 ip6_rcv_finish+0x1e7/0x320 net/ipv6/ip6_input.c:76
 NF_HOOK include/linux/netfilter.h:289 [inline]
 NF_HOOK include/linux/netfilter.h:283 [inline]
 ipv6_rcv+0x10e/0x420 net/ipv6/ip6_input.c:272
 __netif_receive_skb_one_core+0x115/0x1a0 net/core/dev.c:4987
 __netif_receive_skb+0x2c/0x1c0 net/core/dev.c:5099
 netif_receive_skb_internal+0x117/0x660 net/core/dev.c:5202
 napi_frags_finish net/core/dev.c:5769 [inline]
 napi_gro_frags+0xade/0xd10 net/core/dev.c:5843
 tun_get_user+0x2f24/0x3fb0 drivers/net/tun.c:1981
 tun_chr_write_iter+0xbd/0x156 drivers/net/tun.c:2027
 call_write_iter include/linux/fs.h:1866 [inline]
 do_iter_readv_writev+0x5e1/0x8e0 fs/read_write.c:681
 do_iter_write fs/read_write.c:957 [inline]
 do_iter_write+0x184/0x610 fs/read_write.c:938
 vfs_writev+0x1b3/0x2f0 fs/read_write.c:1002
 do_writev+0x15e/0x370 fs/read_write.c:1037
 __do_sys_writev fs/read_write.c:1110 [inline]
 __se_sys_writev fs/read_write.c:1107 [inline]
 __x64_sys_writev+0x75/0xb0 fs/read_write.c:1107

Re: [PATCH net-next 1/3] ipv6: fib6_info_destroy_rcu() cleanup

[David Ahern]

On 4/23/19 11:40 AM, Eric Dumazet wrote:
> I believe we have a problem with f6i->rt6i_pcpu rules :
> 
> It seems concurrent lookups can happily use it while
> the call_rcu() is ongoing before fib6_info_destroy_rcu() is finally called.
> 
> We lack proper RCU rules here. Concurrent lookups should be warned
> that the destroy operation is ongoing.
> 
> One solution could look at fib6_ref value : If it is 0, then we are
> not allowed to mess with rt6i_pcpu.

ok. I'll take a look as part of the testing I am doing for other changes.

Re: [PATCH net-next 0/3] ipv6: fib6_ref conversion to refcount_t

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Mon, 22 Apr 2019 18:35:00 -0700

> We are chasing use-after-free in IPv6 that could have their origin
> in fib6_ref 0 -> 1 transitions.
> 
> This patch series should help finding the root causes if these
> illegal transitions ever happen.

Series applied, thanks Eric.