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