diff mbox series

[net] net/ipv6: Revert remove expired routes with a separated list of routes

Message ID 20231217185505.22867-1-dsahern@kernel.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net] net/ipv6: Revert remove expired routes with a separated list of routes | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/apply fail Patch does not apply to net

Commit Message

David Ahern Dec. 17, 2023, 6:55 p.m. UTC 
Revert the remainder of 5a08d0065a915 which added a warn on if a fib
entry is still on the gc_link list, and then revert  all of the commit
in the Fixes tag. The commit has some race conditions given how expires
is managed on a fib6_info in relation to timer start, adding the entry
to the gc list and setting the timer value leading to UAF. Revert
the commit and try again in a later release.

Fixes: 3dec89b14d37 ("net/ipv6: Remove expired routes with a separated list of routes")
Cc: Kui-Feng Lee <thinker.li@gmail.com>
Signed-off-by: David Ahern <dsahern@kernel.org>
---
 include/net/ip6_fib.h | 68 +++++++++----------------------------------
 net/ipv6/ip6_fib.c    | 55 ++++------------------------------
 net/ipv6/route.c      |  6 ++--
 3 files changed, 23 insertions(+), 106 deletions(-)

Comments

Eric Dumazet Dec. 18, 2023, 4:31 p.m. UTC | #1 
On Sun, Dec 17, 2023 at 7:55 PM David Ahern <dsahern@kernel.org> wrote:
>
> Revert the remainder of 5a08d0065a915 which added a warn on if a fib
> entry is still on the gc_link list, and then revert  all of the commit
> in the Fixes tag. The commit has some race conditions given how expires
> is managed on a fib6_info in relation to timer start, adding the entry
> to the gc list and setting the timer value leading to UAF. Revert
> the commit and try again in a later release.
>
> Fixes: 3dec89b14d37 ("net/ipv6: Remove expired routes with a separated list of routes")
> Cc: Kui-Feng Lee <thinker.li@gmail.com>
> Signed-off-by: David Ahern <dsahern@kernel.org>

I guess we need two patches, one in net-next, one in net ?

If this patch targets net tree, it probably should be a plain revert
(no need to mention 5a08d0065a915)
Kui-Feng Lee Dec. 19, 2023, 1:14 a.m. UTC | #2 
On 12/17/23 10:55, David Ahern wrote:
> Revert the remainder of 5a08d0065a915 which added a warn on if a fib
> entry is still on the gc_link list, and then revert  all of the commit
> in the Fixes tag. The commit has some race conditions given how expires
> is managed on a fib6_info in relation to timer start, adding the entry
> to the gc list and setting the timer value leading to UAF. Revert
> the commit and try again in a later release.

May I know what your concerns are about the patch I provided?
Even I try it again later, I still need to know what I miss and should
address.
David Ahern Dec. 19, 2023, 2:38 a.m. UTC | #3 
On 12/18/23 6:14 PM, Kui-Feng Lee wrote:
> 
> 
> On 12/17/23 10:55, David Ahern wrote:
>> Revert the remainder of 5a08d0065a915 which added a warn on if a fib
>> entry is still on the gc_link list, and then revert  all of the commit
>> in the Fixes tag. The commit has some race conditions given how expires
>> is managed on a fib6_info in relation to timer start, adding the entry
>> to the gc list and setting the timer value leading to UAF. Revert
>> the commit and try again in a later release.
> 
> May I know what your concerns are about the patch I provided?
> Even I try it again later, I still need to know what I miss and should
> address.

This is a judgement call based on 6.7-rc number and upcoming holidays
with people offline. A bug fix is needed for a performance optimization;
the smart response here is to revert the patch and try again after the
holidays.
Kui-Feng Lee Dec. 19, 2023, 2:45 a.m. UTC | #4 
On 12/18/23 18:38, David Ahern wrote:
> On 12/18/23 6:14 PM, Kui-Feng Lee wrote:
>>
>>
>> On 12/17/23 10:55, David Ahern wrote:
>>> Revert the remainder of 5a08d0065a915 which added a warn on if a fib
>>> entry is still on the gc_link list, and then revert  all of the commit
>>> in the Fixes tag. The commit has some race conditions given how expires
>>> is managed on a fib6_info in relation to timer start, adding the entry
>>> to the gc list and setting the timer value leading to UAF. Revert
>>> the commit and try again in a later release.
>>
>> May I know what your concerns are about the patch I provided?
>> Even I try it again later, I still need to know what I miss and should
>> address.
> 
> This is a judgement call based on 6.7-rc number and upcoming holidays
> with people offline. A bug fix is needed for a performance optimization;
> the smart response here is to revert the patch and try again after the
> holidays.
Got it! Thanks!
David Ahern Dec. 19, 2023, 3:22 a.m. UTC | #5 
On 12/18/23 7:45 PM, Kui-Feng Lee wrote:
> 
> 
> On 12/18/23 18:38, David Ahern wrote:
>> On 12/18/23 6:14 PM, Kui-Feng Lee wrote:
>>>
>>>
>>> On 12/17/23 10:55, David Ahern wrote:
>>>> Revert the remainder of 5a08d0065a915 which added a warn on if a fib
>>>> entry is still on the gc_link list, and then revert  all of the commit
>>>> in the Fixes tag. The commit has some race conditions given how expires
>>>> is managed on a fib6_info in relation to timer start, adding the entry
>>>> to the gc list and setting the timer value leading to UAF. Revert
>>>> the commit and try again in a later release.
>>>
>>> May I know what your concerns are about the patch I provided?
>>> Even I try it again later, I still need to know what I miss and should
>>> address.
>>
>> This is a judgement call based on 6.7-rc number and upcoming holidays
>> with people offline. A bug fix is needed for a performance optimization;
>> the smart response here is to revert the patch and try again after the
>> holidays.
> Got it! Thanks!

In January, send a new set (RFC if it is still the merge window) with
the following:

1. audit all of the uses of RTF_EXPIRES. Some of the places need to be
cleaned up first. For example, rt6_add_dflt_router sets RTF_EXPIRES flag
but does not set fc_expires. I believe that function can be updated to
take the expires value and removes the need to set it later in
ndisc_router_discovery. See if the management of expires value can be
consolidated under the table lock.

2. Use gc_list instead of gc_link to be consistent with other gc lists
(and the fact that it is a list)

3. Add selftests using veth and namespaces to test RAs. It can depend on
and check for the existence of the ra or radvd command; the key is to
attempt a stress test on the router discovery path. Similarly for addrconf.

That is in addition to your new selftests in the last set you sent.
diff mbox series

Patch

diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 95ed495c3a40..9ba6413fd2e3 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -179,9 +179,6 @@  struct fib6_info {
 
 	refcount_t			fib6_ref;
 	unsigned long			expires;
-
-	struct hlist_node		gc_link;
-
 	struct dst_metrics		*fib6_metrics;
 #define fib6_pmtu		fib6_metrics->metrics[RTAX_MTU-1]
 
@@ -250,6 +247,19 @@  static inline bool fib6_requires_src(const struct fib6_info *rt)
 	return rt->fib6_src.plen > 0;
 }
 
+static inline void fib6_clean_expires(struct fib6_info *f6i)
+{
+	f6i->fib6_flags &= ~RTF_EXPIRES;
+	f6i->expires = 0;
+}
+
+static inline void fib6_set_expires(struct fib6_info *f6i,
+				    unsigned long expires)
+{
+	f6i->expires = expires;
+	f6i->fib6_flags |= RTF_EXPIRES;
+}
+
 static inline bool fib6_check_expired(const struct fib6_info *f6i)
 {
 	if (f6i->fib6_flags & RTF_EXPIRES)
@@ -257,11 +267,6 @@  static inline bool fib6_check_expired(const struct fib6_info *f6i)
 	return false;
 }
 
-static inline bool fib6_has_expires(const struct fib6_info *f6i)
-{
-	return f6i->fib6_flags & RTF_EXPIRES;
-}
-
 /* Function to safely get fn->fn_sernum for passed in rt
  * and store result in passed in cookie.
  * Return true if we can get cookie safely
@@ -328,10 +333,8 @@  static inline bool fib6_info_hold_safe(struct fib6_info *f6i)
 
 static inline void fib6_info_release(struct fib6_info *f6i)
 {
-	if (f6i && refcount_dec_and_test(&f6i->fib6_ref)) {
-		DEBUG_NET_WARN_ON_ONCE(!hlist_unhashed(&f6i->gc_link));
+	if (f6i && refcount_dec_and_test(&f6i->fib6_ref))
 		call_rcu(&f6i->rcu, fib6_info_destroy_rcu);
-	}
 }
 
 enum fib6_walk_state {
@@ -385,7 +388,6 @@  struct fib6_table {
 	struct inet_peer_base	tb6_peers;
 	unsigned int		flags;
 	unsigned int		fib_seq;
-	struct hlist_head       tb6_gc_hlist;	/* GC candidates */
 #define RT6_TABLE_HAS_DFLT_ROUTER	BIT(0)
 };
 
@@ -502,48 +504,6 @@  void fib6_gc_cleanup(void);
 
 int fib6_init(void);
 
-/* fib6_info must be locked by the caller, and fib6_info->fib6_table can be
- * NULL.
- */
-static inline void fib6_set_expires_locked(struct fib6_info *f6i,
-					   unsigned long expires)
-{
-	struct fib6_table *tb6;
-
-	tb6 = f6i->fib6_table;
-	f6i->expires = expires;
-	if (tb6 && !fib6_has_expires(f6i))
-		hlist_add_head(&f6i->gc_link, &tb6->tb6_gc_hlist);
-	f6i->fib6_flags |= RTF_EXPIRES;
-}
-
-/* fib6_info must be locked by the caller, and fib6_info->fib6_table can be
- * NULL.  If fib6_table is NULL, the fib6_info will no be inserted into the
- * list of GC candidates until it is inserted into a table.
- */
-static inline void fib6_set_expires(struct fib6_info *f6i,
-				    unsigned long expires)
-{
-	spin_lock_bh(&f6i->fib6_table->tb6_lock);
-	fib6_set_expires_locked(f6i, expires);
-	spin_unlock_bh(&f6i->fib6_table->tb6_lock);
-}
-
-static inline void fib6_clean_expires_locked(struct fib6_info *f6i)
-{
-	if (fib6_has_expires(f6i))
-		hlist_del_init(&f6i->gc_link);
-	f6i->fib6_flags &= ~RTF_EXPIRES;
-	f6i->expires = 0;
-}
-
-static inline void fib6_clean_expires(struct fib6_info *f6i)
-{
-	spin_lock_bh(&f6i->fib6_table->tb6_lock);
-	fib6_clean_expires_locked(f6i);
-	spin_unlock_bh(&f6i->fib6_table->tb6_lock);
-}
-
 struct ipv6_route_iter {
 	struct seq_net_private p;
 	struct fib6_walker w;
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 7772f42ff2b9..4fc2cae0d116 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -160,8 +160,6 @@  struct fib6_info *fib6_info_alloc(gfp_t gfp_flags, bool with_fib6_nh)
 	INIT_LIST_HEAD(&f6i->fib6_siblings);
 	refcount_set(&f6i->fib6_ref, 1);
 
-	INIT_HLIST_NODE(&f6i->gc_link);
-
 	return f6i;
 }
 
@@ -248,7 +246,6 @@  static struct fib6_table *fib6_alloc_table(struct net *net, u32 id)
 				   net->ipv6.fib6_null_entry);
 		table->tb6_root.fn_flags = RTN_ROOT | RTN_TL_ROOT | RTN_RTINFO;
 		inet_peer_base_init(&table->tb6_peers);
-		INIT_HLIST_HEAD(&table->tb6_gc_hlist);
 	}
 
 	return table;
@@ -1060,8 +1057,6 @@  static void fib6_purge_rt(struct fib6_info *rt, struct fib6_node *fn,
 				    lockdep_is_held(&table->tb6_lock));
 		}
 	}
-
-	fib6_clean_expires_locked(rt);
 }
 
 /*
@@ -1123,10 +1118,9 @@  static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
 				if (!(iter->fib6_flags & RTF_EXPIRES))
 					return -EEXIST;
 				if (!(rt->fib6_flags & RTF_EXPIRES))
-					fib6_clean_expires_locked(iter);
+					fib6_clean_expires(iter);
 				else
-					fib6_set_expires_locked(iter,
-								rt->expires);
+					fib6_set_expires(iter, rt->expires);
 
 				if (rt->fib6_pmtu)
 					fib6_metric_set(iter, RTAX_MTU,
@@ -1485,10 +1479,6 @@  int fib6_add(struct fib6_node *root, struct fib6_info *rt,
 		if (rt->nh)
 			list_add(&rt->nh_list, &rt->nh->f6i_list);
 		__fib6_update_sernum_upto_root(rt, fib6_new_sernum(info->nl_net));
-
-		if (fib6_has_expires(rt))
-			hlist_add_head(&rt->gc_link, &table->tb6_gc_hlist);
-
 		fib6_start_gc(info->nl_net, rt);
 	}
 
@@ -2291,8 +2281,9 @@  static void fib6_flush_trees(struct net *net)
  *	Garbage collection
  */
 
-static int fib6_age(struct fib6_info *rt, struct fib6_gc_args *gc_args)
+static int fib6_age(struct fib6_info *rt, void *arg)
 {
+	struct fib6_gc_args *gc_args = arg;
 	unsigned long now = jiffies;
 
 	/*
@@ -2300,7 +2291,7 @@  static int fib6_age(struct fib6_info *rt, struct fib6_gc_args *gc_args)
 	 *	Routes are expired even if they are in use.
 	 */
 
-	if (fib6_has_expires(rt) && rt->expires) {
+	if (rt->fib6_flags & RTF_EXPIRES && rt->expires) {
 		if (time_after(now, rt->expires)) {
 			RT6_TRACE("expiring %p\n", rt);
 			return -1;
@@ -2317,40 +2308,6 @@  static int fib6_age(struct fib6_info *rt, struct fib6_gc_args *gc_args)
 	return 0;
 }
 
-static void fib6_gc_table(struct net *net,
-			  struct fib6_table *tb6,
-			  struct fib6_gc_args *gc_args)
-{
-	struct fib6_info *rt;
-	struct hlist_node *n;
-	struct nl_info info = {
-		.nl_net = net,
-		.skip_notify = false,
-	};
-
-	hlist_for_each_entry_safe(rt, n, &tb6->tb6_gc_hlist, gc_link)
-		if (fib6_age(rt, gc_args) == -1)
-			fib6_del(rt, &info);
-}
-
-static void fib6_gc_all(struct net *net, struct fib6_gc_args *gc_args)
-{
-	struct fib6_table *table;
-	struct hlist_head *head;
-	unsigned int h;
-
-	rcu_read_lock();
-	for (h = 0; h < FIB6_TABLE_HASHSZ; h++) {
-		head = &net->ipv6.fib_table_hash[h];
-		hlist_for_each_entry_rcu(table, head, tb6_hlist) {
-			spin_lock_bh(&table->tb6_lock);
-			fib6_gc_table(net, table, gc_args);
-			spin_unlock_bh(&table->tb6_lock);
-		}
-	}
-	rcu_read_unlock();
-}
-
 void fib6_run_gc(unsigned long expires, struct net *net, bool force)
 {
 	struct fib6_gc_args gc_args;
@@ -2366,7 +2323,7 @@  void fib6_run_gc(unsigned long expires, struct net *net, bool force)
 			  net->ipv6.sysctl.ip6_rt_gc_interval;
 	gc_args.more = 0;
 
-	fib6_gc_all(net, &gc_args);
+	fib6_clean_all(net, fib6_age, &gc_args);
 	now = jiffies;
 	net->ipv6.ip6_rt_last_gc = now;
 
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index b132feae3393..ea1dec8448fc 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3763,10 +3763,10 @@  static struct fib6_info *ip6_route_info_create(struct fib6_config *cfg,
 		rt->dst_nocount = true;
 
 	if (cfg->fc_flags & RTF_EXPIRES)
-		fib6_set_expires_locked(rt, jiffies +
-					clock_t_to_jiffies(cfg->fc_expires));
+		fib6_set_expires(rt, jiffies +
+				clock_t_to_jiffies(cfg->fc_expires));
 	else
-		fib6_clean_expires_locked(rt);
+		fib6_clean_expires(rt);
 
 	if (cfg->fc_protocol == RTPROT_UNSPEC)
 		cfg->fc_protocol = RTPROT_BOOT;