Re: [PATCH net-next] pppoe: pass bound packets directly to generic PPP

[Jakub Kicinski]

This is (mostly) an AI-generated review of your patch.

One non-AI question - presumably you're doing this to improve performance?
Could you add some numbers to the cover letter?
---
pppoe: pass bound packets directly to generic PPP

This drops __sk_receive_skb() for PPPOX_BOUND PPPoE sockets and hands
session frames straight to ppp_input() from pppoe_rcv(), relying on the
RCU-deferred channel free added by ec4215683e47.  It also adds a
synchronize_net() in pppoe_connect() after the old session is unhashed,
before po->chan is cleared and reused.

No Link: tags are present in the commit header.

A few questions below.

> diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> index 6874a1a8edaf1..0624116241823 100644
> --- a/drivers/net/ppp/pppoe.c
> +++ b/drivers/net/ppp/pppoe.c
> @@ -345,10 +345,10 @@ static struct notifier_block pppoe_notifier = {
>  
>  /************************************************************************
>   *
> - * Do the real work of receiving a PPPoE Session frame.
> + * Backlog receive a PPPoE Session frame and deliver to userspace.
>   *
>   ***********************************************************************/
> -static int pppoe_rcv_core(struct sock *sk, struct sk_buff *skb)
> +static int pppoe_backlog_rcv(struct sock *sk, struct sk_buff *skb)
>  {
>  	struct pppox_sock *po = pppox_sk(sk);

[Severity: Low]
Is the new banner comment accurate?  The function body still delivers to
the generic PPP layer, not to userspace:

drivers/net/ppp/pppoe.c:pppoe_backlog_rcv() {
	if (sk->sk_state & PPPOX_BOUND) {
		ppp_input(&po->chan, skb);
	} else {
		if (sock_queue_rcv_skb(sk, skb))
			goto abort_kfree;
	}
}

That branch still looks reachable: pppoe_rcv() only takes the new direct
path when it observes PPPOX_BOUND, and a frame observed while !BOUND is put
on the socket backlog by __sk_receive_skb() and only evaluated at
release_sock() time.  By then pppox_ioctl(PPPIOCGCHAN) may already have run
sk->sk_state |= PPPOX_BOUND under lock_sock().

Could the comment mention that the bound case still goes to ppp_input(), so
that a later reader does not remove the branch on the strength of the banner
text?

[ ... ]

> @@ -420,6 +420,10 @@ static int pppoe_rcv(struct sk_buff *skb, struct net_device *dev,
>  	if (!po)
>  		goto drop;
>  
> +	if (likely(po->sk.sk_state & PPPOX_BOUND)) {
> +		ppp_input(&po->chan, skb);
> +		return NET_RX_SUCCESS;
> +	}
>  	return __sk_receive_skb(&po->sk, skb, 0, 1, false);

[Severity: Medium]
Does this skip socket filters and the skb->dev clearing that
__sk_receive_skb() used to perform for bound sessions?

net/core/sock.c:__sk_receive_skb() {
	reason = sk_filter_trim_cap(sk, skb, trim_cap);
	if (reason)
		goto discard_and_relse;

	skb->dev = NULL;
	...
}

Two things in sk_filter_trim_cap() look relevant here.  A filter attached
with SO_ATTACH_FILTER/SO_ATTACH_BPF is now silently ignored for all bound
session traffic; do_sock_setsockopt() routes SOL_SOCKET options to
sock_setsockopt() for PF_PPPOX since pppoe does not set SOCK_CUSTOM_SOCKOPT,
so sk->sk_filter can be non-NULL.  The pfmemalloc guard is also bypassed:

net/core/filter.c:sk_filter_trim_cap() {
	if (skb_pfmemalloc(skb) && !sock_flag(sk, SOCK_MEMALLOC)) {
		NET_INC_STATS(sock_net(sk), LINUX_MIB_PFMEMALLOCDROP);
		return SKB_DROP_REASON_PFMEMALLOC;
	}
	...
}

The dropped skb->dev = NULL also matters for the frames ppp_input() parks on
pch->file.rq (proto >= 0xc000, PPP_CCPFRAG, or pch->ppp == NULL) -- those can
now sit queued indefinitely holding a non-refcounted net_device pointer.  Is
that intentional?  The cgroup-BPF and LSM parts of sk_filter_trim_cap() do
appear to be no-ops for PF_PPPOX, so those are not a concern, and the
sk_rcvqueues_full() check is not meaningful for the bound path.

Note the PPPOX_BOUND branch in pppoe_backlog_rcv() still runs under the old
contract, so the same class of frame is handled two different ways depending
on timing.  Could the changelog say something about the filtering and
skb->dev behaviour change?

[Severity: Low]
Should this read be annotated?  po->sk.sk_state is loaded here from softirq
with no lock held, while every writer stores it with a plain access under
lock_sock():

drivers/net/ppp/pppox.c:pppox_unbind_sock()	sk->sk_state = PPPOX_DEAD;
drivers/net/ppp/pppox.c:pppox_ioctl()		sk->sk_state |= PPPOX_BOUND;
drivers/net/ppp/pppoe.c:pppoe_connect()		sk->sk_state = PPPOX_CONNECTED;

Before the patch the test happened inside bh_lock_sock() in
__sk_receive_skb(), so it was serialized against those writers.  Would
KCSAN report this now, and would READ_ONCE()/WRITE_ONCE() be appropriate?

There is also a bare control dependency: pppoe_connect() fills in
po->chan.{hdrlen,mtu,private,ops}, calls ppp_register_net_channel() (which
sets chan->ppp), and only then publishes sk->sk_state, with no release
store to pair with.  The reader then loads po->chan.ppp inside ppp_input()
with no acquire.  As far as I can tell the worst outcome is ppp_input()
seeing !pch and dropping the frame, so this looks like an annotation
question rather than a correctness one.  pppol2tp_recv() has the same
unannotated pattern.

[Severity: Low]
Can frames reach ppp_input() out of arrival order now?  Two paths can feed
the same channel:

pppoe_rcv() -> ppp_input()			(softirq, no socket lock)
release_sock() -> pppoe_backlog_rcv() -> ppp_input()	(process context)

pppox_ioctl(PPPIOCGCHAN) sets sk->sk_state |= PPPOX_BOUND under lock_sock().
A frame that arrived just before that, while the socket was owned by the
ioctl caller, went to the socket backlog.  A frame arriving after the BOUND
store but before release_sock() takes the new fast path and is handed to
ppp_input() immediately, ahead of the older backlogged one.

ppp_input() serializes internally, and the window only exists once per
session while LCP/auth frames are in flight, so this may be acceptable --
but is it worth a note?

[Severity: Low]
Does this leave the documented channel contract stale?

include/linux/ppp_channel.h {
 * SMP locking notes:
 * The channel code must ensure that when it calls ppp_unregister_channel,
 * nothing is executing in any of the procedures above, for that
 * channel.
}

ppp_input() is one of the listed procedures.  After this change pppoe_rcv()
calls it from softirq with no serialization, while every pppoe caller that
reaches ppp_unregister_channel() via pppox_unbind_sock() -- pppoe_release(),
pppoe_flush_dev(), pppoe_unbind_sock_work(), pppoe_connect() -- does so
without draining in-flight ppp_input() callers.  In pppoe_connect() the new
synchronize_net() sits after pppox_unbind_sock(), so it fences the later
memset(&po->chan) and re-registration rather than the unregister itself.

The RCU-deferred struct channel free from ec4215683e47 does appear to close
the use-after-free, and pppol2tp_recv() already calls ppp_input() the same
lockless way, so this is about the text rather than a runtime failure.
Should include/linux/ppp_channel.h (and possibly
Documentation/networking/ppp_generic.rst) be updated to describe the
RCU-based rule that now makes this safe?

[ ... ]

> @@ -625,6 +629,13 @@ static int pppoe_connect(struct socket *sock, struct sockaddr_unsized *uservaddr
>  		pn = pppoe_pernet(sock_net(sk));
>  		delete_item(pn, po->pppoe_pa.sid,
>  			    po->pppoe_pa.remote, po->pppoe_ifindex);
> +
> +		/* pppoe_rcv() can call ppp_input() without taking the socket
> +		 * lock. Once the socket is unhashed, wait for any receive path
> +		 * that found it earlier before clearing and reusing po->chan.
> +		 */
> +		synchronize_net();
> +
>  		if (po->pppoe_dev) {
>  			dev_put(po->pppoe_dev);
>  			po->pppoe_dev = NULL;

[Severity: Low]
Is holding the socket lock across the grace period necessary here?
pppoe_connect() takes lock_sock(sk) at the top, so every disconnect/rebind
of a session now blocks other users of that socket for a full,
non-expedited RCU grace period.  The drain is only needed to protect the
following memset(&po->chan, 0, sizeof(po->chan)), so could it be arranged
without the socket lock held?

For the record, this does not look like it can stall RTNL:
delete_item() unhashes the socket before synchronize_net(), and
pppoe_flush_dev() only locks sockets it finds by walking pn->hash_table, so
the RTNL-holding notifier cannot find the socket sleeping in the grace
period.
-- 
pw-bot: cr