Re: [PATCH net-next] ppp: consolidate RX skb queueing
From: Qingfang Deng
Date: Tue May 05 2026 - 22:55:38 EST
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 queuedNote that after __skb_queue_tail(), skb_queue_len(&pf->rq) could be ==
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);
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 could consolidate this, but it tail-drops the skb instead of head-dropping.
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.