Re: [PATCH net v2] ppp: defer channel free to an RCU grace period to fix pppol2tp RX UAF

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:
> > On Tue, Jul 14, 2026 at 06:01:04PM +0200, Sebastian Andrzej Siewior wrote:
> > > On 2026-07-08 12:22:58 [-0700], Paul E. McKenney wrote:
> > > > The RCU-callback ordering guarantees are quite weak:
> > > …
> > > So this is not working.
> > > 
> > > > 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.

> > 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