[PATCH] ip6mr: Fix lockdep and sparse RCU warnings

[PATCH] ip6mr: Fix lockdep and sparse RCU warnings

[Petr Malat]

ip6mr_vif_seq_start() must lock RCU even in a case of error, because
stop callback is called unconditionally.

When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table
should be done under RCU or RTNL lock. Lock RCU before the call unless
it's done already or RTNL lock is held.

Signed-off-by: Petr Malat <oss@malat.biz>
---
 net/ipv6/ip6mr.c | 52 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 18 deletions(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index cb0ee81a068a..bf6932535d6d 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -411,13 +411,14 @@ 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);
 }
 
@@ -1885,17 +1886,21 @@ int ip6mr_ioctl(struct sock *sk, int cmd, void *arg)
 	struct net *net = sock_net(sk);
 	struct mr_table *mrt;
 
+	rcu_read_lock();
 	mrt = ip6mr_get_table(net, raw6_sk(sk)->ip6mr_table ? : RT6_TABLE_DFLT);
-	if (!mrt)
+	if (!mrt) {
+		rcu_read_unlock();
 		return -ENOENT;
+	}
 
 	switch (cmd) {
 	case SIOCGETMIFCNT_IN6:
 		vr = (struct sioc_mif_req6 *)arg;
-		if (vr->mifi >= mrt->maxvif)
+		if (vr->mifi >= mrt->maxvif) {
+			rcu_read_unlock();
 			return -EINVAL;
+		}
 		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);
@@ -1910,7 +1915,6 @@ int ip6mr_ioctl(struct sock *sk, int cmd, void *arg)
 	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) {
@@ -1923,6 +1927,7 @@ int ip6mr_ioctl(struct sock *sk, int cmd, void *arg)
 		rcu_read_unlock();
 		return -EADDRNOTAVAIL;
 	default:
+		rcu_read_unlock();
 		return -ENOIOCTLCMD;
 	}
 }
@@ -1953,18 +1958,33 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
 	struct net *net = sock_net(sk);
 	struct mr_table *mrt;
 
+	switch (cmd) {
+	case SIOCGETMIFCNT_IN6:
+		if (copy_from_user(&vr, arg, sizeof(vr)))
+			return -EFAULT;
+		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)
+	if (!mrt) {
+		rcu_read_unlock();
 		return -ENOENT;
+	}
 
 	switch (cmd) {
 	case SIOCGETMIFCNT_IN6:
-		if (copy_from_user(&vr, arg, sizeof(vr)))
-			return -EFAULT;
-		if (vr.mifi >= mrt->maxvif)
+		if (vr.mifi >= mrt->maxvif) {
+			rcu_read_unlock();
 			return -EINVAL;
+		}
 		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);
@@ -1980,10 +2000,6 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
 		rcu_read_unlock();
 		return -EADDRNOTAVAIL;
 	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;
@@ -1997,8 +2013,6 @@ int ip6mr_compat_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
 		}
 		rcu_read_unlock();
 		return -EADDRNOTAVAIL;
-	default:
-		return -ENOIOCTLCMD;
 	}
 }
 #endif
@@ -2275,11 +2289,13 @@ int ip6mr_get_route(struct net *net, struct sk_buff *skb, struct rtmsg *rtm,
 	struct mfc6_cache *cache;
 	struct rt6_info *rt = (struct rt6_info *)skb_dst(skb);
 
+	rcu_read_lock();
 	mrt = ip6mr_get_table(net, RT6_TABLE_DFLT);
-	if (!mrt)
+	if (!mrt) {
+		rcu_read_lock();
 		return -ENOENT;
+	}
 
-	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);
-- 
2.39.2

Re: [PATCH] ip6mr: Fix lockdep and sparse RCU warnings

[Jakub Kicinski]

On Wed,  5 Jun 2024 21:53:55 +0200 Petr Malat wrote:
> ip6mr_vif_seq_start() must lock RCU even in a case of error, because
> stop callback is called unconditionally.
> 
> When IPV6_MROUTE_MULTIPLE_TABLES is enabled, calls to ip6mr_get_table
> should be done under RCU or RTNL lock. Lock RCU before the call unless
> it's done already or RTNL lock is held.

Patch does not apply, please rebase on:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
if its still legit.

And please add a Fixes tag, since its a fix.

> Signed-off-by: Petr Malat <oss@malat.biz>
> ---
>  net/ipv6/ip6mr.c | 52 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 34 insertions(+), 18 deletions(-)
> 
> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
> index cb0ee81a068a..bf6932535d6d 100644
> --- a/net/ipv6/ip6mr.c
> +++ b/net/ipv6/ip6mr.c
> @@ -411,13 +411,14 @@ 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);

Double check for bugs like missing unlock, too...