diff mbox series

[net,v6,07/10] ip6mr: Lock RCU before ip6mr_get_table() call in ip6_mroute_setsockopt()

Message ID 20241017174109.85717-8-stefan.wiehler@nokia.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Lock RCU before calling ip6mr_get_table() | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/apply fail Patch does not apply to net-0

Commit Message

Stefan Wiehler Oct. 17, 2024, 5:37 p.m. UTC 
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(-)

Comments

Florian Westphal Oct. 17, 2024, 6:28 p.m. UTC | #1 
Stefan 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?
Paolo Abeni Oct. 23, 2024, 10:24 a.m. UTC | #2 
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
diff mbox series

Patch

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;
 }
 
 /*