drivers/net/hamradio/6pack.c | 57 ++++-------------------------------- 1 file changed, 5 insertions(+), 52 deletions(-)
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.
Reported-by: syzbot+5fd749c74105b0e1b302@syzkaller.appspotmail.com
Signed-off-by: Qingfang Deng <dqfext@gmail.com>
---
drivers/net/hamradio/6pack.c | 57 ++++--------------------------------
1 file changed, 5 insertions(+), 52 deletions(-)
diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
index c5e5423e1863..885992951e8a 100644
--- a/drivers/net/hamradio/6pack.c
+++ b/drivers/net/hamradio/6pack.c
@@ -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;
}
--
2.43.0On Tue, Sep 23, 2025 at 02:07:06PM +0800, Qingfang Deng wrote: > 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. > > Reported-by: syzbot+5fd749c74105b0e1b302@syzkaller.appspotmail.com > Signed-off-by: Qingfang Deng <dqfext@gmail.com> checkpatch says: WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report Which is relevant here because Google has apparently deleted their search button and is only displaying the AI button. "The email address syzbot+5fd749c74105b0e1b302@syzkaller.appspotmail.com is an automated sender used by ..." Thanks, AI! I can still press enter to do a Google search but there are no results with syzbot ID. I can't find a search button on the syzbot website. Ah. Let's check lore. Hooray! How did we ever survive before lore? https://lore.kernel.org/all/000000000000e8231f0601095c8e@google.com/ Please add the Closes tag and resend. Otherwise it looks good. Thanks! This code was copy and pasted from drivers/net/ppp/ppp_synctty.c btw so that's a similar thing if anyone wants to fix that. KTODO: remove sp_get/put() from ppp_synctty.c regards, dan carpenter
On Tue, Sep 23, 2025 at 3:11 PM Dan Carpenter <dan.carpenter@linaro.org> wrote: > checkpatch says: > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > > Which is relevant here because Google has apparently deleted their > search button and is only displaying the AI button. "The email address > syzbot+5fd749c74105b0e1b302@syzkaller.appspotmail.com is an automated > sender used by ..." Thanks, AI! I can still press enter to do a Google > search but there are no results with syzbot ID. > > I can't find a search button on the syzbot website. > > Ah. Let's check lore. Hooray! How did we ever survive before lore? > https://lore.kernel.org/all/000000000000e8231f0601095c8e@google.com/ > > Please add the Closes tag and resend. Otherwise it looks good. Thanks! checkpatch also says: WARNING: The commit message has 'syzkaller', perhaps it also needs a 'Fixes:' tag? Should I add a Fixes tag, even though this is not a bug in the code? > > This code was copy and pasted from drivers/net/ppp/ppp_synctty.c btw so > that's a similar thing if anyone wants to fix that. > > KTODO: remove sp_get/put() from ppp_synctty.c > > regards, > dan carpenter > > -- Qingfang
On Tue, Sep 23, 2025 at 04:10:07PM +0800, Qingfang Deng wrote: > On Tue, Sep 23, 2025 at 3:11 PM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > checkpatch says: > > > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > > > > Which is relevant here because Google has apparently deleted their > > search button and is only displaying the AI button. "The email address > > syzbot+5fd749c74105b0e1b302@syzkaller.appspotmail.com is an automated > > sender used by ..." Thanks, AI! I can still press enter to do a Google > > search but there are no results with syzbot ID. > > > > I can't find a search button on the syzbot website. > > > > Ah. Let's check lore. Hooray! How did we ever survive before lore? > > https://lore.kernel.org/all/000000000000e8231f0601095c8e@google.com/ > > > > Please add the Closes tag and resend. Otherwise it looks good. Thanks! > > checkpatch also says: > WARNING: The commit message has 'syzkaller', perhaps it also needs a > 'Fixes:' tag? > > Should I add a Fixes tag, even though this is not a bug in the code? > I don't have strong feelings about this since it doesn't affect real life users. Some people would say yes, other people would say no. Probably you should since it technically is a bug. regards, dan carpenter
© 2016 - 2025 Red Hat, Inc.