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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iJ1G8vU-Jw6gaTsZHamQv1ncLmoJ1FOop25OzrYmjh4kA@mail.gmail.com>
Date:   Tue, 30 Mar 2021 17:40:06 +0200
From:   Eric Dumazet <edumazet@...gle.com>
To:     Taehee Yoo <ap420073@...il.com>
Cc:     netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next] mld: add missing rtnl_lock() in do_ipv6_getsockopt()

On Tue, Mar 30, 2021 at 5:31 PM Taehee Yoo <ap420073@...il.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@...gle.com>
> Signed-off-by: Taehee Yoo <ap420073@...il.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.