| Message ID | 20210330153106.31614-1-ap420073@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] mld: add missing rtnl_lock() in do_ipv6_getsockopt() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 14 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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.
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 <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)
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!
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;
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(-)