[PATCH net v5 00/10] Lock RCU before calling ip6mr_get_table()

[PATCH net v5 00/10] Lock RCU before calling ip6mr_get_table()

[Stefan Wiehler]

Lock RCU before calling ip6mr_get_table() in several ip6mr functions.

v5:
  - add missing RCU locks in ip6mr_new_table(), ip6mr_mfc_seq_start(),
    ip6_mroute_setsockopt(), ip6_mroute_getsockopt() and
    ip6mr_rtm_getroute()
  - fix double RCU unlock in ip6mr_compat_ioctl()
  - always jump to out label in ip6mr_ioctl()
v4: https://patchwork.kernel.org/project/netdevbpf/cover/20241011074811.2308043-3-stefan.wiehler@nokia.com/
  - mention in commit message that ip6mr_vif_seq_stop() would be called
    in case ip6mr_vif_seq_start() returns an error
  - fix unitialised use of mrt variable
  - revert commit b6dd5acde3f1 ("ipv6: Fix suspicious RCU usage warning
    in ip6mr")
v3: https://patchwork.kernel.org/project/netdevbpf/patch/20241010090741.1980100-2-stefan.wiehler@nokia.com/
  - split into separate patches
v2: https://patchwork.kernel.org/project/netdevbpf/patch/20241001100119.230711-2-stefan.wiehler@nokia.com/
  - rebase on top of net tree
  - add Fixes tag
  - refactor out paths
v1: https://patchwork.kernel.org/project/netdevbpf/patch/20240605195355.363936-1-oss@malat.biz/

Stefan Wiehler (10):
  ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_vif_seq_start()
  ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_ioctl()
  ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_compat_ioctl()
  ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_get_route()
  ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_new_table()
  ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_mfc_seq_start()
  ip6mr: Lock RCU before ip6mr_get_table() call in
    ip6_mroute_setsockopt()
  ip6mr: Lock RCU before ip6mr_get_table() call in
    ip6_mroute_getsockopt()
  ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_rtm_getroute()
  Revert "ipv6: Fix suspicious RCU usage warning in ip6mr"

 net/ipv6/ip6mr.c | 125 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 80 insertions(+), 45 deletions(-)

-- 
2.42.0

[PATCH net v5 01/10] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_vif_seq_start()

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
must be done under RCU or RTNL lock.

In case ip6mr_vif_seq_start() fails with an error (currently not
possible, as RT6_TABLE_DFLT always exists, ip6mr_get_table() cannot fail
in this case), ip6mr_vif_seq_stop() would be called and release the RCU
lock.

Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv6/ip6mr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 2ce4ae0d8dc3..268e77196753 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -411,13 +411,13 @@ static void *ip6mr_vif_seq_start(struct seq_file *seq, loff_t *pos)
 	struct net *net = seq_file_net(seq);
 	struct mr_table *mrt;
 
+	rcu_read_lock();
 	mrt = ip6mr_get_table(net, RT6_TABLE_DFLT);
 	if (!mrt)
 		return ERR_PTR(-ENOENT);
 
 	iter->mrt = mrt;
 
-	rcu_read_lock();
 	return mr_vif_seq_start(seq, pos);
 }
 
-- 
2.42.0

[PATCH net v5 02/10] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_ioctl()

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to
ip6mr_get_table() must be done under RCU or RTNL lock.

Detected by Lockdep-RCU:

  [   48.834645] WARNING: suspicious RCU usage
  [   48.834647] 6.1.103-584209f6d5-nokia_sm_x86 #1 Tainted: G S         O
  [   48.834649] -----------------------------
  [   48.834651] /net/ipv6/ip6mr.c:132 RCU-list traversed in non-reader section!!
  [   48.834654]
                 other info that might help us debug this:

  [   48.834656]
                 rcu_scheduler_active = 2, debug_locks = 1
  [   48.834658] no locks held by radvd/5777.
  [   48.834660]
                 stack backtrace:
  [   48.834663] CPU: 0 PID: 5777 Comm: radvd Tainted: G S         O       6.1.103-584209f6d5-nokia_sm_x86 #1
  [   48.834666] Hardware name: Nokia Asil/Default string, BIOS 0ACNA113 06/07/2024
  [   48.834673] Call Trace:
  [   48.834674]  <TASK>
  [   48.834677]  dump_stack_lvl+0xb7/0xe9
  [   48.834687]  lockdep_rcu_suspicious.cold+0x2d/0x64
  [   48.834697]  ip6mr_get_table+0x9f/0xb0
  [   48.834704]  ip6mr_ioctl+0x50/0x360
  [   48.834713]  ? sk_ioctl+0x5f/0x1c0
  [   48.834719]  sk_ioctl+0x5f/0x1c0
  [   48.834723]  ? find_held_lock+0x2b/0x80
  [   48.834731]  sock_do_ioctl+0x7b/0x140
  [   48.834737]  ? proc_nr_files+0x30/0x30
  [   48.834744]  sock_ioctl+0x1f5/0x360
  [   48.834754]  __x64_sys_ioctl+0x8d/0xd0
  [   48.834760]  do_syscall_64+0x3c/0x90
  [   48.834765]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
  [   48.834769] RIP: 0033:0x7fda5f56e2ac                                                                   [   48.834773] Code: 1e fa 48 8d 44 24 08 48 89 54 24 e0 48 89 44 24 c0 48 8d 44 24 d0 48 89 44 24 c8 b8 1 0 00 00 00 c7 44 24 b8 10 00 00 00 0f 05 <3d> 00 f0 ff ff 89 c2 77 0b 89 d0 c3 0f 1f 84
  00 00 00 00 00 48 8b
  [   48.834776] RSP: 002b:00007fff52d4bda8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
  [   48.834782] RAX: ffffffffffffffda RBX: 000000000171cd80 RCX: 00007fda5f56e2ac
  [   48.834784] RDX: 00007fff52d4bdb0 RSI: 0000000000008913 RDI: 0000000000000003
  [   48.834787] RBP: 000000000171cd30 R08: 0000000000000007 R09: 000000000000003c
  [   48.834789] R10: 0000000000000004 R11: 0000000000000246 R12: 0000000000000003
  [   48.834791] R13: 0000000000000000 R14: 0000000000000004 R15: 000000000040d43c
  [   48.834802]  </TASK>

Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
---
 net/ipv6/ip6mr.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 268e77196753..2085342c1fcd 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -1884,47 +1884,56 @@ int ip6mr_ioctl(struct sock *sk, int cmd, void *arg)
 	struct mfc6_cache *c;
 	struct net *net = sock_net(sk);
 	struct mr_table *mrt;
+	int err;
 
+	rcu_read_lock();
 	mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
-	if (!mrt)
-		return -ENOENT;
+	if (!mrt) {
+		err = -ENOENT;
+		goto out;
+	}
 
 	switch (cmd) {
 	case SIOCGETMIFCNT_IN6:
 		vr = (struct sioc_mif_req6 *)arg;
-		if (vr->mifi >= mrt->maxvif)
-			return -EINVAL;
+		if (vr->mifi >= mrt->maxvif) {
+			err = -EINVAL;
+			goto out;
+		}
 		vr->mifi = array_index_nospec(vr->mifi, mrt->maxvif);
-		rcu_read_lock();
 		vif = &mrt->vif_table[vr->mifi];
 		if (VIF_EXISTS(mrt, vr->mifi)) {
 			vr->icount = READ_ONCE(vif->pkt_in);
 			vr->ocount = READ_ONCE(vif->pkt_out);
 			vr->ibytes = READ_ONCE(vif->bytes_in);
 			vr->obytes = READ_ONCE(vif->bytes_out);
-			rcu_read_unlock();
-			return 0;
+			err = 0;
+			goto out;
 		}
-		rcu_read_unlock();
-		return -EADDRNOTAVAIL;
+		err = -EADDRNOTAVAIL;
+		goto out;
 	case SIOCGETSGCNT_IN6:
 		sr = (struct sioc_sg_req6 *)arg;
 
-		rcu_read_lock();
 		c = ip6mr_cache_find(mrt, &sr->src.sin6_addr,
 				     &sr->grp.sin6_addr);
 		if (c) {
 			sr->pktcnt = c->_c.mfc_un.res.pkt;
 			sr->bytecnt = c->_c.mfc_un.res.bytes;
 			sr->wrong_if = c->_c.mfc_un.res.wrong_if;
-			rcu_read_unlock();
-			return 0;
+			err = 0;
+			goto out;
 		}
-		rcu_read_unlock();
-		return -EADDRNOTAVAIL;
+		err = -EADDRNOTAVAIL;
+		goto out;
 	default:
-		return -ENOIOCTLCMD;
+		err = -ENOIOCTLCMD;
+		goto out;
 	}
+
+out:
+	rcu_read_unlock();
+	return err;
 }
 
 #ifdef CONFIG_COMPAT
-- 
2.42.0

[PATCH net v5 03/10] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_compat_ioctl()

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
must be done under RCU or RTNL lock. Copy from user space must be
performed beforehand as we are not allowed to sleep under RCU lock.

Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
---
 net/ipv6/ip6mr.c | 49 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 2085342c1fcd..b84444040e0e 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -1961,19 +1961,36 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
 	struct mfc6_cache *c;
 	struct net *net = sock_net(sk);
 	struct mr_table *mrt;
-
-	mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
-	if (!mrt)
-		return -ENOENT;
+	int err;
 
 	switch (cmd) {
 	case SIOCGETMIFCNT_IN6:
 		if (copy_from_user(&vr, arg, sizeof(vr)))
 			return -EFAULT;
-		if (vr.mifi >= mrt->maxvif)
-			return -EINVAL;
+		break;
+	case SIOCGETSGCNT_IN6:
+		if (copy_from_user(&sr, arg, sizeof(sr)))
+			return -EFAULT;
+		break;
+	default:
+		return -ENOIOCTLCMD;
+	}
+
+
+	rcu_read_lock();
+	mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
+	if (!mrt) {
+		err = -ENOENT;
+		goto out;
+	}
+
+	switch (cmd) {
+	case SIOCGETMIFCNT_IN6:
+		if (vr.mifi >= mrt->maxvif) {
+			err = -EINVAL;
+			goto out;
+		}
 		vr.mifi = array_index_nospec(vr.mifi, mrt->maxvif);
-		rcu_read_lock();
 		vif = &mrt->vif_table[vr.mifi];
 		if (VIF_EXISTS(mrt, vr.mifi)) {
 			vr.icount = READ_ONCE(vif->pkt_in);
@@ -1986,13 +2003,9 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
 				return -EFAULT;
 			return 0;
 		}
-		rcu_read_unlock();
-		return -EADDRNOTAVAIL;
+		err = -EADDRNOTAVAIL;
+		goto out;
 	case SIOCGETSGCNT_IN6:
-		if (copy_from_user(&sr, arg, sizeof(sr)))
-			return -EFAULT;
-
-		rcu_read_lock();
 		c = ip6mr_cache_find(mrt, &sr.src.sin6_addr, &sr.grp.sin6_addr);
 		if (c) {
 			sr.pktcnt = c->_c.mfc_un.res.pkt;
@@ -2004,11 +2017,13 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
 				return -EFAULT;
 			return 0;
 		}
-		rcu_read_unlock();
-		return -EADDRNOTAVAIL;
-	default:
-		return -ENOIOCTLCMD;
+		err = -EADDRNOTAVAIL;
+		goto out;
 	}
+
+out:
+	rcu_read_unlock();
+	return err;
 }
 #endif
 
-- 
2.42.0

[PATCH net v5 04/10] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_get_route()

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
must be done under RCU or RTNL lock.

Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
---
 net/ipv6/ip6mr.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index b84444040e0e..c47564f0c868 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -2299,11 +2299,13 @@ int ip6mr_get_route(struct net *net, struct sk_buff *skb, struct rtmsg *rtm,
 	struct mfc6_cache *cache;
 	struct rt6_info *rt = dst_rt6_info(skb_dst(skb));
 
+	rcu_read_lock();
 	mrt = ip6mr_get_table(net, RT6_TABLE_DFLT);
-	if (!mrt)
-		return -ENOENT;
+	if (!mrt) {
+		err = -ENOENT;
+		goto out;
+	}
 
-	rcu_read_lock();
 	cache = ip6mr_cache_find(mrt, &rt->rt6i_src.addr, &rt->rt6i_dst.addr);
 	if (!cache && skb->dev) {
 		int vif = ip6mr_find_vif(mrt, skb->dev);
@@ -2321,15 +2323,15 @@ int ip6mr_get_route(struct net *net, struct sk_buff *skb, struct rtmsg *rtm,
 
 		dev = skb->dev;
 		if (!dev || (vif = ip6mr_find_vif(mrt, dev)) < 0) {
-			rcu_read_unlock();
-			return -ENODEV;
+			err = -ENODEV;
+			goto out;
 		}
 
 		/* really correct? */
 		skb2 = alloc_skb(sizeof(struct ipv6hdr), GFP_ATOMIC);
 		if (!skb2) {
-			rcu_read_unlock();
-			return -ENOMEM;
+			err = -ENOMEM;
+			goto out;
 		}
 
 		NETLINK_CB(skb2).portid = portid;
@@ -2351,12 +2353,13 @@ int ip6mr_get_route(struct net *net, struct sk_buff *skb, struct rtmsg *rtm,
 		iph->daddr = rt->rt6i_dst.addr;
 
 		err = ip6mr_cache_unresolved(mrt, vif, skb2, dev);
-		rcu_read_unlock();
 
-		return err;
+		goto out;
 	}
 
 	err = mr_fill_mroute(mrt, skb, &cache->_c, rtm);
+
+out:
 	rcu_read_unlock();
 	return err;
 }
-- 
2.42.0

[PATCH net v5 05/10] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_new_table()

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
must be done under RCU or RTNL lock.

Detected by Lockdep-RCU:

  [   10.247131] WARNING: suspicious RCU usage
  [   10.247133] 6.1.103-49518b10de-nokia_sm_x86 #1 Not tainted
  [   10.247135] -----------------------------
  [   10.247137] /net/ipv6/ip6mr.c:131 RCU-list traversed in non-reader section!!
  [   10.247140]
                 other info that might help us debug this:

  [   10.247142]
                 rcu_scheduler_active = 2, debug_locks = 1
  [   10.247144] 1 lock held by swapper/0/1:
  [   10.247147]  #0: ffffffff82b374d0 (pernet_ops_rwsem){+.+.}-{3:3}, at: register_pernet_subsys+0x15/0x40
  [   10.247164]
                 stack backtrace:
  [   10.247166] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.1.103-49518b10de-nokia_sm_x86 #1
  [   10.247170] Hardware name: Nokia Asil/Default string, BIOS 0ACNA114 07/18/2024
  [   10.247175] Call Trace:
  [   10.247178]  <TASK>
  [   10.247181]  dump_stack_lvl+0xb7/0xe9
  [   10.247189]  lockdep_rcu_suspicious.cold+0x2d/0x64
  [   10.247198]  ip6mr_get_table+0x8a/0x90
  [   10.247203]  ip6mr_net_init+0x7c/0x200
  [   10.247209]  ops_init+0x37/0x1f0
  [   10.247215]  register_pernet_operations+0x129/0x230
  [   10.247221]  ? af_unix_init+0xca/0xca
  [   10.247227]  register_pernet_subsys+0x24/0x40
  [   10.247231]  ip6_mr_init+0x42/0xf2
  [   10.247235]  inet6_init+0x133/0x3b9
  [   10.247238]  do_one_initcall+0x74/0x290
  [   10.247247]  kernel_init_freeable+0x251/0x294
  [   10.247253]  ? rest_init+0x174/0x174
  [   10.247257]  kernel_init+0x16/0x12c
  [   10.247260]  ret_from_fork+0x1f/0x30
  [   10.247271]  </TASK>

Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
---
 net/ipv6/ip6mr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index c47564f0c868..5171d64e046b 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -382,7 +382,9 @@ static struct mr_table *ip6mr_new_table(struct net *net, u32 id)
 {
 	struct mr_table *mrt;
 
+	rcu_read_lock();
 	mrt = ip6mr_get_table(net, id);
+	rcu_read_unlock();
 	if (mrt)
 		return mrt;
 
-- 
2.42.0

[PATCH net v5 06/10] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_mfc_seq_start()

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
must be done under RCU or RTNL lock.

Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
---
 net/ipv6/ip6mr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 5171d64e046b..ecb9e86fe45a 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -467,7 +467,9 @@ static void *ipmr_mfc_seq_start(struct seq_file *seq, loff_t *pos)
 	struct net *net = seq_file_net(seq);
 	struct mr_table *mrt;
 
+	rcu_read_lock();
 	mrt = ip6mr_get_table(net, RT6_TABLE_DFLT);
+	rcu_read_unlock();
 	if (!mrt)
 		return ERR_PTR(-ENOENT);
 
-- 
2.42.0

[PATCH net v5 07/10] ip6mr: Lock RCU before ip6mr_get_table() call in ip6_mroute_setsockopt()

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
must be done under RCU or RTNL lock.

Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
---
 net/ipv6/ip6mr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index ecb9e86fe45a..b54353bee2f8 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -1668,7 +1668,9 @@ int ip6_mroute_setsockopt(struct sock *sk, int optname, sockptr_t optval,
 	    inet_sk(sk)->inet_num != IPPROTO_ICMPV6)
 		return -EOPNOTSUPP;
 
+	rcu_read_lock();
 	mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
+	rcu_read_unlock();
 	if (!mrt)
 		return -ENOENT;
 
-- 
2.42.0

[PATCH net v5 08/10] ip6mr: Lock RCU before ip6mr_get_table() call in ip6_mroute_getsockopt()

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
must be done under RCU or RTNL lock.

Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
---
 net/ipv6/ip6mr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index b54353bee2f8..af921e9731ec 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -1845,7 +1845,9 @@ int ip6_mroute_getsockopt(struct sock *sk, int optname, sockptr_t optval,
 	    inet_sk(sk)->inet_num != IPPROTO_ICMPV6)
 		return -EOPNOTSUPP;
 
+	rcu_read_lock();
 	mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
+	rcu_read_unlock();
 	if (!mrt)
 		return -ENOENT;
 
-- 
2.42.0

Re: [PATCH net v5 08/10] ip6mr: Lock RCU before ip6mr_get_table() call in ip6_mroute_getsockopt()

[Jakub Kicinski]

On Mon, 14 Oct 2024 17:05:54 +0200 Stefan Wiehler wrote:
> +	rcu_read_lock();
>  	mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
> +	rcu_read_unlock();
>  	if (!mrt)
>  		return -ENOENT;

presumably you're trying to protect mrt with RCU?
so using mrt after unlocking is not right, you gotta hold the lock
longer
-- 
pw-bot: cr

Re: [PATCH net v5 08/10] ip6mr: Lock RCU before ip6mr_get_table() call in ip6_mroute_getsockopt()

[Stefan Wiehler]

>> +     rcu_read_lock();
>>       mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
>> +     rcu_read_unlock();
>>       if (!mrt)
>>               return -ENOENT;
> 
> presumably you're trying to protect mrt with RCU?
> so using mrt after unlocking is not right, you gotta hold the lock
> longer

Thanks, you're right of course, I'll be fixing this everywhere and send a v6
shortly; also with more extensive reasoning for this series in the cover
letter.

Kind regards,

Stefan

[PATCH net v5 09/10] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_rtm_getroute()

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
must be done under RCU or RTNL lock.

Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
---
 net/ipv6/ip6mr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index af921e9731ec..01b58156e06a 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -2594,14 +2594,15 @@ static int ip6mr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 		grp = nla_get_in6_addr(tb[RTA_DST]);
 	tableid = tb[RTA_TABLE] ? nla_get_u32(tb[RTA_TABLE]) : 0;
 
+	rcu_read_lock();
 	mrt = ip6mr_get_table(net, tableid ?: RT_TABLE_DEFAULT);
 	if (!mrt) {
+		rcu_read_unlock();
 		NL_SET_ERR_MSG_MOD(extack, "MR table does not exist");
 		return -ENOENT;
 	}
 
 	/* entries are added/deleted only under RTNL */
-	rcu_read_lock();
 	cache = ip6mr_cache_find(mrt, &src, &grp);
 	rcu_read_unlock();
 	if (!cache) {
-- 
2.42.0

[PATCH net v5 10/10] Revert "ipv6: Fix suspicious RCU usage warning in ip6mr"

[Stefan Wiehler]

This reverts commit b6dd5acde3f165e364881c36de942c5b252e2a27.

We should not suppress Lockdep-RCU splats when calling ip6mr_get_table()
without RCU or RTNL lock.

Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
---
 net/ipv6/ip6mr.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 01b58156e06a..e52abedafc9e 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -105,8 +105,7 @@ static void ipmr_expire_process(struct timer_list *t);
 #ifdef CONFIG_IPV6_MROUTE_MULTIPLE_TABLES
 #define ip6mr_for_each_table(mrt, net) \
 	list_for_each_entry_rcu(mrt, &net->ipv6.mr6_tables, list, \
-				lockdep_rtnl_is_held() || \
-				list_empty(&net->ipv6.mr6_tables))
+				lockdep_rtnl_is_held())
 
 static struct mr_table *ip6mr_mr_table_iter(struct net *net,
 					    struct mr_table *mrt)
-- 
2.42.0

Re: [PATCH net v5 00/10] Lock RCU before calling ip6mr_get_table()

[Jakub Kicinski]

On Mon, 14 Oct 2024 17:05:46 +0200 Stefan Wiehler wrote:
> v2: https://patchwork.kernel.org/project/netdevbpf/patch/20241001100119.230711-2-stefan.wiehler@nokia.com/
>   - rebase on top of net tree
>   - add Fixes tag
>   - refactor out paths

Please add the relevant info (from patch 2?) to the cover letter.
Otherwise it's hard to figure out at a glance what this series is
fixing.