Thread (14 messages) 14 messages, 3 authors, 2d ago

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help