The ppp->closing flag is used to test if an interface is closing down.
However, when .ndo_uninit() is called (where ppp->closing is set to 1),
dev_close() has already brought down the interface, and
synchronize_net() guarantees that no pending TX/RX in the network path
can take place. Thus, the check in the network path is unnecessary.

For file operations - ppp_read(), ppp_write(), and ppp_poll(), can
normally still send or receive skbs. ppp_read() and ppp_poll() are safe
because ppp_dev_uninit() sets pf->dead before waking them up, causing
both to exit cleanly. ppp_write() does not check pf->dead, but
ppp_push() verifies that ppp->channels list is not empty before sending.

Remove the ppp->closing check.

Signed-off-by: Qingfang Deng <dqfext@gmail.com>
---
v2: explain that the race against file operations is safe.
 - https://lore.kernel.org/linux-ppp/20241104092434.2677-1-dqfext@gmail.com/

 drivers/net/ppp/ppp_generic.c | 38 ++++++++++++-----------------------
 1 file changed, 13 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index f9f0f16c41d1..c24a2721ac9b 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -142,7 +142,6 @@ struct ppp {
 	unsigned long	last_xmit;	/* jiffies when last pkt sent 9c */
 	unsigned long	last_recv;	/* jiffies when last pkt rcvd a0 */
 	struct net_device *dev;		/* network interface device a4 */
-	int		closing;	/* is device closing down? a8 */
 #ifdef CONFIG_PPP_MULTILINK
 	int		nxchan;		/* next channel to send something on */
 	u32		nxseq;		/* next sequence number to send */
@@ -1566,10 +1565,6 @@ static void ppp_dev_uninit(struct net_device *dev)
 	struct ppp *ppp = netdev_priv(dev);
 	struct ppp_net *pn = ppp_pernet(ppp->ppp_net);
 
-	ppp_lock(ppp);
-	ppp->closing = 1;
-	ppp_unlock(ppp);
-
 	mutex_lock(&pn->all_ppp_mutex);
 	unit_put(&pn->units_idr, ppp->file.index);
 	mutex_unlock(&pn->all_ppp_mutex);
@@ -1652,23 +1647,19 @@ static void ppp_setup(struct net_device *dev)
 static void __ppp_xmit_process(struct ppp *ppp, struct sk_buff *skb)
 {
 	ppp_xmit_lock(ppp);
-	if (!ppp->closing) {
-		ppp_push(ppp);
+	ppp_push(ppp);
 
-		if (skb)
-			skb_queue_tail(&ppp->file.xq, skb);
-		while (!ppp->xmit_pending &&
-		       (skb = skb_dequeue(&ppp->file.xq)))
-			ppp_send_frame(ppp, skb);
-		/* If there's no work left to do, tell the core net
-		   code that we can accept some more. */
-		if (!ppp->xmit_pending && !skb_peek(&ppp->file.xq))
-			netif_wake_queue(ppp->dev);
-		else
-			netif_stop_queue(ppp->dev);
-	} else {
-		kfree_skb(skb);
-	}
+	if (skb)
+		skb_queue_tail(&ppp->file.xq, skb);
+	while (!ppp->xmit_pending &&
+	       (skb = skb_dequeue(&ppp->file.xq)))
+		ppp_send_frame(ppp, skb);
+	/* If there's no work left to do, tell the core net
+	   code that we can accept some more. */
+	if (!ppp->xmit_pending && !skb_peek(&ppp->file.xq))
+		netif_wake_queue(ppp->dev);
+	else
+		netif_stop_queue(ppp->dev);
 	ppp_xmit_unlock(ppp);
 }
 
@@ -2218,10 +2209,7 @@ static inline void
 ppp_do_recv(struct ppp *ppp, struct sk_buff *skb, struct channel *pch)
 {
 	ppp_recv_lock(ppp);
-	if (!ppp->closing)
-		ppp_receive_frame(ppp, skb, pch);
-	else
-		kfree_skb(skb);
+	ppp_receive_frame(ppp, skb, pch);
 	ppp_recv_unlock(ppp);
 }
 
-- 
2.43.0

On 2/2/26 10:21 AM, Qingfang Deng wrote:
> The ppp->closing flag is used to test if an interface is closing down.
> However, when .ndo_uninit() is called (where ppp->closing is set to 1),
> dev_close() has already brought down the interface, and
> synchronize_net() guarantees that no pending TX/RX in the network path
> can take place. Thus, the check in the network path is unnecessary.
> 
> For file operations - ppp_read(), ppp_write(), and ppp_poll(), can
> normally still send or receive skbs. ppp_read() and ppp_poll() are safe
> because ppp_dev_uninit() sets pf->dead before waking them up, causing
> both to exit cleanly. 

Please report the accurate call sequence that would lead to such syscall
complete cleanly. Also what if ndo_uninit() happens just after the
user-space has been woken-up?

> ppp_write() does not check pf->dead, but
> ppp_push() verifies that ppp->channels list is not empty before sending.
> 
> Remove the ppp->closing check.

This still feel risky to me and it's not clear which would be the
goal/gain. It this change performance oriented? If so please included
actual figures.

Thanks,

Paolo

On Thu, Feb 5, 2026 at 7:40 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On 2/2/26 10:21 AM, Qingfang Deng wrote:
> > The ppp->closing flag is used to test if an interface is closing down.
> > However, when .ndo_uninit() is called (where ppp->closing is set to 1),
> > dev_close() has already brought down the interface, and
> > synchronize_net() guarantees that no pending TX/RX in the network path
> > can take place. Thus, the check in the network path is unnecessary.
> >
> > For file operations - ppp_read(), ppp_write(), and ppp_poll(), can
> > normally still send or receive skbs. ppp_read() and ppp_poll() are safe
> > because ppp_dev_uninit() sets pf->dead before waking them up, causing
> > both to exit cleanly.
>
> Please report the accurate call sequence that would lead to such syscall
> complete cleanly. Also what if ndo_uninit() happens just after the
> user-space has been woken-up?

ppp_read() and ppp_poll() do not care about ppp->closing so their
behaviours remain unchanged. And since ppp_file is refcounted there
isn't a UAF issue.

>
> > ppp_write() does not check pf->dead, but
> > ppp_push() verifies that ppp->channels list is not empty before sending.
> >
> > Remove the ppp->closing check.
>
> This still feel risky to me and it's not clear which would be the
> goal/gain. It this change performance oriented? If so please included
> actual figures.

For both performance and size, but I don't have the figures yet.

>
> Thanks,
>
> Paolo
>