netdev - Re: [PATCH net-next] net/sched: acp_api: no longer acquire RTNL in tc_action_net

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: <CANn89iKA23zDtt3+3K46QrFx-3iUP-Ef4+n87xWdQJhTWA_zcA@mail.gmail.com>
Date: Wed, 2 Jul 2025 00:08:01 -0700
From: Eric Dumazet <edumazet@...gle.com>
To: Cong Wang <xiyou.wangcong@...il.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	Jamal Hadi Salim <jhs@...atatu.com>, Jiri Pirko <jiri@...nulli.us>, 
	Kuniyuki Iwashima <kuniyu@...gle.com>, netdev@...r.kernel.org, eric.dumazet@...il.com, 
	Vlad Buslov <vladbu@...dia.com>, Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Subject: Re: [PATCH net-next] net/sched: acp_api: no longer acquire RTNL in tc_action_net_exit()

On Tue, Jul 1, 2025 at 7:35 PM Cong Wang <xiyou.wangcong@...il.com> wrote:
>
> On Tue, Jul 01, 2025 at 01:30:06PM +0000, Eric Dumazet wrote:
> > tc_action_net_exit() got an rtnl exclusion in commit
> > a159d3c4b829 ("net_sched: acquire RTNL in tc_action_net_exit()")
> >
> > Since then, commit 16af6067392c ("net: sched: implement reference
> > counted action release") made this RTNL exclusion obsolete.
>
> I am not sure removing RTNL is safe even we have action refcnt.
>
> For example, are you sure tcf_action_offload_del() is safe to call
> without RTNL?

My thinking was that at the time of these calls, devices were already
gone from the dismantling netns, but this might be wrong.

We can conditionally acquire rtnl from tcf_idrinfo_destroy() when
there is at least one offloaded action in the idr.

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 057e20cef3754f33357c4c1e30034f6b9b872d91..9e468e46346710c85c3a85b905d27dfe3972916a
100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -933,18 +933,25 @@ void tcf_idrinfo_destroy(const struct tc_action_ops *ops,
                         struct tcf_idrinfo *idrinfo)
 {
        struct idr *idr = &idrinfo->action_idr;
+       bool mutex_taken = false;
        struct tc_action *p;
-       int ret;
        unsigned long id = 1;
        unsigned long tmp;
+       int ret;

        idr_for_each_entry_ul(idr, p, tmp, id) {
+               if (tc_act_in_hw(p) && !mutex_taken) {
+                       rtnl_lock();
+                       mutex_taken = true;
+               }
                ret = __tcf_idr_release(p, false, true);
                if (ret == ACT_P_DELETED)
                        module_put(ops->owner);
                else if (ret < 0)
                        return;
        }
+       if (mutex_taken)
+               rtnl_unlock();
        idr_destroy(&idrinfo->action_idr);
 }
 EXPORT_SYMBOL(tcf_idrinfo_destroy);


>
> What are you trying to improve here?

Yeah, some of us are spending months of work to improve the RTNL
situation, and we do not copy/paste why on every single patch :)

I will capture the following in V2, thanks !

Most netns do not have actions, yet deleting them is adding a lot of
pressure on RTNL, which is for us the most contended mutex in the
kernel.

We are moving to a per-netns 'rtnl', so tc_action_net_exit() will not
be able to grab 'rtnl' a single time for a batch of netns.


Before the patch:

perf probe -a rtnl_lock  # Note: This does not capture all calls, some
of them might be inlined in net/core/rtnetlink.c

perf record -e probe:rtnl_lock -a /bin/bash -c 'unshare -n "/bin/true"; sleep 1'
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.305 MB perf.data (25 samples) ]

After the patch:

perf record -e probe:rtnl_lock -a /bin/bash -c 'unshare -n "/bin/true"; sleep 1'
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.304 MB perf.data (9 samples) ]