Re: [PATCH net v2] ppp: defer channel free to an RCU grace period to fix pppol2tp RX UAF
From: "Paul E. McKenney" <paulmck@kernel.org>
Date: 2026-07-15 16:26:10
On Wed, Jul 15, 2026 at 10:25:18AM +0200, Sebastian Andrzej Siewior wrote:
On 2026-07-14 10:47:58 [-0700], Paul E. McKenney wrote:quoted
On Tue, Jul 14, 2026 at 06:01:04PM +0200, Sebastian Andrzej Siewior wrote:quoted
On 2026-07-08 12:22:58 [-0700], Paul E. McKenney wrote:quoted
The RCU-callback ordering guarantees are quite weak:… So this is not working.quoted
What you maybe *could* do is to have the two RCU callbacks communicate, so that the last one to be invoked did the work of both of them. For example, use a shared variable initialized to 2, then have each callback do atomic_dec_and_test(), with the "winner" doing the work. Would that do the trick?This would work but we would have to fix each one or make it slower for everyone in the common case. So I think adding a rcu_barrier() to module unload wouldn't be that bad and Petr did not say "get out" so ;)I am not opposed to rcu_barrier() in module unload. But just to fill out the other similar situations... In addition to call_rcu(), there is call_srcu(), call_rcu_tasks(), and call_rcu_tasks_rude(). I think that we can rule out call_srcu() as ridiculous because there could have been an arbitrarily large number of srcu_struct structures passed to call_srcu() along with callback functions defined within this module. For their parts, call_rcu_tasks() and call_rcu_tasks_rude() are special-purpose functions that are rarely used and could therefore be ignored.
[ Just to be clear, I doubt that these problems all need any better solution than the status quo. After all, any added mechanism can easily produce more bugs than it consumes. I just want people to be aware. ]
Regarding srcu, module unload should have cleanup_srcu_struct() which would flush pending item, no?
Unless the srcu_struct was global, but the callback was located in this module.
quoted
But what about things like timers and hrtimers? Or are people already doing the right thing for such things?Right. What people often did wrong with timers is that they enqueued a timer then forgot about it and released the memory with the timer active. Then someone who got all the timer bugs assigned came up with debugobjects. This checks only that the memory is not released while the object is active. It does not verify the callback.
Ah, so *that* is where debugobjects came from! ;-)
I think it depends on what we want to achieve. We could iterate over all RCU callbacks checking if it belongs to the memory that we intend to free. The same could be done for timers, too. They should be caught by debugbjects unless they leak the memory…
Iterating over RCU callbacks is a no-go, except maybe under extreme debugging levels. There can easily be millions of those things!
The rcu_barrier() before module_exit() would ensure the callbacks are done before a possible kmem_cache_free() where the cache is removed in the exit function. Now I see that for !SLUB_TINY there is a barrier if the cache has sheaves (kmem_cache_destroy() -> kvfree_rcu_barrier_on_cache()). Buh…
;-) ;-) ;-) Thanx, Paul
Sebastian