Re: [PATCH v1 net-next 10/13] ipv6: Factorise ip6_route_multipath_add().

Re: [PATCH v1 net-next 10/13] ipv6: Factorise ip6_route_multipath_add().

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20250321040131.21057-11-kuniyu@amazon.com>
References: <20250321040131.21057-11-kuniyu@amazon.com>
TO: Kuniyuki Iwashima <kuniyu@amazon.com>
TO: "David S. Miller" <davem@davemloft.net>
CC: netdev@vger.kernel.org
TO: David Ahern <dsahern@kernel.org>
TO: Eric Dumazet <edumazet@google.com>
TO: Jakub Kicinski <kuba@kernel.org>
TO: Paolo Abeni <pabeni@redhat.com>
CC: Simon Horman <horms@kernel.org>
CC: Kuniyuki Iwashima <kuniyu@amazon.com>

Hi Kuniyuki,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Kuniyuki-Iwashima/ipv6-Validate-RTA_GATEWAY-of-RTA_MULTIPATH-in-rtm_to_fib6_config/20250321-120836
base:   net-next/main
patch link:    https://lore.kernel.org/r/20250321040131.21057-11-kuniyu%40amazon.com
patch subject: [PATCH v1 net-next 10/13] ipv6: Factorise ip6_route_multipath_add().
:::::: branch date: 7 days ago
:::::: commit date: 7 days ago
config: i386-randconfig-141-20250328 (https://download.01.org/0day-ci/archive/20250328/202503282150.4vc017y0-lkp@intel.com/config)
compiler: clang version 20.1.1 (https://github.com/llvm/llvm-project 424c2d9b7e4de40d0804dd374721e6411c27d1d1)

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>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202503282150.4vc017y0-lkp@intel.com/

New smatch warnings:
net/ipv6/route.c:5405 ip6_route_mpath_info_create_nh() error: uninitialized symbol 'err'.

Old smatch warnings:
net/ipv6/route.c:3427 ip6_route_check_nh() error: we previously assumed '_dev' could be null (see line 3387)
net/ipv6/route.c:5873 rt6_fill_node() error: we previously assumed 'dst' could be null (see line 5858)

vim +/err +5405 net/ipv6/route.c

f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5374  
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5375  static int ip6_route_mpath_info_create_nh(struct list_head *rt6_nh_list,
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5376  					  struct netlink_ext_ack *extack)
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5377  {
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5378  	struct rt6_nh *nh, *nh_next, *nh_tmp;
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5379  	LIST_HEAD(tmp);
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5380  	int err;
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5381  
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5382  	list_for_each_entry_safe(nh, nh_next, rt6_nh_list, next) {
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5383  		struct fib6_info *rt = nh->fib6_info;
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5384  
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5385  		err = ip6_route_info_create_nh(rt, &nh->r_cfg, extack);
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5386  		if (err) {
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5387  			nh->fib6_info = NULL;
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5388  			goto err;
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5389  		}
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5390  
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5391  		rt->fib6_nh->fib_nh_weight = nh->weight;
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5392  
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5393  		list_move_tail(&nh->next, &tmp);
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5394  
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5395  		list_for_each_entry(nh_tmp, rt6_nh_list, next) {
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5396  			/* check if fib6_info already exists */
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5397  			if (rt6_duplicate_nexthop(nh_tmp->fib6_info, rt)) {
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5398  				err = -EEXIST;
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5399  				goto err;
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5400  			}
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5401  		}
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5402  	}
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5403  out:
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5404  	list_splice(&tmp, rt6_nh_list);
f7d850c5de621f Kuniyuki Iwashima 2025-03-20 @5405  	return err;
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5406  err:
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5407  	ip6_route_mpath_info_cleanup(rt6_nh_list);
f7d850c5de621f Kuniyuki Iwashima 2025-03-20  5408  	goto out;
6b9ea5a64ed5ee Roopa Prabhu      2015-09-08  5409  }
6b9ea5a64ed5ee Roopa Prabhu      2015-09-08  5410  

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

[PATCH v1 net-next 00/13] ipv6: No RTNL for IPv6 routing table.

[Kuniyuki Iwashima]

IPv6 routing tables are protected by each table's lock and work in
the interrupt context, which means we basically don't need RTNL to
modify an IPv6 routing table itself.

Currently, the control paths require RTNL because we may need to
perform device and nexthop lookups; we must prevent dev/nexthop from
going away from the netns.

This, however, can be achieved by RCU as well.

If we are in the RCU critical section while adding an IPv6 route,
synchronize_net() in netif_change_net_namespace() and
unregister_netdevice_many_notify() guarantee that the dev will not be
moved to another netns or removed.

Also, nexthop is guaranteed not to be freed during the RCU grace period.

If we care about a race between nexthop removal and IPv6 route addition,
we can get rid of RTNL from the control paths.

Patch 1 moves a validation for RTA_MULTIPATH earlier.
Patch 2 removes RTNL for SIOCDELRT and RTM_DELROUTE.
Patch 3 ~ 10 move validation and memory allocation earlier.
Patch 11 prevents a race between two requests for the same table.
Patch 12 prevents the race mentioned above.
Patch 13 removes RTNL for SIOCADDRT and RTM_NEWROUTE.


Test:

The script [0] lets each CPU-X create 100000 routes on table-X in a
batch.

On c7a.metal-48xl EC2 instance with 192 CPUs,

With this series:

  $ sudo ./route_test.sh
  start adding routes
  added 19200000 routes (100000 routes * 192 tables).
  Time elapsed: 189154 milliseconds.

Without series:

  $ sudo ./route_test.sh
  start adding routes
  added 19200000 routes (100000 routes * 192 tables).
  Time elapsed: 62531 milliseconds.

I changed the number of routes (1000 ~ 100000 per CPU/table) and
constantly saw it complete 3x faster with this series.


[0]
#!/bin/bash

mkdir tmp

NS="test"
ip netns add $NS
ip -n $NS link add veth0 type veth peer veth1
ip -n $NS link set veth0 up
ip -n $NS link set veth1 up

TABLES=()
for i in $(seq $(nproc)); do
    TABLES+=("$i")
done

ROUTES=()
for i in {1..100}; do
    for j in {1..1000}; do
	ROUTES+=("2001:$i:$j::/64")
    done
done

for TABLE in "${TABLES[@]}"; do
    FILE="./tmp/batch-table-$TABLE.txt"
    > $FILE
    for ROUTE in "${ROUTES[@]}"; do
        echo "route add $ROUTE dev veth0 table $TABLE" >> $FILE
    done
done

echo "start adding routes"

START_TIME=$(date +%s%3N)
for TABLE in "${TABLES[@]}"; do
    ip -n $NS -6 -batch "./tmp/batch-table-$TABLE.txt" &
done

wait
END_TIME=$(date +%s%3N)
ELAPSED_TIME=$((END_TIME - START_TIME))

echo "added $((${#ROUTES[@]} * ${#TABLES[@]})) routes (${#ROUTES[@]} routes * ${#TABLES[@]} tables)."
echo "Time elapsed: ${ELAPSED_TIME} milliseconds."
echo $(ip -n $NS -6 route show table all | wc -l)  # Just for debug

ip netns del $NS
rm -fr ./tmp/


Kuniyuki Iwashima (13):
  ipv6: Validate RTA_GATEWAY of RTA_MULTIPATH in rtm_to_fib6_config().
  ipv6: Get rid of RTNL for SIOCDELRT and RTM_DELROUTE.
  ipv6: Move some validation from ip6_route_info_create() to
    rtm_to_fib6_config().
  ipv6: Check GATEWAY in rtm_to_fib6_multipath_config().
  ipv6: Move nexthop_find_by_id() after fib6_info_alloc().
  ipv6: Split ip6_route_info_create().
  ipv6: Preallocate rt->fib6_nh->rt6i_pcpu in ip6_route_info_create().
  ipv6: Preallocate nhc_pcpu_rth_output in ip6_route_info_create().
  ipv6: Don't pass net to ip6_route_info_append().
  ipv6: Factorise ip6_route_multipath_add().
  ipv6: Protect fib6_link_table() with spinlock.
  ipv6: Protect nh->f6i_list with spinlock and flag.
  ipv6: Get rid of RTNL for SIOCADDRT and RTM_NEWROUTE.

 include/net/netns/ipv6.h |   1 +
 include/net/nexthop.h    |   2 +
 net/ipv4/fib_semantics.c |  10 +-
 net/ipv4/nexthop.c       |  24 +-
 net/ipv6/ip6_fib.c       |  51 +++-
 net/ipv6/route.c         | 555 ++++++++++++++++++++++++---------------
 6 files changed, 415 insertions(+), 228 deletions(-)

-- 
2.48.1

[PATCH v1 net-next 10/13] ipv6: Factorise ip6_route_multipath_add().

[Kuniyuki Iwashima]

We will get rid of RTNL from RTM_NEWROUTE and SIOCADDRT.

Then, the RCU section will start before ip6_route_info_create_nh()
in ip6_route_multipath_add(), but ip6_route_info_create() is called
in the same loop and will sleep.

Let's split the loop into ip6_route_mpath_info_create() and
ip6_route_mpath_info_create_nh().

Note that ip6_route_info_append() is now integrated into
ip6_route_mpath_info_create_nh() because we need to call different
free functions for nexthops that passed ip6_route_info_create_nh().

In case of failure, the remaining nexthops that ip6_route_info_create_nh()
has not been called for will be freed by ip6_route_mpath_info_cleanup().

OTOH, if a nexthop passes ip6_route_info_create_nh(), it will be linked
to a local temporary list, which will be spliced back to rt6_nh_list.
In case of failure, these nexthops will be released by fib6_info_release()
in ip6_route_multipath_add().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/ipv6/route.c | 205 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 130 insertions(+), 75 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 26e5a372a9cd..a209d8c8ff75 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -5281,29 +5281,131 @@ struct rt6_nh {
 	struct fib6_info *fib6_info;
 	struct fib6_config r_cfg;
 	struct list_head next;
+	int weight;
 };
 
-static int ip6_route_info_append(struct list_head *rt6_nh_list,
-				 struct fib6_info *rt,
-				 struct fib6_config *r_cfg)
+static void ip6_route_mpath_info_cleanup(struct list_head *rt6_nh_list)
 {
-	struct rt6_nh *nh;
-	int err = -EEXIST;
+	struct rt6_nh *nh, *nh_next;
 
-	list_for_each_entry(nh, rt6_nh_list, next) {
-		/* check if fib6_info already exists */
-		if (rt6_duplicate_nexthop(nh->fib6_info, rt))
-			return err;
+	list_for_each_entry_safe(nh, nh_next, rt6_nh_list, next) {
+		struct fib6_info *rt = nh->fib6_info;
+
+		if (rt) {
+			free_percpu(rt->fib6_nh->nh_common.nhc_pcpu_rth_output);
+			free_percpu(rt->fib6_nh->rt6i_pcpu);
+			ip_fib_metrics_put(rt->fib6_metrics);
+			kfree(rt);
+		}
+
+		list_del(&nh->next);
+		kfree(nh);
 	}
+}
 
-	nh = kzalloc(sizeof(*nh), GFP_KERNEL);
-	if (!nh)
-		return -ENOMEM;
-	nh->fib6_info = rt;
-	memcpy(&nh->r_cfg, r_cfg, sizeof(*r_cfg));
-	list_add_tail(&nh->next, rt6_nh_list);
+static int ip6_route_mpath_info_create(struct list_head *rt6_nh_list,
+				       struct fib6_config *cfg,
+				       struct netlink_ext_ack *extack)
+{
+	struct rtnexthop *rtnh;
+	int remaining;
+	int err;
+
+	remaining = cfg->fc_mp_len;
+	rtnh = (struct rtnexthop *)cfg->fc_mp;
+
+	/* Parse a Multipath Entry and build a list (rt6_nh_list) of
+	 * fib6_info structs per nexthop
+	 */
+	while (rtnh_ok(rtnh, remaining)) {
+		struct fib6_config r_cfg;
+		struct fib6_info *rt;
+		struct rt6_nh *nh;
+		int attrlen;
+
+		nh = kzalloc(sizeof(*nh), GFP_KERNEL);
+		if (!nh) {
+			err = -ENOMEM;
+			goto err;
+		}
+
+		list_add_tail(&nh->next, rt6_nh_list);
+
+		memcpy(&r_cfg, cfg, sizeof(*cfg));
+		if (rtnh->rtnh_ifindex)
+			r_cfg.fc_ifindex = rtnh->rtnh_ifindex;
+
+		attrlen = rtnh_attrlen(rtnh);
+		if (attrlen > 0) {
+			struct nlattr *nla, *attrs = rtnh_attrs(rtnh);
+
+			nla = nla_find(attrs, attrlen, RTA_GATEWAY);
+			if (nla) {
+				r_cfg.fc_gateway = nla_get_in6_addr(nla);
+				r_cfg.fc_flags |= RTF_GATEWAY;
+			}
+
+			r_cfg.fc_encap = nla_find(attrs, attrlen, RTA_ENCAP);
+			nla = nla_find(attrs, attrlen, RTA_ENCAP_TYPE);
+			if (nla)
+				r_cfg.fc_encap_type = nla_get_u16(nla);
+		}
+
+		r_cfg.fc_flags |= (rtnh->rtnh_flags & RTNH_F_ONLINK);
+
+		rt = ip6_route_info_create(&r_cfg, GFP_KERNEL, extack);
+		if (IS_ERR(rt)) {
+			err = PTR_ERR(rt);
+			goto err;
+		}
+
+		nh->fib6_info = rt;
+		nh->weight = rtnh->rtnh_hops + 1;
+		memcpy(&nh->r_cfg, &r_cfg, sizeof(r_cfg));
+
+		rtnh = rtnh_next(rtnh, &remaining);
+	}
 
 	return 0;
+err:
+	ip6_route_mpath_info_cleanup(rt6_nh_list);
+	return err;
+}
+
+static int ip6_route_mpath_info_create_nh(struct list_head *rt6_nh_list,
+					  struct netlink_ext_ack *extack)
+{
+	struct rt6_nh *nh, *nh_next, *nh_tmp;
+	LIST_HEAD(tmp);
+	int err;
+
+	list_for_each_entry_safe(nh, nh_next, rt6_nh_list, next) {
+		struct fib6_info *rt = nh->fib6_info;
+
+		err = ip6_route_info_create_nh(rt, &nh->r_cfg, extack);
+		if (err) {
+			nh->fib6_info = NULL;
+			goto err;
+		}
+
+		rt->fib6_nh->fib_nh_weight = nh->weight;
+
+		list_move_tail(&nh->next, &tmp);
+
+		list_for_each_entry(nh_tmp, rt6_nh_list, next) {
+			/* check if fib6_info already exists */
+			if (rt6_duplicate_nexthop(nh_tmp->fib6_info, rt)) {
+				err = -EEXIST;
+				goto err;
+			}
+		}
+	}
+out:
+	list_splice(&tmp, rt6_nh_list);
+	return err;
+err:
+	ip6_route_mpath_info_cleanup(rt6_nh_list);
+	goto out;
 }
 
 static void ip6_route_mpath_notify(struct fib6_info *rt,
@@ -5362,75 +5464,28 @@ static int ip6_route_multipath_add(struct fib6_config *cfg,
 {
 	struct fib6_info *rt_notif = NULL, *rt_last = NULL;
 	struct nl_info *info = &cfg->fc_nlinfo;
-	struct fib6_config r_cfg;
-	struct rtnexthop *rtnh;
-	struct fib6_info *rt;
-	struct rt6_nh *err_nh;
 	struct rt6_nh *nh, *nh_safe;
+	LIST_HEAD(rt6_nh_list);
+	struct rt6_nh *err_nh;
 	__u16 nlflags;
-	int remaining;
-	int attrlen;
-	int err = 1;
 	int nhn = 0;
-	int replace = (cfg->fc_nlinfo.nlh &&
-		       (cfg->fc_nlinfo.nlh->nlmsg_flags & NLM_F_REPLACE));
-	LIST_HEAD(rt6_nh_list);
+	int replace;
+	int err;
+
+	replace = (cfg->fc_nlinfo.nlh &&
+		   (cfg->fc_nlinfo.nlh->nlmsg_flags & NLM_F_REPLACE));
 
 	nlflags = replace ? NLM_F_REPLACE : NLM_F_CREATE;
 	if (info->nlh && info->nlh->nlmsg_flags & NLM_F_APPEND)
 		nlflags |= NLM_F_APPEND;
 
-	remaining = cfg->fc_mp_len;
-	rtnh = (struct rtnexthop *)cfg->fc_mp;
-
-	/* Parse a Multipath Entry and build a list (rt6_nh_list) of
-	 * fib6_info structs per nexthop
-	 */
-	while (rtnh_ok(rtnh, remaining)) {
-		memcpy(&r_cfg, cfg, sizeof(*cfg));
-		if (rtnh->rtnh_ifindex)
-			r_cfg.fc_ifindex = rtnh->rtnh_ifindex;
-
-		attrlen = rtnh_attrlen(rtnh);
-		if (attrlen > 0) {
-			struct nlattr *nla, *attrs = rtnh_attrs(rtnh);
-
-			nla = nla_find(attrs, attrlen, RTA_GATEWAY);
-			if (nla) {
-				r_cfg.fc_gateway = nla_get_in6_addr(nla);
-				r_cfg.fc_flags |= RTF_GATEWAY;
-			}
-
-			r_cfg.fc_encap = nla_find(attrs, attrlen, RTA_ENCAP);
-			nla = nla_find(attrs, attrlen, RTA_ENCAP_TYPE);
-			if (nla)
-				r_cfg.fc_encap_type = nla_get_u16(nla);
-		}
-
-		r_cfg.fc_flags |= (rtnh->rtnh_flags & RTNH_F_ONLINK);
-		rt = ip6_route_info_create(&r_cfg, GFP_KERNEL, extack);
-		if (IS_ERR(rt)) {
-			err = PTR_ERR(rt);
-			rt = NULL;
-			goto cleanup;
-		}
-
-		err = ip6_route_info_create_nh(rt, &r_cfg, extack);
-		if (err) {
-			rt = NULL;
-			goto cleanup;
-		}
-
-		rt->fib6_nh->fib_nh_weight = rtnh->rtnh_hops + 1;
-
-		err = ip6_route_info_append(&rt6_nh_list, rt, &r_cfg);
-		if (err) {
-			fib6_info_release(rt);
-			goto cleanup;
-		}
+	err = ip6_route_mpath_info_create(&rt6_nh_list, cfg, extack);
+	if (err)
+		return err;
 
-		rtnh = rtnh_next(rtnh, &remaining);
-	}
+	err = ip6_route_mpath_info_create_nh(&rt6_nh_list, extack);
+	if (err)
+		goto cleanup;
 
 	/* for add and replace send one notification with all nexthops.
 	 * Skip the notification in fib6_add_rt2node and send one with
-- 
2.48.1