[PATCH v1 net-next 0/3] dev: Covnert dev_change_name() to per-netns RTNL.

[PATCH v1 net-next 0/3] dev: Covnert dev_change_name() to per-netns RTNL.

[Kuniyuki Iwashima]

Patch 1 adds a missing netdev_rename_lock in dev_change_name()
and Patch 2 removes unnecessary devnet_rename_sem there.

Patch 3 replaces RTNL with rtnl_net_lock() in dev_ifsioc(),
and now dev_change_name() is always called under per-netns RTNL.

Given it's close to -rc8 and Patch 1 touches the trivial unlikely
path, can Patch 1 go into net-next ?  Otherwise I'll post Patch 2 & 3
separately in the next cycle.


Kuniyuki Iwashima (3):
  dev: Acquire netdev_rename_lock before restoring dev->name in
    dev_change_name().
  dev: Remove devnet_rename_sem.
  dev: Hold rtnl_net_lock() for dev_ifsioc().

 net/core/dev.c            | 25 ++++++-------------------
 net/core/dev_ioctl.c      | 26 +++++++++++++++++---------
 net/core/rtnl_net_debug.c | 15 +++------------
 3 files changed, 26 insertions(+), 40 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 1/3] dev: Acquire netdev_rename_lock before restoring dev->name in dev_change_name().

[Kuniyuki Iwashima]

The cited commit forgot to add netdev_rename_lock in one of the
error paths in dev_change_name().

Let's hold netdev_rename_lock before restoring the old dev->name.

Fixes: 0840556e5a3a ("net: Protect dev->name by seqlock.")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/core/dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index fda4e1039bf0..0237687d4a41 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1277,7 +1277,9 @@ int dev_change_name(struct net_device *dev, const char *newname)
 rollback:
 	ret = device_rename(&dev->dev, dev->name);
 	if (ret) {
+		write_seqlock_bh(&netdev_rename_lock);
 		memcpy(dev->name, oldname, IFNAMSIZ);
+		write_sequnlock_bh(&netdev_rename_lock);
 		WRITE_ONCE(dev->name_assign_type, old_assign_type);
 		up_write(&devnet_rename_sem);
 		return ret;
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 2/3] dev: Remove devnet_rename_sem.

[Kuniyuki Iwashima]

devnet_rename_sem is no longer used since commit
0840556e5a3a ("net: Protect dev->name by seqlock.").

Also, RTNL serialises dev_change_name().

Let's remove devnet_rename_sem.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/core/dev.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 0237687d4a41..7d30129bf2a0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -180,8 +180,6 @@ static DEFINE_SPINLOCK(napi_hash_lock);
 static unsigned int napi_gen_id = NR_CPUS;
 static DEFINE_READ_MOSTLY_HASHTABLE(napi_hash, 8);
 
-static DECLARE_RWSEM(devnet_rename_sem);
-
 static inline void dev_base_seq_inc(struct net *net)
 {
 	unsigned int val = net->dev_base_seq + 1;
@@ -1249,12 +1247,8 @@ int dev_change_name(struct net_device *dev, const char *newname)
 
 	net = dev_net(dev);
 
-	down_write(&devnet_rename_sem);
-
-	if (strncmp(newname, dev->name, IFNAMSIZ) == 0) {
-		up_write(&devnet_rename_sem);
+	if (!strncmp(newname, dev->name, IFNAMSIZ))
 		return 0;
-	}
 
 	memcpy(oldname, dev->name, IFNAMSIZ);
 
@@ -1262,10 +1256,8 @@ int dev_change_name(struct net_device *dev, const char *newname)
 	err = dev_get_valid_name(net, dev, newname);
 	write_sequnlock_bh(&netdev_rename_lock);
 
-	if (err < 0) {
-		up_write(&devnet_rename_sem);
+	if (err < 0)
 		return err;
-	}
 
 	if (oldname[0] && !strchr(oldname, '%'))
 		netdev_info(dev, "renamed from %s%s\n", oldname,
@@ -1281,12 +1273,9 @@ int dev_change_name(struct net_device *dev, const char *newname)
 		memcpy(dev->name, oldname, IFNAMSIZ);
 		write_sequnlock_bh(&netdev_rename_lock);
 		WRITE_ONCE(dev->name_assign_type, old_assign_type);
-		up_write(&devnet_rename_sem);
 		return ret;
 	}
 
-	up_write(&devnet_rename_sem);
-
 	netdev_adjacent_rename_links(dev, oldname);
 
 	netdev_name_node_del(dev->name_node);
@@ -1302,7 +1291,6 @@ int dev_change_name(struct net_device *dev, const char *newname)
 		/* err >= 0 after dev_alloc_name() or stores the first errno */
 		if (err >= 0) {
 			err = ret;
-			down_write(&devnet_rename_sem);
 			write_seqlock_bh(&netdev_rename_lock);
 			memcpy(dev->name, oldname, IFNAMSIZ);
 			write_sequnlock_bh(&netdev_rename_lock);
-- 
2.39.5 (Apple Git-154)

[PATCH v1 net-next 3/3] dev: Hold rtnl_net_lock() for dev_ifsioc().

[Kuniyuki Iwashima]

Basically, dev_ifsioc() operates on the passed single netns (except
for netdev notifier chains with lower/upper devices for which we will
need more changes).

Let's hold rtnl_net_lock() for dev_ifsioc().

Now that NETDEV_CHANGENAME is always triggered under rtnl_net_lock()
of the device's netns. (do_setlink() and dev_ifsioc())

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/core/dev.c            |  7 ++-----
 net/core/dev_ioctl.c      | 26 +++++++++++++++++---------
 net/core/rtnl_net_debug.c | 15 +++------------
 3 files changed, 22 insertions(+), 26 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 7d30129bf2a0..01b6e1b1f983 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1236,16 +1236,13 @@ static int dev_get_valid_name(struct net *net, struct net_device *dev,
  */
 int dev_change_name(struct net_device *dev, const char *newname)
 {
+	struct net *net = dev_net(dev);
 	unsigned char old_assign_type;
 	char oldname[IFNAMSIZ];
 	int err = 0;
 	int ret;
-	struct net *net;
-
-	ASSERT_RTNL();
-	BUG_ON(!dev_net(dev));
 
-	net = dev_net(dev);
+	ASSERT_RTNL_NET(net);
 
 	if (!strncmp(newname, dev->name, IFNAMSIZ))
 		return 0;
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 087a57b7e4fa..4c2098ac9d72 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -543,7 +543,7 @@ static int dev_siocwandev(struct net_device *dev, struct if_settings *ifs)
 }
 
 /*
- *	Perform the SIOCxIFxxx calls, inside rtnl_lock()
+ *	Perform the SIOCxIFxxx calls, inside rtnl_net_lock()
  */
 static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
 		      unsigned int cmd)
@@ -620,11 +620,14 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
 			return -ENODEV;
 		if (!netif_is_bridge_master(dev))
 			return -EOPNOTSUPP;
+
 		netdev_hold(dev, &dev_tracker, GFP_KERNEL);
-		rtnl_unlock();
+		rtnl_net_unlock(net);
+
 		err = br_ioctl_call(net, netdev_priv(dev), cmd, ifr, NULL);
+
 		netdev_put(dev, &dev_tracker);
-		rtnl_lock();
+		rtnl_net_lock(net);
 		return err;
 
 	case SIOCDEVPRIVATE ... SIOCDEVPRIVATE + 15:
@@ -770,9 +773,11 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr,
 		dev_load(net, ifr->ifr_name);
 		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
 			return -EPERM;
-		rtnl_lock();
+
+		rtnl_net_lock(net);
 		ret = dev_ifsioc(net, ifr, data, cmd);
-		rtnl_unlock();
+		rtnl_net_unlock(net);
+
 		if (colon)
 			*colon = ':';
 		return ret;
@@ -816,9 +821,11 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr,
 	case SIOCBONDSLAVEINFOQUERY:
 	case SIOCBONDINFOQUERY:
 		dev_load(net, ifr->ifr_name);
-		rtnl_lock();
+
+		rtnl_net_lock(net);
 		ret = dev_ifsioc(net, ifr, data, cmd);
-		rtnl_unlock();
+		rtnl_net_unlock(net);
+
 		if (need_copyout)
 			*need_copyout = false;
 		return ret;
@@ -841,9 +848,10 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr,
 		    (cmd >= SIOCDEVPRIVATE &&
 		     cmd <= SIOCDEVPRIVATE + 15)) {
 			dev_load(net, ifr->ifr_name);
-			rtnl_lock();
+
+			rtnl_net_lock(net);
 			ret = dev_ifsioc(net, ifr, data, cmd);
-			rtnl_unlock();
+			rtnl_net_unlock(net);
 			return ret;
 		}
 		return -ENOTTY;
diff --git a/net/core/rtnl_net_debug.c b/net/core/rtnl_net_debug.c
index f406045cbd0e..7ecd28cc1c22 100644
--- a/net/core/rtnl_net_debug.c
+++ b/net/core/rtnl_net_debug.c
@@ -27,7 +27,6 @@ static int rtnl_net_debug_event(struct notifier_block *nb,
 	case NETDEV_CHANGEADDR:
 	case NETDEV_PRE_CHANGEADDR:
 	case NETDEV_GOING_DOWN:
-	case NETDEV_CHANGENAME:
 	case NETDEV_FEAT_CHANGE:
 	case NETDEV_BONDING_FAILOVER:
 	case NETDEV_PRE_UP:
@@ -60,18 +59,10 @@ static int rtnl_net_debug_event(struct notifier_block *nb,
 		ASSERT_RTNL();
 		break;
 
-	/* Once an event fully supports RTNL_NET, move it here
-	 * and remove "if (0)" below.
-	 *
-	 * case NETDEV_XXX:
-	 *	ASSERT_RTNL_NET(net);
-	 *	break;
-	 */
-	}
-
-	/* Just to avoid unused-variable error for dev and net. */
-	if (0)
+	case NETDEV_CHANGENAME:
 		ASSERT_RTNL_NET(net);
+		break;
+	}
 
 	return NOTIFY_DONE;
 }
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v1 net-next 1/3] dev: Acquire netdev_rename_lock before restoring dev->name in dev_change_name().

[Eric Dumazet]

On Wed, Jan 15, 2025 at 10:56 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> The cited commit forgot to add netdev_rename_lock in one of the
> error paths in dev_change_name().
>
> Let's hold netdev_rename_lock before restoring the old dev->name.
>
> Fixes: 0840556e5a3a ("net: Protect dev->name by seqlock.")
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

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

Re: [PATCH v1 net-next 2/3] dev: Remove devnet_rename_sem.

[Eric Dumazet]

On Wed, Jan 15, 2025 at 10:57 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> devnet_rename_sem is no longer used since commit
> 0840556e5a3a ("net: Protect dev->name by seqlock.").
>
> Also, RTNL serialises dev_change_name().
>
> Let's remove devnet_rename_sem.
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

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

Re: [PATCH v1 net-next 3/3] dev: Hold rtnl_net_lock() for dev_ifsioc().

[Eric Dumazet]

On Wed, Jan 15, 2025 at 10:57 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> Basically, dev_ifsioc() operates on the passed single netns (except
> for netdev notifier chains with lower/upper devices for which we will
> need more changes).
>
> Let's hold rtnl_net_lock() for dev_ifsioc().
>
> Now that NETDEV_CHANGENAME is always triggered under rtnl_net_lock()
> of the device's netns. (do_setlink() and dev_ifsioc())
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

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

Re: [PATCH v1 net-next 0/3] dev: Covnert dev_change_name() to per-netns RTNL.

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 15 Jan 2025 18:55:42 +0900 you wrote:
> Patch 1 adds a missing netdev_rename_lock in dev_change_name()
> and Patch 2 removes unnecessary devnet_rename_sem there.
> 
> Patch 3 replaces RTNL with rtnl_net_lock() in dev_ifsioc(),
> and now dev_change_name() is always called under per-netns RTNL.
> 
> Given it's close to -rc8 and Patch 1 touches the trivial unlikely
> path, can Patch 1 go into net-next ?  Otherwise I'll post Patch 2 & 3
> separately in the next cycle.
> 
> [...]

Here is the summary with links:
  - [v1,net-next,1/3] dev: Acquire netdev_rename_lock before restoring dev->name in dev_change_name().
    https://git.kernel.org/netdev/net-next/c/e361560a7912
  - [v1,net-next,2/3] dev: Remove devnet_rename_sem.
    https://git.kernel.org/netdev/net-next/c/2f1bb1e2cc00
  - [v1,net-next,3/3] dev: Hold rtnl_net_lock() for dev_ifsioc().
    https://git.kernel.org/netdev/net-next/c/be94cfdb993f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html