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

[Paolo Abeni <pabeni@redhat.com>]

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