Re: [PATCH v1] pppoe: Add PPP interface name to /proc/net/pppoe output

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/#/patchset/20260728070636.3462165-1-pavan.m@xxxxxxxxxxxxxxxx?part=1
---
--- Patch [1]: [PATCH v1] pppoe: Add PPP interface name to /proc/net/pppoe output ---
commit fe1f42402d1a0992d1fdaf152ef4812c313dc548
Author: Pavan Kumar <pavan.m@xxxxxxxxxxxxxxxx>
pppoe: Add PPP interface name to /proc/net/pppoe output

This patch modifies the pppoe_seq_show() function to include the associated
PPP interface name in the /proc/net/pppoe output. It retrieves the interface
name using ppp_dev_name() and appends it as a new "PPP-Device" column to 
help correlate PPPoE sessions with their pppX interfaces.

> diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> index 6874a1a8edaf1..cb31f5db43e81 100644
> --- a/drivers/net/ppp/pppoe.c
> +++ b/drivers/net/ppp/pppoe.c
> @@ -963,17 +963,19 @@ static int pppoe_seq_show(struct seq_file *seq, void *v)
>  {
>  	struct pppox_sock *po;
>  	char *dev_name;
> +	char *ppp_name;
>  
>  	if (v == SEQ_START_TOKEN) {
> -		seq_puts(seq, "Id       Address              Device\n");
> +		seq_puts(seq, "Id       Address              Device   PPP-Device\n");
>  		goto out;
>  	}
>  
>  	po = v;
>  	dev_name = po->pppoe_pa.dev;
> +	ppp_name = ppp_dev_name(&po->chan);

[Severity: Critical]
Can this call lead to a wild pointer dereference and a kernel panic?

Reading /proc/net/pppoe invokes pppoe_seq_show() under rcu_read_lock().
However, it appears pppoe_connect() might modify po->chan without RCU
synchronization in a couple of racing scenarios.

First, during a rebind, pppoe_connect() unlinks the socket via delete_item(),
but then immediately zeroes the channel without waiting for a grace period:

drivers/net/ppp/pppoe.c:pppoe_connect() {
    ...
    delete_item(pn, po->pppoe_pa.sid, po->pppoe_pa.remote,
                po->pppoe_ifindex);
    ...
    memset(&po->chan, 0, sizeof(po->chan));
    ...
}

Would a concurrent /proc reader observe a partially zeroed (torn) chan->ppp
pointer here?

Second, during a new connection, __set_item() exposes the socket to
/proc readers before ppp_register_net_channel() initializes it:

drivers/net/ppp/pppoe.c:pppoe_connect() {
    ...
    error = __set_item(pn, po);
    ...
    ppp_register_net_channel(dev_net(dev), &po->chan);
    ...
}

Since ppp_register_net_channel() assigns the pointer with a plain store:

drivers/net/ppp/ppp_generic.c:ppp_register_net_channel() {
    ...
    chan->ppp = pch;
    ...
}

And ppp_dev_name() reads it via a plain load:

drivers/net/ppp/ppp_generic.c:ppp_dev_name() {
    ...
    struct channel *pch = chan->ppp;
    ...
}

Could readers on weakly-ordered architectures observe the new pointer
but read uninitialized memory for its contents?

Since /proc/net/pppoe is world-readable, could an unprivileged user 
continuously read this file while connections are managed, reliably
triggering a local denial of service?

>  
> -	seq_printf(seq, "%08X %pM %8s\n",
> -		po->pppoe_pa.sid, po->pppoe_pa.remote, dev_name);
> +	seq_printf(seq, "%08X %pM %8s %10s\n",
> +		   po->pppoe_pa.sid, po->pppoe_pa.remote, dev_name, ppp_name ? ppp_name : "");
>  out:
>  	return 0;
>  }