| 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
| ||
|
Message-ID: <20250504201116.30743-1-kuniyu@amazon.com>
Date: Sun, 4 May 2025 13:11:11 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <edumazet@...gle.com>
CC: <davem@...emloft.net>, <dsahern@...nel.org>, <horms@...nel.org>,
<kuba@...nel.org>, <kuni1840@...il.com>, <kuniyu@...zon.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.
From: Eric Dumazet <edumazet@...gle.com>
Date: Sun, 4 May 2025 12:34:49 -0700
> 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.
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!
Powered by blists - more mailing lists