netdev - Re: tc: network egress frozen during qdisc update with debug kernel

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: <20250319-truthful-whispering-moth-d308b4@leitao>
Date: Wed, 19 Mar 2025 11:12:24 -0700
From: Breno Leitao <leitao@...ian.org>
To: "Paul E. McKenney" <paulmck@...nel.org>, longman@...hat.com,
	bvanassche@....org
Cc: Eric Dumazet <edumazet@...gle.com>, kuba@...nel.org, jhs@...atatu.com,
	xiyou.wangcong@...il.com, jiri@...nulli.us, kuniyu@...zon.com,
	rcu@...r.kernel.org, kasan-dev@...glegroups.com,
	netdev@...r.kernel.org
Subject: Re: tc: network egress frozen during qdisc update with debug kernel

On Wed, Mar 19, 2025 at 09:05:07AM -0700, Paul E. McKenney wrote:

> > I think we should redesign lockdep_unregister_key() to work on a separately
> > allocated piece of memory,
> > then use kfree_rcu() in it.
> > 
> > Ie not embed a "struct lock_class_key" in the struct Qdisc, but a pointer to
> > 
> > struct ... {
> >      struct lock_class_key;
> >      struct rcu_head  rcu;
> > }
> 
> Works for me!

I've tested a different approach, using synchronize_rcu_expedited()
instead of synchronize_rcu(), given how critical this function is
called, and the command performance improves dramatically.

This approach has some IPI penalties, but, it might be quicker to review
and get merged, mitigating the network issue.

Does it sound a bad approach?

Date:   Wed Mar 19 10:23:56 2025 -0700

    lockdep: Speed up lockdep_unregister_key() with expedited RCU synchronization
    
    lockdep_unregister_key() is called from critical code paths, including
    sections where rtnl_lock() is held. When replacing a qdisc in a network
    device, network egress traffic is disabled while __qdisc_destroy() is
    called for every queue. This function calls lockdep_unregister_key(),
    which was blocked waiting for synchronize_rcu() to complete.
    
    For example, a simple tc command to replace a qdisc could take 13
    seconds:
    
      # time /usr/sbin/tc qdisc replace dev eth0 root handle 0x1234: mq
        real    0m13.195s
        user    0m0.001s
        sys     0m2.746s
    
    During this time, network egress is completely frozen while waiting for
    RCU synchronization.
    
    Use synchronize_rcu_expedite() instead to minimize the impact on
    critical operations like network connectivity changes.
    
    Signed-off-by: Breno Leitao <leitao@...ian.org>

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 4470680f02269..96b87f1853f4f 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -6595,8 +6595,10 @@ void lockdep_unregister_key(struct lock_class_key *key)
 	if (need_callback)
 		call_rcu(&delayed_free.rcu_head, free_zapped_rcu);
 
-	/* Wait until is_dynamic_key() has finished accessing k->hash_entry. */
-	synchronize_rcu();
+	/* Wait until is_dynamic_key() has finished accessing k->hash_entry.
+	 * This needs to be quick, since it is called in critical sections
+	 */
+	synchronize_rcu_expedite();
 }
 EXPORT_SYMBOL_GPL(lockdep_unregister_key);