netdev - Re: [PATCH v3 net-next 15/15] ipv6: Get rid of RTNL for SIOCADDRT 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: <CANn89i+fLC6fd-1ea7W3OhL+562qpjBEbKj7sgtCG7ogZj=dOQ@mail.gmail.com>
Date: Sun, 4 May 2025 12:34:49 -0700
From: Eric Dumazet <edumazet@...gle.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, dsahern@...nel.org, horms@...nel.org, kuba@...nel.org, 
	kuni1840@...il.com, netdev@...r.kernel.org, pabeni@...hat.com
Subject: Re: [PATCH v3 net-next 15/15] ipv6: Get rid of RTNL for SIOCADDRT and RTM_NEWROUTE.

On Sun, May 4, 2025 at 10:22 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> From: Eric Dumazet <edumazet@...gle.com>
> Date: Sun, 4 May 2025 02:16:13 -0700
> > On Thu, Apr 17, 2025 at 5:10 PM Kuniyuki Iwashima <kuniyu@...zon.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@...zon.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.

>
>
> 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