Re: [PATCH v3 net-next 15/15] ipv6: Get rid of RTNL for SIOCADDRT and RTM_NEWROUTE.

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Eric Dumazet <edumazet@google.com>
Date: Sun, 4 May 2025 12:34:49 -0700
> On Sun, May 4, 2025 at 10:22 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> >
> > From: Eric Dumazet <edumazet@google.com>
> > Date: Sun, 4 May 2025 02:16:13 -0700
> > > On Thu, Apr 17, 2025 at 5:10 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> > > >
> > > > Now we are ready to remove RTNL from SIOCADDRT and RTM_NEWROUTE.
> > > >
> > > > The remaining things to do are
> > > >
> > > >   1. pass false to lwtunnel_valid_encap_type_attr()
> > > >   2. use rcu_dereference_rtnl() in fib6_check_nexthop()
> > > >   3. place rcu_read_lock() before ip6_route_info_create_nh().
> > > >
> > > > Let's complete the RTNL-free conversion.
> > > >
> > > > When each CPU-X adds 100000 routes on table-X in a batch
> > > > concurrently on c7a.metal-48xl EC2 instance with 192 CPUs,
> > > >
> > > > without this series:
> > > >
> > > >   $ sudo ./route_test.sh
> > > >   ...
> > > >   added 19200000 routes (100000 routes * 192 tables).
> > > >   time elapsed: 191577 milliseconds.
> > > >
> > > > with this series:
> > > >
> > > >   $ sudo ./route_test.sh
> > > >   ...
> > > >   added 19200000 routes (100000 routes * 192 tables).
> > > >   time elapsed: 62854 milliseconds.
> > > >
> > > > I changed the number of routes in each table (1000 ~ 100000)
> > > > and consistently saw it finish 3x faster with this series.
> > > >
> > > > Note that now every caller of lwtunnel_valid_encap_type() passes
> > > > false as the last argument, and this can be removed later.
> > > >
> > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > > > ---
> > > >  net/ipv4/nexthop.c |  4 ++--
> > > >  net/ipv6/route.c   | 18 ++++++++++++------
> > > >  2 files changed, 14 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> > > > index 6ba6cb1340c1..823e4a783d2b 100644
> > > > --- a/net/ipv4/nexthop.c
> > > > +++ b/net/ipv4/nexthop.c
> > > > @@ -1556,12 +1556,12 @@ int fib6_check_nexthop(struct nexthop *nh, struct fib6_config *cfg,
> > > >         if (nh->is_group) {
> > > >                 struct nh_group *nhg;
> > > >
> > > > -               nhg = rtnl_dereference(nh->nh_grp);
> > > > +               nhg = rcu_dereference_rtnl(nh->nh_grp);
> > >
> > > Or add a condition about table lock being held ?
> >
> > I think in this context the caller is more of an rcu reader
> > than a ipv6 route writer.
> >
> >
> >
> > >
> > > >                 if (nhg->has_v4)
> > > >                         goto no_v4_nh;
> > > >                 is_fdb_nh = nhg->fdb_nh;
> > > >         } else {
> > > > -               nhi = rtnl_dereference(nh->nh_info);
> > > > +               nhi = rcu_dereference_rtnl(nh->nh_info);
> > > >                 if (nhi->family == AF_INET)
> > > >                         goto no_v4_nh;
> > > >                 is_fdb_nh = nhi->fdb_nh;
> > > > diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> > > > index c8c1c75268e3..bb46e724db73 100644
> > > > --- a/net/ipv6/route.c
> > > > +++ b/net/ipv6/route.c
> > > > @@ -3902,12 +3902,16 @@ int ip6_route_add(struct fib6_config *cfg, gfp_t gfp_flags,
> > > >         if (IS_ERR(rt))
> > > >                 return PTR_ERR(rt);
> > > >
> > > > +       rcu_read_lock();
> > >
> > > This looks bogus to me (and syzbot)
> > >
> > > Holding rcu_read_lock() from writers is almost always wrong.
> >
> > Yes, but I added it as a reader of netdev and nexthop to guarantee
> > that they won't go away.
> 
> We have standard ways for preventing this, acquiring a refcount on the objects.
> >
> >
> > >
> > > In this case, this prevents any GFP_KERNEL allocations to happen
> > > (among other things)
> >
> > Oh, I completely missed this path, thanks!
> >
> > Fortunately, it seems all ->build_state() except for
> > ip_tun_build_state() use GFP_ATOMIC.
> >
> > So, simply changing GFP_KERNEL to GFP_ATOMIC is acceptable ?
> 
> What protects against writers' mutual exclusion ?
> 
> I dislike using GFP_ATOMIC in control paths. This is something that we
> must avoid.
> 
> This will make admin operations unpredictable. Many scripts would
> break under pressure.

I see.  I'll rework this and convert RCU to refcounting of
netdev and nexthop.

Then, I'll change the rcu_dereference_rtnl() above and friends to
check the table lock as you suggested.


> 
> >
> >
> > lwtunnel_state_alloc
> >   - kzalloc(GFP_ATOMIC)
> >
> > ip_tun_build_state
> >   - dst_cache_init(GFP_KERNEL)
> >
> > ip6_tun_build_state / mpls_build_state / xfrmi_build_state
> > -> no alloc other than lwtunnel_state_alloc()
> >
> > bpf_build_state
> >   - bpf_parse_prog
> >     - nla_memdup(GFP_ATOMIC)
> >
> > ila_build_state / ioam6_build_state / rpl_build_state / seg6_build_state
> >   - dst_cache_init(GFP_ATOMIC)
> >
> > seg6_local_build_state
> >   - seg6_end_dt4_build / seg6_end_dt6_build / seg6_end_dt46_build
> >     -> no alloc other than lwtunnel_state_alloc()
> >
> 
> You mean, following the wrong fix done in :
> 
> 14a0087e7236228d56bfa3fab7084c19fcb513fb ipv6: sr: switch to
> GFP_ATOMIC flag to allocate memory during seg6local LWT setup

Oh it's last week... somehow I missed this mail..
maybe I filtered it with ipv6: sr.. :/

In the rework, I'll include the revert of that.

Thanks!