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

On 2026/4/30 16:54, Paolo Abeni wrote:
>
> 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 <[email protected]>
>> ---
>>   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().

The check in ppp_prepare_tx_skb() is for demand dialing mode. As the 
name and comment suggest, when waiting for traffic a ppp interface is 
not able to receive packets, until we see a tx packet and then do the 
actual dial-up to resume normal operation, so that can't happen.

>
> 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.
I could consolidate this, but it tail-drops the skb instead of 
head-dropping.