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

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

[Norbert Szetei]

pppol2tp_recv() runs in the L2TP UDP-encap softirq RX path:

 l2tp_udp_encap_recv() -> l2tp_recv_common() -> pppol2tp_recv()
   -> ppp_input(&po->chan)

It runs under rcu_read_lock() holding only an l2tp_session reference and
takes NO reference on the internal PPP channel (struct channel,
chan->ppp) that ppp_input() dereferences.

The pppox socket is SOCK_RCU_FREE, so 'po' and the embedded ppp_channel
are RCU-safe.  But the internal struct channel is a separate allocation
that ppp_release_channel() frees with a plain kfree():

 close(data socket) -> pppol2tp_release() -> pppox_unbind_sock()
   -> ppp_unregister_channel() -> ppp_release_channel() -> kfree(pch)

For a channel that is bound (PPPIOCGCHAN) but not attached to a ppp unit
(no PPPIOCCONNECT, pch->ppp == NULL) and not bridged, teardown skips
both ppp_disconnect_channel()'s synchronize_net() and
ppp_unbridge_channels()'s synchronize_rcu(), so the kfree() has no grace
period.  rcu_read_lock() in pppol2tp_recv() does not protect against a
plain kfree(), so an in-flight ppp_input() on one CPU can dereference
the channel just freed by close() on another CPU.

The bug is reachable by an unprivileged user.

Defer the channel free to an RCU callback via call_rcu() so the grace
period fences any in-flight ppp_input(). The disconnect and unbridge
teardown paths already fence with synchronize_net()/synchronize_rcu();
call_rcu() does the same here without stalling the close() path.

Fixes: ee40fb2e1eb5 ("l2tp: protect sock pointer of struct pppol2tp_session with RCU")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Norbert Szetei <norbert@doyensec.com>
---
v2:
- Moved skb_queue_purge() to a dedicated RCU callback to prevent leaking
  skbs added by an in-flight ppp_input() during the grace period (Sebastian).
- Retained call_rcu() to avoid introducing synchronous multi-millisecond
  latency into the teardown path.
v1: https://lore.kernel.org/netdev/C954A7EA-AA98-4E3C-80B5-42C34B3183A3@doyensec.com/

 drivers/net/ppp/ppp_generic.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 57c68efa5ff8..2d57de77780f 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -184,6 +184,7 @@ struct channel {
 	struct list_head clist;		/* link in list of channels per unit */
 	spinlock_t	upl;		/* protects `ppp' and 'bridge' */
 	struct channel __rcu *bridge;	/* "bridged" ppp channel */
+	struct rcu_head rcu;		/* for RCU-deferred free of the channel */
 #ifdef CONFIG_PPP_MULTILINK
 	u8		avail;		/* flag used in multilink stuff */
 	u8		had_frag;	/* >= 1 fragments have been sent */
@@ -3562,6 +3563,18 @@ ppp_disconnect_channel(struct channel *pch)
 	return err;
 }
 
+/* Purge after the grace period: a late ppp_input() may still queue an
+ * skb on pch->file.rq before the last RCU reader drains.
+ */
+static void ppp_release_channel_free(struct rcu_head *rcu)
+{
+	struct channel *pch = container_of(rcu, struct channel, rcu);
+
+	skb_queue_purge(&pch->file.xq);
+	skb_queue_purge(&pch->file.rq);
+	kfree(pch);
+}
+
 /*
  * Drop a reference to a ppp channel and free its memory if the refcount reaches
  * zero.
@@ -3581,9 +3594,7 @@ static void ppp_release_channel(struct channel *pch)
 		pr_err("ppp: destroying undead channel %p !\n", pch);
 		return;
 	}
-	skb_queue_purge(&pch->file.xq);
-	skb_queue_purge(&pch->file.rq);
-	kfree(pch);
+	call_rcu(&pch->rcu, ppp_release_channel_free);
 }
 
 static void __exit ppp_cleanup(void)
-- 
2.54.0

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

[Qingfang Deng]

Add: Guillaume

On 2026/7/2 at 2:12, Norbert Szetei wrote:
> pppol2tp_recv() runs in the L2TP UDP-encap softirq RX path:
>
>   l2tp_udp_encap_recv() -> l2tp_recv_common() -> pppol2tp_recv()
>     -> ppp_input(&po->chan)
>
> It runs under rcu_read_lock() holding only an l2tp_session reference and
> takes NO reference on the internal PPP channel (struct channel,
> chan->ppp) that ppp_input() dereferences.
>
> The pppox socket is SOCK_RCU_FREE, so 'po' and the embedded ppp_channel
> are RCU-safe.  But the internal struct channel is a separate allocation
> that ppp_release_channel() frees with a plain kfree():
>
>   close(data socket) -> pppol2tp_release() -> pppox_unbind_sock()
>     -> ppp_unregister_channel() -> ppp_release_channel() -> kfree(pch)
>
> For a channel that is bound (PPPIOCGCHAN) but not attached to a ppp unit
> (no PPPIOCCONNECT, pch->ppp == NULL) and not bridged, teardown skips
> both ppp_disconnect_channel()'s synchronize_net() and
> ppp_unbridge_channels()'s synchronize_rcu(), so the kfree() has no grace
> period.  rcu_read_lock() in pppol2tp_recv() does not protect against a
> plain kfree(), so an in-flight ppp_input() on one CPU can dereference
> the channel just freed by close() on another CPU.
>
> The bug is reachable by an unprivileged user.
>
> Defer the channel free to an RCU callback via call_rcu() so the grace
> period fences any in-flight ppp_input(). The disconnect and unbridge
> teardown paths already fence with synchronize_net()/synchronize_rcu();
> call_rcu() does the same here without stalling the close() path.
>
> Fixes: ee40fb2e1eb5 ("l2tp: protect sock pointer of struct pppol2tp_session with RCU")
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Norbert Szetei <norbert@doyensec.com>
> ---
> v2:
> - Moved skb_queue_purge() to a dedicated RCU callback to prevent leaking
>    skbs added by an in-flight ppp_input() during the grace period (Sebastian).
> - Retained call_rcu() to avoid introducing synchronous multi-millisecond
>    latency into the teardown path.
> v1: https://lore.kernel.org/netdev/C954A7EA-AA98-4E3C-80B5-42C34B3183A3@doyensec.com/
>
>   drivers/net/ppp/ppp_generic.c | 17 ++++++++++++++---
>   1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 57c68efa5ff8..2d57de77780f 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -184,6 +184,7 @@ struct channel {
>   	struct list_head clist;		/* link in list of channels per unit */
>   	spinlock_t	upl;		/* protects `ppp' and 'bridge' */
>   	struct channel __rcu *bridge;	/* "bridged" ppp channel */
> +	struct rcu_head rcu;		/* for RCU-deferred free of the channel */
>   #ifdef CONFIG_PPP_MULTILINK
>   	u8		avail;		/* flag used in multilink stuff */
>   	u8		had_frag;	/* >= 1 fragments have been sent */
> @@ -3562,6 +3563,18 @@ ppp_disconnect_channel(struct channel *pch)
>   	return err;
>   }
>   
> +/* Purge after the grace period: a late ppp_input() may still queue an
> + * skb on pch->file.rq before the last RCU reader drains.
> + */
> +static void ppp_release_channel_free(struct rcu_head *rcu)
> +{
> +	struct channel *pch = container_of(rcu, struct channel, rcu);
> +
> +	skb_queue_purge(&pch->file.xq);
> +	skb_queue_purge(&pch->file.rq);
> +	kfree(pch);
> +}
> +
>   /*
>    * Drop a reference to a ppp channel and free its memory if the refcount reaches
>    * zero.
> @@ -3581,9 +3594,7 @@ static void ppp_release_channel(struct channel *pch)
>   		pr_err("ppp: destroying undead channel %p !\n", pch);
>   		return;
>   	}
> -	skb_queue_purge(&pch->file.xq);
> -	skb_queue_purge(&pch->file.rq);
> -	kfree(pch);
> +	call_rcu(&pch->rcu, ppp_release_channel_free);
>   }
>   
>   static void __exit ppp_cleanup(void)

Reviewed-by: Qingfang Deng <qingfang.deng@linux.dev>

FYI, I attempted to merge the two channel structs and AI-review found a 
UAF [1], so this patch addresses the issue.

[1] 
https://lore.kernel.org/netdev/590d7931-02b0-45d6-8f43-ef909c9bde89@redhat.com/

Best regards,

Qingfang

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

[Guillaume Nault]

On Thu, Jul 02, 2026 at 04:19:02PM +0800, Qingfang Deng wrote:
> Add: Guillaume
> 
> On 2026/7/2 at 2:12, Norbert Szetei wrote:
> > pppol2tp_recv() runs in the L2TP UDP-encap softirq RX path:
> > 
> >   l2tp_udp_encap_recv() -> l2tp_recv_common() -> pppol2tp_recv()
> >     -> ppp_input(&po->chan)

Hi Qingfang,

Thanks for Cc-ing me. I haven't had time to look at this problem yet,
and I'll be offline next week. So not sure if I'll get the possibility
to provide any feedback to this patch in time.

> > It runs under rcu_read_lock() holding only an l2tp_session reference and
> > takes NO reference on the internal PPP channel (struct channel,
> > chan->ppp) that ppp_input() dereferences.
> > 
> > The pppox socket is SOCK_RCU_FREE, so 'po' and the embedded ppp_channel
> > are RCU-safe.  But the internal struct channel is a separate allocation
> > that ppp_release_channel() frees with a plain kfree():
> > 
> >   close(data socket) -> pppol2tp_release() -> pppox_unbind_sock()
> >     -> ppp_unregister_channel() -> ppp_release_channel() -> kfree(pch)
> > 
> > For a channel that is bound (PPPIOCGCHAN) but not attached to a ppp unit
> > (no PPPIOCCONNECT, pch->ppp == NULL) and not bridged, teardown skips
> > both ppp_disconnect_channel()'s synchronize_net() and
> > ppp_unbridge_channels()'s synchronize_rcu(), so the kfree() has no grace
> > period.  rcu_read_lock() in pppol2tp_recv() does not protect against a
> > plain kfree(), so an in-flight ppp_input() on one CPU can dereference
> > the channel just freed by close() on another CPU.
> > 
> > The bug is reachable by an unprivileged user.
> > 
> > Defer the channel free to an RCU callback via call_rcu() so the grace
> > period fences any in-flight ppp_input(). The disconnect and unbridge
> > teardown paths already fence with synchronize_net()/synchronize_rcu();
> > call_rcu() does the same here without stalling the close() path.
> > 
> > Fixes: ee40fb2e1eb5 ("l2tp: protect sock pointer of struct pppol2tp_session with RCU")
> > Assisted-by: Claude:claude-opus-4-8
> > Signed-off-by: Norbert Szetei <norbert@doyensec.com>
> > ---
> > v2:
> > - Moved skb_queue_purge() to a dedicated RCU callback to prevent leaking
> >    skbs added by an in-flight ppp_input() during the grace period (Sebastian).
> > - Retained call_rcu() to avoid introducing synchronous multi-millisecond
> >    latency into the teardown path.
> > v1: https://lore.kernel.org/netdev/C954A7EA-AA98-4E3C-80B5-42C34B3183A3@doyensec.com/
> > 
> >   drivers/net/ppp/ppp_generic.c | 17 ++++++++++++++---
> >   1 file changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> > index 57c68efa5ff8..2d57de77780f 100644
> > --- a/drivers/net/ppp/ppp_generic.c
> > +++ b/drivers/net/ppp/ppp_generic.c
> > @@ -184,6 +184,7 @@ struct channel {
> >   	struct list_head clist;		/* link in list of channels per unit */
> >   	spinlock_t	upl;		/* protects `ppp' and 'bridge' */
> >   	struct channel __rcu *bridge;	/* "bridged" ppp channel */
> > +	struct rcu_head rcu;		/* for RCU-deferred free of the channel */
> >   #ifdef CONFIG_PPP_MULTILINK
> >   	u8		avail;		/* flag used in multilink stuff */
> >   	u8		had_frag;	/* >= 1 fragments have been sent */
> > @@ -3562,6 +3563,18 @@ ppp_disconnect_channel(struct channel *pch)
> >   	return err;
> >   }
> > +/* Purge after the grace period: a late ppp_input() may still queue an
> > + * skb on pch->file.rq before the last RCU reader drains.
> > + */
> > +static void ppp_release_channel_free(struct rcu_head *rcu)
> > +{
> > +	struct channel *pch = container_of(rcu, struct channel, rcu);
> > +
> > +	skb_queue_purge(&pch->file.xq);
> > +	skb_queue_purge(&pch->file.rq);
> > +	kfree(pch);
> > +}
> > +
> >   /*
> >    * Drop a reference to a ppp channel and free its memory if the refcount reaches
> >    * zero.
> > @@ -3581,9 +3594,7 @@ static void ppp_release_channel(struct channel *pch)
> >   		pr_err("ppp: destroying undead channel %p !\n", pch);
> >   		return;
> >   	}
> > -	skb_queue_purge(&pch->file.xq);
> > -	skb_queue_purge(&pch->file.rq);
> > -	kfree(pch);
> > +	call_rcu(&pch->rcu, ppp_release_channel_free);
> >   }
> >   static void __exit ppp_cleanup(void)
> 
> Reviewed-by: Qingfang Deng <qingfang.deng@linux.dev>
> 
> FYI, I attempted to merge the two channel structs and AI-review found a UAF
> [1], so this patch addresses the issue.
> 
> [1] https://lore.kernel.org/netdev/590d7931-02b0-45d6-8f43-ef909c9bde89@redhat.com/
> 
> Best regards,
> 
> Qingfang
> 
> 

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

[Qingfang Deng]

Hi,

On 2026/7/2 2:12, Norbert Szetei wrote:
> +/* Purge after the grace period: a late ppp_input() may still queue an
> + * skb on pch->file.rq before the last RCU reader drains.
> + */
> +static void ppp_release_channel_free(struct rcu_head *rcu)
> +{
> +	struct channel *pch = container_of(rcu, struct channel, rcu);
> +
> +	skb_queue_purge(&pch->file.xq);
> +	skb_queue_purge(&pch->file.rq);
> +	kfree(pch);
> +}
> +
>   /*
>    * Drop a reference to a ppp channel and free its memory if the refcount reaches
>    * zero.
> @@ -3581,9 +3594,7 @@ static void ppp_release_channel(struct channel *pch)
>   		pr_err("ppp: destroying undead channel %p !\n", pch);
>   		return;
>   	}
> -	skb_queue_purge(&pch->file.xq);
> -	skb_queue_purge(&pch->file.rq);
> -	kfree(pch);
> +	call_rcu(&pch->rcu, ppp_release_channel_free);
>   }
>   
>   static void __exit ppp_cleanup(void)

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().

Regards,

Qingfang

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

[Breno Leitao]

On Fri, Jul 03, 2026 at 03:27:00PM +0800, Qingfang Deng wrote:
> Hi,
> 
> On 2026/7/2 2:12, Norbert Szetei wrote:
> > +/* Purge after the grace period: a late ppp_input() may still queue an
> > + * skb on pch->file.rq before the last RCU reader drains.
> > + */
> > +static void ppp_release_channel_free(struct rcu_head *rcu)
> > +{
> > +	struct channel *pch = container_of(rcu, struct channel, rcu);
> > +
> > +	skb_queue_purge(&pch->file.xq);
> > +	skb_queue_purge(&pch->file.rq);
> > +	kfree(pch);
> > +}
> > +
> >   /*
> >    * Drop a reference to a ppp channel and free its memory if the refcount reaches
> >    * zero.
> > @@ -3581,9 +3594,7 @@ static void ppp_release_channel(struct channel *pch)
> >   		pr_err("ppp: destroying undead channel %p !\n", pch);
> >   		return;
> >   	}
> > -	skb_queue_purge(&pch->file.xq);
> > -	skb_queue_purge(&pch->file.rq);
> > -	kfree(pch);
> > +	call_rcu(&pch->rcu, ppp_release_channel_free);
> >   }
> >   static void __exit ppp_cleanup(void)
> 
> 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?

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

[Qingfang Deng]

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.

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

[Norbert Szetei]

Hi,

> On Jul 3, 2026, at 09:27, Qingfang Deng <qingfang.deng@linux.dev> wrote:
> 
> Hi,
> 
> On 2026/7/2 2:12, Norbert Szetei wrote:
>> +/* Purge after the grace period: a late ppp_input() may still queue an
>> + * skb on pch->file.rq before the last RCU reader drains.
>> + */
>> +static void ppp_release_channel_free(struct rcu_head *rcu)
>> +{
>> + struct channel *pch = container_of(rcu, struct channel, rcu);
>> +
>> + skb_queue_purge(&pch->file.xq);
>> + skb_queue_purge(&pch->file.rq);
>> + kfree(pch);
>> +}
>> +
>>  /*
>>   * Drop a reference to a ppp channel and free its memory if the refcount reaches
>>   * zero.
>> @@ -3581,9 +3594,7 @@ static void ppp_release_channel(struct channel *pch)
>>   pr_err("ppp: destroying undead channel %p !\n", pch);
>>   return;
>>   }
>> - skb_queue_purge(&pch->file.xq);
>> - skb_queue_purge(&pch->file.rq);
>> - kfree(pch);
>> + call_rcu(&pch->rcu, ppp_release_channel_free);
>>  }
>>    static void __exit ppp_cleanup(void)
> 
> 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().


Thanks for reviewing. I'll add it and send out a v3.

N.

> 
> Regards,
> 
> Qingfang
>