netdev - Re: [PATCH RESEND v2 net-next 02/14] ipv6: Get rid of RTNL for SIOCDELRT and RTM

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: <20250417174557.65721-1-kuniyu@amazon.com>
Date: Thu, 17 Apr 2025 10:45:53 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <pabeni@...hat.com>
CC: <davem@...emloft.net>, <dsahern@...nel.org>, <edumazet@...gle.com>,
	<horms@...nel.org>, <kuba@...nel.org>, <kuni1840@...il.com>,
	<kuniyu@...zon.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH RESEND v2 net-next 02/14] ipv6: Get rid of RTNL for SIOCDELRT and RTM_DELROUTE.

From: Paolo Abeni <pabeni@...hat.com>
Date: Thu, 17 Apr 2025 08:46:01 +0200
> On 4/16/25 8:45 PM, Kuniyuki Iwashima wrote:
> > From: Paolo Abeni <pabeni@...hat.com>
> >> but acquiring the rcu lock after grabbing the rcu protected struct
> >> is confusing. It should be good adding a comment or moving the rcu lock
> >> before the lookup (and dropping the RCU lock from fib6_get_table())
> > 
> > There are other callers of fib6_get_table(), so I'd move rcu_read_lock()
> > before it, and will look into them if we can drop it from fib6_get_table().
> 
> you could provide a RCU-lockless __fib6_get_table() variant, use it
> here, and with time (outside this series) such helper usage could be
> extended.

I think it's worth a separate patch, but now the number of patches
is 17, so I'll add a note in the commit message that we don't need
rcu for fib6_get_table() and will post a follow up series to convert
the lockless version in some places.

Thanks!

---8<---
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 7c87873ae211..e5b28a732796 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -430,6 +430,7 @@ struct fib6_entry_notifier_info {
  *	exported functions
  */
 
+struct fib6_table *__fib6_get_table(struct net *net, u32 id);
 struct fib6_table *fib6_get_table(struct net *net, u32 id);
 struct fib6_table *fib6_new_table(struct net *net, u32 id);
 struct dst_entry *fib6_rule_lookup(struct net *net, struct flowi6 *fl6,
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index bf727149fdec..ba3e290a9da4 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -265,27 +265,36 @@ struct fib6_table *fib6_new_table(struct net *net, u32 id)
 }
 EXPORT_SYMBOL_GPL(fib6_new_table);
 
-struct fib6_table *fib6_get_table(struct net *net, u32 id)
+struct fib6_table *__fib6_get_table(struct net *net, u32 id)
 {
-	struct fib6_table *tb;
 	struct hlist_head *head;
-	unsigned int h;
+	struct fib6_table *tb;
 
-	if (id == 0)
+	if (!id)
 		id = RT6_TABLE_MAIN;
-	h = id & (FIB6_TABLE_HASHSZ - 1);
-	rcu_read_lock();
-	head = &net->ipv6.fib_table_hash[h];
-	hlist_for_each_entry_rcu(tb, head, tb6_hlist) {
-		if (tb->tb6_id == id) {
-			rcu_read_unlock();
+
+	head = &net->ipv6.fib_table_hash[id & (FIB6_TABLE_HASHSZ - 1)];
+
+	/* Once allocated, fib6_table is not freed until netns dismantle, so
+	 * RCU is not required (but rcu_dereference_raw() is for data-race).
+	 */
+	hlist_for_each_entry_rcu(tb, head, tb6_hlist, true)
+		if (tb->tb6_id == id)
 			return tb;
-		}
-	}
-	rcu_read_unlock();
 
 	return NULL;
 }
+
+struct fib6_table *fib6_get_table(struct net *net, u32 id)
+{
+	struct fib6_table *tb;
+
+	rcu_read_lock();
+	tb = __fib6_get_table(net, id);
+	rcu_read_unlock();
+
+	return tb;
+}
 EXPORT_SYMBOL_GPL(fib6_get_table);
 
 static void __net_init fib6_tables_init(struct net *net)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 237e31f64a4a..af311c19dcc6 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -4076,7 +4076,7 @@ static int ip6_route_del(struct fib6_config *cfg,
 	struct fib6_node *fn;
 	int err = -ESRCH;
 
-	table = fib6_get_table(cfg->fc_nlinfo.nl_net, cfg->fc_table);
+	table = __fib6_get_table(cfg->fc_nlinfo.nl_net, cfg->fc_table);
 	if (!table) {
 		NL_SET_ERR_MSG(extack, "FIB table does not exist");
 		return err;
---8<---