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

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

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
must be done under RCU lock, except:
- call in ip6mr_rule_action is safe because fib_rules_lookup() holds RCU
  lock
- call in ip6mr_rtm_dumproute() is safe because rtnl_register_internal()
  holds the RTNL lock

Detected by Lockdep-RCU in the following two scenarios:

  [   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>


  [   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.834802]  </TASK>

v6:
  - hold RCU/RTNL lock for the complete duration multicast routing
    tables are in use
  - fix duplicate newline
v5: https://patchwork.kernel.org/project/netdevbpf/cover/20241014151247.1902637-1-stefan.wiehler@nokia.com/
  - 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 RTNL before ip6mr_new_table() call in ip6mr_rules_init()
  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 | 309 +++++++++++++++++++++++++++++------------------
 1 file changed, 190 insertions(+), 119 deletions(-)

-- 
2.42.0

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

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, multicast routing tables
must be read 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 76db80a7124c..3acc4c8a226d 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 v6 02/10] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_ioctl()

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, multicast routing tables
must be read 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.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 3acc4c8a226d..19ce010016b9 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -1887,47 +1887,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 v6 03/10] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_compat_ioctl()

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, multicast routing tables
must be read 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 | 48 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 19ce010016b9..968642bde8f8 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -1964,19 +1964,35 @@ 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);
@@ -1989,13 +2005,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;
@@ -2007,11 +2019,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 v6 04/10] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_get_route()

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, multicast routing tables
must be read 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 968642bde8f8..017f9e31edfb 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -2303,11 +2303,13 @@ int ip6mr_get_route(struct net *net, struct sk_buff *skb, struct rtmsg *rtm,
 	struct mfc6_cache *cache;
 	struct rt6_info *rt = (struct 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);
@@ -2325,15 +2327,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;
@@ -2355,12 +2357,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 v6 05/10] ip6mr: Lock RTNL before ip6mr_new_table() call in ip6mr_rules_init()

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, multicast routing tables
must be read under RCU or RTNL lock. Since mr_table_alloc() can sleep,
we call ip6mr_new_table() under the 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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 017f9e31edfb..9bf42aafc7f0 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -236,11 +236,13 @@ static int __net_init ip6mr_rules_init(struct net *net)
 
 	INIT_LIST_HEAD(&net->ipv6.mr6_tables);
 
+	rtnl_lock();
 	mrt = ip6mr_new_table(net, RT6_TABLE_DFLT);
 	if (IS_ERR(mrt)) {
 		err = PTR_ERR(mrt);
 		goto err1;
 	}
+	rtnl_unlock();
 
 	err = fib_default_rule_add(ops, 0x7fff, RT6_TABLE_DFLT, 0);
 	if (err < 0)
@@ -254,6 +256,7 @@ static int __net_init ip6mr_rules_init(struct net *net)
 	ip6mr_free_table(mrt);
 	rtnl_unlock();
 err1:
+	rtnl_unlock();
 	fib_rules_unregister(ops);
 	return err;
 }
-- 
2.42.0

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

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, multicast routing tables
must be read under RCU or RTNL lock.

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

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

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 9bf42aafc7f0..90d0f09cdd4e 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -464,10 +464,12 @@ static const struct seq_operations ip6mr_vif_seq_ops = {
 };
 
 static void *ipmr_mfc_seq_start(struct seq_file *seq, loff_t *pos)
+	__acquires(RCU)
 {
 	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);
@@ -475,6 +477,12 @@ static void *ipmr_mfc_seq_start(struct seq_file *seq, loff_t *pos)
 	return mr_mfc_seq_start(seq, pos, mrt, &mfc_unres_lock);
 }
 
+static void ipmr_mfc_seq_stop(struct seq_file *seq, void *v)
+	__releases(RCU)
+{
+	rcu_read_unlock();
+}
+
 static int ipmr_mfc_seq_show(struct seq_file *seq, void *v)
 {
 	int n;
@@ -520,7 +528,7 @@ static int ipmr_mfc_seq_show(struct seq_file *seq, void *v)
 static const struct seq_operations ipmr_mfc_seq_ops = {
 	.start = ipmr_mfc_seq_start,
 	.next  = mr_mfc_seq_next,
-	.stop  = mr_mfc_seq_stop,
+	.stop  = ipmr_mfc_seq_stop,
 	.show  = ipmr_mfc_seq_show,
 };
 #endif
-- 
2.42.0

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

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, multicast routing tables
must be read 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 | 165 +++++++++++++++++++++++++++--------------------
 1 file changed, 95 insertions(+), 70 deletions(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 90d0f09cdd4e..4726b9e156c7 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -1667,7 +1667,7 @@ EXPORT_SYMBOL(mroute6_is_socket);
 int ip6_mroute_setsockopt(struct sock *sk, int optname, sockptr_t optval,
 			  unsigned int optlen)
 {
-	int ret, parent = 0;
+	int ret, flags, v, parent = 0;
 	struct mif6ctl vif;
 	struct mf6cctl mfc;
 	mifi_t mifi;
@@ -1678,48 +1678,103 @@ int ip6_mroute_setsockopt(struct sock *sk, int optname, sockptr_t optval,
 	    inet_sk(sk)->inet_num != IPPROTO_ICMPV6)
 		return -EOPNOTSUPP;
 
+	switch (optname) {
+	case MRT6_ADD_MIF:
+		if (optlen < sizeof(vif))
+			return -EINVAL;
+		if (copy_from_sockptr(&vif, optval, sizeof(vif)))
+			return -EFAULT;
+		break;
+
+	case MRT6_DEL_MIF:
+		if (optlen < sizeof(mifi_t))
+			return -EINVAL;
+		if (copy_from_sockptr(&mifi, optval, sizeof(mifi_t)))
+			return -EFAULT;
+		break;
+
+	case MRT6_ADD_MFC:
+	case MRT6_DEL_MFC:
+	case MRT6_ADD_MFC_PROXY:
+	case MRT6_DEL_MFC_PROXY:
+		if (optlen < sizeof(mfc))
+			return -EINVAL;
+		if (copy_from_sockptr(&mfc, optval, sizeof(mfc)))
+			return -EFAULT;
+		break;
+
+	case MRT6_FLUSH:
+		if (optlen != sizeof(flags))
+			return -EINVAL;
+		if (copy_from_sockptr(&flags, optval, sizeof(flags)))
+			return -EFAULT;
+		break;
+
+	case MRT6_ASSERT:
+#ifdef CONFIG_IPV6_PIMSM_V2
+	case MRT6_PIM:
+#endif
+#ifdef CONFIG_IPV6_MROUTE_MULTIPLE_TABLES
+	case MRT6_TABLE:
+#endif
+		if (optlen != sizeof(v))
+			return -EINVAL;
+		if (copy_from_sockptr(&v, optval, sizeof(v)))
+			return -EFAULT;
+		break;
+	/*
+	 *	Spurious command, or MRT6_VERSION which you cannot
+	 *	set.
+	 */
+	default:
+		return -ENOPROTOOPT;
+	}
+
+	rcu_read_lock();
 	mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
-	if (!mrt)
-		return -ENOENT;
+	if (!mrt) {
+		ret = -ENOENT;
+		goto out;
+	}
 
 	if (optname != MRT6_INIT) {
 		if (sk != rcu_access_pointer(mrt->mroute_sk) &&
-		    !ns_capable(net->user_ns, CAP_NET_ADMIN))
-			return -EACCES;
+		    !ns_capable(net->user_ns, CAP_NET_ADMIN)) {
+			ret = -EACCES;
+			goto out;
+		}
 	}
 
 	switch (optname) {
 	case MRT6_INIT:
-		if (optlen < sizeof(int))
-			return -EINVAL;
+		if (optlen < sizeof(int)) {
+			ret = -EINVAL;
+			goto out;
+		}
 
-		return ip6mr_sk_init(mrt, sk);
+		ret = ip6mr_sk_init(mrt, sk);
+		goto out;
 
 	case MRT6_DONE:
-		return ip6mr_sk_done(sk);
+		ret = ip6mr_sk_done(sk);
+		goto out;
 
 	case MRT6_ADD_MIF:
-		if (optlen < sizeof(vif))
-			return -EINVAL;
-		if (copy_from_sockptr(&vif, optval, sizeof(vif)))
-			return -EFAULT;
-		if (vif.mif6c_mifi >= MAXMIFS)
-			return -ENFILE;
+		if (vif.mif6c_mifi >= MAXMIFS) {
+			ret = -ENFILE;
+			goto out;
+		}
 		rtnl_lock();
 		ret = mif6_add(net, mrt, &vif,
 			       sk == rtnl_dereference(mrt->mroute_sk));
 		rtnl_unlock();
-		return ret;
+		goto out;
 
 	case MRT6_DEL_MIF:
-		if (optlen < sizeof(mifi_t))
-			return -EINVAL;
-		if (copy_from_sockptr(&mifi, optval, sizeof(mifi_t)))
-			return -EFAULT;
 		rtnl_lock();
 		ret = mif6_delete(mrt, mifi, 0, NULL);
 		rtnl_unlock();
-		return ret;
+		goto out;
 
 	/*
 	 *	Manipulate the forwarding caches. These live
@@ -1731,10 +1786,6 @@ int ip6_mroute_setsockopt(struct sock *sk, int optname, sockptr_t optval,
 		fallthrough;
 	case MRT6_ADD_MFC_PROXY:
 	case MRT6_DEL_MFC_PROXY:
-		if (optlen < sizeof(mfc))
-			return -EINVAL;
-		if (copy_from_sockptr(&mfc, optval, sizeof(mfc)))
-			return -EFAULT;
 		if (parent == 0)
 			parent = mfc.mf6cc_parent;
 		rtnl_lock();
@@ -1746,47 +1797,27 @@ int ip6_mroute_setsockopt(struct sock *sk, int optname, sockptr_t optval,
 					    rtnl_dereference(mrt->mroute_sk),
 					    parent);
 		rtnl_unlock();
-		return ret;
+		goto out;
 
 	case MRT6_FLUSH:
-	{
-		int flags;
-
-		if (optlen != sizeof(flags))
-			return -EINVAL;
-		if (copy_from_sockptr(&flags, optval, sizeof(flags)))
-			return -EFAULT;
 		rtnl_lock();
 		mroute_clean_tables(mrt, flags);
 		rtnl_unlock();
-		return 0;
-	}
+		ret = 0;
+		goto out;
 
 	/*
 	 *	Control PIM assert (to activate pim will activate assert)
 	 */
 	case MRT6_ASSERT:
-	{
-		int v;
-
-		if (optlen != sizeof(v))
-			return -EINVAL;
-		if (copy_from_sockptr(&v, optval, sizeof(v)))
-			return -EFAULT;
 		mrt->mroute_do_assert = v;
-		return 0;
-	}
+		ret = 0;
+		goto out;
 
 #ifdef CONFIG_IPV6_PIMSM_V2
 	case MRT6_PIM:
 	{
 		bool do_wrmifwhole;
-		int v;
-
-		if (optlen != sizeof(v))
-			return -EINVAL;
-		if (copy_from_sockptr(&v, optval, sizeof(v)))
-			return -EFAULT;
 
 		do_wrmifwhole = (v == MRT6MSG_WRMIFWHOLE);
 		v = !!v;
@@ -1798,24 +1829,21 @@ int ip6_mroute_setsockopt(struct sock *sk, int optname, sockptr_t optval,
 			mrt->mroute_do_wrvifwhole = do_wrmifwhole;
 		}
 		rtnl_unlock();
-		return ret;
+		goto out;
 	}
 
 #endif
 #ifdef CONFIG_IPV6_MROUTE_MULTIPLE_TABLES
 	case MRT6_TABLE:
-	{
-		u32 v;
-
-		if (optlen != sizeof(u32))
-			return -EINVAL;
-		if (copy_from_sockptr(&v, optval, sizeof(v)))
-			return -EFAULT;
 		/* "pim6reg%u" should not exceed 16 bytes (IFNAMSIZ) */
-		if (v != RT_TABLE_DEFAULT && v >= 100000000)
-			return -EINVAL;
-		if (sk == rcu_access_pointer(mrt->mroute_sk))
-			return -EBUSY;
+		if (v != RT_TABLE_DEFAULT && v >= 100000000) {
+			ret = -EINVAL;
+			goto out;
+		}
+		if (sk == rcu_access_pointer(mrt->mroute_sk)) {
+			ret = -EBUSY;
+			goto out;
+		}
 
 		rtnl_lock();
 		ret = 0;
@@ -1825,16 +1853,13 @@ int ip6_mroute_setsockopt(struct sock *sk, int optname, sockptr_t optval,
 		else
 			raw6_sk(sk)->ip6mr_table = v;
 		rtnl_unlock();
-		return ret;
-	}
+		goto out;
 #endif
-	/*
-	 *	Spurious command, or MRT6_VERSION which you cannot
-	 *	set.
-	 */
-	default:
-		return -ENOPROTOOPT;
 	}
+
+out:
+	rcu_read_unlock();
+	return ret;
 }
 
 /*
-- 
2.42.0

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

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, multicast routing tables
must be read 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 | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 4726b9e156c7..b169b27de7e1 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -1878,9 +1878,12 @@ 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);
-	if (!mrt)
+	if (!mrt) {
+		rcu_read_unlock();
 		return -ENOENT;
+	}
 
 	switch (optname) {
 	case MRT6_VERSION:
@@ -1895,9 +1898,12 @@ int ip6_mroute_getsockopt(struct sock *sk, int optname, sockptr_t optval,
 		val = mrt->mroute_do_assert;
 		break;
 	default:
+		rcu_read_unlock();
 		return -ENOPROTOOPT;
 	}
 
+	rcu_read_unlock();
+
 	if (copy_from_sockptr(&olr, optlen, sizeof(int)))
 		return -EFAULT;
 
-- 
2.42.0

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

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, multicast routing tables
must be read 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 | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index b169b27de7e1..39aac81a30f1 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -2633,27 +2633,31 @@ 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) {
+		rcu_read_unlock();
 		NL_SET_ERR_MSG_MOD(extack, "MR cache entry not found");
 		return -ENOENT;
 	}
 
 	skb = nlmsg_new(mr6_msgsize(false, mrt->maxvif), GFP_KERNEL);
-	if (!skb)
+	if (!skb) {
+		rcu_read_unlock();
 		return -ENOBUFS;
+	}
 
 	err = ip6mr_fill_mroute(mrt, skb, NETLINK_CB(in_skb).portid,
 				nlh->nlmsg_seq, cache, RTM_NEWROUTE, 0);
+	rcu_read_unlock();
 	if (err < 0) {
 		kfree_skb(skb);
 		return err;
-- 
2.42.0

[PATCH net v6 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 39aac81a30f1..66cf3ea344aa 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 v6 05/10] ip6mr: Lock RTNL before ip6mr_new_table() call in ip6mr_rules_init()

[Florian Westphal]

Stefan Wiehler <stefan.wiehler@nokia.com> wrote:
> +	rtnl_lock();
>  	mrt = ip6mr_new_table(net, RT6_TABLE_DFLT);
>  	if (IS_ERR(mrt)) {
>  		err = PTR_ERR(mrt);
>  		goto err1;
>  	}
> +	rtnl_unlock();
>  
>  	err = fib_default_rule_add(ops, 0x7fff, RT6_TABLE_DFLT, 0);
>  	if (err < 0)
> @@ -254,6 +256,7 @@ static int __net_init ip6mr_rules_init(struct net *net)
>  	ip6mr_free_table(mrt);
>  	rtnl_unlock();
>  err1:
> +	rtnl_unlock();

Looks like a double-unlock?

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

[Florian Westphal]

Stefan Wiehler <stefan.wiehler@nokia.com> wrote:
> When IPV6_MROUTE_MULTIPLE_TABLES is enabled, multicast routing tables
> must be read 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 | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
> index b169b27de7e1..39aac81a30f1 100644
> --- a/net/ipv6/ip6mr.c
> +++ b/net/ipv6/ip6mr.c
> @@ -2633,27 +2633,31 @@ 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();

AFAICS ip6mr_rtm_getroute() runs with RTNL held, so I don't see
why this patch is needed.

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

[Florian Westphal]

Stefan Wiehler <stefan.wiehler@nokia.com> wrote:
> When IPV6_MROUTE_MULTIPLE_TABLES is enabled, multicast routing tables
> must be read 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 | 165 +++++++++++++++++++++++++++--------------------
>  1 file changed, 95 insertions(+), 70 deletions(-)
> 
> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
> index 90d0f09cdd4e..4726b9e156c7 100644
> --- a/net/ipv6/ip6mr.c
> +++ b/net/ipv6/ip6mr.c
> @@ -1667,7 +1667,7 @@ EXPORT_SYMBOL(mroute6_is_socket);
>  int ip6_mroute_setsockopt(struct sock *sk, int optname, sockptr_t optval,
>  			  unsigned int optlen)
>  {
> -	int ret, parent = 0;
> +	int ret, flags, v, parent = 0;
>  	struct mif6ctl vif;
>  	struct mf6cctl mfc;
>  	mifi_t mifi;
> @@ -1678,48 +1678,103 @@ int ip6_mroute_setsockopt(struct sock *sk, int optname, sockptr_t optval,
>  	    inet_sk(sk)->inet_num != IPPROTO_ICMPV6)
>  		return -EOPNOTSUPP;
>  
> +	switch (optname) {
> +	case MRT6_ADD_MIF:
> +		if (optlen < sizeof(vif))
> +			return -EINVAL;
> +		if (copy_from_sockptr(&vif, optval, sizeof(vif)))
> +			return -EFAULT;
> +		break;
> +
> +	case MRT6_DEL_MIF:
> +		if (optlen < sizeof(mifi_t))
> +			return -EINVAL;
> +		if (copy_from_sockptr(&mifi, optval, sizeof(mifi_t)))
> +			return -EFAULT;
> +		break;
> +
> +	case MRT6_ADD_MFC:
> +	case MRT6_DEL_MFC:
> +	case MRT6_ADD_MFC_PROXY:
> +	case MRT6_DEL_MFC_PROXY:
> +		if (optlen < sizeof(mfc))
> +			return -EINVAL;
> +		if (copy_from_sockptr(&mfc, optval, sizeof(mfc)))
> +			return -EFAULT;
> +		break;
> +
> +	case MRT6_FLUSH:
> +		if (optlen != sizeof(flags))
> +			return -EINVAL;
> +		if (copy_from_sockptr(&flags, optval, sizeof(flags)))
> +			return -EFAULT;
> +		break;
> +
> +	case MRT6_ASSERT:
> +#ifdef CONFIG_IPV6_PIMSM_V2
> +	case MRT6_PIM:
> +#endif
> +#ifdef CONFIG_IPV6_MROUTE_MULTIPLE_TABLES
> +	case MRT6_TABLE:
> +#endif
> +		if (optlen != sizeof(v))
> +			return -EINVAL;
> +		if (copy_from_sockptr(&v, optval, sizeof(v)))
> +			return -EFAULT;
> +		break;
> +	/*
> +	 *	Spurious command, or MRT6_VERSION which you cannot
> +	 *	set.
> +	 */
> +	default:
> +		return -ENOPROTOOPT;
> +	}
> +
> +	rcu_read_lock();

RCU read section start ...

>  	mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
> +	if (!mrt) {
> +		ret = -ENOENT;
> +		goto out;
> +	}
>  
>  	if (optname != MRT6_INIT) {
>  		if (sk != rcu_access_pointer(mrt->mroute_sk) &&
> -		    !ns_capable(net->user_ns, CAP_NET_ADMIN))
> -			return -EACCES;
> +		    !ns_capable(net->user_ns, CAP_NET_ADMIN)) {
> +			ret = -EACCES;
> +			goto out;
> +		}
>  	}
>  
>  	switch (optname) {
>  	case MRT6_INIT:
> -		if (optlen < sizeof(int))
> -			return -EINVAL;
> +		if (optlen < sizeof(int)) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
>  
> -		return ip6mr_sk_init(mrt, sk);
> +		ret = ip6mr_sk_init(mrt, sk);

ip6mr_sk_init() calls rtnl_lock(), which can sleep.

> +		goto out;
>  
>  	case MRT6_DONE:
> -		return ip6mr_sk_done(sk);
> +		ret = ip6mr_sk_done(sk);

Likewise.

>  	case MRT6_ADD_MIF:
> -		if (optlen < sizeof(vif))
> -			return -EINVAL;
> -		if (copy_from_sockptr(&vif, optval, sizeof(vif)))
> -			return -EFAULT;
> -		if (vif.mif6c_mifi >= MAXMIFS)
> -			return -ENFILE;
> +		if (vif.mif6c_mifi >= MAXMIFS) {
> +			ret = -ENFILE;
> +			goto out;
> +		}
>  		rtnl_lock();

Same, sleeping function called in rcu read side section.

Maybe its time to add refcount_t to struct mr_table?

Re: [PATCH net v6 05/10] ip6mr: Lock RTNL before ip6mr_new_table() call in ip6mr_rules_init()

[Stefan Wiehler]

>> +     rtnl_lock();
>>       mrt = ip6mr_new_table(net, RT6_TABLE_DFLT);
>>       if (IS_ERR(mrt)) {
>>               err = PTR_ERR(mrt);
>>               goto err1;
>>       }
>> +     rtnl_unlock();
>>
>>       err = fib_default_rule_add(ops, 0x7fff, RT6_TABLE_DFLT, 0);
>>       if (err < 0)
>> @@ -254,6 +256,7 @@ static int __net_init ip6mr_rules_init(struct net *net)
>>       ip6mr_free_table(mrt);
>>       rtnl_unlock();
>>  err1:
>> +     rtnl_unlock();
> 
> Looks like a double-unlock?

Thanks, will fix in v7.

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

[Stefan Wiehler]

>> When IPV6_MROUTE_MULTIPLE_TABLES is enabled, multicast routing tables
>> must be read 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 | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
>> index b169b27de7e1..39aac81a30f1 100644
>> --- a/net/ipv6/ip6mr.c
>> +++ b/net/ipv6/ip6mr.c
>> @@ -2633,27 +2633,31 @@ 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();
> 
> AFAICS ip6mr_rtm_getroute() runs with RTNL held, so I don't see
> why this patch is needed.

That's true, but it's called neither with RCU nor RTNL lock when
RTNL_FLAG_DOIT_UNLOCKED is set in rtnetlink_rcv_msg():

> if (flags & RTNL_FLAG_DOIT_UNLOCKED) {
> 	doit = link->doit;
> 	rcu_read_unlock();
> 	if (doit)
> 		err = doit(skb, nlh, extack);
> 	module_put(owner);
> 	return err;
> }
> rcu_read_unlock();

I realized now that I completely misunderstood how ip6mr_rtm_dumproute() gets
called - it should be still safe though because mpls_netconf_dump_devconf() and
getaddr_dumpit() hold the RCU lock while mpls_dump_routes() asserts that the
RTNL lock is held. Is that observation correct?

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

[Florian Westphal]

Stefan Wiehler <stefan.wiehler@nokia.com> wrote:
> >> When IPV6_MROUTE_MULTIPLE_TABLES is enabled, multicast routing tables
> >> must be read 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 | 10 +++++++---
> >>  1 file changed, 7 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
> >> index b169b27de7e1..39aac81a30f1 100644
> >> --- a/net/ipv6/ip6mr.c
> >> +++ b/net/ipv6/ip6mr.c
> >> @@ -2633,27 +2633,31 @@ 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();
> > 
> > AFAICS ip6mr_rtm_getroute() runs with RTNL held, so I don't see
> > why this patch is needed.
> 
> That's true, but it's called neither with RCU nor RTNL lock when
> RTNL_FLAG_DOIT_UNLOCKED is set in rtnetlink_rcv_msg():

Sure, but RTNL_FLAG_DOIT_UNLOCKED is not set for this function:

        err = rtnl_register_module(THIS_MODULE, RTNL_FAMILY_IP6MR, RTM_GETROUTE,
                                   ip6mr_rtm_getroute, ip6mr_rtm_dumproute, 0);

(0 == flag field).  So RNTL is held.  Would of course be nice to convert it to RCU
eventually but thats an enhancement, not a bug fix, so this must be in
separate changesets, targetting net and net-next, respectively.

> I realized now that I completely misunderstood how ip6mr_rtm_dumproute() gets
> called - it should be still safe though because mpls_netconf_dump_devconf() and
> getaddr_dumpit() hold the RCU lock while mpls_dump_routes() asserts that the
> RTNL lock is held. Is that observation correct?

{THIS_MODULE, PF_PHONET, RTM_GETADDR, NULL, getaddr_dumpit, 0},
{THIS_MODULE, PF_MPLS, RTM_GETROUTE, mpls_getroute, mpls_dump_routes, 0},

Both get called with RTNL mutex held, but not within an RCU read side
section.

but:
{THIS_MODULE, PF_MPLS, RTM_GETNETCONF, [..] mpls_netconf_dump_devconf,
       	RTNL_FLAG_DUMP_UNLOCKED}, // == no RTNL held

Means: dump callback is invoked without RTNL mutex.  Those functions
are also called without and RCU read-side section.

Both the get (doit) and the dumper function callbacks need to
explicitly opt-in for RTNL-less invocation at register time.

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

[Paolo Abeni]

On 10/17/24 20:28, Florian Westphal wrote:
> Stefan Wiehler <stefan.wiehler@nokia.com> wrote:
>>  	case MRT6_ADD_MIF:
>> -		if (optlen < sizeof(vif))
>> -			return -EINVAL;
>> -		if (copy_from_sockptr(&vif, optval, sizeof(vif)))
>> -			return -EFAULT;
>> -		if (vif.mif6c_mifi >= MAXMIFS)
>> -			return -ENFILE;
>> +		if (vif.mif6c_mifi >= MAXMIFS) {
>> +			ret = -ENFILE;
>> +			goto out;
>> +		}
>>  		rtnl_lock();
> 
> Same, sleeping function called in rcu read side section.
> 
> Maybe its time to add refcount_t to struct mr_table?

FTR, I agree using a refcount could be a better approach (and would
avoid keeping the RCU lock held across seq start/stop which sounds
dangerous, too.

@Stefan: in any case before your next submission, please have test run
with a debug build, so that the run-time checker could catch similar issue.

Side minor nit: your  'Fixes' tag should come before your Sob in the tag
area.

Cheers,

Paolo