6pack: drop redundant locking and refcounting
Subsystems (2)
PATCH v2 1/1 6pack: drop redundant locking and refcounting
The TTY layer already serializes line discipline operations with tty->ldisc_sem, so the extra disc_data_lock and refcnt in 6pack are unnecessary.
Removing them simplifies the code and also resolves a lockdep warning reported by syzbot. The warning did not indicate a real deadlock, since the write-side lock was only taken in process context with hardirqs disabled.
---
v2: add Closes tag
- https://lore.kernel.org/netdev/[email protected]/
drivers/net/hamradio/6pack.c | 57 ++++-------------------------------- 1 file changed, 5 insertions(+), 52 deletions(-)
drivers/net/hamradio/6pack.c +5 -52
@@ -115,8 +115,6 @@ struct sixpack {struct timer_list tx_t;struct timer_list resync_t; -refcount_t refcnt; -struct completion dead;spinlock_t lock; };
@@ -353,42 +351,13 @@ static void sp_bump(struct sixpack *sp, char cmd)/* ----------------------------------------------------------------------- */ -/* -* 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 -* sixpack_receive_buf while another calls sixpack_close, which zeroes -* tty->disc_data and frees the memory that sixpack_receive_buf 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. -*/ -static DEFINE_RWLOCK (disc_data_lock); - -static struct sixpack *sp_get (struct tty_struct *tty) -{ -struct sixpack *sp; - -read_lock (&disc_data_lock); - sp = tty->disc_data; -if (sp) -refcount_inc (&sp->refcnt); -read_unlock (&disc_data_lock); - -return sp; -} - -static void sp_put (struct sixpack *sp) -{ -if (refcount_dec_and_test (&sp->refcnt)) -complete (&sp->dead); -} -/* * Called by the TTY driver when there's room for more data. If we have * more packets to send, we send them here. */ static void sixpack_write_wakeup (struct tty_struct *tty) { -struct sixpack *sp =sp_get (tty); +struct sixpack *sp = tty->disc_data;int actual;if (!sp)
@@ -400,7 +369,7 @@ static void sixpack_write_wakeup(struct tty_struct *tty)clear_bit (TTY_DO_WRITE_WAKEUP , &tty->flags); sp->tx_enable =0 ;netif_wake_queue (sp->dev); - goto out; +return ; }if (sp->tx_enable) {
@@ -408,9 +377,6 @@ static void sixpack_write_wakeup(struct tty_struct *tty) sp->xleft -= actual; sp->xhead += actual; } - -out: -sp_put (sp); }/* ----------------------------------------------------------------------- */
@@ -430,7 +396,7 @@ static void sixpack_receive_buf(struct tty_struct *tty, const u8 *cp,if (!count)return ; - sp =sp_get (tty); + sp = tty->disc_data;if (!sp)return ;
@@ -446,7 +412,6 @@ static void sixpack_receive_buf(struct tty_struct *tty, const u8 *cp, }sixpack_decode (sp, cp, count1); -sp_put (sp);tty_unthrottle (tty); }
@@ -561,8 +526,6 @@ static int sixpack_open(struct tty_struct *tty)spin_lock_init (&sp->lock);spin_lock_init (&sp->rxlock); -refcount_set (&sp->refcnt,1 ); -init_completion (&sp->dead);/* !!! length of the buffers. MTU is IP MTU, not PACLEN! */
@@ -638,19 +601,11 @@ static void sixpack_close(struct tty_struct *tty) {struct sixpack *sp; -write_lock_irq (&disc_data_lock); sp = tty->disc_data; - tty->disc_data =NULL ; -write_unlock_irq (&disc_data_lock);if (!sp)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. -*/ -if (!refcount_dec_and_test (&sp->refcnt)) -wait_for_completion (&sp->dead); + tty->disc_data =NULL ;/* We must stop the queue to avoid potentially scribbling * on the free buffers. The sp->dead completion is not sufficient
@@ -673,7 +628,7 @@ static void sixpack_close(struct tty_struct *tty)static int sixpack_ioctl (struct tty_struct *tty,unsigned int cmd,unsigned long arg) { -struct sixpack *sp =sp_get (tty); +struct sixpack *sp = tty->disc_data;struct net_device *dev;unsigned int tmp, err;
@@ -725,8 +680,6 @@ static int sixpack_ioctl(struct tty_struct *tty, unsigned int cmd, err =tty_mode_ioctl (tty, cmd, arg); } -sp_put (sp); -return err; }
Signature · 1 line
2.43.0
Quoted 9 lines · Qingfang Deng
The TTY layer already serializes line discipline operations with
tty->ldisc_sem, so the extradisc_data_lockand refcnt in 6pack are unnecessary.Removing them simplifies the code and also resolves a lockdep warning reported by syzbot. The warning did not indicate a real deadlock, since the write-side lock was only taken in process context with hardirqs disabled.
Quoted 7 lines
Reported-by [email protected]Signed-off-by Qingfang Deng <[email protected]>---
v2: add Closes tag
- https://lore.kernel.org/netdev/[email protected]/
Thanks!
regards,
dan carpenter
This patch was applied to netdev/net-next.git (main) by Paolo Abeni <[email protected]>:
Hello:
This patch was applied to netdev/net-next.git (main) by Paolo Abeni <[email protected]>:
Quoted 10 lines · On Thu, 25 Sep 2025 13:10:59 +0800 you wrote:
The TTY layer already serializes line discipline operations with
tty->ldisc_sem, so the extradisc_data_lockand refcnt in 6pack are unnecessary.Removing them simplifies the code and also resolves a lockdep warning reported by syzbot. The warning did not indicate a real deadlock, since the write-side lock was only taken in process context with hardirqs disabled.
[...]
Here is the summary with links:
- [net-next,v2] 6pack: drop redundant locking and refcounting
https://git.kernel.org/netdev/net-next/c/38b04ed7072e
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html