Skip to contentSkip to conversation

ppp: ppp_async: simplify tty disc_data access

PATCH Applied Qingfang Deng 3 participants 5 messages Aug 28 – Sep 1

Subsystems (3)
Networking Network drivers PPP protocol drivers and compressors
lore
Share & tools
lore

Qingfang Deng

author
#0 lore raw

PATCH 1/2 ppp: ppp_async: simplify tty disc_data access

tty_ldisc_hangup() invokes the hangup callback while holding only a read lock on tty->ldisc_sem, so it can run concurrently with other line discipline callbacks. This currently forces async PPP to maintain separate lifetime protection around tty->disc_data.

Line discipline close is called under the write lock during hangup processing. Remove the hangup callback and rely on close for teardown, as done for SLIP by commit 23c53269f2ba ("slip: remove slip_hangup() to fix use-after-free in slip_receive_buf()"). This serializes teardown with all other line discipline operations.

disc_data_lock, refcount and completion are redundant with that serialization. Remove them and access tty->disc_data directly.

This also eliminates a lockdep warning reported by syzbot. The warning does not indicate a real deadlock because the write side runs only in process context with hardirqs disabled.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by [email protected]
Closes https://lore.kernel.org/all/[email protected]/ open here
Signed-off-by Qingfang Deng <[email protected]>
---
 drivers/net/ppp/ppp_async.c | 82 ++++---------------------------------
 1 file changed, 7 insertions(+), 75 deletions(-)
drivers/net/ppp/ppp_async.c +7 -75
index 583426d06381..ea7fe9608ffd 100644
@@ -63,8 +63,6 @@ struct asyncppp {
 
 	struct tasklet_struct tsk;
 
-	refcount_t	refcnt;
-	struct completion dead;
 	struct ppp_channel chan;	/* interface to generic ppp layer */
 	unsigned char	obuf[OBUFSIZE];
 };

@@ -114,38 +112,6 @@ static const struct ppp_channel_ops async_ops = {
  * Routines implementing the PPP line discipline.
  */
 
-/*
- * We have a potential race on dereferencing tty->disc_data,
- * because the tty layer provides no locking at all - thus one
- * cpu could be running ppp_asynctty_receive while another
- * calls ppp_asynctty_close, which zeroes tty->disc_data and
- * frees the memory that ppp_asynctty_receive is using.  The best
- * way to fix this is to use a rwlock in the tty struct, but for now
- * we use a single global rwlock for all ttys in ppp line discipline.
- *
- * FIXME: this is no longer true. The _close path for the ldisc is
- * now guaranteed to be sane.
- */
-static DEFINE_RWLOCK(disc_data_lock);
-
-static struct asyncppp *ap_get(struct tty_struct *tty)
-{
-	struct asyncppp *ap;
-
-	read_lock(&disc_data_lock);
-	ap = tty->disc_data;
-	if (ap != NULL)
-		refcount_inc(&ap->refcnt);
-	read_unlock(&disc_data_lock);
-	return ap;
-}
-
-static void ap_put(struct asyncppp *ap)
-{
-	if (refcount_dec_and_test(&ap->refcnt))
-		complete(&ap->dead);
-}
-
 /*
  * Called when a tty is put into PPP line discipline. Called in process
  * context.

@@ -180,9 +146,6 @@ ppp_asynctty_open(struct tty_struct *tty)
 	skb_queue_head_init(&ap->rqueue);
 	tasklet_setup(&ap->tsk, ppp_async_process);
 
-	refcount_set(&ap->refcnt, 1);
-	init_completion(&ap->dead);
-
 	ap->chan.private = ap;
 	ap->chan.ops = &async_ops;
 	ap->chan.mtu = PPP_MRU;

@@ -203,34 +166,18 @@ ppp_asynctty_open(struct tty_struct *tty)
 }
 
 /*
- * Called when the tty is put into another line discipline
- * or it hangs up.  We have to wait for any cpu currently
- * executing in any of the other ppp_asynctty_* routines to
- * finish before we can call ppp_unregister_channel and free
- * the asyncppp struct.  This routine must be called from
- * process context, not interrupt or softirq context.
+ * Called when the tty is put into another line discipline or it hangs up.
+ * This call is serialized against other ldisc functions.
  */
 static void
 ppp_asynctty_close(struct tty_struct *tty)
 {
-	struct asyncppp *ap;
+	struct asyncppp *ap = tty->disc_data;
 
-	write_lock_irq(&disc_data_lock);
-	ap = tty->disc_data;
-	tty->disc_data = NULL;
-	write_unlock_irq(&disc_data_lock);
 	if (!ap)
 		return;
 
-	/*
-	 * We have now ensured that nobody can start using ap from now
-	 * on, but we have to wait for all existing users to finish.
-	 * Note that ppp_unregister_channel ensures that no calls to
-	 * our channel ops (i.e. ppp_async_send/ioctl) are in progress
-	 * by the time it returns.
-	 */
-	if (!refcount_dec_and_test(&ap->refcnt))
-		wait_for_completion(&ap->dead);
+	tty->disc_data = NULL;
 	tasklet_kill(&ap->tsk);
 
 	ppp_unregister_channel(&ap->chan);

@@ -240,17 +187,6 @@ ppp_asynctty_close(struct tty_struct *tty)
 	kfree(ap);
 }
 
-/*
- * Called on tty hangup in process context.
- *
- * Wait for I/O to driver to complete and unregister PPP channel.
- * This is already done by the close routine, so just call that.
- */
-static void ppp_asynctty_hangup(struct tty_struct *tty)
-{
-	ppp_asynctty_close(tty);
-}
-
 /*
  * Read does nothing - no data is ever available this way.
  * Pppd reads and writes packets via /dev/ppp instead.

@@ -281,7 +217,7 @@ ppp_asynctty_write(struct tty_struct *tty, struct file *file, const u8 *buf,
 static int
 ppp_asynctty_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
 {
-	struct asyncppp *ap = ap_get(tty);
+	struct asyncppp *ap = tty->disc_data;
 	int err, val;
 	int __user *p = (int __user *)arg;
 

@@ -322,7 +258,6 @@ ppp_asynctty_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
 		err = tty_mode_ioctl(tty, cmd, arg);
 	}
 
-	ap_put(ap);
 	return err;
 }
 

@@ -331,7 +266,7 @@ static void
 ppp_asynctty_receive(struct tty_struct *tty, const u8 *buf, const u8 *cflags,
 		     size_t count)
 {
-	struct asyncppp *ap = ap_get(tty);
+	struct asyncppp *ap = tty->disc_data;
 	unsigned long flags;
 
 	if (!ap)

@@ -341,21 +276,19 @@ ppp_asynctty_receive(struct tty_struct *tty, const u8 *buf, const u8 *cflags,
 	spin_unlock_irqrestore(&ap->recv_lock, flags);
 	if (!skb_queue_empty(&ap->rqueue))
 		tasklet_schedule(&ap->tsk);
-	ap_put(ap);
 	tty_unthrottle(tty);
 }
 
 static void
 ppp_asynctty_wakeup(struct tty_struct *tty)
 {
-	struct asyncppp *ap = ap_get(tty);
+	struct asyncppp *ap = tty->disc_data;
 
 	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
 	if (!ap)
 		return;
 	set_bit(XMIT_WAKEUP, &ap->xmit_flags);
 	tasklet_schedule(&ap->tsk);
-	ap_put(ap);
 }
 
 

@@ -365,7 +298,6 @@ static struct tty_ldisc_ops ppp_ldisc = {
 	.name	= "ppp",
 	.open	= ppp_asynctty_open,
 	.close	= ppp_asynctty_close,
-	.hangup	= ppp_asynctty_hangup,
 	.read	= ppp_asynctty_read,
 	.write	= ppp_asynctty_write,
 	.ioctl	= ppp_asynctty_ioctl,
Signature · 1 line
2.43.0
author
#1 lore raw

PATCH 2/2 ppp: ppp_synctty: simplify tty disc_data access

Apply the same simplification as the preceding ppp_async change.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by [email protected]
Closes https://syzbot.org/bug?extid=b503105c2410c3433459
Signed-off-by Qingfang Deng <[email protected]>
---
 drivers/net/ppp/ppp_synctty.c | 83 +++--------------------------------
 1 file changed, 7 insertions(+), 76 deletions(-)
drivers/net/ppp/ppp_synctty.c +7 -76
index 0b1bd1635c39..f87d43faeeab 100644
@@ -38,11 +38,9 @@
 #include <linux/ppp-ioctl.h>
 #include <linux/ppp_channel.h>
 #include <linux/spinlock.h>
-#include <linux/completion.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/slab.h>
-#include <linux/refcount.h>
 #include <linux/unaligned.h>
 #include <linux/uaccess.h>
 

@@ -67,8 +65,6 @@ struct syncppp {
 
 	struct tasklet_struct tsk;
 
-	refcount_t	refcnt;
-	struct completion dead_cmp;
 	struct ppp_channel chan;	/* interface to generic ppp layer */
 };
 

@@ -116,37 +112,6 @@ ppp_print_buffer (const char *name, const __u8 *buf, int count)
  * Routines implementing the synchronous PPP line discipline.
  */
 
-/*
- * We have a potential race on dereferencing tty->disc_data,
- * because the tty layer provides no locking at all - thus one
- * cpu could be running ppp_synctty_receive while another
- * calls ppp_synctty_close, which zeroes tty->disc_data and
- * frees the memory that ppp_synctty_receive is using.  The best
- * way to fix this is to use a rwlock in the tty struct, but for now
- * we use a single global rwlock for all ttys in ppp line discipline.
- *
- * FIXME: Fixed in tty_io nowadays.
- */
-static DEFINE_RWLOCK(disc_data_lock);
-
-static struct syncppp *sp_get(struct tty_struct *tty)
-{
-	struct syncppp *ap;
-
-	read_lock(&disc_data_lock);
-	ap = tty->disc_data;
-	if (ap != NULL)
-		refcount_inc(&ap->refcnt);
-	read_unlock(&disc_data_lock);
-	return ap;
-}
-
-static void sp_put(struct syncppp *ap)
-{
-	if (refcount_dec_and_test(&ap->refcnt))
-		complete(&ap->dead_cmp);
-}
-
 /*
  * Called when a tty is put into sync-PPP line discipline.
  */

@@ -177,9 +142,6 @@ ppp_sync_open(struct tty_struct *tty)
 	skb_queue_head_init(&ap->rqueue);
 	tasklet_setup(&ap->tsk, ppp_sync_process);
 
-	refcount_set(&ap->refcnt, 1);
-	init_completion(&ap->dead_cmp);
-
 	ap->chan.private = ap;
 	ap->chan.ops = &sync_ops;
 	ap->chan.mtu = PPP_MRU;

@@ -201,34 +163,18 @@ ppp_sync_open(struct tty_struct *tty)
 }
 
 /*
- * Called when the tty is put into another line discipline
- * or it hangs up.  We have to wait for any cpu currently
- * executing in any of the other ppp_synctty_* routines to
- * finish before we can call ppp_unregister_channel and free
- * the syncppp struct.  This routine must be called from
- * process context, not interrupt or softirq context.
+ * Called when the tty is put into another line discipline or it hangs up.
+ * This call is serialized against other ldisc functions.
  */
 static void
 ppp_sync_close(struct tty_struct *tty)
 {
-	struct syncppp *ap;
+	struct syncppp *ap = tty->disc_data;
 
-	write_lock_irq(&disc_data_lock);
-	ap = tty->disc_data;
-	tty->disc_data = NULL;
-	write_unlock_irq(&disc_data_lock);
 	if (!ap)
 		return;
 
-	/*
-	 * We have now ensured that nobody can start using ap from now
-	 * on, but we have to wait for all existing users to finish.
-	 * Note that ppp_unregister_channel ensures that no calls to
-	 * our channel ops (i.e. ppp_sync_send/ioctl) are in progress
-	 * by the time it returns.
-	 */
-	if (!refcount_dec_and_test(&ap->refcnt))
-		wait_for_completion(&ap->dead_cmp);
+	tty->disc_data = NULL;
 	tasklet_kill(&ap->tsk);
 
 	ppp_unregister_channel(&ap->chan);

@@ -237,17 +183,6 @@ ppp_sync_close(struct tty_struct *tty)
 	kfree(ap);
 }
 
-/*
- * Called on tty hangup in process context.
- *
- * Wait for I/O to driver to complete and unregister PPP channel.
- * This is already done by the close routine, so just call that.
- */
-static void ppp_sync_hangup(struct tty_struct *tty)
-{
-	ppp_sync_close(tty);
-}
-
 /*
  * Read does nothing - no data is ever available this way.
  * Pppd reads and writes packets via /dev/ppp instead.

@@ -273,7 +208,7 @@ ppp_sync_write(struct tty_struct *tty, struct file *file, const u8 *buf,
 static int
 ppp_synctty_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
 {
-	struct syncppp *ap = sp_get(tty);
+	struct syncppp *ap = tty->disc_data;
 	int __user *p = (int __user *)arg;
 	int err, val;
 

@@ -314,7 +249,6 @@ ppp_synctty_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
 		break;
 	}
 
-	sp_put(ap);
 	return err;
 }
 

@@ -323,7 +257,7 @@ static void
 ppp_sync_receive(struct tty_struct *tty, const u8 *buf, const u8 *cflags,
 		 size_t count)
 {
-	struct syncppp *ap = sp_get(tty);
+	struct syncppp *ap = tty->disc_data;
 	unsigned long flags;
 
 	if (!ap)

@@ -333,21 +267,19 @@ ppp_sync_receive(struct tty_struct *tty, const u8 *buf, const u8 *cflags,
 	spin_unlock_irqrestore(&ap->recv_lock, flags);
 	if (!skb_queue_empty(&ap->rqueue))
 		tasklet_schedule(&ap->tsk);
-	sp_put(ap);
 	tty_unthrottle(tty);
 }
 
 static void
 ppp_sync_wakeup(struct tty_struct *tty)
 {
-	struct syncppp *ap = sp_get(tty);
+	struct syncppp *ap = tty->disc_data;
 
 	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
 	if (!ap)
 		return;
 	set_bit(XMIT_WAKEUP, &ap->xmit_flags);
 	tasklet_schedule(&ap->tsk);
-	sp_put(ap);
 }
 
 

@@ -357,7 +289,6 @@ static struct tty_ldisc_ops ppp_sync_ldisc = {
 	.name	= "pppsync",
 	.open	= ppp_sync_open,
 	.close	= ppp_sync_close,
-	.hangup	= ppp_sync_hangup,
 	.read	= ppp_sync_read,
 	.write	= ppp_sync_write,
 	.ioctl	= ppp_synctty_ioctl,
Signature · 1 line
2.43.0
Reviewed-by on v1 2/2
#2 lore raw

Re: [PATCH net 2/2] ppp: ppp_synctty: simplify tty disc_data access

Quoted 10 lines · Qingfang Deng

Apply the same simplification as the preceding ppp_async change.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by [email protected]
Closes https://syzbot.org/bug?extid=b503105c2410c3433459
Signed-off-by Qingfang Deng <[email protected]>
---
 drivers/net/ppp/ppp_synctty.c | 83 +++--------------------------------
 1 file changed, 7 insertions(+), 76 deletions(-)
Reviewed-by Eric Dumazet <[email protected]>

Thanks!

Reviewed-by on v1 1/2
#3 lore raw
In reply to Qingfang Deng
Quoted 19 lines · Qingfang Deng

tty_ldisc_hangup() invokes the hangup callback while holding only a read lock on tty->ldisc_sem, so it can run concurrently with other line discipline callbacks. This currently forces async PPP to maintain separate lifetime protection around tty->disc_data.

Line discipline close is called under the write lock during hangup processing. Remove the hangup callback and rely on close for teardown, as done for SLIP by commit 23c53269f2ba ("slip: remove slip_hangup() to fix use-after-free in slip_receive_buf()"). This serializes teardown with all other line discipline operations.

disc_data_lock, refcount and completion are redundant with that serialization. Remove them and access tty->disc_data directly.

This also eliminates a lockdep warning reported by syzbot. The warning does not indicate a real deadlock because the write side runs only in process context with hardirqs disabled.

Quoted 7 lines
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by [email protected]
Closes https://lore.kernel.org/all/[email protected]/ open here
Signed-off-by Qingfang Deng <[email protected]>
---
 drivers/net/ppp/ppp_async.c | 82 ++++---------------------------------
 1 file changed, 7 insertions(+), 75 deletions(-)

Sweet.

Reviewed-by Eric Dumazet <[email protected]>
CI Applied
#4 lore raw
In reply to Qingfang Deng
This series was applied to netdev/net.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:

Quoted 5 lines · On Fri, 28 Aug 2026 15:32:36 +0800 you wrote:

tty_ldisc_hangup() invokes the hangup callback while holding only a read lock on tty->ldisc_sem, so it can run concurrently with other line discipline callbacks. This currently forces async PPP to maintain separate lifetime protection around tty->disc_data.

Quoted 7 lines

Line discipline close is called under the write lock during hangup processing. Remove the hangup callback and rely on close for teardown, as done for SLIP by commit 23c53269f2ba ("slip: remove slip_hangup() to fix use-after-free in slip_receive_buf()"). This serializes teardown with all other line discipline operations.

[...]

Here is the summary with links:
- [net,1/2] ppp: ppp_async: simplify tty disc_data access

    https://git.kernel.org/netdev/net/c/9feb069e5ed0
  - [net,2/2] ppp: ppp_synctty: simplify tty disc_data access
    https://git.kernel.org/netdev/net/c/d8d4d1cf40d5

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html