[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241017182843.GD25857@breakpoint.cc>
Date: Thu, 17 Oct 2024 20:28:43 +0200
From: Florian Westphal <fw@...len.de>
To: Stefan Wiehler <stefan.wiehler@...ia.com>
Cc: "David S . Miller" <davem@...emloft.net>,
David Ahern <dsahern@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v6 07/10] ip6mr: Lock RCU before ip6mr_get_table()
call in ip6_mroute_setsockopt()
Stefan Wiehler <stefan.wiehler@...ia.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@...ia.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?
Powered by blists - more mailing lists