Re: [PATCH net-next] ppp: consolidate RX skb queueing

[Paolo Abeni]

On 4/28/26 4:44 AM, Qingfang Deng wrote:
> In ppp_input() and ppp_receive_nonmp_frame(), received skbs are queued
> for userspace delivery using the same open-coded pattern:
> 
> 	skb_queue_tail(&pf->rq, skb);
> 	while (pf->rq.qlen > PPP_MAX_RQLEN &&
> 	       (skb = skb_dequeue(&pf->rq)))
> 		kfree_skb(skb);
> 	wake_up_interruptible(&pf->rwait);
> 
> This has a potential race: skb_queue_tail() releases the queue lock,
> then qlen is read locklessly before skb_dequeue() re-acquires it.
> Another CPU enqueueing concurrently could cause the length check to see
> stale data. This race is benign, as it only causes extra skbs to be
> freed in the worst case.
> 
> Introduce ppp_file_queue_rx_skb() to perform the enqueue, length check,
> and trim atomically under a single pf->rq.lock critical section. As both
> callers have softirq disabled, plain spin_lock() can be used instead of
> _bh()/_irqsave() variants. Since only one skb is enqueued at a time, the
> queue can exceed PPP_MAX_RQLEN by at most one frame, so replace the
> while-loop with an if-statement. While at it, use skb_queue_len()
> instead of open-coding the qlen access.
> 
> Signed-off-by: Qingfang Deng <qingfang.deng@xxxxxxxxx>
> ---
>  drivers/net/ppp/ppp_generic.c | 37 ++++++++++++++++++++++-------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 57c68efa5ff8..6ab5011540a0 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -2307,6 +2307,27 @@ static bool ppp_channel_bridge_input(struct channel *pch, struct sk_buff *skb)
>  	return !!pchb;
>  }
>  
> +/* Queue up and deliver a received skb to userspace.
> + * Must be called in softirq.
> + */
> +static void ppp_file_queue_rx_skb(struct ppp_file *pf, struct sk_buff *skb)
> +{
> +	spin_lock(&pf->rq.lock);
> +	__skb_queue_tail(&pf->rq, skb);
> +	/* limit queue length by dropping old frames */
> +	if (unlikely(skb_queue_len(&pf->rq) > PPP_MAX_RQLEN)) {
> +		struct sk_buff *old = __skb_peek(&pf->rq);
> +
> +		__skb_unlink(old, &pf->rq);
> +		spin_unlock(&pf->rq.lock);
> +		kfree_skb(old);
> +	} else {
> +		spin_unlock(&pf->rq.lock);

Note that after __skb_queue_tail(), skb_queue_len(&pf->rq) could be ==
PPP_MAX_RQLEN + 2, due to the slightly different check in
ppp_prepare_tx_skb().

I think the above it could/should be simplified to:
	while (unlikely(skb_queue_len(&pf->rq) > PPP_MAX_RQLEN))
		kfree_skb(__skb_dequeue(&pf->rq));
	spin_unlock(&pf->rq.lock);

And possibly it would make sense to consolidate the test in
ppp_prepare_tx_skb(), too for consistency - in that case an `if`
statement should become enough.

/P