Re: [PATCH net] pppox: drain queued packets on channel handoff

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
pppox: drain queued packets on channel handoff

PPPIOCGCHAN marks an AF_PPPOX socket as bound to generic PPP as a side
effect of returning the channel index. This patch adds a synchronize_net()
after setting PPPOX_BOUND and then drains sk->sk_receive_queue into
ppp_input(), since a bound socket is no longer readable from userspace.

A few questions about the new code below.

> diff --git a/drivers/net/ppp/pppox.c b/drivers/net/ppp/pppox.c
> index 5861a2f6ce3e6..a6f72c813bef9 100644
> --- a/drivers/net/ppp/pppox.c
> +++ b/drivers/net/ppp/pppox.c

[ ... ]

> @@ -85,7 +87,22 @@ int pppox_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
>  			break;
>  
>  		rc = 0;
> +		/* PPPIOCGCHAN historically marks the userspace handoff to
> +		 * generic PPP; pppd then attaches the returned channel to
> +		 * /dev/ppp.
> +		 */
>  		sk->sk_state |= PPPOX_BOUND;
> +		/* Let lockless receive paths finish queueing against the old
> +		 * state.
> +		 */
> +		synchronize_net();

[Severity: Medium]
Should this skip the grace period when PPPOX_BOUND is already set?

The only gate before it in pppox_ioctl() is:

	rc = -ENOTCONN;
	if (!(sk->sk_state & PPPOX_CONNECTED))
		break;

so every repeated PPPIOCGCHAN on the same connected socket re-sets
PPPOX_BOUND and runs another full RCU grace period. Neither pppox_create()
nor pppox_ioctl() has a capability check, and pppoe_connect() only needs a
session tuple plus an up device, so an ordinary local process can loop the
ioctl and drive grace periods back to back.

synchronize_net() escalates to synchronize_rcu_expedited() whenever
rtnl_is_locked() is true:

net/core/dev.c:synchronize_net() {
	might_sleep();
	if (from_cleanup_net() || rtnl_is_locked())
		synchronize_rcu_expedited();
	else
		synchronize_rcu();
}

which sends IPIs to every CPU. The wait also happens with the socket lock
held, so the socket's receive path is pushed into sk_backlog for the whole
duration.

Would something like

	if (!(sk->sk_state & PPPOX_BOUND)) {
		sk->sk_state |= PPPOX_BOUND;
		synchronize_net();
		...
	}

be enough here? For PPTP sockets pptp_bind() already sets PPPOX_BOUND
before any ioctl, so for that transport the first synchronize_net() has
nothing to wait for either.

[Severity: Medium]
Can this reorder frames for the pppol2tp transport?

PPPOX_BOUND becomes visible before the sleep, but the already-queued frames
are only delivered after it. Meanwhile pppol2tp_recv() reads the state
under rcu_read_lock() only, with no socket lock:

net/l2tp/l2tp_ppp.c:pppol2tp_recv() {
	...
	if (sk->sk_state & PPPOX_BOUND) {
		struct pppox_sock *po;

		po = pppox_sk(sk);
		ppp_input(&po->chan, skb);
	} else {
	...
}

So for the length of the grace period newly arriving frames go straight to
ppp_input() while the older ones still sit in sk->sk_receive_queue, and get
injected only afterwards. PPP receive processing is stateful (VJ header
decompression, CCP/MPPE history and sequence numbers, multilink
reassembly) - would injecting the stale frames after the newer ones
desynchronize that state?

Would draining before publishing PPPOX_BOUND, or draining under the
receive queue lock in the same critical section that sets the state, avoid
the inversion?

> +		/* Drain packets queued before the handoff because a bound
> +		 * socket is no longer readable.
> +		 */
> +		while ((skb = skb_dequeue(&sk->sk_receive_queue))) {
> +			skb_orphan(skb);
> +			ppp_input(&po->chan, skb);
> +		}

[Severity: Medium]
Does this feed raw PPTP-GRE frames into ppp_input()?

pppox_ioctl() is also the .ioctl of pptp_ops, and pptp queues frames that
are not yet decapsulated:

drivers/net/ppp/pptp.c:pptp_rcv_core() {
	...
	if (!(sk->sk_state & PPPOX_CONNECTED)) {
		if (sock_queue_rcv_skb(sk, skb))
			goto drop;
		return NET_RX_SUCCESS;
	}
	...
}

That branch runs before the skb_pull(headersize), before the
GRE_IS_SEQ()/payload_len validation, before the seq_recv/ack_recv check,
and before the PPP_ALLSTATIONS/PPP_UI strip that the connected path does
prior to calling ppp_input(). Once drained, skb->data points at the GRE
flags/version word, which ppp_decompress_proto()/PPP_PROTO() then
reinterpret as the PPP protocol field, and ownership is gone so the frame
cannot be recovered - it lands on pch->file.rq and pppd reads it as a bogus
PPP control frame, or it is dropped with rx_length_errors.

The commit message says "a bound socket is no longer readable", but for
pptp PPPOX_BOUND has a different meaning entirely:

drivers/net/ppp/pptp.c:pptp_bind() {
	...
	if (add_chan(po, &sp->sa_addr.pptp))
		error = -EBUSY;
	else
		sk->sk_state |= PPPOX_BOUND;
	...
}

pptp_rcv_core() never consults PPPOX_BOUND, and pptp_ops.recvmsg is
sock_no_recvmsg, so that queue was never readable by userspace to begin
with.

The queueing path does look narrow: lookup_chan() only matches when

	if (opt->dst_addr.sin_addr.s_addr != s_addr)
		sock = NULL;

and dst_addr is zero before pptp_connect(), so a frame with IP source
0.0.0.0 is required; net/ipv4/route.c treats that as martian for ordinary
unicast but still routes it through brd_input() when daddr is
255.255.255.255, and a unicast destination MAC keeps skb->pkt_type ==
PACKET_HOST as pptp_rcv() requires. A same-segment sender that guesses the
bound call-id could therefore park a raw GRE frame on the queue before
connect.

Should the drain be limited to the transports whose queued frames really
are bare PPP frames gated on PPPOX_BOUND (pppoe, pppol2tp), with the
queue simply purged for pptp?

[Severity: Medium]
Does the socket lock actually exclude concurrent readers here?

recvmsg() consumes the same queue without lock_sock():

drivers/net/ppp/pppoe.c:pppoe_recvmsg() {
	...
	if (sk->sk_state & PPPOX_BOUND)
		return -EIO;

	skb = skb_recv_datagram(sk, flags, &error);
	...
}

net/l2tp/l2tp_ppp.c:pppol2tp_recvmsg() is the same shape. Only the receive
queue spinlock is shared with the drain, and the PPPOX_BOUND test is a
check-then-act.

Three things seem to follow:

A reader that passed the PPPOX_BOUND test just before the ioctl set it can
dequeue a packet the drain meant to hand to PPP, so the PPP session loses
it silently.

The drain can also steal the packet a blocking recvmsg() was woken for.
__skb_wait_for_more_packets() watches the queue, socket errors and
shutdown, but not PPPOX_BOUND, so the waiter finds an empty queue and
sleeps again - and since PPPOX_BOUND is now set nothing is ever queued
again, leaving a recvmsg() with no timeout asleep indefinitely. Is that
possible?

With MSG_PEEK, __skb_try_recv_from_queue() bumps skb->users and leaves the
skb linked. The drain's skb_dequeue() then unlinks the skb the reader is
still copying from, skb_orphan() runs sock_rfree() and clears skb->sk, and
ppp_input() mutates the buffer (ppp_decompress_proto() pull/push, then
skb_queue_tail() to pch->file.rq or netif_rx()) while userspace copies from
it. Should the drain check skb_shared()/skb_cloned(), or otherwise exclude
peeking readers, before handing the skb on?

>  		break;
>  	}
>  	default:
-- 
pw-bot: cr