[PATCH net-next] mld: add missing rtnl_lock() in do_ipv6_getsockopt()

ip6_mc_msfget() should be called under RTNL because it accesses RTNL
protected data. but the caller doesn't acquire rtnl_lock().
So, data couldn't be protected.
Therefore, it adds rtnl_lock() in do_ipv6_getsockopt(),
which is the caller of ip6_mc_msfget().

Splat looks like:
=============================
WARNING: suspicious RCU usage
5.12.0-rc4+ #480 Tainted: G        W
-----------------------------
include/net/addrconf.h:314 suspicious rcu_dereference_check() usage!

other info that might help us debug this:

rcu_scheduler_active = 2, debug_locks = 1
1 lock held by sockopt_msfilte/4955:
 #0: ffff88800aa21370 (sk_lock-AF_INET6){+.+.}-{0:0}, at: \
	ipv6_get_msfilter+0xaf/0x190

stack backtrace:
Call Trace:
 dump_stack+0xa4/0xe5
 ip6_mc_find_dev_rtnl+0x117/0x150
 ip6_mc_msfget+0x17d/0x700
 ? lock_acquire+0x191/0x720
 ? ipv6_sock_mc_join_ssm+0x10/0x10
 ? lockdep_hardirqs_on_prepare+0x3e0/0x3e0
 ? mark_held_locks+0xb7/0x120
 ? lockdep_hardirqs_on_prepare+0x27c/0x3e0
 ? __local_bh_enable_ip+0xa5/0xf0
 ? lock_sock_nested+0x82/0xf0
 ipv6_get_msfilter+0xc3/0x190
 ? compat_ipv6_get_msfilter+0x300/0x300
 ? lock_downgrade+0x690/0x690
 do_ipv6_getsockopt.isra.6.constprop.13+0x1706/0x29f0
 ? do_ipv6_mcast_group_source+0x150/0x150
 ? __wake_up_common+0x620/0x620
 ? mutex_trylock+0x23f/0x2a0
[ ... ]

Fixes: 88e2ca308094 ("mld: convert ifmcaddr6 to RCU")
Reported-by: Eric Dumazet <redacted>
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---

commit 88e2ca308094 ("mld: convert ifmcaddr6 to RCU") is not yet merged
to "net" branch. So, target branch is "net-next"

 net/ipv6/ipv6_sockglue.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index a6804a7e34c1..55dc35e09c68 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c

@@ -1137,9 +1137,12 @@ static int do_ipv6_getsockopt(struct sock *sk, int level, int optname,
 		val = sk->sk_family;
 		break;
 	case MCAST_MSFILTER:
+		rtnl_lock();
 		if (in_compat_syscall())
-			return compat_ipv6_get_msfilter(sk, optval, optlen);
-		return ipv6_get_msfilter(sk, optval, optlen, len);
+			val = compat_ipv6_get_msfilter(sk, optval, optlen);
+		val = ipv6_get_msfilter(sk, optval, optlen, len);
+		rtnl_unlock();
+		return val;
 	case IPV6_2292PKTOPTIONS:
 	{
 		struct msghdr msg;

-- 
2.17.1

On Tue, Mar 30, 2021 at 5:31 PM Taehee Yoo [off-list ref] wrote:

This seems a serious regression compared to old code (in net tree)

Have you added RTNL requirement in all this code ?

We would like to use RTNL only if strictly needed.

On 3/31/21 12:40 AM, Eric Dumazet wrote:

This seems a serious regression compared to old code (in net tree)

Have you added RTNL requirement in all this code ?

We would like to use RTNL only if strictly needed.

Yes, I agree with you.
This patchset actually relies on existed RTNL, which is setsockopt_needs_rtnl().
And remained RTNL was replaced by mc_lock.
So, this patchset actually doesn't add new RTNL except in this case.

Fortunately, I think It can be replaced by RCU because,
1. ip6_mc_msfget() doesn't need the sleepable functions.
2. It is not the write critical section.
So, RCU can be used instead of RTNL for ip6_mc_msfget().
How do you think about it?

On Tue, Mar 30, 2021 at 6:02 PM Taehee Yoo [off-list ref] wrote:

On 3/31/21 12:40 AM, Eric Dumazet wrote:
 > This seems a serious regression compared to old code (in net tree)
 >
 > Have you added RTNL requirement in all this code ?
 >
 > We would like to use RTNL only if strictly needed.

Yes, I agree with you.
This patchset actually relies on existed RTNL, which is
setsockopt_needs_rtnl().
And remained RTNL was replaced by mc_lock.
So, this patchset actually doesn't add new RTNL except in this case.

Fortunately, I think It can be replaced by RCU because,
1. ip6_mc_msfget() doesn't need the sleepable functions.
2. It is not the write critical section.
So, RCU can be used instead of RTNL for ip6_mc_msfget().
How do you think about it?

Yes please, do not add RTNL here if we can avoid it.

Otherwise some applications will slow down the whole stack, even with
different containers/netns.

(There is a single RTNL for the whole machine)

On 3/31/21 1:08 AM, Eric Dumazet wrote:

On Tue, Mar 30, 2021 at 6:02 PM Taehee Yoo [off-list ref] wrote:

quoted

On 3/31/21 12:40 AM, Eric Dumazet wrote:
  > This seems a serious regression compared to old code (in net tree)
  >
  > Have you added RTNL requirement in all this code ?
  >
  > We would like to use RTNL only if strictly needed.

Yes, I agree with you.
This patchset actually relies on existed RTNL, which is
setsockopt_needs_rtnl().
And remained RTNL was replaced by mc_lock.
So, this patchset actually doesn't add new RTNL except in this case.

Fortunately, I think It can be replaced by RCU because,
1. ip6_mc_msfget() doesn't need the sleepable functions.
2. It is not the write critical section.
So, RCU can be used instead of RTNL for ip6_mc_msfget().
How do you think about it?

Yes please, do not add RTNL here if we can avoid it.

Okay, I will send a new patch.

Otherwise some applications will slow down the whole stack, even with
different containers/netns.

(There is a single RTNL for the whole machine)

Thanks a lot for the review!