netdev - Re: [PATCH RESEND v2 net-next 02/14] ipv6: Get rid of RTNL for SIOCDELRT and RTM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250416184559.99881-1-kuniyu@amazon.com>
Date: Wed, 16 Apr 2025 11:45:57 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <pabeni@...hat.com>
CC: <davem@...emloft.net>, <dsahern@...nel.org>, <edumazet@...gle.com>,
	<horms@...nel.org>, <kuba@...nel.org>, <kuni1840@...il.com>,
	<kuniyu@...zon.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH RESEND v2 net-next 02/14] ipv6: Get rid of RTNL for SIOCDELRT and RTM_DELROUTE.

From: Paolo Abeni <pabeni@...hat.com>
Date: Wed, 16 Apr 2025 10:49:53 +0200
> On 4/14/25 8:14 PM, Kuniyuki Iwashima wrote:
> > Basically, removing an IPv6 route does not require RTNL because
> > the IPv6 routing tables are protected by per table lock.
> > 
> > inet6_rtm_delroute() calls nexthop_find_by_id() to check if the
> > nexthop specified by RTA_NH_ID exists.  nexthop uses rbtree and
> > the top-down walk can be safely performed under RCU.
> > 
> > ip6_route_del() already relies on RCU and the table lock, but we
> > need to extend the RCU critical section a bit more to cover
> > __ip6_del_rt().  For example, nexthop_for_each_fib6_nh() and
> > inet6_rt_notify() needs RCU.
> 
> The last statement is not clear to me. I don't see __ip6_del_rt()
> calling nexthop_for_each_fib6_nh() or inet6_rt_notify() ?!?

Thank you for review!

It's burried in the depths, and I noticed this from the v1 test result.
https://lore.kernel.org/netdev/Z91yk90LZy9yJexG@mini-arch/

inet6_rtm_delroute
 ip6_route_del
  __ip6_del_rt
   fib6_del
    fib6_del_route
     fib6_purge_rt
      nexthop_for_each_fib6_nh

inet6_rtm_delroute
 ip6_route_del
  __ip6_del_rt
   fib6_del
    fib6_del_route
     inet6_rt_notify


> 
> Also after this patch we have this chunk in ip6_route_del():
> 
> 	table = fib6_get_table(cfg->fc_nlinfo.nl_net, cfg->fc_table);
> 	if (!table)
> 		//..
> 
> 	rcu_read_lock();
> 
> which AFAICS should be safe because 'table' is freed only at netns exit
> time,

Right, and there are few other functions assuming the same thing.

addrconf_get_prefix_route()
rt6_get_route_info()

> but acquiring the rcu lock after grabbing the rcu protected struct
> is confusing. It should be good adding a comment or moving the rcu lock
> before the lookup (and dropping the RCU lock from fib6_get_table())

There are other callers of fib6_get_table(), so I'd move rcu_read_lock()
before it, and will look into them if we can drop it from fib6_get_table().