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

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

[Sebastian Andrzej Siewior]

+ MODULE maintainer

On 2026-07-05 10:57:44 [+0800], Qingfang Deng wrote:
> On 7/4/2026 at 12:32 AM, Breno Leitao wrote:
> > On Fri, Jul 03, 2026 at 03:27:00PM +0800, Qingfang Deng wrote:
> > > AI-review found an issue: https://sashiko.dev/#/patchset/D9C0245B-608B-4884-8A09-F55BA4A9F948%40doyensec.com
> > > 
> > > An rcu_barrier() call is needed at the end of ppp_cleanup().
> > 
> > I was initially unclear why rcu_barrier() would be necessary on a kfree path,
> > but it appears to be required during module unload to ensure that
> > ppp_release_channel_free() completes before the module's struct rcu_head is
> > destroyed. Is that the correct understanding?
> 
> It's required to ensure that all ppp_release_channel_free() callback
> complete before the text segment of the module is unloaded.

So either a rcu_barrier() in ppp's module_exit() callback or a
synchronize_rcu() instead of the call_rcu(). And all this because the
module RCU callbacks pending which can be invoked after the module has
been removed. There is a synchronize_rcu() during module exit but this
is after the module code is gone.

I'm curious how many modules have a call_rcu() within their code but
don't have anything to enforce its completion before module removal is
complete? Wouldn't something like


diff --git a/kernel/module/main.c b/kernel/module/main.c
index 46dd8d25a6058..8eae1ea2d6eb4 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -858,6 +858,9 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 		goto out;
 
 	mutex_unlock(&module_mutex);
+
+	/* Ensure all rcu callbacks issued by the module have completed */
+	rcu_barrier();
 	/* Final destruction now no one is using it. */
 	if (mod->exit != NULL)
 		mod->exit();

make sense?

Sebastian

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

[Petr Pavlu]

On 7/6/26 11:29 AM, Sebastian Andrzej Siewior wrote:
> + MODULE maintainer

+ Paul E. McKenney

> 
> On 2026-07-05 10:57:44 [+0800], Qingfang Deng wrote:
>> On 7/4/2026 at 12:32 AM, Breno Leitao wrote:
>>> On Fri, Jul 03, 2026 at 03:27:00PM +0800, Qingfang Deng wrote:
>>>> AI-review found an issue: https://sashiko.dev/#/patchset/D9C0245B-608B-4884-8A09-F55BA4A9F948%40doyensec.com
>>>>
>>>> An rcu_barrier() call is needed at the end of ppp_cleanup().
>>>
>>> I was initially unclear why rcu_barrier() would be necessary on a kfree path,
>>> but it appears to be required during module unload to ensure that
>>> ppp_release_channel_free() completes before the module's struct rcu_head is
>>> destroyed. Is that the correct understanding?
>>
>> It's required to ensure that all ppp_release_channel_free() callback
>> complete before the text segment of the module is unloaded.
> 
> So either a rcu_barrier() in ppp's module_exit() callback or a
> synchronize_rcu() instead of the call_rcu(). And all this because the
> module RCU callbacks pending which can be invoked after the module has
> been removed. There is a synchronize_rcu() during module exit but this
> is after the module code is gone.
> 
> I'm curious how many modules have a call_rcu() within their code but
> don't have anything to enforce its completion before module removal is
> complete? Wouldn't something like
> 
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 46dd8d25a6058..8eae1ea2d6eb4 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -858,6 +858,9 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>  		goto out;
>  
>  	mutex_unlock(&module_mutex);
> +
> +	/* Ensure all rcu callbacks issued by the module have completed */
> +	rcu_barrier();
>  	/* Final destruction now no one is using it. */
>  	if (mod->exit != NULL)
>  		mod->exit();
> 
> make sense?

This is discussed in Documentation/RCU/rcubarrier.rst and
Documentation/RCU/Design/Requirements/Requirements.rst. The latter
contains:

| Loadable Modules
| ~~~~~~~~~~~~~~~~
| 
| The Linux kernel has loadable modules, and these modules can also be
| unloaded. After a given module has been unloaded, any attempt to call
| one of its functions results in a segmentation fault. The module-unload
| functions must therefore cancel any delayed calls to loadable-module
| functions, for example, any outstanding mod_timer() must be dealt
| with via timer_shutdown_sync() or similar.
| 
| Unfortunately, there is no way to cancel an RCU callback; once you
| invoke call_rcu(), the callback function is eventually going to be
| invoked, unless the system goes down first. Because it is normally
| considered socially irresponsible to crash the system in response to a
| module unload request, we need some other way to deal with in-flight RCU
| callbacks.
| 
| RCU therefore provides rcu_barrier(), which waits until all
| in-flight RCU callbacks have been invoked. If a module uses
| call_rcu(), its exit function should therefore prevent any future
| invocation of call_rcu(), then invoke rcu_barrier(). In theory,
| the underlying module-unload code could invoke rcu_barrier()
| unconditionally, but in practice this would incur unacceptable
| latencies.

I don't know if the last part about unacceptable latencies is still
relevant. I haven't done any measurements myself.

-- 
Thanks,
Petr

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

[Paul E. McKenney]

On Tue, Jul 07, 2026 at 05:32:10PM +0200, Petr Pavlu wrote:
> On 7/6/26 11:29 AM, Sebastian Andrzej Siewior wrote:
> > + MODULE maintainer
> 
> + Paul E. McKenney
> 
> > 
> > On 2026-07-05 10:57:44 [+0800], Qingfang Deng wrote:
> >> On 7/4/2026 at 12:32 AM, Breno Leitao wrote:
> >>> On Fri, Jul 03, 2026 at 03:27:00PM +0800, Qingfang Deng wrote:
> >>>> AI-review found an issue: https://sashiko.dev/#/patchset/D9C0245B-608B-4884-8A09-F55BA4A9F948%40doyensec.com
> >>>>
> >>>> An rcu_barrier() call is needed at the end of ppp_cleanup().
> >>>
> >>> I was initially unclear why rcu_barrier() would be necessary on a kfree path,
> >>> but it appears to be required during module unload to ensure that
> >>> ppp_release_channel_free() completes before the module's struct rcu_head is
> >>> destroyed. Is that the correct understanding?
> >>
> >> It's required to ensure that all ppp_release_channel_free() callback
> >> complete before the text segment of the module is unloaded.
> > 
> > So either a rcu_barrier() in ppp's module_exit() callback or a
> > synchronize_rcu() instead of the call_rcu(). And all this because the
> > module RCU callbacks pending which can be invoked after the module has
> > been removed. There is a synchronize_rcu() during module exit but this
> > is after the module code is gone.
> > 
> > I'm curious how many modules have a call_rcu() within their code but
> > don't have anything to enforce its completion before module removal is
> > complete? Wouldn't something like
> > 
> > 
> > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > index 46dd8d25a6058..8eae1ea2d6eb4 100644
> > --- a/kernel/module/main.c
> > +++ b/kernel/module/main.c
> > @@ -858,6 +858,9 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
> >  		goto out;
> >  
> >  	mutex_unlock(&module_mutex);
> > +
> > +	/* Ensure all rcu callbacks issued by the module have completed */
> > +	rcu_barrier();
> >  	/* Final destruction now no one is using it. */
> >  	if (mod->exit != NULL)
> >  		mod->exit();
> > 
> > make sense?

There was some discussion of doing exactly this back in the day, but
at that time there were many modules that didn't do call_rcu() at all,
let alone call_rcu() with a function defined in that module.  And yes,
there were performance concerns.

Now rcu_barrier() has seen some performance work in the meantime, but
careful benchmarking would be required to justify the above patch.

That said, some automation would be very good, given that this sort of
bug happens from time to time.

> This is discussed in Documentation/RCU/rcubarrier.rst and
> Documentation/RCU/Design/Requirements/Requirements.rst. The latter
> contains:
> 
> | Loadable Modules
> | ~~~~~~~~~~~~~~~~
> | 
> | The Linux kernel has loadable modules, and these modules can also be
> | unloaded. After a given module has been unloaded, any attempt to call
> | one of its functions results in a segmentation fault. The module-unload
> | functions must therefore cancel any delayed calls to loadable-module
> | functions, for example, any outstanding mod_timer() must be dealt
> | with via timer_shutdown_sync() or similar.
> | 
> | Unfortunately, there is no way to cancel an RCU callback; once you
> | invoke call_rcu(), the callback function is eventually going to be
> | invoked, unless the system goes down first. Because it is normally
> | considered socially irresponsible to crash the system in response to a
> | module unload request, we need some other way to deal with in-flight RCU
> | callbacks.
> | 
> | RCU therefore provides rcu_barrier(), which waits until all
> | in-flight RCU callbacks have been invoked. If a module uses
> | call_rcu(), its exit function should therefore prevent any future
> | invocation of call_rcu(), then invoke rcu_barrier(). In theory,
> | the underlying module-unload code could invoke rcu_barrier()
> | unconditionally, but in practice this would incur unacceptable
> | latencies.
> 
> I don't know if the last part about unacceptable latencies is still
> relevant. I haven't done any measurements myself.

Actual measurements would most definitely be needed!

Alternatives include:

o	Provide a patch like that above, but only execute the
	rcu_barrier() in some debug mode.  If your code works when
	that debug is enabled but does not otherwise, you add the
	rcu_barrier().

o	If debug is enabled, make rcu_do_batch() check the function
	before invoking it.  If the function is not mapped, issue a
	diagnostic, and don't try to invoke the function.  (But is
	there a sufficiently cheap way to check for the function not
	being mapped?)

o	Make the page-fault code check this possibility.  (But it would
	need to know that rcu_do_batch() was involved, which could no
	doubt be arranged.)

o	Make call_rcu() keep track of the fact that it was passed a
	function defined in a module, and set a flag that caused the
	module-exit code for that module to do rcu_barrier().  The
	trick here would be doing this without unacceptable increases
	to call_rcu() overheads.

o	Some sort of static analysis that determines that call_rcu()
	was passed a function defined in a module and either issues
	needed diagnostics or (somehow) letting the module-unload
	code know that rcu_barrier() is needed.

o	One challenge for many of these alternatives is that the module is
	already gone.  Maybe a KASAN-like trick that tracks the module's
	old memory for some time afterwards?  Or maybe the user usually
	knows which module was just now unloaded?  (Except for modules
	being dependent on each other...)

o	Your ideas here!!!

							Thanx, Paul

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

[Sebastian Andrzej Siewior]

On 2026-07-07 09:39:01 [-0700], Paul E. McKenney wrote:
> Alternatives include:
> 
> o	Provide a patch like that above, but only execute the
> 	rcu_barrier() in some debug mode.  If your code works when
> 	that debug is enabled but does not otherwise, you add the
> 	rcu_barrier().
> 
> o	If debug is enabled, make rcu_do_batch() check the function
> 	before invoking it.  If the function is not mapped, issue a
> 	diagnostic, and don't try to invoke the function.  (But is
> 	there a sufficiently cheap way to check for the function not
> 	being mapped?)

In both cases you would see a backtrace and the name of the last
unloaded module. And since we don't see a lot of these reports, people
either don't run into this because it does not exist or RCU is quick
enough.

> 
> 							Thanx, Paul

Sebastian

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

[Paul E. McKenney]

On Wed, Jul 08, 2026 at 11:11:47AM +0200, Sebastian Andrzej Siewior wrote:
> On 2026-07-07 09:39:01 [-0700], Paul E. McKenney wrote:
> > Alternatives include:
> > 
> > o	Provide a patch like that above, but only execute the
> > 	rcu_barrier() in some debug mode.  If your code works when
> > 	that debug is enabled but does not otherwise, you add the
> > 	rcu_barrier().
> > 
> > o	If debug is enabled, make rcu_do_batch() check the function
> > 	before invoking it.  If the function is not mapped, issue a
> > 	diagnostic, and don't try to invoke the function.  (But is
> > 	there a sufficiently cheap way to check for the function not
> > 	being mapped?)
> 
> In both cases you would see a backtrace and the name of the last
> unloaded module. And since we don't see a lot of these reports, people
> either don't run into this because it does not exist or RCU is quick
> enough.

Good point, the splat from calling the no-longer-mapped function should
call out the offending module.  So maybe our debug code is good enough
already.

							Thanx, Paul

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

[Sebastian Andrzej Siewior]

On 2026-07-07 17:32:10 [+0200], Petr Pavlu wrote:
> > --- a/kernel/module/main.c
> > +++ b/kernel/module/main.c
> > @@ -858,6 +858,9 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
> >  		goto out;
> >  
> >  	mutex_unlock(&module_mutex);
> > +
> > +	/* Ensure all rcu callbacks issued by the module have completed */
> > +	rcu_barrier();
> >  	/* Final destruction now no one is using it. */
> >  	if (mod->exit != NULL)
> >  		mod->exit();
> > 
> > make sense?
> 
> This is discussed in Documentation/RCU/rcubarrier.rst and
> Documentation/RCU/Design/Requirements/Requirements.rst. The latter
> contains:

I am aware of this. It is just not the first time I stumble about this.
But maybe with the AI review these days there won't be a miss.

> I don't know if the last part about unacceptable latencies is still
> relevant. I haven't done any measurements myself.

There is a synchronize_rcu() later on. I think I could replace it with a
call_rcu() so we might end up even. I was thinking about about it last
time I was touching modules but somehow I stopped where I stopped.
The question is just, is it worth doing it or is it reasonable to expect
that it is done correctly.

Sebastian

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

[Petr Pavlu]

On 7/8/26 9:49 AM, Sebastian Andrzej Siewior wrote:
> On 2026-07-07 17:32:10 [+0200], Petr Pavlu wrote:
>>> --- a/kernel/module/main.c
>>> +++ b/kernel/module/main.c
>>> @@ -858,6 +858,9 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>>>  		goto out;
>>>  
>>>  	mutex_unlock(&module_mutex);
>>> +
>>> +	/* Ensure all rcu callbacks issued by the module have completed */
>>> +	rcu_barrier();
>>>  	/* Final destruction now no one is using it. */
>>>  	if (mod->exit != NULL)
>>>  		mod->exit();
>>>
>>> make sense?
>>
>> This is discussed in Documentation/RCU/rcubarrier.rst and
>> Documentation/RCU/Design/Requirements/Requirements.rst. The latter
>> contains:
> 
> I am aware of this. It is just not the first time I stumble about this.
> But maybe with the AI review these days there won't be a miss.
> 
>> I don't know if the last part about unacceptable latencies is still
>> relevant. I haven't done any measurements myself.
> 
> There is a synchronize_rcu() later on. I think I could replace it with a
> call_rcu() so we might end up even. I was thinking about about it last
> time I was touching modules but somehow I stopped where I stopped.
> The question is just, is it worth doing it or is it reasonable to expect
> that it is done correctly.

As RCU usage in modules is now more common, I see an argument for the
module loader to invoke rcu_barrier() during module unload to make RCU
usage easier. In general, module unloading is a rare operation, so even
if it becomes somewhat slower, I don't expect it to be a significant
issue.

One problem is that I'm not sure where the new rcu_barrier() call should
be placed. The prototype adds it before calling the module's exit
function. Would this actually fit all modules? From a quick look, I can
see that various modules call it at different points during their exit.

-- 
Thanks,
Petr

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

[Sebastian Andrzej Siewior]

On 2026-07-08 15:04:32 [+0200], Petr Pavlu wrote:
> As RCU usage in modules is now more common, I see an argument for the
> module loader to invoke rcu_barrier() during module unload to make RCU
> usage easier. In general, module unloading is a rare operation, so even
> if it becomes somewhat slower, I don't expect it to be a significant
> issue.

Okay.

> One problem is that I'm not sure where the new rcu_barrier() call should
> be placed. The prototype adds it before calling the module's exit
> function. Would this actually fit all modules? From a quick look, I can
> see that various modules call it at different points during their exit.

I don't know why you would use call_rcu() in your module_exit()
(pointing to the same module). But you could have call_rcu() invoking
kmem_cache_free() and destroying that cache (kmem_cache_destroy()) in
your exit path. From that perspective it would make sense to flush all
calls before invoking module_exit().

> -- 
> Thanks,
> Petr

Sebastian

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

[Sebastian Andrzej Siewior]

-*, +module +Paul.

On 2026-07-08 15:56:53 [+0200], To Petr Pavlu wrote:
> > One problem is that I'm not sure where the new rcu_barrier() call should
> > be placed. The prototype adds it before calling the module's exit
> > function. Would this actually fit all modules? From a quick look, I can
> > see that various modules call it at different points during their exit.
> 
> I don't know why you would use call_rcu() in your module_exit()
> (pointing to the same module). But you could have call_rcu() invoking
> kmem_cache_free() and destroying that cache (kmem_cache_destroy()) in
> your exit path. From that perspective it would make sense to flush all
> calls before invoking module_exit().

Paul, are the RCU callbacks always invoked in FIFO order?
If we put the module unmap into a call_rcu() (instead of the current
synchronize_rcu()) would we invoke the callback's of the module's
callback before the unmap of the module? Or is this not guaranteed due
callbacks on CPU0 vs CPU1 are executed in different order?

Because if the FIFO order is guaranteed then it would be cheapest
solution. But it would require a rcu_barrier() in kmem_cache_destroy()).

> > -- 
> > Thanks,
> > Petr

Sebastian

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

[Paul E. McKenney]

On Wed, Jul 08, 2026 at 09:00:03PM +0200, Sebastian Andrzej Siewior wrote:
> -*, +module +Paul.
> 
> On 2026-07-08 15:56:53 [+0200], To Petr Pavlu wrote:
> > > One problem is that I'm not sure where the new rcu_barrier() call should
> > > be placed. The prototype adds it before calling the module's exit
> > > function. Would this actually fit all modules? From a quick look, I can
> > > see that various modules call it at different points during their exit.
> > 
> > I don't know why you would use call_rcu() in your module_exit()
> > (pointing to the same module). But you could have call_rcu() invoking
> > kmem_cache_free() and destroying that cache (kmem_cache_destroy()) in
> > your exit path. From that perspective it would make sense to flush all
> > calls before invoking module_exit().
> 
> Paul, are the RCU callbacks always invoked in FIFO order?
> If we put the module unmap into a call_rcu() (instead of the current
> synchronize_rcu()) would we invoke the callback's of the module's
> callback before the unmap of the module? Or is this not guaranteed due
> callbacks on CPU0 vs CPU1 are executed in different order?

The RCU-callback ordering guarantees are quite weak:

o	If callback A executes call_rcu() that queues callback B, then
	A will be invoked before B.  (Just in case anyone had any doubt.)

o	The callback queued prior to a call to rcu_barrier() will be
	invoked before any that are queued after return from that same
	call to rcu_barrier().

The current *implementation* orders callbacks queued on a given CPU
(but please see below), but there are absolutely no ordering guarantees
among CPUs.  To see why, consider the following sequence of events:

o	CPU 0 does an "rm -rf" of a large file tree containing huge
	numbers of small files.  This results in an RCU callback being
	queued for each file.

o	CPU 0 queues RCU callback A.

o	The corresponding grace period completes, so CPU 0 starts
	invoking callbacks.

o	CPU 1 queues RCU callback B.

o	The corresponding grace period completes, so CPU 1 invokes
	callback B.

o	Meanwhile, CPU 0 is still working off its RCU callback backlog.

o	CPU 0 eventually invokes RCU callback A

Worse yet, if we here in RCU-land ever allow a given CPU's callbacks
to be offloaded or deoffloaded while that CPU is online, then there
really will be cases where callbacks queued even by a single given CPU
get invoked out of order.  And there are people who would dearly love
us to make this happen.  (The last time we tried, we were inundated in
odd race conditions, but maybe we will come up with a better way.)

So these RCU callback ordering guarantees are probably too weak for your
use case.

> Because if the FIFO order is guaranteed then it would be cheapest
> solution. But it would require a rcu_barrier() in kmem_cache_destroy()).

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?

							Thanx, Paul