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: <89ca1978-de9e-4502-8a3b-970ad8fd9fcf@paulmck-laptop>
Date: Wed, 19 Mar 2025 11:40:40 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Breno Leitao <leitao@...ian.org>
Cc: longman@...hat.com, bvanassche@....org,
	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 11:12:24AM -0700, Breno Leitao wrote:
> 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>

The IPIs are not fun, but in the interest of getting *some* solution
moving forward...  ;-)

Reviewed-by: Paul E. McKenney <paulmck@...nel.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);
>  
>