[PATCH v2 net-next 0/7] nexthop: Convert RTM_{NEW,DEL}NEXTHOP to per-netns RTNL.

[PATCH v2 net-next 0/7] nexthop: Convert RTM_{NEW,DEL}NEXTHOP to per-netns RTNL.

[Kuniyuki Iwashima]

Patch 1 - 5 move some validation for RTM_NEWNEXTHOP so that it can be
called without RTNL.

Patch 6 & 7 converts RTM_NEWNEXTHOP and RTM_DELNEXTHOP to per-netns RTNL.

Note that RTM_GETNEXTHOP and RTM_GETNEXTHOPBUCKET are not touched in
this series.

rtm_get_nexthop() can be easily converted to RCU, but rtm_dump_nexthop()
needs more work due to the left-to-right rbtree walk, which looks prone
to node deletion and tree rotation without a retry mechanism.


Changes:
  v2:
    * Patch 2
      * Correct err check in rtm_new_nexthop()

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


Kuniyuki Iwashima (7):
  nexthop: Move nlmsg_parse() in rtm_to_nh_config() to
    rtm_new_nexthop().
  nexthop: Split nh_check_attr_group().
  nexthop: Move NHA_OIF validation to rtm_to_nh_config_rtnl().
  nexthop: Check NLM_F_REPLACE and NHA_ID in rtm_new_nexthop().
  nexthop: Remove redundant group len check in nexthop_create_group().
  nexthop: Convert RTM_NEWNEXTHOP to per-netns RTNL.
  nexthop: Convert RTM_DELNEXTHOP to per-netns RTNL.

 net/ipv4/nexthop.c | 183 +++++++++++++++++++++++++++------------------
 1 file changed, 112 insertions(+), 71 deletions(-)

-- 
2.48.1

[PATCH v2 net-next 1/7] nexthop: Move nlmsg_parse() in rtm_to_nh_config() to rtm_new_nexthop().

[Kuniyuki Iwashima]

We will split rtm_to_nh_config() into non-RTNL and RTNL parts,
and then the latter also needs tb.

As a prep, let's move nlmsg_parse() to rtm_new_nexthop().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/ipv4/nexthop.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 01df7dd795f0..487933ecdb68 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -3016,19 +3016,13 @@ static int rtm_to_nh_config_grp_res(struct nlattr *res, struct nh_config *cfg,
 }
 
 static int rtm_to_nh_config(struct net *net, struct sk_buff *skb,
-			    struct nlmsghdr *nlh, struct nh_config *cfg,
+			    struct nlmsghdr *nlh, struct nlattr **tb,
+			    struct nh_config *cfg,
 			    struct netlink_ext_ack *extack)
 {
 	struct nhmsg *nhm = nlmsg_data(nlh);
-	struct nlattr *tb[ARRAY_SIZE(rtm_nh_policy_new)];
 	int err;
 
-	err = nlmsg_parse(nlh, sizeof(*nhm), tb,
-			  ARRAY_SIZE(rtm_nh_policy_new) - 1,
-			  rtm_nh_policy_new, extack);
-	if (err < 0)
-		return err;
-
 	err = -EINVAL;
 	if (nhm->resvd || nhm->nh_scope) {
 		NL_SET_ERR_MSG(extack, "Invalid values in ancillary header");
@@ -3093,7 +3087,8 @@ static int rtm_to_nh_config(struct net *net, struct sk_buff *skb,
 			NL_SET_ERR_MSG(extack, "Invalid group type");
 			goto out;
 		}
-		err = nh_check_attr_group(net, tb, ARRAY_SIZE(tb),
+
+		err = nh_check_attr_group(net, tb, ARRAY_SIZE(rtm_nh_policy_new),
 					  cfg->nh_grp_type, extack);
 		if (err)
 			goto out;
@@ -3211,18 +3206,26 @@ static int rtm_to_nh_config(struct net *net, struct sk_buff *skb,
 static int rtm_new_nexthop(struct sk_buff *skb, struct nlmsghdr *nlh,
 			   struct netlink_ext_ack *extack)
 {
+	struct nlattr *tb[ARRAY_SIZE(rtm_nh_policy_new)];
 	struct net *net = sock_net(skb->sk);
 	struct nh_config cfg;
 	struct nexthop *nh;
 	int err;
 
-	err = rtm_to_nh_config(net, skb, nlh, &cfg, extack);
-	if (!err) {
-		nh = nexthop_add(net, &cfg, extack);
-		if (IS_ERR(nh))
-			err = PTR_ERR(nh);
-	}
+	err = nlmsg_parse(nlh, sizeof(struct nhmsg), tb,
+			  ARRAY_SIZE(rtm_nh_policy_new) - 1,
+			  rtm_nh_policy_new, extack);
+	if (err < 0)
+		goto out;
 
+	err = rtm_to_nh_config(net, skb, nlh, tb, &cfg, extack);
+	if (err)
+		goto out;
+
+	nh = nexthop_add(net, &cfg, extack);
+	if (IS_ERR(nh))
+		err = PTR_ERR(nh);
+out:
 	return err;
 }
 
-- 
2.48.1

[PATCH v2 net-next 2/7] nexthop: Split nh_check_attr_group().

[Kuniyuki Iwashima]

We will push RTNL down to rtm_new_nexthop(), and then we
want to move non-RTNL operations out of the scope.

nh_check_attr_group() validates NHA_GROUP attributes, and
nexthop_find_by_id() and some validation requires RTNL.

Let's factorise such parts as nh_check_attr_group_rtnl()
and call it from rtm_to_nh_config_rtnl().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/ipv4/nexthop.c | 68 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 47 insertions(+), 21 deletions(-)

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 487933ecdb68..d30edc14b039 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -1272,10 +1272,8 @@ static int nh_check_attr_group(struct net *net,
 			       u16 nh_grp_type, struct netlink_ext_ack *extack)
 {
 	unsigned int len = nla_len(tb[NHA_GROUP]);
-	u8 nh_family = AF_UNSPEC;
 	struct nexthop_grp *nhg;
 	unsigned int i, j;
-	u8 nhg_fdb = 0;
 
 	if (!len || len & (sizeof(struct nexthop_grp) - 1)) {
 		NL_SET_ERR_MSG(extack,
@@ -1307,10 +1305,41 @@ static int nh_check_attr_group(struct net *net,
 		}
 	}
 
-	if (tb[NHA_FDB])
-		nhg_fdb = 1;
 	nhg = nla_data(tb[NHA_GROUP]);
-	for (i = 0; i < len; ++i) {
+	for (i = NHA_GROUP_TYPE + 1; i < tb_size; ++i) {
+		if (!tb[i])
+			continue;
+		switch (i) {
+		case NHA_HW_STATS_ENABLE:
+		case NHA_FDB:
+			continue;
+		case NHA_RES_GROUP:
+			if (nh_grp_type == NEXTHOP_GRP_TYPE_RES)
+				continue;
+			break;
+		}
+		NL_SET_ERR_MSG(extack,
+			       "No other attributes can be set in nexthop groups");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int nh_check_attr_group_rtnl(struct net *net, struct nlattr *tb[],
+				    struct netlink_ext_ack *extack)
+{
+	u8 nh_family = AF_UNSPEC;
+	struct nexthop_grp *nhg;
+	unsigned int len;
+	unsigned int i;
+	u8 nhg_fdb;
+
+	len = nla_len(tb[NHA_GROUP]) / sizeof(*nhg);
+	nhg = nla_data(tb[NHA_GROUP]);
+	nhg_fdb = !!tb[NHA_FDB];
+
+	for (i = 0; i < len; i++) {
 		struct nexthop *nh;
 		bool is_fdb_nh;
 
@@ -1330,22 +1359,6 @@ static int nh_check_attr_group(struct net *net,
 			return -EINVAL;
 		}
 	}
-	for (i = NHA_GROUP_TYPE + 1; i < tb_size; ++i) {
-		if (!tb[i])
-			continue;
-		switch (i) {
-		case NHA_HW_STATS_ENABLE:
-		case NHA_FDB:
-			continue;
-		case NHA_RES_GROUP:
-			if (nh_grp_type == NEXTHOP_GRP_TYPE_RES)
-				continue;
-			break;
-		}
-		NL_SET_ERR_MSG(extack,
-			       "No other attributes can be set in nexthop groups");
-		return -EINVAL;
-	}
 
 	return 0;
 }
@@ -3202,6 +3215,15 @@ static int rtm_to_nh_config(struct net *net, struct sk_buff *skb,
 	return err;
 }
 
+static int rtm_to_nh_config_rtnl(struct net *net, struct nlattr **tb,
+				 struct netlink_ext_ack *extack)
+{
+	if (tb[NHA_GROUP])
+		return nh_check_attr_group_rtnl(net, tb, extack);
+
+	return 0;
+}
+
 /* rtnl */
 static int rtm_new_nexthop(struct sk_buff *skb, struct nlmsghdr *nlh,
 			   struct netlink_ext_ack *extack)
@@ -3222,6 +3244,10 @@ static int rtm_new_nexthop(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err)
 		goto out;
 
+	err = rtm_to_nh_config_rtnl(net, tb, extack);
+	if (err)
+		goto out;
+
 	nh = nexthop_add(net, &cfg, extack);
 	if (IS_ERR(nh))
 		err = PTR_ERR(nh);
-- 
2.48.1

[PATCH v2 net-next 3/7] nexthop: Move NHA_OIF validation to rtm_to_nh_config_rtnl().

[Kuniyuki Iwashima]

NHA_OIF needs to look up a device by __dev_get_by_index(),
which requires RTNL.

Let's move NHA_OIF validation to rtm_to_nh_config_rtnl().

Note that the proceeding checks made the original !cfg->nh_fdb
check redundant.

  NHA_FDB is set           -> NHA_OIF cannot be set
  NHA_FDB is set but false -> NHA_OIF must be set
  NHA_FDB is not set       -> NHA_OIF must be set

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/ipv4/nexthop.c | 43 +++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index d30edc14b039..426cdf301c6f 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -3134,25 +3134,6 @@ static int rtm_to_nh_config(struct net *net, struct sk_buff *skb,
 		goto out;
 	}
 
-	if (!cfg->nh_fdb && tb[NHA_OIF]) {
-		cfg->nh_ifindex = nla_get_u32(tb[NHA_OIF]);
-		if (cfg->nh_ifindex)
-			cfg->dev = __dev_get_by_index(net, cfg->nh_ifindex);
-
-		if (!cfg->dev) {
-			NL_SET_ERR_MSG(extack, "Invalid device index");
-			goto out;
-		} else if (!(cfg->dev->flags & IFF_UP)) {
-			NL_SET_ERR_MSG(extack, "Nexthop device is not up");
-			err = -ENETDOWN;
-			goto out;
-		} else if (!netif_carrier_ok(cfg->dev)) {
-			NL_SET_ERR_MSG(extack, "Carrier for nexthop device is down");
-			err = -ENETDOWN;
-			goto out;
-		}
-	}
-
 	err = -EINVAL;
 	if (tb[NHA_GATEWAY]) {
 		struct nlattr *gwa = tb[NHA_GATEWAY];
@@ -3216,11 +3197,33 @@ static int rtm_to_nh_config(struct net *net, struct sk_buff *skb,
 }
 
 static int rtm_to_nh_config_rtnl(struct net *net, struct nlattr **tb,
+				 struct nh_config *cfg,
 				 struct netlink_ext_ack *extack)
 {
 	if (tb[NHA_GROUP])
 		return nh_check_attr_group_rtnl(net, tb, extack);
 
+	if (tb[NHA_OIF]) {
+		cfg->nh_ifindex = nla_get_u32(tb[NHA_OIF]);
+		if (cfg->nh_ifindex)
+			cfg->dev = __dev_get_by_index(net, cfg->nh_ifindex);
+
+		if (!cfg->dev) {
+			NL_SET_ERR_MSG(extack, "Invalid device index");
+			return -EINVAL;
+		}
+
+		if (!(cfg->dev->flags & IFF_UP)) {
+			NL_SET_ERR_MSG(extack, "Nexthop device is not up");
+			return -ENETDOWN;
+		}
+
+		if (!netif_carrier_ok(cfg->dev)) {
+			NL_SET_ERR_MSG(extack, "Carrier for nexthop device is down");
+			return -ENETDOWN;
+		}
+	}
+
 	return 0;
 }
 
@@ -3244,7 +3247,7 @@ static int rtm_new_nexthop(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err)
 		goto out;
 
-	err = rtm_to_nh_config_rtnl(net, tb, extack);
+	err = rtm_to_nh_config_rtnl(net, tb, &cfg, extack);
 	if (err)
 		goto out;
 
-- 
2.48.1

[PATCH v2 net-next 4/7] nexthop: Check NLM_F_REPLACE and NHA_ID in rtm_new_nexthop().

[Kuniyuki Iwashima]

nexthop_add() checks if NLM_F_REPLACE is specified without
non-zero NHA_ID, which does not require RTNL.

Let's move the check to rtm_new_nexthop().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/ipv4/nexthop.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 426cdf301c6f..fb129c830040 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -2928,11 +2928,6 @@ static struct nexthop *nexthop_add(struct net *net, struct nh_config *cfg,
 	struct nexthop *nh;
 	int err;
 
-	if (cfg->nlflags & NLM_F_REPLACE && !cfg->nh_id) {
-		NL_SET_ERR_MSG(extack, "Replace requires nexthop id");
-		return ERR_PTR(-EINVAL);
-	}
-
 	if (!cfg->nh_id) {
 		cfg->nh_id = nh_find_unused_id(net);
 		if (!cfg->nh_id) {
@@ -3247,6 +3242,12 @@ static int rtm_new_nexthop(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err)
 		goto out;
 
+	if (cfg.nlflags & NLM_F_REPLACE && !cfg.nh_id) {
+		NL_SET_ERR_MSG(extack, "Replace requires nexthop id");
+		err = -EINVAL;
+		goto out;
+	}
+
 	err = rtm_to_nh_config_rtnl(net, tb, &cfg, extack);
 	if (err)
 		goto out;
-- 
2.48.1

[PATCH v2 net-next 5/7] nexthop: Remove redundant group len check in nexthop_create_group().

[Kuniyuki Iwashima]

The number of NHA_GROUP entries is guaranteed to be non-zero in
nh_check_attr_group().

Let's remove the redundant check in nexthop_create_group().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/ipv4/nexthop.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index fb129c830040..c552bb46aa23 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -2692,9 +2692,6 @@ static struct nexthop *nexthop_create_group(struct net *net,
 	int err;
 	int i;
 
-	if (WARN_ON(!num_nh))
-		return ERR_PTR(-EINVAL);
-
 	nh = nexthop_alloc();
 	if (!nh)
 		return ERR_PTR(-ENOMEM);
-- 
2.48.1

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

[Kuniyuki Iwashima]

If we pass false to the rtnl_held param of lwtunnel_valid_encap_type(),
we can move RTNL down before rtm_to_nh_config_rtnl().

Let's use rtnl_net_lock() in rtm_new_nexthop().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/ipv4/nexthop.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index c552bb46aa23..06d5467eadc1 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -3169,7 +3169,7 @@ static int rtm_to_nh_config(struct net *net, struct sk_buff *skb,
 
 		cfg->nh_encap_type = nla_get_u16(tb[NHA_ENCAP_TYPE]);
 		err = lwtunnel_valid_encap_type(cfg->nh_encap_type,
-						extack, true);
+						extack, false);
 		if (err < 0)
 			goto out;
 
@@ -3245,13 +3245,18 @@ static int rtm_new_nexthop(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto out;
 	}
 
+	rtnl_net_lock(net);
+
 	err = rtm_to_nh_config_rtnl(net, tb, &cfg, extack);
 	if (err)
-		goto out;
+		goto unlock;
 
 	nh = nexthop_add(net, &cfg, extack);
 	if (IS_ERR(nh))
 		err = PTR_ERR(nh);
+
+unlock:
+	rtnl_net_unlock(net);
 out:
 	return err;
 }
@@ -4067,18 +4072,19 @@ static struct pernet_operations nexthop_net_ops = {
 };
 
 static const struct rtnl_msg_handler nexthop_rtnl_msg_handlers[] __initconst = {
-	{.msgtype = RTM_NEWNEXTHOP, .doit = rtm_new_nexthop},
+	{.msgtype = RTM_NEWNEXTHOP, .doit = rtm_new_nexthop,
+	 .flags = RTNL_FLAG_DOIT_PERNET},
 	{.msgtype = RTM_DELNEXTHOP, .doit = rtm_del_nexthop},
 	{.msgtype = RTM_GETNEXTHOP, .doit = rtm_get_nexthop,
 	 .dumpit = rtm_dump_nexthop},
 	{.msgtype = RTM_GETNEXTHOPBUCKET, .doit = rtm_get_nexthop_bucket,
 	 .dumpit = rtm_dump_nexthop_bucket},
 	{.protocol = PF_INET, .msgtype = RTM_NEWNEXTHOP,
-	 .doit = rtm_new_nexthop},
+	 .doit = rtm_new_nexthop, .flags = RTNL_FLAG_DOIT_PERNET},
 	{.protocol = PF_INET, .msgtype = RTM_GETNEXTHOP,
 	 .dumpit = rtm_dump_nexthop},
 	{.protocol = PF_INET6, .msgtype = RTM_NEWNEXTHOP,
-	 .doit = rtm_new_nexthop},
+	 .doit = rtm_new_nexthop, .flags = RTNL_FLAG_DOIT_PERNET},
 	{.protocol = PF_INET6, .msgtype = RTM_GETNEXTHOP,
 	 .dumpit = rtm_dump_nexthop},
 };
-- 
2.48.1

[PATCH v2 net-next 7/7] nexthop: Convert RTM_DELNEXTHOP to per-netns RTNL.

[Kuniyuki Iwashima]

In rtm_del_nexthop(), only nexthop_find_by_id() and remove_nexthop()
require RTNL as they touch net->nexthop.rb_root.

Let's move RTNL down as rtnl_net_lock() before nexthop_find_by_id().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/ipv4/nexthop.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 06d5467eadc1..467151517023 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -3314,13 +3314,17 @@ static int rtm_del_nexthop(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err)
 		return err;
 
+	rtnl_net_lock(net);
+
 	nh = nexthop_find_by_id(net, id);
-	if (!nh)
-		return -ENOENT;
+	if (nh)
+		remove_nexthop(net, nh, &nlinfo);
+	else
+		err = -ENOENT;
 
-	remove_nexthop(net, nh, &nlinfo);
+	rtnl_net_unlock(net);
 
-	return 0;
+	return err;
 }
 
 /* rtnl */
@@ -4074,7 +4078,8 @@ static struct pernet_operations nexthop_net_ops = {
 static const struct rtnl_msg_handler nexthop_rtnl_msg_handlers[] __initconst = {
 	{.msgtype = RTM_NEWNEXTHOP, .doit = rtm_new_nexthop,
 	 .flags = RTNL_FLAG_DOIT_PERNET},
-	{.msgtype = RTM_DELNEXTHOP, .doit = rtm_del_nexthop},
+	{.msgtype = RTM_DELNEXTHOP, .doit = rtm_del_nexthop,
+	 .flags = RTNL_FLAG_DOIT_PERNET},
 	{.msgtype = RTM_GETNEXTHOP, .doit = rtm_get_nexthop,
 	 .dumpit = rtm_dump_nexthop},
 	{.msgtype = RTM_GETNEXTHOPBUCKET, .doit = rtm_get_nexthop_bucket,
-- 
2.48.1

Re: [PATCH v2 net-next 1/7] nexthop: Move nlmsg_parse() in rtm_to_nh_config() to rtm_new_nexthop().

[David Ahern]

On 3/19/25 5:06 PM, Kuniyuki Iwashima wrote:
> We will split rtm_to_nh_config() into non-RTNL and RTNL parts,
> and then the latter also needs tb.
> 
> As a prep, let's move nlmsg_parse() to rtm_new_nexthop().
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  net/ipv4/nexthop.c | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH v2 net-next 1/7] nexthop: Move nlmsg_parse() in rtm_to_nh_config() to rtm_new_nexthop().

[Eric Dumazet]

On Thu, Mar 20, 2025 at 12:08 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> We will split rtm_to_nh_config() into non-RTNL and RTNL parts,
> and then the latter also needs tb.
>
> As a prep, let's move nlmsg_parse() to rtm_new_nexthop().
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

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

Re: [PATCH v2 net-next 2/7] nexthop: Split nh_check_attr_group().

[Eric Dumazet]

On Thu, Mar 20, 2025 at 12:08 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> We will push RTNL down to rtm_new_nexthop(), and then we
> want to move non-RTNL operations out of the scope.
>
> nh_check_attr_group() validates NHA_GROUP attributes, and
> nexthop_find_by_id() and some validation requires RTNL.
>
> Let's factorise such parts as nh_check_attr_group_rtnl()
> and call it from rtm_to_nh_config_rtnl().
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

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

Re: [PATCH v2 net-next 3/7] nexthop: Move NHA_OIF validation to rtm_to_nh_config_rtnl().

[Eric Dumazet]

On Thu, Mar 20, 2025 at 12:09 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> NHA_OIF needs to look up a device by __dev_get_by_index(),
> which requires RTNL.
>
> Let's move NHA_OIF validation to rtm_to_nh_config_rtnl().
>
> Note that the proceeding checks made the original !cfg->nh_fdb
> check redundant.
>
>   NHA_FDB is set           -> NHA_OIF cannot be set
>   NHA_FDB is set but false -> NHA_OIF must be set
>   NHA_FDB is not set       -> NHA_OIF must be set
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

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

Re: [PATCH v2 net-next 4/7] nexthop: Check NLM_F_REPLACE and NHA_ID in rtm_new_nexthop().

[Eric Dumazet]

On Thu, Mar 20, 2025 at 12:09 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> nexthop_add() checks if NLM_F_REPLACE is specified without
> non-zero NHA_ID, which does not require RTNL.
>
> Let's move the check to rtm_new_nexthop().
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

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

Re: [PATCH v2 net-next 5/7] nexthop: Remove redundant group len check in nexthop_create_group().

[Eric Dumazet]

On Thu, Mar 20, 2025 at 12:10 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> The number of NHA_GROUP entries is guaranteed to be non-zero in
> nh_check_attr_group().
>
> Let's remove the redundant check in nexthop_create_group().
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

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

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

[Eric Dumazet]

On Thu, Mar 20, 2025 at 12:10 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> If we pass false to the rtnl_held param of lwtunnel_valid_encap_type(),
> we can move RTNL down before rtm_to_nh_config_rtnl().
>
> Let's use rtnl_net_lock() in rtm_new_nexthop().
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

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

Re: [PATCH v2 net-next 7/7] nexthop: Convert RTM_DELNEXTHOP to per-netns RTNL.

[Eric Dumazet]

On Thu, Mar 20, 2025 at 12:11 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> In rtm_del_nexthop(), only nexthop_find_by_id() and remove_nexthop()
> require RTNL as they touch net->nexthop.rb_root.
>
> Let's move RTNL down as rtnl_net_lock() before nexthop_find_by_id().
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>

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

Re: [PATCH v2 net-next 2/7] nexthop: Split nh_check_attr_group().

[David Ahern]

On 3/19/25 5:06 PM, Kuniyuki Iwashima wrote:
> We will push RTNL down to rtm_new_nexthop(), and then we
> want to move non-RTNL operations out of the scope.
> 
> nh_check_attr_group() validates NHA_GROUP attributes, and
> nexthop_find_by_id() and some validation requires RTNL.
> 
> Let's factorise such parts as nh_check_attr_group_rtnl()
> and call it from rtm_to_nh_config_rtnl().
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  net/ipv4/nexthop.c | 68 ++++++++++++++++++++++++++++++++--------------
>  1 file changed, 47 insertions(+), 21 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH v2 net-next 3/7] nexthop: Move NHA_OIF validation to rtm_to_nh_config_rtnl().

[David Ahern]

On 3/19/25 5:06 PM, Kuniyuki Iwashima wrote:
> NHA_OIF needs to look up a device by __dev_get_by_index(),
> which requires RTNL.
> 
> Let's move NHA_OIF validation to rtm_to_nh_config_rtnl().
> 
> Note that the proceeding checks made the original !cfg->nh_fdb
> check redundant.
> 
>   NHA_FDB is set           -> NHA_OIF cannot be set
>   NHA_FDB is set but false -> NHA_OIF must be set
>   NHA_FDB is not set       -> NHA_OIF must be set
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  net/ipv4/nexthop.c | 43 +++++++++++++++++++++++--------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH v2 net-next 4/7] nexthop: Check NLM_F_REPLACE and NHA_ID in rtm_new_nexthop().

[David Ahern]

On 3/19/25 5:06 PM, Kuniyuki Iwashima wrote:
> nexthop_add() checks if NLM_F_REPLACE is specified without
> non-zero NHA_ID, which does not require RTNL.
> 
> Let's move the check to rtm_new_nexthop().
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  net/ipv4/nexthop.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH v2 net-next 5/7] nexthop: Remove redundant group len check in nexthop_create_group().

[David Ahern]

On 3/19/25 5:06 PM, Kuniyuki Iwashima wrote:
> The number of NHA_GROUP entries is guaranteed to be non-zero in
> nh_check_attr_group().
> 
> Let's remove the redundant check in nexthop_create_group().
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  net/ipv4/nexthop.c | 3 ---
>  1 file changed, 3 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

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

[David Ahern]

On 3/19/25 5:06 PM, Kuniyuki Iwashima wrote:
> If we pass false to the rtnl_held param of lwtunnel_valid_encap_type(),
> we can move RTNL down before rtm_to_nh_config_rtnl().
> 
> Let's use rtnl_net_lock() in rtm_new_nexthop().
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  net/ipv4/nexthop.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH v2 net-next 7/7] nexthop: Convert RTM_DELNEXTHOP to per-netns RTNL.

[David Ahern]

On 3/19/25 5:06 PM, Kuniyuki Iwashima wrote:
> In rtm_del_nexthop(), only nexthop_find_by_id() and remove_nexthop()
> require RTNL as they touch net->nexthop.rb_root.
> 
> Let's move RTNL down as rtnl_net_lock() before nexthop_find_by_id().
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  net/ipv4/nexthop.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH v2 net-next 0/7] nexthop: Convert RTM_{NEW,DEL}NEXTHOP to per-netns RTNL.

[Ido Schimmel]

On Wed, Mar 19, 2025 at 04:06:45PM -0700, Kuniyuki Iwashima wrote:
> Patch 1 - 5 move some validation for RTM_NEWNEXTHOP so that it can be
> called without RTNL.
> 
> Patch 6 & 7 converts RTM_NEWNEXTHOP and RTM_DELNEXTHOP to per-netns RTNL.
> 
> Note that RTM_GETNEXTHOP and RTM_GETNEXTHOPBUCKET are not touched in
> this series.
> 
> rtm_get_nexthop() can be easily converted to RCU, but rtm_dump_nexthop()
> needs more work due to the left-to-right rbtree walk, which looks prone
> to node deletion and tree rotation without a retry mechanism.

Thanks for the series, looks good, but note that dump/get can block when
fetching hardware statistics:

rtm_get_nexthop
    nh_fill_node
        nla_put_nh_group
	    nla_put_nh_group_stats
	        nh_grp_hw_stats_update
		    blocking_notifier_call_chain

Re: [PATCH v2 net-next 0/7] nexthop: Convert RTM_{NEW,DEL}NEXTHOP to per-netns RTNL.

[Kuniyuki Iwashima]

From: Ido Schimmel <idosch@idosch.org>
Date: Mon, 24 Mar 2025 11:35:04 +0200
> On Wed, Mar 19, 2025 at 04:06:45PM -0700, Kuniyuki Iwashima wrote:
> > Patch 1 - 5 move some validation for RTM_NEWNEXTHOP so that it can be
> > called without RTNL.
> > 
> > Patch 6 & 7 converts RTM_NEWNEXTHOP and RTM_DELNEXTHOP to per-netns RTNL.
> > 
> > Note that RTM_GETNEXTHOP and RTM_GETNEXTHOPBUCKET are not touched in
> > this series.
> > 
> > rtm_get_nexthop() can be easily converted to RCU, but rtm_dump_nexthop()
> > needs more work due to the left-to-right rbtree walk, which looks prone
> > to node deletion and tree rotation without a retry mechanism.
> 
> Thanks for the series, looks good, but note that dump/get can block when
> fetching hardware statistics:

Oh thanks!

I was puzzled by the left-right rbtree iteration under RCU and wondering
if I should try harder, but converting the notifier to non-blocking one
is not worth that, and I can simply use per-netns RTNL :)


> 
> rtm_get_nexthop
>     nh_fill_node
>         nla_put_nh_group
> 	    nla_put_nh_group_stats
> 	        nh_grp_hw_stats_update
> 		    blocking_notifier_call_chain

Re: [PATCH v2 net-next 0/7] nexthop: Convert RTM_{NEW,DEL}NEXTHOP 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, 19 Mar 2025 16:06:45 -0700 you wrote:
> Patch 1 - 5 move some validation for RTM_NEWNEXTHOP so that it can be
> called without RTNL.
> 
> Patch 6 & 7 converts RTM_NEWNEXTHOP and RTM_DELNEXTHOP to per-netns RTNL.
> 
> Note that RTM_GETNEXTHOP and RTM_GETNEXTHOPBUCKET are not touched in
> this series.
> 
> [...]

Here is the summary with links:
  - [v2,net-next,1/7] nexthop: Move nlmsg_parse() in rtm_to_nh_config() to rtm_new_nexthop().
    https://git.kernel.org/netdev/net-next/c/ec8de7544778
  - [v2,net-next,2/7] nexthop: Split nh_check_attr_group().
    https://git.kernel.org/netdev/net-next/c/9b9674f3e73a
  - [v2,net-next,3/7] nexthop: Move NHA_OIF validation to rtm_to_nh_config_rtnl().
    https://git.kernel.org/netdev/net-next/c/caa074573ca0
  - [v2,net-next,4/7] nexthop: Check NLM_F_REPLACE and NHA_ID in rtm_new_nexthop().
    https://git.kernel.org/netdev/net-next/c/53b18aa998b7
  - [v2,net-next,5/7] nexthop: Remove redundant group len check in nexthop_create_group().
    https://git.kernel.org/netdev/net-next/c/b6af3890574a
  - [v2,net-next,6/7] nexthop: Convert RTM_NEWNEXTHOP to per-netns RTNL.
    https://git.kernel.org/netdev/net-next/c/f5fabaff86cb
  - [v2,net-next,7/7] nexthop: Convert RTM_DELNEXTHOP to per-netns RTNL.
    https://git.kernel.org/netdev/net-next/c/29c8e323320f

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