netdev - [PATCH net-next v2 1/2] net/ipv6: insert a f6i to a GC list only if the f6i is in a fib6

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20231208194523.312416-2-thinker.li@gmail.com>
Date: Fri,  8 Dec 2023 11:45:22 -0800
From: thinker.li@...il.com
To: netdev@...r.kernel.org,
	martin.lau@...ux.dev,
	kernel-team@...a.com,
	davem@...emloft.net,
	kuba@...nel.org,
	pabeni@...hat.com,
	dsahern@...nel.org,
	edumazet@...gle.com
Cc: sinquersw@...il.com,
	kuifeng@...a.com,
	Kui-Feng Lee <thinker.li@...il.com>,
	syzbot+c15aa445274af8674f41@...kaller.appspotmail.com
Subject: [PATCH net-next v2 1/2] net/ipv6: insert a f6i to a GC list only if the f6i is in a fib6_table tree.

From: Kui-Feng Lee <thinker.li@...il.com>

Check f6i->fib6_node and hlist_unhashed(&f6i->gc_link) before inserting a
f6i (fib6_info) to tb6_gc_hlist.

The current implementation checks if f6i->fib6_table is not NULL to
determines if a f6i is on a tree, however it is not enough. When a f6i is
removed from a fib6_table, f6i->fib6_table is not reset. However, fib6_node
is always reset when a f6i is removed from a fib6_table and is set when a
f6i is added to a fib6_table. So, f6i->fib6_node is a reliable way to
determine if a f6i is on a tree.

The current implementation checks RTF_EXPIRES on f6i->fib6_flags to
determine if a f6i is on a GC list. It also consider if the f6i is on a
tree before making a conclusion. This is indirect and complicated. The new
solution is checking hlist_unhashed(&f6i->gc_link), a clear evidence.

Putting them together, these changes provide more reliable signals to
determines if a f6i should be added/or removed to a GC list.

Fixes: 3dec89b14d37 ("net/ipv6: Remove expired routes with a separated list of routes.")
Reported-by: syzbot+c15aa445274af8674f41@...kaller.appspotmail.com
Signed-off-by: Kui-Feng Lee <thinker.li@...il.com>
Cc: Eric Dumazet <edumazet@...gle.com>
Cc: dsahern@...nel.org
---
 include/net/ip6_fib.h | 46 ++++++++++++++++++++++++++++++++-----------
 net/ipv6/route.c      |  6 +++---
 2 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 1ba9f4ddf2f6..1213722c394f 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -500,21 +500,47 @@ 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)
+static inline void fib6_add_gc_list(struct fib6_info *f6i)
 {
 	struct fib6_table *tb6;
 
 	tb6 = f6i->fib6_table;
-	f6i->expires = expires;
-	if (tb6 && !fib6_has_expires(f6i))
+	if (tb6 &&
+	    rcu_dereference_protected(f6i->fib6_node,
+				      lockdep_is_held(&tb6->tb6_lock)) &&
+	    hlist_unhashed(&f6i->gc_link))
 		hlist_add_head(&f6i->gc_link, &tb6->tb6_gc_hlist);
+}
+
+static inline void fib6_del_gc_list(struct fib6_info *f6i)
+{
+	if (!hlist_unhashed(&f6i->gc_link))
+		hlist_del_init(&f6i->gc_link);
+}
+
+static inline void __fib6_set_expires(struct fib6_info *f6i,
+				      unsigned long expires)
+{
+	f6i->expires = expires;
 	f6i->fib6_flags |= RTF_EXPIRES;
 }
 
+static inline void __fib6_clean_expires(struct fib6_info *f6i)
+{
+	f6i->fib6_flags &= ~RTF_EXPIRES;
+	f6i->expires = 0;
+}
+
+/* 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)
+{
+	__fib6_set_expires(f6i, expires);
+	fib6_add_gc_list(f6i);
+}
+
 /* 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.
@@ -529,10 +555,8 @@ static inline void fib6_set_expires(struct fib6_info *f6i,
 
 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;
+	fib6_del_gc_list(f6i);
+	__fib6_clean_expires(f6i);
 }
 
 static inline void fib6_clean_expires(struct fib6_info *f6i)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index b132feae3393..dcaeb88d73aa 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;
-- 
2.34.1