Re: [PATCH net-next 2/3] ppp: unify two channel structs

Re: [PATCH net-next 2/3] ppp: unify two channel structs

[Paolo Abeni]

On 4/30/26 11:05 AM, Qingfang Deng wrote:
> Historically, PPP maintained two separate structures for a channel:
> 'struct channel' was internal to ppp_generic.c, while 'struct ppp_channel'
> was the public interface that drivers were required to embed. This
> duplication was redundant and forced drivers to manage the lifecycle of
> the public structure.
> 
> Unify these two structures into a single 'struct ppp_channel', which is
> now internal to ppp_generic.c. Drivers now use a 'ppp_channel_conf'
> structure to specify registration parameters and receive an opaque
> pointer to the allocated channel.
> 
> Key changes:
> - ppp_register_channel() and ppp_register_net_channel() now return
>   a 'struct ppp_channel *' instead of taking a pointer to a driver-
>   embedded structure.
> - 'struct ppp_channel_ops' methods now take the driver's 'private'
>   pointer directly as their first argument, simplifying driver logic.
> - ppp_unregister_channel() now takes the opaque pointer.
> - Multilink-specific fields are unified and handled via the new
>   configuration structure.
> 
> This cleanup simplifies the driver interface and makes the channel
> lifecycle management more robust by centralizing allocation in the PPP
> generic layer.
> 
> Assisted-by: Gemini:gemini-3-flash
> Signed-off-by: Qingfang Deng <qingfang.deng@linux.dev>
> ---
>  drivers/net/ppp/ppp_async.c      |  51 +++++-----
>  drivers/net/ppp/ppp_generic.c    | 161 +++++++++++++++----------------
>  drivers/net/ppp/ppp_synctty.c    |  51 +++++-----
>  drivers/net/ppp/pppoe.c          |  34 ++++---
>  drivers/net/ppp/pppox.c          |   4 +-
>  drivers/net/ppp/pptp.c           |  40 ++++----
>  drivers/tty/ipwireless/network.c |  30 +++---
>  include/linux/if_pppox.h         |   2 +-
>  include/linux/ppp_channel.h      |  49 ++++++----
>  net/atm/pppoatm.c                |  61 ++++++------
>  net/l2tp/l2tp_ppp.c              |  34 ++++---
>  11 files changed, 271 insertions(+), 246 deletions(-)

This patch is IMHO a bit too big and should be split. Also this kind of
refactor looks very invasive and potentially regression prone. I think
it should include a signficant self-test coverage increase.

> @@ -391,9 +396,9 @@ ppp_async_init(void)
>   * The following routines provide the PPP channel interface.
>   */
>  static int
> -ppp_async_ioctl(struct ppp_channel *chan, unsigned int cmd, unsigned long arg)
> +ppp_async_ioctl(void *private, unsigned int cmd, unsigned long arg)
>  {
> -	struct asyncppp *ap = chan->private;
> +	struct asyncppp *ap = private;
>  	void __user *argp = (void __user *)arg;
>  	int __user *p = argp;
>  	int err, val;

Minor nit: reverse christmas tree above

> @@ -2985,16 +2983,13 @@ char *ppp_dev_name(struct ppp_channel *chan)
>   * This must be called in process context.
>   */
>  void
> -ppp_unregister_channel(struct ppp_channel *chan)
> +ppp_unregister_channel(struct ppp_channel *pch)
>  {
> -	struct channel *pch = chan->ppp;
>  	struct ppp_net *pn;
>  
>  	if (!pch)
>  		return;		/* should never happen */
>  
> -	chan->ppp = NULL;
> -

Sashiko says:

Could this specific ordering introduce a race condition that might lead
to a use-after-free?
If userspace has a file descriptor attached to this channel, it can
concurrently invoke the PPPIOCCONNECT ioctl. Because
ppp_disconnect_channel()
clears pch->ppp and removes the channel from its current unit before
pch->file.dead is set to 1, the concurrent ioctl could observe
pch->file.dead == 0.
This would allow ppp_connect_channel() to successfully attach the dying
channel to a new PPP unit. Once ppp_unregister_channel() completes and
frees the channel, the new PPP unit would retain a pointer to the freed
memory in its channels list, which might be accessed during a later
packet transmission via ppp_push().

> @@ -215,7 +210,8 @@ static void pppoatm_push(struct atm_vcc *atmvcc, struct sk_buff *skb)
>  		    !memcmp(skb->data, &pppllc[LLC_LEN],
>  		    sizeof(pppllc) - LLC_LEN)) {
>  			pvcc->encaps = e_vc;
> -			pvcc->chan.mtu += LLC_LEN;
> +			ppp_channel_update_mtu(pvcc->chan,
> +					       atmvcc->qos.txtp.max_sdu - PPP_HDRLEN);

Does the above introduce a functional change? At very least some comment
needed. Also possibly better move the update_mtu wrapper to a pre-req patch.

> @@ -221,7 +221,7 @@ static void pppol2tp_recv(struct l2tp_session *session, struct sk_buff *skb, int
>  		struct pppox_sock *po;
>  
>  		po = pppox_sk(sk);
> -		ppp_input(&po->chan, skb);
> +		ppp_input(po->chan, skb);

Sashiko says:

Does decoupling the channel lifetime from the driver structure introduce
a use-after-free when receiving packets?
Previously, the struct ppp_channel was embedded in struct pppox_sock,
meaning its lifecycle was safely tied to the socket's refcount. Now that
po->chan is dynamically allocated and freed during pppox_unbind_sock()
via ppp_unregister_channel(), it seems possible for pppol2tp_recv() to
access freed memory.
Since pppol2tp_recv() runs locklessly in softirq context while holding
only rcu_read_lock() and a socket reference, can it observe the
PPPOX_BOUND state and dereference po->chan just after it was freed on
another CPU?

/P

Re: [PATCH net-next 2/3] ppp: unify two channel structs

[Qingfang Deng]

On 2026/5/5 19:16, Paolo Abeni wrote:
> On 4/30/26 11:05 AM, Qingfang Deng wrote:
>> Historically, PPP maintained two separate structures for a channel:
>> 'struct channel' was internal to ppp_generic.c, while 'struct ppp_channel'
>> was the public interface that drivers were required to embed. This
>> duplication was redundant and forced drivers to manage the lifecycle of
>> the public structure.
>>
>> Unify these two structures into a single 'struct ppp_channel', which is
>> now internal to ppp_generic.c. Drivers now use a 'ppp_channel_conf'
>> structure to specify registration parameters and receive an opaque
>> pointer to the allocated channel.
>>
>> Key changes:
>> - ppp_register_channel() and ppp_register_net_channel() now return
>>    a 'struct ppp_channel *' instead of taking a pointer to a driver-
>>    embedded structure.
>> - 'struct ppp_channel_ops' methods now take the driver's 'private'
>>    pointer directly as their first argument, simplifying driver logic.
>> - ppp_unregister_channel() now takes the opaque pointer.
>> - Multilink-specific fields are unified and handled via the new
>>    configuration structure.
>>
>> This cleanup simplifies the driver interface and makes the channel
>> lifecycle management more robust by centralizing allocation in the PPP
>> generic layer.
>>
>> Assisted-by: Gemini:gemini-3-flash
>> Signed-off-by: Qingfang Deng <qingfang.deng@linux.dev>
>> ---
>>   drivers/net/ppp/ppp_async.c      |  51 +++++-----
>>   drivers/net/ppp/ppp_generic.c    | 161 +++++++++++++++----------------
>>   drivers/net/ppp/ppp_synctty.c    |  51 +++++-----
>>   drivers/net/ppp/pppoe.c          |  34 ++++---
>>   drivers/net/ppp/pppox.c          |   4 +-
>>   drivers/net/ppp/pptp.c           |  40 ++++----
>>   drivers/tty/ipwireless/network.c |  30 +++---
>>   include/linux/if_pppox.h         |   2 +-
>>   include/linux/ppp_channel.h      |  49 ++++++----
>>   net/atm/pppoatm.c                |  61 ++++++------
>>   net/l2tp/l2tp_ppp.c              |  34 ++++---
>>   11 files changed, 271 insertions(+), 246 deletions(-)
> This patch is IMHO a bit too big and should be split. Also this kind of
> refactor looks very invasive and potentially regression prone. I think
> it should include a signficant self-test coverage increase.
This is indeed too big. But how do I split it without breaking the build?

Re: [PATCH net-next 2/3] ppp: unify two channel structs

[Paolo Abeni]

On 5/7/26 7:53 AM, Qingfang Deng wrote:
> On 2026/5/5 19:16, Paolo Abeni wrote:
>> On 4/30/26 11:05 AM, Qingfang Deng wrote:
>>> Historically, PPP maintained two separate structures for a channel:
>>> 'struct channel' was internal to ppp_generic.c, while 'struct ppp_channel'
>>> was the public interface that drivers were required to embed. This
>>> duplication was redundant and forced drivers to manage the lifecycle of
>>> the public structure.
>>>
>>> Unify these two structures into a single 'struct ppp_channel', which is
>>> now internal to ppp_generic.c. Drivers now use a 'ppp_channel_conf'
>>> structure to specify registration parameters and receive an opaque
>>> pointer to the allocated channel.
>>>
>>> Key changes:
>>> - ppp_register_channel() and ppp_register_net_channel() now return
>>>    a 'struct ppp_channel *' instead of taking a pointer to a driver-
>>>    embedded structure.
>>> - 'struct ppp_channel_ops' methods now take the driver's 'private'
>>>    pointer directly as their first argument, simplifying driver logic.
>>> - ppp_unregister_channel() now takes the opaque pointer.
>>> - Multilink-specific fields are unified and handled via the new
>>>    configuration structure.
>>>
>>> This cleanup simplifies the driver interface and makes the channel
>>> lifecycle management more robust by centralizing allocation in the PPP
>>> generic layer.
>>>
>>> Assisted-by: Gemini:gemini-3-flash
>>> Signed-off-by: Qingfang Deng <qingfang.deng@linux.dev>
>>> ---
>>>   drivers/net/ppp/ppp_async.c      |  51 +++++-----
>>>   drivers/net/ppp/ppp_generic.c    | 161 +++++++++++++++----------------
>>>   drivers/net/ppp/ppp_synctty.c    |  51 +++++-----
>>>   drivers/net/ppp/pppoe.c          |  34 ++++---
>>>   drivers/net/ppp/pppox.c          |   4 +-
>>>   drivers/net/ppp/pptp.c           |  40 ++++----
>>>   drivers/tty/ipwireless/network.c |  30 +++---
>>>   include/linux/if_pppox.h         |   2 +-
>>>   include/linux/ppp_channel.h      |  49 ++++++----
>>>   net/atm/pppoatm.c                |  61 ++++++------
>>>   net/l2tp/l2tp_ppp.c              |  34 ++++---
>>>   11 files changed, 271 insertions(+), 246 deletions(-)
>> This patch is IMHO a bit too big and should be split. Also this kind of
>> refactor looks very invasive and potentially regression prone. I think
>> it should include a signficant self-test coverage increase.
> This is indeed too big. But how do I split it without breaking the build?

This is indeed a good question, but I'm really unable to give you a good
answer without allocating to this topic much more time than I have
available.

I think that the (indeed smallish) mtu changes could easily go in a
separate patch.

You could try introducing the struct and/or variables renaming
separately, with no actual functional change, i.e.

- one patch to rename ppp_channel -> ppp_channel_conf
- one patch to rename channel -> ppp_channel
(possibly adjust accordingly the variables name if can done mechanically)

no idea if the end result would be more palatable, but possibly worth a try.

/P

Re: [PATCH net-next 2/3] ppp: unify two channel structs

[Sebastian Andrzej Siewior]

On 2026-05-07 13:53:30 [+0800], Qingfang Deng wrote:
> > This patch is IMHO a bit too big and should be split. Also this kind of
> > refactor looks very invasive and potentially regression prone. I think
> > it should include a signficant self-test coverage increase.
> This is indeed too big. But how do I split it without breaking the build?

The current ppp tests would yell if you accidentally broke something?

Sebastian

Re: [PATCH net-next 2/3] ppp: unify two channel structs

[Qingfang Deng]

On 2026/5/7 15:40, Sebastian Andrzej Siewior wrote:
> On 2026-05-07 13:53:30 [+0800], Qingfang Deng wrote:
>>> This patch is IMHO a bit too big and should be split. Also this kind of
>>> refactor looks very invasive and potentially regression prone. I think
>>> it should include a signficant self-test coverage increase.
>> This is indeed too big. But how do I split it without breaking the build?
> The current ppp tests would yell if you accidentally broke something?
By "breaking the build" I meant compile-time errors (due to API changes).

Re: [PATCH net-next 2/3] ppp: unify two channel structs

[Sebastian Andrzej Siewior]

On 2026-05-07 16:33:36 [+0800], Qingfang Deng wrote:
> On 2026/5/7 15:40, Sebastian Andrzej Siewior wrote:
> > On 2026-05-07 13:53:30 [+0800], Qingfang Deng wrote:
> > > > This patch is IMHO a bit too big and should be split. Also this kind of
> > > > refactor looks very invasive and potentially regression prone. I think
> > > > it should include a signficant self-test coverage increase.
> > > This is indeed too big. But how do I split it without breaking the build?
> > The current ppp tests would yell if you accidentally broke something?
> By "breaking the build" I meant compile-time errors (due to API changes).

If this change would flip the logic somewhere and as such break ppp at
runtime.
Would the existing test suite be able to catch it?

Sebastian

Re: [PATCH net-next 2/3] ppp: unify two channel structs

[Qingfang Deng]

On 2026/5/7 16:46, Sebastian Andrzej Siewior wrote:
> On 2026-05-07 16:33:36 [+0800], Qingfang Deng wrote:
>> On 2026/5/7 15:40, Sebastian Andrzej Siewior wrote:
>>> On 2026-05-07 13:53:30 [+0800], Qingfang Deng wrote:
>>>>> This patch is IMHO a bit too big and should be split. Also this kind of
>>>>> refactor looks very invasive and potentially regression prone. I think
>>>>> it should include a signficant self-test coverage increase.
>>>> This is indeed too big. But how do I split it without breaking the build?
>>> The current ppp tests would yell if you accidentally broke something?
>> By "breaking the build" I meant compile-time errors (due to API changes).
> If this change would flip the logic somewhere and as such break ppp at
> runtime.
> Would the existing test suite be able to catch it?


The current self-test only covers PPP async and PPPoE, and that's why 
Paolo suggests more self-tests.