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@nokia.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();
mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
- if (!mrt)
- return -ENOENT;
+ 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);
+ goto out;
case MRT6_DONE:
- return ip6mr_sk_done(sk);
+ ret = ip6mr_sk_done(sk);
+ goto out;
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();
ret = mif6_add(net, mrt, &vif,
sk == rtnl_dereference(mrt->mroute_sk));
rtnl_unlock();
- return ret;
+ goto out;
case MRT6_DEL_MIF:
- if (optlen < sizeof(mifi_t))
- return -EINVAL;
- if (copy_from_sockptr(&mifi, optval, sizeof(mifi_t)))
- return -EFAULT;
rtnl_lock();
ret = mif6_delete(mrt, mifi, 0, NULL);
rtnl_unlock();
- return ret;
+ goto out;
/*
* Manipulate the forwarding caches. These live
@@ -1731,10 +1786,6 @@ int ip6_mroute_setsockopt(struct sock *sk, int optname, sockptr_t optval,
fallthrough;
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;
if (parent == 0)
parent = mfc.mf6cc_parent;
rtnl_lock();
@@ -1746,47 +1797,27 @@ int ip6_mroute_setsockopt(struct sock *sk, int optname, sockptr_t optval,
rtnl_dereference(mrt->mroute_sk),
parent);
rtnl_unlock();
- return ret;
+ goto out;
case MRT6_FLUSH:
- {
- int flags;
-
- if (optlen != sizeof(flags))
- return -EINVAL;
- if (copy_from_sockptr(&flags, optval, sizeof(flags)))
- return -EFAULT;
rtnl_lock();
mroute_clean_tables(mrt, flags);
rtnl_unlock();
- return 0;
- }
+ ret = 0;
+ goto out;
/*
* Control PIM assert (to activate pim will activate assert)
*/
case MRT6_ASSERT:
- {
- int v;
-
- if (optlen != sizeof(v))
- return -EINVAL;
- if (copy_from_sockptr(&v, optval, sizeof(v)))
- return -EFAULT;
mrt->mroute_do_assert = v;
- return 0;
- }
+ ret = 0;
+ goto out;
#ifdef CONFIG_IPV6_PIMSM_V2
case MRT6_PIM:
{
bool do_wrmifwhole;
- int v;
-
- if (optlen != sizeof(v))
- return -EINVAL;
- if (copy_from_sockptr(&v, optval, sizeof(v)))
- return -EFAULT;
do_wrmifwhole = (v == MRT6MSG_WRMIFWHOLE);
v = !!v;
@@ -1798,24 +1829,21 @@ int ip6_mroute_setsockopt(struct sock *sk, int optname, sockptr_t optval,
mrt->mroute_do_wrvifwhole = do_wrmifwhole;
}
rtnl_unlock();
- return ret;
+ goto out;
}
#endif
#ifdef CONFIG_IPV6_MROUTE_MULTIPLE_TABLES
case MRT6_TABLE:
- {
- u32 v;
-
- if (optlen != sizeof(u32))
- return -EINVAL;
- if (copy_from_sockptr(&v, optval, sizeof(v)))
- return -EFAULT;
/* "pim6reg%u" should not exceed 16 bytes (IFNAMSIZ) */
- if (v != RT_TABLE_DEFAULT && v >= 100000000)
- return -EINVAL;
- if (sk == rcu_access_pointer(mrt->mroute_sk))
- return -EBUSY;
+ if (v != RT_TABLE_DEFAULT && v >= 100000000) {
+ ret = -EINVAL;
+ goto out;
+ }
+ if (sk == rcu_access_pointer(mrt->mroute_sk)) {
+ ret = -EBUSY;
+ goto out;
+ }
rtnl_lock();
ret = 0;
@@ -1825,16 +1853,13 @@ int ip6_mroute_setsockopt(struct sock *sk, int optname, sockptr_t optval,
else
raw6_sk(sk)->ip6mr_table = v;
rtnl_unlock();
- return ret;
- }
+ goto out;
#endif
- /*
- * Spurious command, or MRT6_VERSION which you cannot
- * set.
- */
- default:
- return -ENOPROTOOPT;
}
+
+out:
+ rcu_read_unlock();
+ return ret;
}
/*
--
2.42.0Stefan Wiehler <stefan.wiehler@nokia.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@nokia.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?
On 10/17/24 20:28, Florian Westphal wrote:
> Stefan Wiehler <stefan.wiehler@nokia.com> wrote:
>> 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?
FTR, I agree using a refcount could be a better approach (and would
avoid keeping the RCU lock held across seq start/stop which sounds
dangerous, too.
@Stefan: in any case before your next submission, please have test run
with a debug build, so that the run-time checker could catch similar issue.
Side minor nit: your 'Fixes' tag should come before your Sob in the tag
area.
Cheers,
Paolo
© 2016 - 2024 Red Hat, Inc.