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

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

[Taehee Yoo]

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 <edumazet@google.com>
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

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

[Eric Dumazet]

On Tue, Mar 30, 2021 at 5:31 PM Taehee Yoo <ap420073@gmail.com> wrote:
>
> 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 <edumazet@google.com>
> 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;


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.

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

[Taehee Yoo]

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?

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

[Eric Dumazet]

On Tue, Mar 30, 2021 at 6:02 PM Taehee Yoo <ap420073@gmail.com> 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)

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

[Taehee Yoo]

On 3/31/21 1:08 AM, Eric Dumazet wrote:
 > On Tue, Mar 30, 2021 at 6:02 PM Taehee Yoo <ap420073@gmail.com> 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.
 >
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!