[PATCH net v3 1/4] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_vif_seq_start()

[PATCH net v3 1/4] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_vif_seq_start()

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
must be done under RCU or RTNL lock.

Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
---
v3:
  - split into separate patches
v2: https://patchwork.kernel.org/project/netdevbpf/patch/20241001100119.230711-2-stefan.wiehler@nokia.com/
  - rebase on top of net tree
  - add Fixes tag
  - refactor out paths
v1: https://patchwork.kernel.org/project/netdevbpf/patch/20240605195355.363936-1-oss@malat.biz/
---
 net/ipv6/ip6mr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 2ce4ae0d8dc3..268e77196753 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -411,13 +411,13 @@ static void *ip6mr_vif_seq_start(struct seq_file *seq, loff_t *pos)
 	struct net *net = seq_file_net(seq);
 	struct mr_table *mrt;
 
+	rcu_read_lock();
 	mrt = ip6mr_get_table(net, RT6_TABLE_DFLT);
 	if (!mrt)
 		return ERR_PTR(-ENOENT);
 
 	iter->mrt = mrt;
 
-	rcu_read_lock();
 	return mr_vif_seq_start(seq, pos);
 }
 
-- 
2.42.0

[PATCH net v3 2/4] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_ioctl()

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to
ip6mr_get_table() must be done under RCU or RTNL lock.

Detected by Lockdep-RCU:

  [   48.834645] WARNING: suspicious RCU usage
  [   48.834647] 6.1.103-584209f6d5-nokia_sm_x86 #1 Tainted: G S         O
  [   48.834649] -----------------------------
  [   48.834651] /net/ipv6/ip6mr.c:132 RCU-list traversed in non-reader section!!
  [   48.834654]
                 other info that might help us debug this:

  [   48.834656]
                 rcu_scheduler_active = 2, debug_locks = 1
  [   48.834658] no locks held by radvd/5777.
  [   48.834660]
                 stack backtrace:
  [   48.834663] CPU: 0 PID: 5777 Comm: radvd Tainted: G S         O       6.1.103-584209f6d5-nokia_sm_x86 #1
  [   48.834666] Hardware name: Nokia Asil/Default string, BIOS 0ACNA113 06/07/2024
  [   48.834673] Call Trace:
  [   48.834674]  <TASK>
  [   48.834677]  dump_stack_lvl+0xb7/0xe9
  [   48.834687]  lockdep_rcu_suspicious.cold+0x2d/0x64
  [   48.834697]  ip6mr_get_table+0x9f/0xb0
  [   48.834704]  ip6mr_ioctl+0x50/0x360
  [   48.834713]  ? sk_ioctl+0x5f/0x1c0
  [   48.834719]  sk_ioctl+0x5f/0x1c0
  [   48.834723]  ? find_held_lock+0x2b/0x80
  [   48.834731]  sock_do_ioctl+0x7b/0x140
  [   48.834737]  ? proc_nr_files+0x30/0x30
  [   48.834744]  sock_ioctl+0x1f5/0x360
  [   48.834754]  __x64_sys_ioctl+0x8d/0xd0
  [   48.834760]  do_syscall_64+0x3c/0x90
  [   48.834765]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
  [   48.834769] RIP: 0033:0x7fda5f56e2ac                                                                   [   48.834773] Code: 1e fa 48 8d 44 24 08 48 89 54 24 e0 48 89 44 24 c0 48 8d 44 24 d0 48 89 44 24 c8 b8 1 0 00 00 00 c7 44 24 b8 10 00 00 00 0f 05 <3d> 00 f0 ff ff 89 c2 77 0b 89 d0 c3 0f 1f 84
  00 00 00 00 00 48 8b
  [   48.834776] RSP: 002b:00007fff52d4bda8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
  [   48.834782] RAX: ffffffffffffffda RBX: 000000000171cd80 RCX: 00007fda5f56e2ac
  [   48.834784] RDX: 00007fff52d4bdb0 RSI: 0000000000008913 RDI: 0000000000000003
  [   48.834787] RBP: 000000000171cd30 R08: 0000000000000007 R09: 000000000000003c
  [   48.834789] R10: 0000000000000004 R11: 0000000000000246 R12: 0000000000000003
  [   48.834791] R13: 0000000000000000 R14: 0000000000000004 R15: 000000000040d43c
  [   48.834802]  </TASK>

Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
---
v3:
  - split into separate patches
v2: https://patchwork.kernel.org/project/netdevbpf/patch/20241001100119.230711-2-stefan.wiehler@nokia.com/
  - rebase on top of net tree
  - add Fixes tag
  - refactor out paths
v1: https://patchwork.kernel.org/project/netdevbpf/patch/20240605195355.363936-1-oss@malat.biz/
---
 net/ipv6/ip6mr.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 268e77196753..b18eb4ad21e4 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -1884,18 +1884,23 @@ int ip6mr_ioctl(struct sock *sk, int cmd, void *arg)
 	struct mfc6_cache *c;
 	struct net *net = sock_net(sk);
 	struct mr_table *mrt;
+	int err;
 
+	rcu_read_lock();
 	mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
-	if (!mrt)
-		return -ENOENT;
+	if (!mrt) {
+		err = -ENOENT;
+		goto out;
+	}
 
 	switch (cmd) {
 	case SIOCGETMIFCNT_IN6:
 		vr = (struct sioc_mif_req6 *)arg;
-		if (vr->mifi >= mrt->maxvif)
-			return -EINVAL;
+		if (vr->mifi >= mrt->maxvif) {
+			err = -EINVAL;
+			goto out;
+		}
 		vr->mifi = array_index_nospec(vr->mifi, mrt->maxvif);
-		rcu_read_lock();
 		vif = &mrt->vif_table[vr->mifi];
 		if (VIF_EXISTS(mrt, vr->mifi)) {
 			vr->icount = READ_ONCE(vif->pkt_in);
@@ -1905,12 +1910,11 @@ int ip6mr_ioctl(struct sock *sk, int cmd, void *arg)
 			rcu_read_unlock();
 			return 0;
 		}
-		rcu_read_unlock();
-		return -EADDRNOTAVAIL;
+		err = -EADDRNOTAVAIL;
+		goto out;
 	case SIOCGETSGCNT_IN6:
 		sr = (struct sioc_sg_req6 *)arg;
 
-		rcu_read_lock();
 		c = ip6mr_cache_find(mrt, &sr->src.sin6_addr,
 				     &sr->grp.sin6_addr);
 		if (c) {
@@ -1920,11 +1924,16 @@ int ip6mr_ioctl(struct sock *sk, int cmd, void *arg)
 			rcu_read_unlock();
 			return 0;
 		}
-		rcu_read_unlock();
-		return -EADDRNOTAVAIL;
+		err = -EADDRNOTAVAIL;
+		goto out;
 	default:
-		return -ENOIOCTLCMD;
+		err = -ENOIOCTLCMD;
+		goto out;
 	}
+
+out:
+	rcu_read_unlock();
+	return err;
 }
 
 #ifdef CONFIG_COMPAT
-- 
2.42.0

[PATCH net v3 3/4] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_compat_ioctl()

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
must be done under RCU or RTNL lock. Copy from user space must be
performed beforehand as we are not allowed to sleep under RCU lock.

Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
---
v3:
  - split into separate patches
v2: https://patchwork.kernel.org/project/netdevbpf/patch/20241001100119.230711-2-stefan.wiehler@nokia.com/
  - rebase on top of net tree
  - add Fixes tag
  - refactor out paths
v1: https://patchwork.kernel.org/project/netdevbpf/patch/20240605195355.363936-1-oss@malat.biz/
---
 net/ipv6/ip6mr.c | 46 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index b18eb4ad21e4..415ba6f55a44 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -1961,10 +1961,7 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
 	struct mfc6_cache *c;
 	struct net *net = sock_net(sk);
 	struct mr_table *mrt;
-
-	mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
-	if (!mrt)
-		return -ENOENT;
+	int err;
 
 	switch (cmd) {
 	case SIOCGETMIFCNT_IN6:
@@ -1972,8 +1969,30 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
 			return -EFAULT;
 		if (vr.mifi >= mrt->maxvif)
 			return -EINVAL;
+		break;
+	case SIOCGETSGCNT_IN6:
+		if (copy_from_user(&sr, arg, sizeof(sr)))
+			return -EFAULT;
+		break;
+	default:
+		return -ENOIOCTLCMD;
+	}
+
+
+	rcu_read_lock();
+	mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
+	if (!mrt) {
+		err = -ENOENT;
+		goto out;
+	}
+
+	switch (cmd) {
+	case SIOCGETMIFCNT_IN6:
+		if (vr.mifi >= mrt->maxvif) {
+			err = -EINVAL;
+			goto out;
+		}
 		vr.mifi = array_index_nospec(vr.mifi, mrt->maxvif);
-		rcu_read_lock();
 		vif = &mrt->vif_table[vr.mifi];
 		if (VIF_EXISTS(mrt, vr.mifi)) {
 			vr.icount = READ_ONCE(vif->pkt_in);
@@ -1987,12 +2006,9 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
 			return 0;
 		}
 		rcu_read_unlock();
-		return -EADDRNOTAVAIL;
+		err = -EADDRNOTAVAIL;
+		goto out;
 	case SIOCGETSGCNT_IN6:
-		if (copy_from_user(&sr, arg, sizeof(sr)))
-			return -EFAULT;
-
-		rcu_read_lock();
 		c = ip6mr_cache_find(mrt, &sr.src.sin6_addr, &sr.grp.sin6_addr);
 		if (c) {
 			sr.pktcnt = c->_c.mfc_un.res.pkt;
@@ -2004,11 +2020,13 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
 				return -EFAULT;
 			return 0;
 		}
-		rcu_read_unlock();
-		return -EADDRNOTAVAIL;
-	default:
-		return -ENOIOCTLCMD;
+		err = -EADDRNOTAVAIL;
+		goto out;
 	}
+
+out:
+	rcu_read_unlock();
+	return err;
 }
 #endif
 
-- 
2.42.0

[PATCH net v3 4/4] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_get_route()

[Stefan Wiehler]

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
must be done under RCU or RTNL lock.

Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
---
v3:
  - split into separate patches
v2: https://patchwork.kernel.org/project/netdevbpf/patch/20241001100119.230711-2-stefan.wiehler@nokia.com/
  - rebase on top of net tree
  - add Fixes tag
  - refactor out paths
v1: https://patchwork.kernel.org/project/netdevbpf/patch/20240605195355.363936-1-oss@malat.biz/
---
 net/ipv6/ip6mr.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 415ba6f55a44..0bc8d6b0569f 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -2302,11 +2302,13 @@ int ip6mr_get_route(struct net *net, struct sk_buff *skb, struct rtmsg *rtm,
 	struct mfc6_cache *cache;
 	struct rt6_info *rt = dst_rt6_info(skb_dst(skb));
 
+	rcu_read_lock();
 	mrt = ip6mr_get_table(net, RT6_TABLE_DFLT);
-	if (!mrt)
-		return -ENOENT;
+	if (!mrt) {
+		err = -ENOENT;
+		goto out;
+	}
 
-	rcu_read_lock();
 	cache = ip6mr_cache_find(mrt, &rt->rt6i_src.addr, &rt->rt6i_dst.addr);
 	if (!cache && skb->dev) {
 		int vif = ip6mr_find_vif(mrt, skb->dev);
@@ -2324,15 +2326,15 @@ int ip6mr_get_route(struct net *net, struct sk_buff *skb, struct rtmsg *rtm,
 
 		dev = skb->dev;
 		if (!dev || (vif = ip6mr_find_vif(mrt, dev)) < 0) {
-			rcu_read_unlock();
-			return -ENODEV;
+			err = -ENODEV;
+			goto out;
 		}
 
 		/* really correct? */
 		skb2 = alloc_skb(sizeof(struct ipv6hdr), GFP_ATOMIC);
 		if (!skb2) {
-			rcu_read_unlock();
-			return -ENOMEM;
+			err = -ENOMEM;
+			goto out;
 		}
 
 		NETLINK_CB(skb2).portid = portid;
@@ -2354,12 +2356,13 @@ int ip6mr_get_route(struct net *net, struct sk_buff *skb, struct rtmsg *rtm,
 		iph->daddr = rt->rt6i_dst.addr;
 
 		err = ip6mr_cache_unresolved(mrt, vif, skb2, dev);
-		rcu_read_unlock();
 
-		return err;
+		goto out;
 	}
 
 	err = mr_fill_mroute(mrt, skb, &cache->_c, rtm);
+
+out:
 	rcu_read_unlock();
 	return err;
 }
-- 
2.42.0

Re: [PATCH net v3 4/4] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_get_route()

[Eric Dumazet]

On Thu, Oct 10, 2024 at 11:12 AM Stefan Wiehler
<stefan.wiehler@nokia.com> wrote:
>
> When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
> must be done under RCU or RTNL lock.
>
> Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
> Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
> ---

Please send a cover letter in your next version.

Please add a 5th patch in the series reverting

commit b6dd5acde3f165e364881c36de942c5b252e2a27
Author: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
Date:   Sat May 16 13:15:15 2020 +0530

    ipv6: Fix suspicious RCU usage warning in ip6mr

This way, tests will detect more easily if something is wrong.

Thank you.

Re: [PATCH net v3 3/4] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_compat_ioctl()

[Simon Horman]

On Thu, Oct 10, 2024 at 11:07:44AM +0200, Stefan Wiehler wrote:
> When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
> must be done under RCU or RTNL lock. Copy from user space must be
> performed beforehand as we are not allowed to sleep under RCU lock.
> 
> Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
> Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
> ---
> v3:
>   - split into separate patches
> v2: https://patchwork.kernel.org/project/netdevbpf/patch/20241001100119.230711-2-stefan.wiehler@nokia.com/
>   - rebase on top of net tree
>   - add Fixes tag
>   - refactor out paths
> v1: https://patchwork.kernel.org/project/netdevbpf/patch/20240605195355.363936-1-oss@malat.biz/
> ---
>  net/ipv6/ip6mr.c | 46 ++++++++++++++++++++++++++++++++--------------
>  1 file changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
> index b18eb4ad21e4..415ba6f55a44 100644
> --- a/net/ipv6/ip6mr.c
> +++ b/net/ipv6/ip6mr.c
> @@ -1961,10 +1961,7 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
>  	struct mfc6_cache *c;
>  	struct net *net = sock_net(sk);
>  	struct mr_table *mrt;
> -
> -	mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
> -	if (!mrt)
> -		return -ENOENT;
> +	int err;
>  
>  	switch (cmd) {
>  	case SIOCGETMIFCNT_IN6:
> @@ -1972,8 +1969,30 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
>  			return -EFAULT;
>  		if (vr.mifi >= mrt->maxvif)
>  			return -EINVAL;

Hi Stefan,

mrt is now used uninitialised here.

> +		break;
> +	case SIOCGETSGCNT_IN6:
> +		if (copy_from_user(&sr, arg, sizeof(sr)))
> +			return -EFAULT;
> +		break;
> +	default:
> +		return -ENOIOCTLCMD;
> +	}
> +
> +
> +	rcu_read_lock();
> +	mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
> +	if (!mrt) {
> +		err = -ENOENT;
> +		goto out;
> +	}
> +
> +	switch (cmd) {
> +	case SIOCGETMIFCNT_IN6:
> +		if (vr.mifi >= mrt->maxvif) {
> +			err = -EINVAL;
> +			goto out;
> +		}
>  		vr.mifi = array_index_nospec(vr.mifi, mrt->maxvif);
> -		rcu_read_lock();
>  		vif = &mrt->vif_table[vr.mifi];
>  		if (VIF_EXISTS(mrt, vr.mifi)) {
>  			vr.icount = READ_ONCE(vif->pkt_in);

...

> @@ -2004,11 +2020,13 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
>  				return -EFAULT;
>  			return 0;
>  		}
> -		rcu_read_unlock();
> -		return -EADDRNOTAVAIL;
> -	default:
> -		return -ENOIOCTLCMD;
> +		err = -EADDRNOTAVAIL;
> +		goto out;
>  	}
> +

I think that this out label should be used consistently once rcu_read_lock
has been taken. With this patch applied there seems to be one case on error
where rcu_read_unlock() before returning, and one case where it isn't
(which looks like it leaks the lock).

> +out:
> +	rcu_read_unlock();
> +	return err;
>  }
>  #endif

-- 
pw-bot: changes-requested

Re: [PATCH net v3 1/4] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_vif_seq_start()

[Eric Dumazet]

On Thu, Oct 10, 2024 at 11:11 AM Stefan Wiehler
<stefan.wiehler@nokia.com> wrote:
>
> When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
> must be done under RCU or RTNL lock.

Could you add in the changelog that ip6mr_vif_seq_stop() would be called
in the (currently not possible) case ip6mr_vif_seq_start() returns an error ?

>
> Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
> Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")

Reviewed-by: Eric Dumazet <edumazet@google.com>

Thanks.

Re: [PATCH net v3 3/4] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_compat_ioctl()

[Stefan Wiehler]

>> When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
>> must be done under RCU or RTNL lock. Copy from user space must be
>> performed beforehand as we are not allowed to sleep under RCU lock.
>>
>> Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
>> Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
>> ---
>> v3:
>>   - split into separate patches
>> v2: https://patchwork.kernel.org/project/netdevbpf/patch/20241001100119.230711-2-stefan.wiehler@nokia.com/
>>   - rebase on top of net tree
>>   - add Fixes tag
>>   - refactor out paths
>> v1: https://patchwork.kernel.org/project/netdevbpf/patch/20240605195355.363936-1-oss@malat.biz/
>> ---
>>  net/ipv6/ip6mr.c | 46 ++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 32 insertions(+), 14 deletions(-)
>>
>> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
>> index b18eb4ad21e4..415ba6f55a44 100644
>> --- a/net/ipv6/ip6mr.c
>> +++ b/net/ipv6/ip6mr.c
>> @@ -1961,10 +1961,7 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
>>       struct mfc6_cache *c;
>>       struct net *net = sock_net(sk);
>>       struct mr_table *mrt;
>> -
>> -     mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
>> -     if (!mrt)
>> -             return -ENOENT;
>> +     int err;
>>
>>       switch (cmd) {
>>       case SIOCGETMIFCNT_IN6:
>> @@ -1972,8 +1969,30 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
>>                       return -EFAULT;
>>               if (vr.mifi >= mrt->maxvif)
>>                       return -EINVAL;
> 
> Hi Stefan,
> 
> mrt is now used uninitialised here.

Thanks, that was an accident, it should have stayed where it is.

>> +             break;
>> +     case SIOCGETSGCNT_IN6:
>> +             if (copy_from_user(&sr, arg, sizeof(sr)))
>> +                     return -EFAULT;
>> +             break;
>> +     default:
>> +             return -ENOIOCTLCMD;
>> +     }
>> +
>> +
>> +     rcu_read_lock();
>> +     mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
>> +     if (!mrt) {
>> +             err = -ENOENT;
>> +             goto out;
>> +     }
>> +
>> +     switch (cmd) {
>> +     case SIOCGETMIFCNT_IN6:
>> +             if (vr.mifi >= mrt->maxvif) {
>> +                     err = -EINVAL;
>> +                     goto out;
>> +             }
>>               vr.mifi = array_index_nospec(vr.mifi, mrt->maxvif);
>> -             rcu_read_lock();
>>               vif = &mrt->vif_table[vr.mifi];
>>               if (VIF_EXISTS(mrt, vr.mifi)) {
>>                       vr.icount = READ_ONCE(vif->pkt_in);
> 
> ...
> 
>> @@ -2004,11 +2020,13 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
>>                               return -EFAULT;
>>                       return 0;
>>               }
>> -             rcu_read_unlock();
>> -             return -EADDRNOTAVAIL;
>> -     default:
>> -             return -ENOIOCTLCMD;
>> +             err = -EADDRNOTAVAIL;
>> +             goto out;
>>       }
>> +
> 
> I think that this out label should be used consistently once rcu_read_lock
> has been taken. With this patch applied there seems to be one case on error
> where rcu_read_unlock() before returning, and one case where it isn't
> (which looks like it leaks the lock).

In the remaining two return paths we need to release the RCU lock before
calling copy_to_user(), so unfortunately we cannot use a common out label.

Kind regards,

Stefan

Re: [PATCH net v3 3/4] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_compat_ioctl()

[Simon Horman]

On Thu, Oct 10, 2024 at 04:43:34PM +0200, Stefan Wiehler wrote:
> >> When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
> >> must be done under RCU or RTNL lock. Copy from user space must be
> >> performed beforehand as we are not allowed to sleep under RCU lock.
> >>
> >> Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
> >> Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")

...

> >> @@ -2004,11 +2020,13 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
> >>                               return -EFAULT;
> >>                       return 0;
> >>               }
> >> -             rcu_read_unlock();
> >> -             return -EADDRNOTAVAIL;
> >> -     default:
> >> -             return -ENOIOCTLCMD;
> >> +             err = -EADDRNOTAVAIL;
> >> +             goto out;
> >>       }
> >> +
> > 
> > I think that this out label should be used consistently once rcu_read_lock
> > has been taken. With this patch applied there seems to be one case on error
> > where rcu_read_unlock() before returning, and one case where it isn't
> > (which looks like it leaks the lock).
> 
> In the remaining two return paths we need to release the RCU lock before
> calling copy_to_user(), so unfortunately we cannot use a common out label.

Ok, sure. But can you check that the code always releases the lock?

Re: [PATCH net v3 3/4] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_compat_ioctl()

[kernel test robot]

Hi Stefan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Stefan-Wiehler/ip6mr-Lock-RCU-before-ip6mr_get_table-call-in-ip6mr_ioctl/20241010-171634
base:   net/main
patch link:    https://lore.kernel.org/r/20241010090741.1980100-7-stefan.wiehler%40nokia.com
patch subject: [PATCH net v3 3/4] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_compat_ioctl()
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20241012/202410120109.QyXpPs23-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241012/202410120109.QyXpPs23-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410120109.QyXpPs23-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> net/ipv6/ip6mr.c:1970:18: warning: variable 'mrt' is uninitialized when used here [-Wuninitialized]
    1970 |                 if (vr.mifi >= mrt->maxvif)
         |                                ^~~
   net/ipv6/ip6mr.c:1963:22: note: initialize the variable 'mrt' to silence this warning
    1963 |         struct mr_table *mrt;
         |                             ^
         |                              = NULL
   1 warning generated.


vim +/mrt +1970 net/ipv6/ip6mr.c

e2d57766e6744f David S. Miller   2011-02-03  1955  
e2d57766e6744f David S. Miller   2011-02-03  1956  int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
e2d57766e6744f David S. Miller   2011-02-03  1957  {
e2d57766e6744f David S. Miller   2011-02-03  1958  	struct compat_sioc_sg_req6 sr;
e2d57766e6744f David S. Miller   2011-02-03  1959  	struct compat_sioc_mif_req6 vr;
6853f21f764b04 Yuval Mintz       2018-02-28  1960  	struct vif_device *vif;
e2d57766e6744f David S. Miller   2011-02-03  1961  	struct mfc6_cache *c;
e2d57766e6744f David S. Miller   2011-02-03  1962  	struct net *net = sock_net(sk);
b70432f7319eb7 Yuval Mintz       2018-02-28  1963  	struct mr_table *mrt;
9135a3aca0c10d Stefan Wiehler    2024-10-10  1964  	int err;
e2d57766e6744f David S. Miller   2011-02-03  1965  
e2d57766e6744f David S. Miller   2011-02-03  1966  	switch (cmd) {
e2d57766e6744f David S. Miller   2011-02-03  1967  	case SIOCGETMIFCNT_IN6:
e2d57766e6744f David S. Miller   2011-02-03  1968  		if (copy_from_user(&vr, arg, sizeof(vr)))
e2d57766e6744f David S. Miller   2011-02-03  1969  			return -EFAULT;
e2d57766e6744f David S. Miller   2011-02-03 @1970  		if (vr.mifi >= mrt->maxvif)
e2d57766e6744f David S. Miller   2011-02-03  1971  			return -EINVAL;
9135a3aca0c10d Stefan Wiehler    2024-10-10  1972  		break;
9135a3aca0c10d Stefan Wiehler    2024-10-10  1973  	case SIOCGETSGCNT_IN6:
9135a3aca0c10d Stefan Wiehler    2024-10-10  1974  		if (copy_from_user(&sr, arg, sizeof(sr)))
9135a3aca0c10d Stefan Wiehler    2024-10-10  1975  			return -EFAULT;
9135a3aca0c10d Stefan Wiehler    2024-10-10  1976  		break;
9135a3aca0c10d Stefan Wiehler    2024-10-10  1977  	default:
9135a3aca0c10d Stefan Wiehler    2024-10-10  1978  		return -ENOIOCTLCMD;
9135a3aca0c10d Stefan Wiehler    2024-10-10  1979  	}
9135a3aca0c10d Stefan Wiehler    2024-10-10  1980  
9135a3aca0c10d Stefan Wiehler    2024-10-10  1981  
638cf4a24a09d1 Eric Dumazet      2022-06-23  1982  	rcu_read_lock();
9135a3aca0c10d Stefan Wiehler    2024-10-10  1983  	mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
9135a3aca0c10d Stefan Wiehler    2024-10-10  1984  	if (!mrt) {
9135a3aca0c10d Stefan Wiehler    2024-10-10  1985  		err = -ENOENT;
9135a3aca0c10d Stefan Wiehler    2024-10-10  1986  		goto out;
9135a3aca0c10d Stefan Wiehler    2024-10-10  1987  	}
9135a3aca0c10d Stefan Wiehler    2024-10-10  1988  
9135a3aca0c10d Stefan Wiehler    2024-10-10  1989  	switch (cmd) {
9135a3aca0c10d Stefan Wiehler    2024-10-10  1990  	case SIOCGETMIFCNT_IN6:
9135a3aca0c10d Stefan Wiehler    2024-10-10  1991  		if (vr.mifi >= mrt->maxvif) {
9135a3aca0c10d Stefan Wiehler    2024-10-10  1992  			err = -EINVAL;
9135a3aca0c10d Stefan Wiehler    2024-10-10  1993  			goto out;
9135a3aca0c10d Stefan Wiehler    2024-10-10  1994  		}
9135a3aca0c10d Stefan Wiehler    2024-10-10  1995  		vr.mifi = array_index_nospec(vr.mifi, mrt->maxvif);
b70432f7319eb7 Yuval Mintz       2018-02-28  1996  		vif = &mrt->vif_table[vr.mifi];
b70432f7319eb7 Yuval Mintz       2018-02-28  1997  		if (VIF_EXISTS(mrt, vr.mifi)) {
638cf4a24a09d1 Eric Dumazet      2022-06-23  1998  			vr.icount = READ_ONCE(vif->pkt_in);
638cf4a24a09d1 Eric Dumazet      2022-06-23  1999  			vr.ocount = READ_ONCE(vif->pkt_out);
638cf4a24a09d1 Eric Dumazet      2022-06-23  2000  			vr.ibytes = READ_ONCE(vif->bytes_in);
638cf4a24a09d1 Eric Dumazet      2022-06-23  2001  			vr.obytes = READ_ONCE(vif->bytes_out);
638cf4a24a09d1 Eric Dumazet      2022-06-23  2002  			rcu_read_unlock();
e2d57766e6744f David S. Miller   2011-02-03  2003  
e2d57766e6744f David S. Miller   2011-02-03  2004  			if (copy_to_user(arg, &vr, sizeof(vr)))
e2d57766e6744f David S. Miller   2011-02-03  2005  				return -EFAULT;
e2d57766e6744f David S. Miller   2011-02-03  2006  			return 0;
e2d57766e6744f David S. Miller   2011-02-03  2007  		}
638cf4a24a09d1 Eric Dumazet      2022-06-23  2008  		rcu_read_unlock();
9135a3aca0c10d Stefan Wiehler    2024-10-10  2009  		err = -EADDRNOTAVAIL;
9135a3aca0c10d Stefan Wiehler    2024-10-10  2010  		goto out;
e2d57766e6744f David S. Miller   2011-02-03  2011  	case SIOCGETSGCNT_IN6:
e2d57766e6744f David S. Miller   2011-02-03  2012  		c = ip6mr_cache_find(mrt, &sr.src.sin6_addr, &sr.grp.sin6_addr);
e2d57766e6744f David S. Miller   2011-02-03  2013  		if (c) {
494fff56379c4a Yuval Mintz       2018-02-28  2014  			sr.pktcnt = c->_c.mfc_un.res.pkt;
494fff56379c4a Yuval Mintz       2018-02-28  2015  			sr.bytecnt = c->_c.mfc_un.res.bytes;
494fff56379c4a Yuval Mintz       2018-02-28  2016  			sr.wrong_if = c->_c.mfc_un.res.wrong_if;
87c418bf1323d5 Yuval Mintz       2018-02-28  2017  			rcu_read_unlock();
e2d57766e6744f David S. Miller   2011-02-03  2018  
e2d57766e6744f David S. Miller   2011-02-03  2019  			if (copy_to_user(arg, &sr, sizeof(sr)))
e2d57766e6744f David S. Miller   2011-02-03  2020  				return -EFAULT;
e2d57766e6744f David S. Miller   2011-02-03  2021  			return 0;
e2d57766e6744f David S. Miller   2011-02-03  2022  		}
9135a3aca0c10d Stefan Wiehler    2024-10-10  2023  		err = -EADDRNOTAVAIL;
9135a3aca0c10d Stefan Wiehler    2024-10-10  2024  		goto out;
e2d57766e6744f David S. Miller   2011-02-03  2025  	}
9135a3aca0c10d Stefan Wiehler    2024-10-10  2026  
9135a3aca0c10d Stefan Wiehler    2024-10-10  2027  out:
9135a3aca0c10d Stefan Wiehler    2024-10-10  2028  	rcu_read_unlock();
9135a3aca0c10d Stefan Wiehler    2024-10-10  2029  	return err;
e2d57766e6744f David S. Miller   2011-02-03  2030  }
e2d57766e6744f David S. Miller   2011-02-03  2031  #endif
7bc570c8b4f75d YOSHIFUJI Hideaki 2008-04-03  2032  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net v3 3/4] ip6mr: Lock RCU before ip6mr_get_table() call in ip6mr_compat_ioctl()

[Stefan Wiehler]

>>>> When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table()
>>>> must be done under RCU or RTNL lock. Copy from user space must be
>>>> performed beforehand as we are not allowed to sleep under RCU lock.
>>>>
>>>> Signed-off-by: Stefan Wiehler <stefan.wiehler@nokia.com>
>>>> Fixes: d1db275dd3f6 ("ipv6: ip6mr: support multiple tables")
> 
> ...
> 
>>>> @@ -2004,11 +2020,13 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
>>>>                               return -EFAULT;
>>>>                       return 0;
>>>>               }
>>>> -             rcu_read_unlock();
>>>> -             return -EADDRNOTAVAIL;
>>>> -     default:
>>>> -             return -ENOIOCTLCMD;
>>>> +             err = -EADDRNOTAVAIL;
>>>> +             goto out;
>>>>       }
>>>> +
>>>
>>> I think that this out label should be used consistently once rcu_read_lock
>>> has been taken. With this patch applied there seems to be one case on error
>>> where rcu_read_unlock() before returning, and one case where it isn't
>>> (which looks like it leaks the lock).
>>
>> In the remaining two return paths we need to release the RCU lock before
>> calling copy_to_user(), so unfortunately we cannot use a common out label.
> 
> Ok, sure. But can you check that the code always releases the lock?

Yes, it should release the lock in all cases.

Kind regards,

Stefan