ppp->closing was used to test if an interface is closing down. But upon
.ndo_uninit() where ppp->closing is set to 1, dev_close() is already
called to bring down an interface and a synchronize_net() guarantees
that no pending TX/RX can take place, so the check is unnecessary.
Remove the check.

Signed-off-by: Qingfang Deng <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 4583e15ad03a..4ff94005f6c1 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -136,7 +136,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 */
@@ -1569,10 +1568,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);
@@ -1651,23 +1646,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);
 }
 
@@ -2208,10 +2199,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 11/4/24 10:24, Qingfang Deng wrote:
> ppp->closing was used to test if an interface is closing down. But upon
> .ndo_uninit() where ppp->closing is set to 1, dev_close() is already
> called to bring down an interface and a synchronize_net() guarantees
> that no pending TX/RX can take place, so the check is unnecessary.
> Remove the check.

I'm unsure we can remote such check. The TX callback can be triggered
even from a write on the controlling file, and it looks like such file
will be untouched by uninit.

Cheers,

Paolo

Hi Paolo,

On Thu, Nov 7, 2024 at 8:08 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
>
>
> On 11/4/24 10:24, Qingfang Deng wrote:
> > ppp->closing was used to test if an interface is closing down. But upon
> > .ndo_uninit() where ppp->closing is set to 1, dev_close() is already
> > called to bring down an interface and a synchronize_net() guarantees
> > that no pending TX/RX can take place, so the check is unnecessary.
> > Remove the check.
>
> I'm unsure we can remote such check. The TX callback can be triggered
> even from a write on the controlling file, and it looks like such file
> will be untouched by uninit.

ppp_release (when the file is closed) calls unregister_netdevice, and
no more writes can happen after that.

>
> Cheers,
>
> Paolo
>

On 11/8/24 07:09, Qingfang Deng wrote:
> On Thu, Nov 7, 2024 at 8:08 PM Paolo Abeni <pabeni@redhat.com> wrote:
>> On 11/4/24 10:24, Qingfang Deng wrote:
>>> ppp->closing was used to test if an interface is closing down. But upon
>>> .ndo_uninit() where ppp->closing is set to 1, dev_close() is already
>>> called to bring down an interface and a synchronize_net() guarantees
>>> that no pending TX/RX can take place, so the check is unnecessary.
>>> Remove the check.
>>
>> I'm unsure we can remote such check. The TX callback can be triggered
>> even from a write on the controlling file, and it looks like such file
>> will be untouched by uninit.
> 
> ppp_release (when the file is closed) calls unregister_netdevice, and
> no more writes can happen after that.

AFAICS the device can be deleted even without closing the file, via
netlink or deleting the namespace. In such cases, AFAICS, the file is
still alive.

In any case we need a more solid explanation describing why the change
is safe (and possibly a test-case deleting the device in different ways).

/P