[PATCH v2 net-next 0/7] rtnetlink: Convert rtnl_newlink() to per-netns RTNL.

[PATCH v2 net-next 0/7] rtnetlink: Convert rtnl_newlink() to per-netns RTNL.

[Kuniyuki Iwashima]

Patch 1 introduces struct rtnl_nets and helper functions to acquire
multiple per-netns RTNL in rtnl_newlink().

Patch 2 - 5 are to prefetch the peer device's netns in rtnl_newlink().

Patch 6 converts rtnl_newlink() to per-netns RTNL.

Patch 7 pushes RTNL down to rtnl_dellink() and rtnl_setlink(), but
the conversion will not be completed unless we support cases with
peer/upper/lower devices.


Changes:
  v2
    * Patch 1
      * Move struct rtnl_nets to rtnetlink.c
      * Unexport rtnl_nets_add()
    * Patch 2
      * Rename the helper to rtnl_link_get_net_ifla()
      * Unexport rtnl_link_get_net_ifla()
      * Change peer_type to u16
    * Patch 6
      * Remove __rtnl_unlock() dance

  v1: https://lore.kernel.org/netdev/20241105020514.41963-1-kuniyu@amazon.com/


Kuniyuki Iwashima (7):
  rtnetlink: Introduce struct rtnl_nets and helpers.
  rtnetlink: Add peer_type in struct rtnl_link_ops.
  veth: Set VETH_INFO_PEER to veth_link_ops.peer_type.
  vxcan: Set VXCAN_INFO_PEER to vxcan_link_ops.peer_type.
  netkit: Set IFLA_NETKIT_PEER_INFO to netkit_link_ops.peer_type.
  rtnetlink: Convert RTM_NEWLINK to per-netns RTNL.
  rtnetlink: Register rtnl_dellink() and rtnl_setlink() with
    RTNL_FLAG_DOIT_PERNET_WIP.

 drivers/net/can/vxcan.c |  12 +--
 drivers/net/netkit.c    |  11 +--
 drivers/net/veth.c      |  18 +----
 include/net/rtnetlink.h |   3 +
 net/core/rtnetlink.c    | 171 +++++++++++++++++++++++++++++++++++++---
 5 files changed, 170 insertions(+), 45 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH v2 net-next 1/7] rtnetlink: Introduce struct rtnl_nets and helpers.

[Kuniyuki Iwashima]

rtnl_newlink() needs to hold 3 per-netns RTNL: 2 for a new device
and 1 for its peer.

We will add rtnl_nets_lock() later, which performs the nested locking
based on struct rtnl_nets, which has an array of struct net pointers.

rtnl_nets_add() adds a net pointer to the array and sorts it so that
rtnl_nets_lock() can simply acquire per-netns RTNL from array[0] to [2].

Before calling rtnl_nets_add(), get_net() must be called for the net,
and rtnl_nets_destroy() will call put_net() for each.

Let's apply the helpers to rtnl_newlink().

When CONFIG_DEBUG_NET_SMALL_RTNL is disabled, we do not call
rtnl_net_lock() thus do not care about the array order, so
rtnl_net_cmp_locks() returns -1 so that the loop in rtnl_nets_add()
can be optimised to NOP.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
v2:
  * Move struct rtnl_nets to net/core/rtnetlink.c
  * Unexport rtnl_nets_add()
---
 net/core/rtnetlink.c | 70 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 67 insertions(+), 3 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 3b33810d92a8..81f4722c1353 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -258,8 +258,67 @@ bool lockdep_rtnl_net_is_held(struct net *net)
 	return lockdep_rtnl_is_held() && lockdep_is_held(&net->rtnl_mutex);
 }
 EXPORT_SYMBOL(lockdep_rtnl_net_is_held);
+#else
+static int rtnl_net_cmp_locks(const struct net *net_a, const struct net *net_b)
+{
+	/* No need to swap */
+	return -1;
+}
 #endif
 
+struct rtnl_nets {
+	/* ->newlink() needs to freeze 3 netns at most;
+	 * 2 for the new device, 1 for its peer.
+	 */
+	struct net *net[3];
+	unsigned char len;
+};
+
+static void rtnl_nets_init(struct rtnl_nets *rtnl_nets)
+{
+	memset(rtnl_nets, 0, sizeof(*rtnl_nets));
+}
+
+static void rtnl_nets_destroy(struct rtnl_nets *rtnl_nets)
+{
+	int i;
+
+	for (i = 0; i < rtnl_nets->len; i++) {
+		put_net(rtnl_nets->net[i]);
+		rtnl_nets->net[i] = NULL;
+	}
+
+	rtnl_nets->len = 0;
+}
+
+/**
+ * rtnl_nets_add - Add netns to be locked before ->newlink().
+ *
+ * @rtnl_nets: rtnl_nets pointer passed to ->get_peer_net().
+ * @net: netns pointer with an extra refcnt held.
+ *
+ * The extra refcnt is released in rtnl_nets_destroy().
+ */
+static void rtnl_nets_add(struct rtnl_nets *rtnl_nets, struct net *net)
+{
+	int i;
+
+	DEBUG_NET_WARN_ON_ONCE(rtnl_nets->len == ARRAY_SIZE(rtnl_nets->net));
+
+	for (i = 0; i < rtnl_nets->len; i++) {
+		switch (rtnl_net_cmp_locks(rtnl_nets->net[i], net)) {
+		case 0:
+			put_net(net);
+			return;
+		case 1:
+			swap(rtnl_nets->net[i], net);
+		}
+	}
+
+	rtnl_nets->net[i] = net;
+	rtnl_nets->len++;
+}
+
 static struct rtnl_link __rcu *__rcu *rtnl_msg_handlers[RTNL_FAMILY_MAX + 1];
 
 static inline int rtm_msgindex(int msgtype)
@@ -3796,6 +3855,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct net *tgt_net, *link_net = NULL;
 	struct rtnl_link_ops *ops = NULL;
 	struct rtnl_newlink_tbs *tbs;
+	struct rtnl_nets rtnl_nets;
 	int ops_srcu_index;
 	int ret;
 
@@ -3839,6 +3899,8 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 #endif
 	}
 
+	rtnl_nets_init(&rtnl_nets);
+
 	if (ops) {
 		if (ops->maxtype > RTNL_MAX_TYPE) {
 			ret = -EINVAL;
@@ -3868,6 +3930,8 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto put_ops;
 	}
 
+	rtnl_nets_add(&rtnl_nets, tgt_net);
+
 	if (tb[IFLA_LINK_NETNSID]) {
 		int id = nla_get_s32(tb[IFLA_LINK_NETNSID]);
 
@@ -3878,6 +3942,8 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 			goto put_net;
 		}
 
+		rtnl_nets_add(&rtnl_nets, link_net);
+
 		if (!netlink_ns_capable(skb, link_net->user_ns, CAP_NET_ADMIN)) {
 			ret = -EPERM;
 			goto put_net;
@@ -3887,9 +3953,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	ret = __rtnl_newlink(skb, nlh, ops, tgt_net, link_net, tbs, data, extack);
 
 put_net:
-	if (link_net)
-		put_net(link_net);
-	put_net(tgt_net);
+	rtnl_nets_destroy(&rtnl_nets);
 put_ops:
 	if (ops)
 		rtnl_link_ops_put(ops, ops_srcu_index);
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v2 net-next 1/7] rtnetlink: Introduce struct rtnl_nets and helpers.

[Nikolay Aleksandrov]

On Tue, Nov 05, 2024 at 06:24:26PM -0800, Kuniyuki Iwashima wrote:
> rtnl_newlink() needs to hold 3 per-netns RTNL: 2 for a new device
> and 1 for its peer.
> 
> We will add rtnl_nets_lock() later, which performs the nested locking
> based on struct rtnl_nets, which has an array of struct net pointers.
> 
> rtnl_nets_add() adds a net pointer to the array and sorts it so that
> rtnl_nets_lock() can simply acquire per-netns RTNL from array[0] to [2].
> 
> Before calling rtnl_nets_add(), get_net() must be called for the net,
> and rtnl_nets_destroy() will call put_net() for each.
> 
> Let's apply the helpers to rtnl_newlink().
> 
> When CONFIG_DEBUG_NET_SMALL_RTNL is disabled, we do not call
> rtnl_net_lock() thus do not care about the array order, so
> rtnl_net_cmp_locks() returns -1 so that the loop in rtnl_nets_add()
> can be optimised to NOP.
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
> ---
> v2:
>   * Move struct rtnl_nets to net/core/rtnetlink.c
>   * Unexport rtnl_nets_add()
> ---
>  net/core/rtnetlink.c | 70 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 67 insertions(+), 3 deletions(-)
> 

Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
 

[PATCH v2 net-next 2/7] rtnetlink: Add peer_type in struct rtnl_link_ops.

[Kuniyuki Iwashima]

In ops->newlink(), veth, vxcan, and netkit call rtnl_link_get_net() with
a net pointer, which is the first argument of ->newlink().

rtnl_link_get_net() could return another netns based on IFLA_NET_NS_PID
and IFLA_NET_NS_FD in the peer device's attributes.

We want to get it and fill rtnl_nets->nets[] in advance in rtnl_newlink()
for per-netns RTNL.

All of the three get the peer netns in the same way:

  1. Call rtnl_nla_parse_ifinfomsg()
  2. Call ops->validate() (vxcan doesn't have)
  3. Call rtnl_link_get_net()

Let's add a new field peer_type to struct rtnl_link_ops and prefetch
netns in the peer ifla to add it to rtnl_nets in rtnl_newlink().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
v2:
  * Rename the helper to rtnl_link_get_net_ifla()
  * Unexport rtnl_link_get_net_ifla() and made it static
  * Change peer_type to u16
  * squash patch 2 & 3 (due to static requires a user)
---
 include/net/rtnetlink.h |  2 ++
 net/core/rtnetlink.c    | 55 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index b260c0cc9671..f17208323c08 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -75,6 +75,7 @@ static inline int rtnl_msg_family(const struct nlmsghdr *nlh)
  *	@srcu: Used internally
  *	@kind: Identifier
  *	@netns_refund: Physical device, move to init_net on netns exit
+ *	@peer_type: Peer device specific netlink attribute number (e.g. VETH_INFO_PEER)
  *	@maxtype: Highest device specific netlink attribute number
  *	@policy: Netlink policy for device specific attribute validation
  *	@validate: Optional validation function for netlink/changelink parameters
@@ -116,6 +117,7 @@ struct rtnl_link_ops {
 	void			(*setup)(struct net_device *dev);
 
 	bool			netns_refund;
+	const u16		peer_type;
 	unsigned int		maxtype;
 	const struct nla_policy	*policy;
 	int			(*validate)(struct nlattr *tb[],
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 81f4722c1353..d5557a621099 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2518,9 +2518,10 @@ int rtnl_nla_parse_ifinfomsg(struct nlattr **tb, const struct nlattr *nla_peer,
 }
 EXPORT_SYMBOL(rtnl_nla_parse_ifinfomsg);
 
-struct net *rtnl_link_get_net(struct net *src_net, struct nlattr *tb[])
+static struct net *rtnl_link_get_net_ifla(struct nlattr *tb[])
 {
-	struct net *net;
+	struct net *net = NULL;
+
 	/* Examine the link attributes and figure out which
 	 * network namespace we are talking about.
 	 */
@@ -2528,8 +2529,17 @@ struct net *rtnl_link_get_net(struct net *src_net, struct nlattr *tb[])
 		net = get_net_ns_by_pid(nla_get_u32(tb[IFLA_NET_NS_PID]));
 	else if (tb[IFLA_NET_NS_FD])
 		net = get_net_ns_by_fd(nla_get_u32(tb[IFLA_NET_NS_FD]));
-	else
+
+	return net;
+}
+
+struct net *rtnl_link_get_net(struct net *src_net, struct nlattr *tb[])
+{
+	struct net *net = rtnl_link_get_net_ifla(tb);
+
+	if (!net)
 		net = get_net(src_net);
+
 	return net;
 }
 EXPORT_SYMBOL(rtnl_link_get_net);
@@ -3794,6 +3804,37 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
 	goto out;
 }
 
+static int rtnl_add_peer_net(struct rtnl_nets *rtnl_nets,
+			     const struct rtnl_link_ops *ops,
+			     struct nlattr *data[],
+			     struct netlink_ext_ack *extack)
+{
+	struct nlattr *tb[IFLA_MAX + 1];
+	struct net *net;
+	int err;
+
+	if (!data || !data[ops->peer_type])
+		return 0;
+
+	err = rtnl_nla_parse_ifinfomsg(tb, data[ops->peer_type], extack);
+	if (err < 0)
+		return err;
+
+	if (ops->validate) {
+		err = ops->validate(tb, NULL, extack);
+		if (err < 0)
+			return err;
+	}
+
+	net = rtnl_link_get_net_ifla(tb);
+	if (IS_ERR(net))
+		return PTR_ERR(net);
+	if (net)
+		rtnl_nets_add(rtnl_nets, net);
+
+	return 0;
+}
+
 static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 			  const struct rtnl_link_ops *ops,
 			  struct net *tgt_net, struct net *link_net,
@@ -3922,12 +3963,18 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 			if (ret < 0)
 				goto put_ops;
 		}
+
+		if (ops->peer_type) {
+			ret = rtnl_add_peer_net(&rtnl_nets, ops, data, extack);
+			if (ret < 0)
+				goto put_ops;
+		}
 	}
 
 	tgt_net = rtnl_link_get_net_capable(skb, sock_net(skb->sk), tb, CAP_NET_ADMIN);
 	if (IS_ERR(tgt_net)) {
 		ret = PTR_ERR(tgt_net);
-		goto put_ops;
+		goto put_net;
 	}
 
 	rtnl_nets_add(&rtnl_nets, tgt_net);
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v2 net-next 2/7] rtnetlink: Add peer_type in struct rtnl_link_ops.

[Nikolay Aleksandrov]

On Tue, Nov 05, 2024 at 06:24:27PM -0800, Kuniyuki Iwashima wrote:
> In ops->newlink(), veth, vxcan, and netkit call rtnl_link_get_net() with
> a net pointer, which is the first argument of ->newlink().
> 
> rtnl_link_get_net() could return another netns based on IFLA_NET_NS_PID
> and IFLA_NET_NS_FD in the peer device's attributes.
> 
> We want to get it and fill rtnl_nets->nets[] in advance in rtnl_newlink()
> for per-netns RTNL.
> 
> All of the three get the peer netns in the same way:
> 
>   1. Call rtnl_nla_parse_ifinfomsg()
>   2. Call ops->validate() (vxcan doesn't have)
>   3. Call rtnl_link_get_net()
> 
> Let's add a new field peer_type to struct rtnl_link_ops and prefetch
> netns in the peer ifla to add it to rtnl_nets in rtnl_newlink().
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
> ---
> v2:
>   * Rename the helper to rtnl_link_get_net_ifla()
>   * Unexport rtnl_link_get_net_ifla() and made it static
>   * Change peer_type to u16
>   * squash patch 2 & 3 (due to static requires a user)
> ---
>  include/net/rtnetlink.h |  2 ++
>  net/core/rtnetlink.c    | 55 ++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 53 insertions(+), 4 deletions(-)
> 
 
Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>

[PATCH v2 net-next 3/7] veth: Set VETH_INFO_PEER to veth_link_ops.peer_type.

[Kuniyuki Iwashima]

For per-netns RTNL, we need to prefetch the peer device's netns.

Let's set rtnl_link_ops.peer_type and accordingly remove duplicated
validation in ->newlink().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
 drivers/net/veth.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 18148e068aa0..0d6d0d749d44 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -1781,19 +1781,11 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 	/*
 	 * create and register peer first
 	 */
-	if (data != NULL && data[VETH_INFO_PEER] != NULL) {
-		struct nlattr *nla_peer;
+	if (data && data[VETH_INFO_PEER]) {
+		struct nlattr *nla_peer = data[VETH_INFO_PEER];
 
-		nla_peer = data[VETH_INFO_PEER];
 		ifmp = nla_data(nla_peer);
-		err = rtnl_nla_parse_ifinfomsg(peer_tb, nla_peer, extack);
-		if (err < 0)
-			return err;
-
-		err = veth_validate(peer_tb, NULL, extack);
-		if (err < 0)
-			return err;
-
+		rtnl_nla_parse_ifinfomsg(peer_tb, nla_peer, extack);
 		tbp = peer_tb;
 	} else {
 		ifmp = NULL;
@@ -1809,9 +1801,6 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 	}
 
 	net = rtnl_link_get_net(src_net, tbp);
-	if (IS_ERR(net))
-		return PTR_ERR(net);
-
 	peer = rtnl_create_link(net, ifname, name_assign_type,
 				&veth_link_ops, tbp, extack);
 	if (IS_ERR(peer)) {
@@ -1952,6 +1941,7 @@ static struct rtnl_link_ops veth_link_ops = {
 	.newlink	= veth_newlink,
 	.dellink	= veth_dellink,
 	.policy		= veth_policy,
+	.peer_type	= VETH_INFO_PEER,
 	.maxtype	= VETH_INFO_MAX,
 	.get_link_net	= veth_get_link_net,
 	.get_num_tx_queues	= veth_get_num_queues,
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v2 net-next 3/7] veth: Set VETH_INFO_PEER to veth_link_ops.peer_type.

[Nikolay Aleksandrov]

On Tue, Nov 05, 2024 at 06:24:28PM -0800, Kuniyuki Iwashima wrote:
> For per-netns RTNL, we need to prefetch the peer device's netns.
> 
> Let's set rtnl_link_ops.peer_type and accordingly remove duplicated
> validation in ->newlink().
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
> ---
>  drivers/net/veth.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
> 

Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org> 

[PATCH v2 net-next 4/7] vxcan: Set VXCAN_INFO_PEER to vxcan_link_ops.peer_type.

[Kuniyuki Iwashima]

For per-netns RTNL, we need to prefetch the peer device's netns.

Let's set rtnl_link_ops.peer_type and accordingly remove duplicated
validation in ->newlink().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
Note for CAN maintainers, this patch needs to go through net-next
directly as the later patch depends on this.
---
 drivers/net/can/vxcan.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index 9e1b7d41005f..da7c72105fb6 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -188,14 +188,10 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
 
 	/* register peer device */
 	if (data && data[VXCAN_INFO_PEER]) {
-		struct nlattr *nla_peer;
+		struct nlattr *nla_peer = data[VXCAN_INFO_PEER];
 
-		nla_peer = data[VXCAN_INFO_PEER];
 		ifmp = nla_data(nla_peer);
-		err = rtnl_nla_parse_ifinfomsg(peer_tb, nla_peer, extack);
-		if (err < 0)
-			return err;
-
+		rtnl_nla_parse_ifinfomsg(peer_tb, nla_peer, extack);
 		tbp = peer_tb;
 	}
 
@@ -208,9 +204,6 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
 	}
 
 	peer_net = rtnl_link_get_net(net, tbp);
-	if (IS_ERR(peer_net))
-		return PTR_ERR(peer_net);
-
 	peer = rtnl_create_link(peer_net, ifname, name_assign_type,
 				&vxcan_link_ops, tbp, extack);
 	if (IS_ERR(peer)) {
@@ -302,6 +295,7 @@ static struct rtnl_link_ops vxcan_link_ops = {
 	.newlink	= vxcan_newlink,
 	.dellink	= vxcan_dellink,
 	.policy		= vxcan_policy,
+	.peer_type	= VXCAN_INFO_PEER,
 	.maxtype	= VXCAN_INFO_MAX,
 	.get_link_net	= vxcan_get_link_net,
 };
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v2 net-next 4/7] vxcan: Set VXCAN_INFO_PEER to vxcan_link_ops.peer_type.

[Nikolay Aleksandrov]

On Tue, Nov 05, 2024 at 06:24:29PM -0800, Kuniyuki Iwashima wrote:
> For per-netns RTNL, we need to prefetch the peer device's netns.
> 
> Let's set rtnl_link_ops.peer_type and accordingly remove duplicated
> validation in ->newlink().
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
> ---
> Note for CAN maintainers, this patch needs to go through net-next
> directly as the later patch depends on this.
> ---
>  drivers/net/can/vxcan.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
 
Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>

[PATCH v2 net-next 5/7] netkit: Set IFLA_NETKIT_PEER_INFO to netkit_link_ops.peer_type.

[Kuniyuki Iwashima]

For per-netns RTNL, we need to prefetch the peer device's netns.

Let's set rtnl_link_ops.peer_type and accordingly remove duplicated
validation in ->newlink().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
 drivers/net/netkit.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/net/netkit.c b/drivers/net/netkit.c
index cd8360b9bbde..bb07725d1c72 100644
--- a/drivers/net/netkit.c
+++ b/drivers/net/netkit.c
@@ -351,12 +351,7 @@ static int netkit_new_link(struct net *src_net, struct net_device *dev,
 		if (data[IFLA_NETKIT_PEER_INFO]) {
 			attr = data[IFLA_NETKIT_PEER_INFO];
 			ifmp = nla_data(attr);
-			err = rtnl_nla_parse_ifinfomsg(peer_tb, attr, extack);
-			if (err < 0)
-				return err;
-			err = netkit_validate(peer_tb, NULL, extack);
-			if (err < 0)
-				return err;
+			rtnl_nla_parse_ifinfomsg(peer_tb, attr, extack);
 			tbp = peer_tb;
 		}
 		if (data[IFLA_NETKIT_SCRUB])
@@ -391,9 +386,6 @@ static int netkit_new_link(struct net *src_net, struct net_device *dev,
 		return -EOPNOTSUPP;
 
 	net = rtnl_link_get_net(src_net, tbp);
-	if (IS_ERR(net))
-		return PTR_ERR(net);
-
 	peer = rtnl_create_link(net, ifname, ifname_assign_type,
 				&netkit_link_ops, tbp, extack);
 	if (IS_ERR(peer)) {
@@ -978,6 +970,7 @@ static struct rtnl_link_ops netkit_link_ops = {
 	.fill_info	= netkit_fill_info,
 	.policy		= netkit_policy,
 	.validate	= netkit_validate,
+	.peer_type	= IFLA_NETKIT_PEER_INFO,
 	.maxtype	= IFLA_NETKIT_MAX,
 };
 
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v2 net-next 5/7] netkit: Set IFLA_NETKIT_PEER_INFO to netkit_link_ops.peer_type.

[Nikolay Aleksandrov]

On Tue, Nov 05, 2024 at 06:24:30PM -0800, Kuniyuki Iwashima wrote:
> For per-netns RTNL, we need to prefetch the peer device's netns.
> 
> Let's set rtnl_link_ops.peer_type and accordingly remove duplicated
> validation in ->newlink().
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
> ---
>  drivers/net/netkit.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
 
Acked-by: Nikolay Aleksandrov <razor@blackwall.org>

[PATCH v2 net-next 6/7] rtnetlink: Convert RTM_NEWLINK to per-netns RTNL.

[Kuniyuki Iwashima]

Now, we are ready to convert rtnl_newlink() to per-netns RTNL;
rtnl_link_ops is protected by SRCU and netns is prefetched in
rtnl_newlink().

Let's register rtnl_newlink() with RTNL_FLAG_DOIT_PERNET and
push RTNL down as rtnl_nets_lock().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
v2: Remove __rtnl_unlock() dance in rtnl_newlink().
---
 net/core/rtnetlink.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d5557a621099..1b58a7c4c912 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -319,6 +319,26 @@ static void rtnl_nets_add(struct rtnl_nets *rtnl_nets, struct net *net)
 	rtnl_nets->len++;
 }
 
+static void rtnl_nets_lock(struct rtnl_nets *rtnl_nets)
+{
+	int i;
+
+	rtnl_lock();
+
+	for (i = 0; i < rtnl_nets->len; i++)
+		__rtnl_net_lock(rtnl_nets->net[i]);
+}
+
+static void rtnl_nets_unlock(struct rtnl_nets *rtnl_nets)
+{
+	int i;
+
+	for (i = 0; i < rtnl_nets->len; i++)
+		__rtnl_net_unlock(rtnl_nets->net[i]);
+
+	rtnl_unlock();
+}
+
 static struct rtnl_link __rcu *__rcu *rtnl_msg_handlers[RTNL_FAMILY_MAX + 1];
 
 static inline int rtm_msgindex(int msgtype)
@@ -3932,9 +3952,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		ops = rtnl_link_ops_get(kind, &ops_srcu_index);
 #ifdef CONFIG_MODULES
 		if (!ops) {
-			__rtnl_unlock();
 			request_module("rtnl-link-%s", kind);
-			rtnl_lock();
 			ops = rtnl_link_ops_get(kind, &ops_srcu_index);
 		}
 #endif
@@ -3997,7 +4015,9 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		}
 	}
 
+	rtnl_nets_lock(&rtnl_nets);
 	ret = __rtnl_newlink(skb, nlh, ops, tgt_net, link_net, tbs, data, extack);
+	rtnl_nets_unlock(&rtnl_nets);
 
 put_net:
 	rtnl_nets_destroy(&rtnl_nets);
@@ -7001,7 +7021,8 @@ static struct pernet_operations rtnetlink_net_ops = {
 };
 
 static const struct rtnl_msg_handler rtnetlink_rtnl_msg_handlers[] __initconst = {
-	{.msgtype = RTM_NEWLINK, .doit = rtnl_newlink},
+	{.msgtype = RTM_NEWLINK, .doit = rtnl_newlink,
+	 .flags = RTNL_FLAG_DOIT_PERNET},
 	{.msgtype = RTM_DELLINK, .doit = rtnl_dellink},
 	{.msgtype = RTM_GETLINK, .doit = rtnl_getlink,
 	 .dumpit = rtnl_dump_ifinfo, .flags = RTNL_FLAG_DUMP_SPLIT_NLM_DONE},
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v2 net-next 6/7] rtnetlink: Convert RTM_NEWLINK to per-netns RTNL.

[Paolo Abeni]

On 11/6/24 03:24, Kuniyuki Iwashima wrote:
> @@ -7001,7 +7021,8 @@ static struct pernet_operations rtnetlink_net_ops = {
>  };
>  
>  static const struct rtnl_msg_handler rtnetlink_rtnl_msg_handlers[] __initconst = {
> -	{.msgtype = RTM_NEWLINK, .doit = rtnl_newlink},
> +	{.msgtype = RTM_NEWLINK, .doit = rtnl_newlink,
> +	 .flags = RTNL_FLAG_DOIT_PERNET},
>  	{.msgtype = RTM_DELLINK, .doit = rtnl_dellink},
>  	{.msgtype = RTM_GETLINK, .doit = rtnl_getlink,
>  	 .dumpit = rtnl_dump_ifinfo, .flags = RTNL_FLAG_DUMP_SPLIT_NLM_DONE},

It looks like this still causes look problems - srcu/rtnl acquired in
both orders:

https://netdev-3.bots.linux.dev/vmksft-net-dbg/results/847881/12-rtnetlink-sh/stderr

It looks like __rtnl_link_unregister() should release the rtnl lock
around synchronize_srcu(). I'm unsure if would cause other problems, too.

Please have a self-tests run with lockdep enabled before the next iteration:

https://github.com/linux-netdev/nipa/wiki/How-to-run-netdev-selftests-CI-style

The whole test suite could be quite cumbersome, but the rtnetlink.sh
test should give a good coverage.

Thanks.

Paolo

p.s. kudos to Jakub for the extra miles to create and maintain the CI
infra: it's really catching up non trivial things.

Re: [PATCH v2 net-next 6/7] rtnetlink: Convert RTM_NEWLINK to per-netns RTNL.

[Kuniyuki Iwashima]

From: Paolo Abeni <pabeni@redhat.com>
Date: Wed, 6 Nov 2024 10:00:26 +0100
> On 11/6/24 03:24, Kuniyuki Iwashima wrote:
> > @@ -7001,7 +7021,8 @@ static struct pernet_operations rtnetlink_net_ops = {
> >  };
> >  
> >  static const struct rtnl_msg_handler rtnetlink_rtnl_msg_handlers[] __initconst = {
> > -	{.msgtype = RTM_NEWLINK, .doit = rtnl_newlink},
> > +	{.msgtype = RTM_NEWLINK, .doit = rtnl_newlink,
> > +	 .flags = RTNL_FLAG_DOIT_PERNET},
> >  	{.msgtype = RTM_DELLINK, .doit = rtnl_dellink},
> >  	{.msgtype = RTM_GETLINK, .doit = rtnl_getlink,
> >  	 .dumpit = rtnl_dump_ifinfo, .flags = RTNL_FLAG_DUMP_SPLIT_NLM_DONE},
> 
> It looks like this still causes look problems - srcu/rtnl acquired in
> both orders:
> 
> https://netdev-3.bots.linux.dev/vmksft-net-dbg/results/847881/12-rtnetlink-sh/stderr
> 
> It looks like __rtnl_link_unregister() should release the rtnl lock
> around synchronize_srcu(). I'm unsure if would cause other problems, too.

It seems I need to unexport __rtnl_link_unregister(), add mutex for
rtnl_link_ops, and move ops deletion before down_write(&pernet_ops_rwsem)
in rtnl_link_unregister().

It would be better than releasing RTNL after rtnl_lock_unregistering_all().

> 
> Please have a self-tests run with lockdep enabled before the next iteration:
> 
> https://github.com/linux-netdev/nipa/wiki/How-to-run-netdev-selftests-CI-style
> 
> The whole test suite could be quite cumbersome, but the rtnetlink.sh
> test should give a good coverage.

Sure, sorry for the churn for other patches..

I noticed rmmod wasn't tested on my QEMU because I enabled drivers
as =y so that I need not copy .ko.  I'll run test with netdevsim
as =m at least.


> 
> Thanks.
> 
> Paolo
> 
> p.s. kudos to Jakub for the extra miles to create and maintain the CI
> infra: it's really catching up non trivial things.

+100

Thank you!

Re: [PATCH v2 net-next 6/7] rtnetlink: Convert RTM_NEWLINK to per-netns RTNL.

[Nikolay Aleksandrov]

On Tue, Nov 05, 2024 at 06:24:31PM -0800, Kuniyuki Iwashima wrote:
> Now, we are ready to convert rtnl_newlink() to per-netns RTNL;
> rtnl_link_ops is protected by SRCU and netns is prefetched in
> rtnl_newlink().
> 
> Let's register rtnl_newlink() with RTNL_FLAG_DOIT_PERNET and
> push RTNL down as rtnl_nets_lock().
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
> ---
> v2: Remove __rtnl_unlock() dance in rtnl_newlink().
> ---
>  net/core/rtnetlink.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
 
Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>

[PATCH v2 net-next 7/7] rtnetlink: Register rtnl_dellink() and rtnl_setlink() with RTNL_FLAG_DOIT_PERNET_WIP.

[Kuniyuki Iwashima]

Currently, rtnl_setlink() and rtnl_dellink() cannot be fully converted
to per-netns RTNL due to a lack of handling peer/lower/upper devices in
different netns.

For example, when we change a device in rtnl_setlink() and need to
propagate that to its upper devices, we want to avoid acquiring all netns
locks, for which we do not know the upper limit.

The same situation happens when we remove a device.

rtnl_dellink() could be transformed to remove a single device in the
requested netns and delegate other devices to per-netns work, and
rtnl_setlink() might be ?

Until we come up with a better idea, let's use a new flag
RTNL_FLAG_DOIT_PERNET_WIP for rtnl_dellink() and rtnl_setlink().

This will unblock converting RTNL users where such devices are not related.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/rtnetlink.h |  1 +
 net/core/rtnetlink.c    | 19 ++++++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index f17208323c08..64b0c18651b5 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -13,6 +13,7 @@ typedef int (*rtnl_dumpit_func)(struct sk_buff *, struct netlink_callback *);
 enum rtnl_link_flags {
 	RTNL_FLAG_DOIT_UNLOCKED		= BIT(0),
 #define RTNL_FLAG_DOIT_PERNET		RTNL_FLAG_DOIT_UNLOCKED
+#define RTNL_FLAG_DOIT_PERNET_WIP	RTNL_FLAG_DOIT_UNLOCKED
 	RTNL_FLAG_BULK_DEL_SUPPORTED	= BIT(1),
 	RTNL_FLAG_DUMP_UNLOCKED		= BIT(2),
 	RTNL_FLAG_DUMP_SPLIT_NLM_DONE	= BIT(3),	/* legacy behavior */
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 1b58a7c4c912..28af19cc36a8 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3408,6 +3408,7 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct net *net = sock_net(skb->sk);
 	struct nlattr *tb[IFLA_MAX+1];
 	struct net_device *dev = NULL;
+	struct rtnl_nets rtnl_nets;
 	struct net *tgt_net;
 	int err;
 
@@ -3426,6 +3427,12 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto errout;
 	}
 
+	rtnl_nets_init(&rtnl_nets);
+	rtnl_nets_add(&rtnl_nets, get_net(net));
+	rtnl_nets_add(&rtnl_nets, tgt_net);
+
+	rtnl_nets_lock(&rtnl_nets);
+
 	if (ifm->ifi_index > 0)
 		dev = __dev_get_by_index(net, ifm->ifi_index);
 	else if (tb[IFLA_IFNAME] || tb[IFLA_ALT_IFNAME])
@@ -3438,7 +3445,7 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	else if (!err)
 		err = -ENODEV;
 
-	put_net(tgt_net);
+	rtnl_nets_unlock(&rtnl_nets);
 errout:
 	return err;
 }
@@ -3523,6 +3530,8 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh,
 			return PTR_ERR(tgt_net);
 	}
 
+	rtnl_net_lock(tgt_net);
+
 	if (ifm->ifi_index > 0)
 		dev = __dev_get_by_index(tgt_net, ifm->ifi_index);
 	else if (tb[IFLA_IFNAME] || tb[IFLA_ALT_IFNAME])
@@ -3537,6 +3546,8 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	else
 		err = -EINVAL;
 
+	rtnl_net_unlock(tgt_net);
+
 	if (netnsid >= 0)
 		put_net(tgt_net);
 
@@ -7023,10 +7034,12 @@ static struct pernet_operations rtnetlink_net_ops = {
 static const struct rtnl_msg_handler rtnetlink_rtnl_msg_handlers[] __initconst = {
 	{.msgtype = RTM_NEWLINK, .doit = rtnl_newlink,
 	 .flags = RTNL_FLAG_DOIT_PERNET},
-	{.msgtype = RTM_DELLINK, .doit = rtnl_dellink},
+	{.msgtype = RTM_DELLINK, .doit = rtnl_dellink,
+	 .flags = RTNL_FLAG_DOIT_PERNET_WIP},
 	{.msgtype = RTM_GETLINK, .doit = rtnl_getlink,
 	 .dumpit = rtnl_dump_ifinfo, .flags = RTNL_FLAG_DUMP_SPLIT_NLM_DONE},
-	{.msgtype = RTM_SETLINK, .doit = rtnl_setlink},
+	{.msgtype = RTM_SETLINK, .doit = rtnl_setlink,
+	 .flags = RTNL_FLAG_DOIT_PERNET_WIP},
 	{.msgtype = RTM_GETADDR, .dumpit = rtnl_dump_all},
 	{.msgtype = RTM_GETROUTE, .dumpit = rtnl_dump_all},
 	{.msgtype = RTM_GETNETCONF, .dumpit = rtnl_dump_all},
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v2 net-next 7/7] rtnetlink: Register rtnl_dellink() and rtnl_setlink() with RTNL_FLAG_DOIT_PERNET_WIP.

[Nikolay Aleksandrov]

On Tue, Nov 05, 2024 at 06:24:32PM -0800, Kuniyuki Iwashima wrote:
> Currently, rtnl_setlink() and rtnl_dellink() cannot be fully converted
> to per-netns RTNL due to a lack of handling peer/lower/upper devices in
> different netns.
> 
> For example, when we change a device in rtnl_setlink() and need to
> propagate that to its upper devices, we want to avoid acquiring all netns
> locks, for which we do not know the upper limit.
> 
> The same situation happens when we remove a device.
> 
> rtnl_dellink() could be transformed to remove a single device in the
> requested netns and delegate other devices to per-netns work, and
> rtnl_setlink() might be ?
> 
> Until we come up with a better idea, let's use a new flag
> RTNL_FLAG_DOIT_PERNET_WIP for rtnl_dellink() and rtnl_setlink().
> 
> This will unblock converting RTNL users where such devices are not related.
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  include/net/rtnetlink.h |  1 +
>  net/core/rtnetlink.c    | 19 ++++++++++++++++---
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 

Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>

Re: [PATCH v2 net-next 7/7] rtnetlink: Register rtnl_dellink() and rtnl_setlink() with RTNL_FLAG_DOIT_PERNET_WIP.

[Eric Dumazet]

On Wed, Nov 6, 2024 at 11:41 AM Nikolay Aleksandrov <razor@blackwall.org> wrote:
>
> On Tue, Nov 05, 2024 at 06:24:32PM -0800, Kuniyuki Iwashima wrote:
> > Currently, rtnl_setlink() and rtnl_dellink() cannot be fully converted
> > to per-netns RTNL due to a lack of handling peer/lower/upper devices in
> > different netns.
> >
> > For example, when we change a device in rtnl_setlink() and need to
> > propagate that to its upper devices, we want to avoid acquiring all netns
> > locks, for which we do not know the upper limit.
> >
> > The same situation happens when we remove a device.
> >
> > rtnl_dellink() could be transformed to remove a single device in the
> > requested netns and delegate other devices to per-netns work, and
> > rtnl_setlink() might be ?
> >
> > Until we come up with a better idea, let's use a new flag
> > RTNL_FLAG_DOIT_PERNET_WIP for rtnl_dellink() and rtnl_setlink().
> >
> > This will unblock converting RTNL users where such devices are not related.
> >
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > ---
> >  include/net/rtnetlink.h |  1 +
> >  net/core/rtnetlink.c    | 19 ++++++++++++++++---
> >  2 files changed, 17 insertions(+), 3 deletions(-)
> >
>
> Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
>

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