* [PATCH v3] tty: n_gsm: avoid call of sleeping functions from atomic context [not found] <20220827130349.2738-1-hdanton@sina.com> @ 2022-08-27 19:55 ` Fedor Pchelkin 2022-08-29 6:59 ` Jiri Slaby 0 siblings, 1 reply; 7+ messages in thread From: Fedor Pchelkin @ 2022-08-27 19:55 UTC (permalink / raw) To: Greg Kroah-Hartman, Daniel Starke Cc: Fedor Pchelkin, Jiri Slaby, linux-kernel, Alexey Khoroshilov, ldv-project, Hillf Danton Syzkaller reports the following problem: BUG: sleeping function called from invalid context at kernel/printk/printk.c:2347 in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 1105, name: syz-executor423 3 locks held by syz-executor423/1105: #0: ffff8881468b9098 (&tty->ldisc_sem){++++}-{0:0}, at: tty_ldisc_ref_wait+0x22/0x90 drivers/tty/tty_ldisc.c:266 #1: ffff8881468b9130 (&tty->atomic_write_lock){+.+.}-{3:3}, at: tty_write_lock drivers/tty/tty_io.c:952 [inline] #1: ffff8881468b9130 (&tty->atomic_write_lock){+.+.}-{3:3}, at: do_tty_write drivers/tty/tty_io.c:975 [inline] #1: ffff8881468b9130 (&tty->atomic_write_lock){+.+.}-{3:3}, at: file_tty_write.constprop.0+0x2a8/0x8e0 drivers/tty/tty_io.c:1118 #2: ffff88801b06c398 (&gsm->tx_lock){....}-{2:2}, at: gsmld_write+0x5e/0x150 drivers/tty/n_gsm.c:2717 irq event stamp: 3482 hardirqs last enabled at (3481): [<ffffffff81d13343>] __get_reqs_available+0x143/0x2f0 fs/aio.c:946 hardirqs last disabled at (3482): [<ffffffff87d39722>] __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:108 [inline] hardirqs last disabled at (3482): [<ffffffff87d39722>] _raw_spin_lock_irqsave+0x52/0x60 kernel/locking/spinlock.c:159 softirqs last enabled at (3408): [<ffffffff87e01002>] asm_call_irq_on_stack+0x12/0x20 softirqs last disabled at (3401): [<ffffffff87e01002>] asm_call_irq_on_stack+0x12/0x20 Preemption disabled at: [<0000000000000000>] 0x0 CPU: 2 PID: 1105 Comm: syz-executor423 Not tainted 5.10.137-syzkaller #0 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x107/0x167 lib/dump_stack.c:118 ___might_sleep.cold+0x1e8/0x22e kernel/sched/core.c:7304 console_lock+0x19/0x80 kernel/printk/printk.c:2347 do_con_write+0x113/0x1de0 drivers/tty/vt/vt.c:2909 con_write+0x22/0xc0 drivers/tty/vt/vt.c:3296 gsmld_write+0xd0/0x150 drivers/tty/n_gsm.c:2720 do_tty_write drivers/tty/tty_io.c:1028 [inline] file_tty_write.constprop.0+0x502/0x8e0 drivers/tty/tty_io.c:1118 call_write_iter include/linux/fs.h:1903 [inline] aio_write+0x355/0x7b0 fs/aio.c:1580 __io_submit_one fs/aio.c:1952 [inline] io_submit_one+0xf45/0x1a90 fs/aio.c:1999 __do_sys_io_submit fs/aio.c:2058 [inline] __se_sys_io_submit fs/aio.c:2028 [inline] __x64_sys_io_submit+0x18c/0x2f0 fs/aio.c:2028 do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46 entry_SYSCALL_64_after_hwframe+0x61/0xc6 The problem happens in the following control flow: gsmld_write(...) spin_lock_irqsave(&gsm->tx_lock, flags) // taken a spinlock on TX data con_write(...) do_con_write(...) console_lock() might_sleep() // -> bug As far as console_lock() might sleep it should not be called with spinlock held. The patch replaces tx_lock spinlock with mutex in order to avoid the problem. Also kick_timer timer_list is replaced with kick_timeout delayed_work to be able to synchronize with mutexes, as suggested by Hillf Danton <hdanton@sina.com>. Found by Linux Verification Center (linuxtesting.org) with Syzkaller. Fixes: 32dd59f96924 ("tty: n_gsm: fix race condition in gsmld_write()") Fixes: c568f7086c6e ("tty: n_gsm: fix missing timer to handle stalled links") Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> --- v1->v2: sorry, now adapted patch from 5.10 to upstream v2->v3: replaced a kick_timer with a delayed_work drivers/tty/n_gsm.c | 64 +++++++++++++++++++++------------------------ 1 file changed, 30 insertions(+), 34 deletions(-) diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c index caa5c14ed57f..31e7a91fec54 100644 --- a/drivers/tty/n_gsm.c +++ b/drivers/tty/n_gsm.c @@ -248,7 +248,7 @@ struct gsm_mux { bool constipated; /* Asked by remote to shut up */ bool has_devices; /* Devices were registered */ - spinlock_t tx_lock; + struct mutex tx_mutex; unsigned int tx_bytes; /* TX data outstanding */ #define TX_THRESH_HI 8192 #define TX_THRESH_LO 2048 @@ -256,7 +256,7 @@ struct gsm_mux { struct list_head tx_data_list; /* Pending data packets */ /* Control messages */ - struct timer_list kick_timer; /* Kick TX queuing on timeout */ + struct delayed_work kick_timeout; /* Kick TX queuing on timeout */ struct timer_list t2_timer; /* Retransmit timer for commands */ int cretries; /* Command retry counter */ struct gsm_control *pending_cmd;/* Our current pending command */ @@ -680,7 +680,6 @@ static int gsm_send(struct gsm_mux *gsm, int addr, int cr, int control) struct gsm_msg *msg; u8 *dp; int ocr; - unsigned long flags; msg = gsm_data_alloc(gsm, addr, 0, control); if (!msg) @@ -702,10 +701,10 @@ static int gsm_send(struct gsm_mux *gsm, int addr, int cr, int control) gsm_print_packet("Q->", addr, cr, control, NULL, 0); - spin_lock_irqsave(&gsm->tx_lock, flags); + mutex_lock(&gsm->tx_mutex); list_add_tail(&msg->list, &gsm->tx_ctrl_list); gsm->tx_bytes += msg->len; - spin_unlock_irqrestore(&gsm->tx_lock, flags); + mutex_unlock(&gsm->tx_mutex); gsmld_write_trigger(gsm); return 0; @@ -730,7 +729,7 @@ static void gsm_dlci_clear_queues(struct gsm_mux *gsm, struct gsm_dlci *dlci) spin_unlock_irqrestore(&dlci->lock, flags); /* Clear data packets in MUX write queue */ - spin_lock_irqsave(&gsm->tx_lock, flags); + mutex_lock(&gsm->tx_mutex); list_for_each_entry_safe(msg, nmsg, &gsm->tx_data_list, list) { if (msg->addr != addr) continue; @@ -738,7 +737,7 @@ static void gsm_dlci_clear_queues(struct gsm_mux *gsm, struct gsm_dlci *dlci) list_del(&msg->list); kfree(msg); } - spin_unlock_irqrestore(&gsm->tx_lock, flags); + mutex_unlock(&gsm->tx_mutex); } /** @@ -1009,7 +1008,7 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg) gsm->tx_bytes += msg->len; gsmld_write_trigger(gsm); - mod_timer(&gsm->kick_timer, jiffies + 10 * gsm->t1 * HZ / 100); + schedule_delayed_work(&gsm->kick_timeout, 10 * gsm->t1 * HZ / 100); } /** @@ -1024,10 +1023,9 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg) static void gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg) { - unsigned long flags; - spin_lock_irqsave(&dlci->gsm->tx_lock, flags); + mutex_lock(&dlci->gsm->tx_mutex); __gsm_data_queue(dlci, msg); - spin_unlock_irqrestore(&dlci->gsm->tx_lock, flags); + mutex_unlock(&dlci->gsm->tx_mutex); } /** @@ -1283,13 +1281,12 @@ static int gsm_dlci_data_sweep(struct gsm_mux *gsm) static void gsm_dlci_data_kick(struct gsm_dlci *dlci) { - unsigned long flags; int sweep; if (dlci->constipated) return; - spin_lock_irqsave(&dlci->gsm->tx_lock, flags); + mutex_lock(&dlci->gsm->tx_mutex); /* If we have nothing running then we need to fire up */ sweep = (dlci->gsm->tx_bytes < TX_THRESH_LO); if (dlci->gsm->tx_bytes == 0) { @@ -1300,7 +1297,7 @@ static void gsm_dlci_data_kick(struct gsm_dlci *dlci) } if (sweep) gsm_dlci_data_sweep(dlci->gsm); - spin_unlock_irqrestore(&dlci->gsm->tx_lock, flags); + mutex_unlock(&dlci->gsm->tx_mutex); } /* @@ -1984,24 +1981,23 @@ static void gsm_dlci_command(struct gsm_dlci *dlci, const u8 *data, int len) } /** - * gsm_kick_timer - transmit if possible - * @t: timer contained in our gsm object + * gsm_kick_timeout - transmit if possible + * @work: work contained in our gsm object * * Transmit data from DLCIs if the queue is empty. We can't rely on * a tty wakeup except when we filled the pipe so we need to fire off * new data ourselves in other cases. */ -static void gsm_kick_timer(struct timer_list *t) +static void gsm_kick_timeout(struct work_struct *work) { - struct gsm_mux *gsm = from_timer(gsm, t, kick_timer); - unsigned long flags; + struct gsm_mux *gsm = container_of(work, struct gsm_mux, kick_timeout.work); int sent = 0; - spin_lock_irqsave(&gsm->tx_lock, flags); + mutex_lock(&gsm->tx_mutex); /* If we have nothing running then we need to fire up */ if (gsm->tx_bytes < TX_THRESH_LO) sent = gsm_dlci_data_sweep(gsm); - spin_unlock_irqrestore(&gsm->tx_lock, flags); + mutex_unlock(&gsm->tx_mutex); if (sent && debug & 4) pr_info("%s TX queue stalled\n", __func__); @@ -2458,7 +2454,7 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm, bool disc) } /* Finish outstanding timers, making sure they are done */ - del_timer_sync(&gsm->kick_timer); + cancel_delayed_work_sync(&gsm->kick_timeout); del_timer_sync(&gsm->t2_timer); /* Finish writing to ldisc */ @@ -2501,12 +2497,12 @@ static int gsm_activate_mux(struct gsm_mux *gsm) if (dlci == NULL) return -ENOMEM; - timer_setup(&gsm->kick_timer, gsm_kick_timer, 0); + INIT_DELAYED_WORK(&gsm->kick_timeout, gsm_kick_timeout); timer_setup(&gsm->t2_timer, gsm_control_retransmit, 0); INIT_WORK(&gsm->tx_work, gsmld_write_task); init_waitqueue_head(&gsm->event); spin_lock_init(&gsm->control_lock); - spin_lock_init(&gsm->tx_lock); + mutex_init(&gsm->tx_mutex); if (gsm->encoding == 0) gsm->receive = gsm0_receive; @@ -2538,6 +2534,7 @@ static void gsm_free_mux(struct gsm_mux *gsm) break; } } + mutex_destroy(&gsm->tx_mutex); mutex_destroy(&gsm->mutex); kfree(gsm->txframe); kfree(gsm->buf); @@ -2609,6 +2606,7 @@ static struct gsm_mux *gsm_alloc_mux(void) } spin_lock_init(&gsm->lock); mutex_init(&gsm->mutex); + mutex_init(&gsm->tx_mutex); kref_init(&gsm->ref); INIT_LIST_HEAD(&gsm->tx_ctrl_list); INIT_LIST_HEAD(&gsm->tx_data_list); @@ -2636,6 +2634,7 @@ static struct gsm_mux *gsm_alloc_mux(void) } spin_unlock(&gsm_mux_lock); if (i == MAX_MUX) { + mutex_destroy(&gsm->tx_mutex); mutex_destroy(&gsm->mutex); kfree(gsm->txframe); kfree(gsm->buf); @@ -2791,17 +2790,16 @@ static void gsmld_write_trigger(struct gsm_mux *gsm) static void gsmld_write_task(struct work_struct *work) { struct gsm_mux *gsm = container_of(work, struct gsm_mux, tx_work); - unsigned long flags; int i, ret; /* All outstanding control channel and control messages and one data * frame is sent. */ ret = -ENODEV; - spin_lock_irqsave(&gsm->tx_lock, flags); + mutex_lock(&gsm->tx_mutex); if (gsm->tty) ret = gsm_data_kick(gsm); - spin_unlock_irqrestore(&gsm->tx_lock, flags); + mutex_unlock(&gsm->tx_mutex); if (ret >= 0) for (i = 0; i < NUM_DLCI; i++) @@ -2946,7 +2944,7 @@ static int gsmld_open(struct tty_struct *tty) gsmld_attach_gsm(tty, gsm); - timer_setup(&gsm->kick_timer, gsm_kick_timer, 0); + INIT_DELAYED_WORK(&gsm->kick_timeout, gsm_kick_timeout); timer_setup(&gsm->t2_timer, gsm_control_retransmit, 0); INIT_WORK(&gsm->tx_work, gsmld_write_task); @@ -3012,7 +3010,6 @@ static ssize_t gsmld_write(struct tty_struct *tty, struct file *file, const unsigned char *buf, size_t nr) { struct gsm_mux *gsm = tty->disc_data; - unsigned long flags; int space; int ret; @@ -3020,13 +3017,13 @@ static ssize_t gsmld_write(struct tty_struct *tty, struct file *file, return -ENODEV; ret = -ENOBUFS; - spin_lock_irqsave(&gsm->tx_lock, flags); + mutex_lock(&gsm->tx_mutex); space = tty_write_room(tty); if (space >= nr) ret = tty->ops->write(tty, buf, nr); else set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags); - spin_unlock_irqrestore(&gsm->tx_lock, flags); + mutex_unlock(&gsm->tx_mutex); return ret; } @@ -3323,14 +3320,13 @@ static struct tty_ldisc_ops tty_ldisc_packet = { static void gsm_modem_upd_via_data(struct gsm_dlci *dlci, u8 brk) { struct gsm_mux *gsm = dlci->gsm; - unsigned long flags; if (dlci->state != DLCI_OPEN || dlci->adaption != 2) return; - spin_lock_irqsave(&gsm->tx_lock, flags); + mutex_lock(&gsm->tx_mutex); gsm_dlci_modem_output(gsm, dlci, brk); - spin_unlock_irqrestore(&gsm->tx_lock, flags); + mutex_unlock(&gsm->tx_mutex); } /** -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3] tty: n_gsm: avoid call of sleeping functions from atomic context 2022-08-27 19:55 ` [PATCH v3] tty: n_gsm: avoid call of sleeping functions from atomic context Fedor Pchelkin @ 2022-08-29 6:59 ` Jiri Slaby 2022-08-29 13:16 ` [PATCH v4 0/2] " Fedor Pchelkin 0 siblings, 1 reply; 7+ messages in thread From: Jiri Slaby @ 2022-08-29 6:59 UTC (permalink / raw) To: Fedor Pchelkin, Greg Kroah-Hartman, Daniel Starke Cc: linux-kernel, Alexey Khoroshilov, ldv-project, Hillf Danton On 27. 08. 22, 21:55, Fedor Pchelkin wrote: > Syzkaller reports the following problem: ... > The patch replaces tx_lock spinlock with mutex in order to avoid the > problem. Also kick_timer timer_list is replaced with kick_timeout > delayed_work to be able to synchronize with mutexes, as suggested by > Hillf Danton <hdanton@sina.com>. > > Found by Linux Verification Center (linuxtesting.org) with Syzkaller. > > Fixes: 32dd59f96924 ("tty: n_gsm: fix race condition in gsmld_write()") > Fixes: c568f7086c6e ("tty: n_gsm: fix missing timer to handle stalled links") > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> > --- > v1->v2: sorry, now adapted patch from 5.10 to upstream > v2->v3: replaced a kick_timer with a delayed_work Please do so separately. That is, split the below 2 changes into two patches. > --- a/drivers/tty/n_gsm.c > +++ b/drivers/tty/n_gsm.c > @@ -248,7 +248,7 @@ struct gsm_mux { > bool constipated; /* Asked by remote to shut up */ > bool has_devices; /* Devices were registered */ > > - spinlock_t tx_lock; > + struct mutex tx_mutex; > unsigned int tx_bytes; /* TX data outstanding */ > #define TX_THRESH_HI 8192 > #define TX_THRESH_LO 2048 > @@ -256,7 +256,7 @@ struct gsm_mux { > struct list_head tx_data_list; /* Pending data packets */ > > /* Control messages */ > - struct timer_list kick_timer; /* Kick TX queuing on timeout */ > + struct delayed_work kick_timeout; /* Kick TX queuing on timeout */ > struct timer_list t2_timer; /* Retransmit timer for commands */ > int cretries; /* Command retry counter */ > struct gsm_control *pending_cmd;/* Our current pending command */ thanks, -- js suse labs ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4 0/2] tty: n_gsm: avoid call of sleeping functions from atomic context 2022-08-29 6:59 ` Jiri Slaby @ 2022-08-29 13:16 ` Fedor Pchelkin 2022-08-29 13:16 ` [PATCH v4 1/2] tty: n_gsm: replace kicktimer with delayed_work Fedor Pchelkin 2022-08-29 13:16 ` [PATCH v4 2/2] tty: n_gsm: avoid call of sleeping functions from atomic context Fedor Pchelkin 0 siblings, 2 replies; 7+ messages in thread From: Fedor Pchelkin @ 2022-08-29 13:16 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Fedor Pchelkin, Jiri Slaby, linux-kernel, Alexey Khoroshilov, ldv-project, Hillf Danton Syzkaller reports calling of sleeping functions from atomic context. This is fixed by the following patch series. 1/2 is a needed prerequisite patch for the fixes made in 2/2 to be integrated. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4 1/2] tty: n_gsm: replace kicktimer with delayed_work 2022-08-29 13:16 ` [PATCH v4 0/2] " Fedor Pchelkin @ 2022-08-29 13:16 ` Fedor Pchelkin 2022-08-30 7:30 ` Jiri Slaby 2022-08-29 13:16 ` [PATCH v4 2/2] tty: n_gsm: avoid call of sleeping functions from atomic context Fedor Pchelkin 1 sibling, 1 reply; 7+ messages in thread From: Fedor Pchelkin @ 2022-08-29 13:16 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Fedor Pchelkin, Jiri Slaby, linux-kernel, Alexey Khoroshilov, ldv-project, Hillf Danton A kick_timer timer_list is replaced with kick_timeout delayed_work to be able to synchronize with mutexes as a prerequisite for the introduction of tx_mutex. Found by Linux Verification Center (linuxtesting.org) with Syzkaller. Fixes: c568f7086c6e ("tty: n_gsm: fix missing timer to handle stalled links") Suggested-by: Hillf Danton <hdanton@sina.com> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> --- v1->v2: sorry, now adapted patch from 5.10 to upstream v2->v3: replaced a kick_timer with a delayed_work v3->v4: separated kick_timer and tx_mutex into different patches drivers/tty/n_gsm.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c index caa5c14ed57f..c4164c85ffd4 100644 --- a/drivers/tty/n_gsm.c +++ b/drivers/tty/n_gsm.c @@ -256,7 +256,7 @@ struct gsm_mux { struct list_head tx_data_list; /* Pending data packets */ /* Control messages */ - struct timer_list kick_timer; /* Kick TX queuing on timeout */ + struct delayed_work kick_timeout; /* Kick TX queuing on timeout */ struct timer_list t2_timer; /* Retransmit timer for commands */ int cretries; /* Command retry counter */ struct gsm_control *pending_cmd;/* Our current pending command */ @@ -1009,7 +1009,7 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg) gsm->tx_bytes += msg->len; gsmld_write_trigger(gsm); - mod_timer(&gsm->kick_timer, jiffies + 10 * gsm->t1 * HZ / 100); + schedule_delayed_work(&gsm->kick_timeout, 10 * gsm->t1 * HZ / 100); } /** @@ -1984,16 +1984,16 @@ static void gsm_dlci_command(struct gsm_dlci *dlci, const u8 *data, int len) } /** - * gsm_kick_timer - transmit if possible - * @t: timer contained in our gsm object + * gsm_kick_timeout - transmit if possible + * @work: work contained in our gsm object * * Transmit data from DLCIs if the queue is empty. We can't rely on * a tty wakeup except when we filled the pipe so we need to fire off * new data ourselves in other cases. */ -static void gsm_kick_timer(struct timer_list *t) +static void gsm_kick_timeout(struct work_struct *work) { - struct gsm_mux *gsm = from_timer(gsm, t, kick_timer); + struct gsm_mux *gsm = container_of(work, struct gsm_mux, kick_timeout.work); unsigned long flags; int sent = 0; @@ -2458,7 +2458,7 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm, bool disc) } /* Finish outstanding timers, making sure they are done */ - del_timer_sync(&gsm->kick_timer); + cancel_delayed_work_sync(&gsm->kick_timeout); del_timer_sync(&gsm->t2_timer); /* Finish writing to ldisc */ @@ -2501,7 +2501,7 @@ static int gsm_activate_mux(struct gsm_mux *gsm) if (dlci == NULL) return -ENOMEM; - timer_setup(&gsm->kick_timer, gsm_kick_timer, 0); + INIT_DELAYED_WORK(&gsm->kick_timeout, gsm_kick_timeout); timer_setup(&gsm->t2_timer, gsm_control_retransmit, 0); INIT_WORK(&gsm->tx_work, gsmld_write_task); init_waitqueue_head(&gsm->event); @@ -2946,7 +2946,7 @@ static int gsmld_open(struct tty_struct *tty) gsmld_attach_gsm(tty, gsm); - timer_setup(&gsm->kick_timer, gsm_kick_timer, 0); + INIT_DELAYED_WORK(&gsm->kick_timeout, gsm_kick_timeout); timer_setup(&gsm->t2_timer, gsm_control_retransmit, 0); INIT_WORK(&gsm->tx_work, gsmld_write_task); -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4 1/2] tty: n_gsm: replace kicktimer with delayed_work 2022-08-29 13:16 ` [PATCH v4 1/2] tty: n_gsm: replace kicktimer with delayed_work Fedor Pchelkin @ 2022-08-30 7:30 ` Jiri Slaby 2022-08-30 12:38 ` Greg Kroah-Hartman 0 siblings, 1 reply; 7+ messages in thread From: Jiri Slaby @ 2022-08-30 7:30 UTC (permalink / raw) To: Fedor Pchelkin, Greg Kroah-Hartman Cc: linux-kernel, Alexey Khoroshilov, ldv-project, Hillf Danton On 29. 08. 22, 15:16, Fedor Pchelkin wrote: > A kick_timer timer_list is replaced with kick_timeout delayed_work to be > able to synchronize with mutexes as a prerequisite for the introduction > of tx_mutex. > > Found by Linux Verification Center (linuxtesting.org) with Syzkaller. Reviewed-by: Jiri Slaby <jirislaby@kernel.org> But I think this conflicts with Tetsuo's cleanup [1]. So one of you will likely have to rebase and resubmit. [1] https://lore.kernel.org/all/2110618e-57f0-c1ce-b2ad-b6cacef3f60e@I-love.SAKURA.ne.jp/ > Fixes: c568f7086c6e ("tty: n_gsm: fix missing timer to handle stalled links") > Suggested-by: Hillf Danton <hdanton@sina.com> > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> > --- > v1->v2: sorry, now adapted patch from 5.10 to upstream > v2->v3: replaced a kick_timer with a delayed_work > v3->v4: separated kick_timer and tx_mutex into different patches > > drivers/tty/n_gsm.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c > index caa5c14ed57f..c4164c85ffd4 100644 > --- a/drivers/tty/n_gsm.c > +++ b/drivers/tty/n_gsm.c > @@ -256,7 +256,7 @@ struct gsm_mux { > struct list_head tx_data_list; /* Pending data packets */ > > /* Control messages */ > - struct timer_list kick_timer; /* Kick TX queuing on timeout */ > + struct delayed_work kick_timeout; /* Kick TX queuing on timeout */ > struct timer_list t2_timer; /* Retransmit timer for commands */ > int cretries; /* Command retry counter */ > struct gsm_control *pending_cmd;/* Our current pending command */ > @@ -1009,7 +1009,7 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg) > gsm->tx_bytes += msg->len; > > gsmld_write_trigger(gsm); > - mod_timer(&gsm->kick_timer, jiffies + 10 * gsm->t1 * HZ / 100); > + schedule_delayed_work(&gsm->kick_timeout, 10 * gsm->t1 * HZ / 100); > } > > /** > @@ -1984,16 +1984,16 @@ static void gsm_dlci_command(struct gsm_dlci *dlci, const u8 *data, int len) > } > > /** > - * gsm_kick_timer - transmit if possible > - * @t: timer contained in our gsm object > + * gsm_kick_timeout - transmit if possible > + * @work: work contained in our gsm object > * > * Transmit data from DLCIs if the queue is empty. We can't rely on > * a tty wakeup except when we filled the pipe so we need to fire off > * new data ourselves in other cases. > */ > -static void gsm_kick_timer(struct timer_list *t) > +static void gsm_kick_timeout(struct work_struct *work) > { > - struct gsm_mux *gsm = from_timer(gsm, t, kick_timer); > + struct gsm_mux *gsm = container_of(work, struct gsm_mux, kick_timeout.work); > unsigned long flags; > int sent = 0; > > @@ -2458,7 +2458,7 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm, bool disc) > } > > /* Finish outstanding timers, making sure they are done */ > - del_timer_sync(&gsm->kick_timer); > + cancel_delayed_work_sync(&gsm->kick_timeout); > del_timer_sync(&gsm->t2_timer); > > /* Finish writing to ldisc */ > @@ -2501,7 +2501,7 @@ static int gsm_activate_mux(struct gsm_mux *gsm) > if (dlci == NULL) > return -ENOMEM; > > - timer_setup(&gsm->kick_timer, gsm_kick_timer, 0); > + INIT_DELAYED_WORK(&gsm->kick_timeout, gsm_kick_timeout); > timer_setup(&gsm->t2_timer, gsm_control_retransmit, 0); > INIT_WORK(&gsm->tx_work, gsmld_write_task); > init_waitqueue_head(&gsm->event); > @@ -2946,7 +2946,7 @@ static int gsmld_open(struct tty_struct *tty) > > gsmld_attach_gsm(tty, gsm); > > - timer_setup(&gsm->kick_timer, gsm_kick_timer, 0); > + INIT_DELAYED_WORK(&gsm->kick_timeout, gsm_kick_timeout); > timer_setup(&gsm->t2_timer, gsm_control_retransmit, 0); > INIT_WORK(&gsm->tx_work, gsmld_write_task); > -- js suse labs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 1/2] tty: n_gsm: replace kicktimer with delayed_work 2022-08-30 7:30 ` Jiri Slaby @ 2022-08-30 12:38 ` Greg Kroah-Hartman 0 siblings, 0 replies; 7+ messages in thread From: Greg Kroah-Hartman @ 2022-08-30 12:38 UTC (permalink / raw) To: Jiri Slaby Cc: Fedor Pchelkin, linux-kernel, Alexey Khoroshilov, ldv-project, Hillf Danton On Tue, Aug 30, 2022 at 09:30:27AM +0200, Jiri Slaby wrote: > On 29. 08. 22, 15:16, Fedor Pchelkin wrote: > > A kick_timer timer_list is replaced with kick_timeout delayed_work to be > > able to synchronize with mutexes as a prerequisite for the introduction > > of tx_mutex. > > > > Found by Linux Verification Center (linuxtesting.org) with Syzkaller. > > Reviewed-by: Jiri Slaby <jirislaby@kernel.org> > > But I think this conflicts with Tetsuo's cleanup [1]. So one of you will > likely have to rebase and resubmit. > > [1] https://lore.kernel.org/all/2110618e-57f0-c1ce-b2ad-b6cacef3f60e@I-love.SAKURA.ne.jp/ I've fixed it up by hand now, no worries. greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4 2/2] tty: n_gsm: avoid call of sleeping functions from atomic context 2022-08-29 13:16 ` [PATCH v4 0/2] " Fedor Pchelkin 2022-08-29 13:16 ` [PATCH v4 1/2] tty: n_gsm: replace kicktimer with delayed_work Fedor Pchelkin @ 2022-08-29 13:16 ` Fedor Pchelkin 1 sibling, 0 replies; 7+ messages in thread From: Fedor Pchelkin @ 2022-08-29 13:16 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Fedor Pchelkin, Jiri Slaby, linux-kernel, Alexey Khoroshilov, ldv-project, Hillf Danton Syzkaller reports the following problem: BUG: sleeping function called from invalid context at kernel/printk/printk.c:2347 in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 1105, name: syz-executor423 3 locks held by syz-executor423/1105: #0: ffff8881468b9098 (&tty->ldisc_sem){++++}-{0:0}, at: tty_ldisc_ref_wait+0x22/0x90 drivers/tty/tty_ldisc.c:266 #1: ffff8881468b9130 (&tty->atomic_write_lock){+.+.}-{3:3}, at: tty_write_lock drivers/tty/tty_io.c:952 [inline] #1: ffff8881468b9130 (&tty->atomic_write_lock){+.+.}-{3:3}, at: do_tty_write drivers/tty/tty_io.c:975 [inline] #1: ffff8881468b9130 (&tty->atomic_write_lock){+.+.}-{3:3}, at: file_tty_write.constprop.0+0x2a8/0x8e0 drivers/tty/tty_io.c:1118 #2: ffff88801b06c398 (&gsm->tx_lock){....}-{2:2}, at: gsmld_write+0x5e/0x150 drivers/tty/n_gsm.c:2717 irq event stamp: 3482 hardirqs last enabled at (3481): [<ffffffff81d13343>] __get_reqs_available+0x143/0x2f0 fs/aio.c:946 hardirqs last disabled at (3482): [<ffffffff87d39722>] __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:108 [inline] hardirqs last disabled at (3482): [<ffffffff87d39722>] _raw_spin_lock_irqsave+0x52/0x60 kernel/locking/spinlock.c:159 softirqs last enabled at (3408): [<ffffffff87e01002>] asm_call_irq_on_stack+0x12/0x20 softirqs last disabled at (3401): [<ffffffff87e01002>] asm_call_irq_on_stack+0x12/0x20 Preemption disabled at: [<0000000000000000>] 0x0 CPU: 2 PID: 1105 Comm: syz-executor423 Not tainted 5.10.137-syzkaller #0 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x107/0x167 lib/dump_stack.c:118 ___might_sleep.cold+0x1e8/0x22e kernel/sched/core.c:7304 console_lock+0x19/0x80 kernel/printk/printk.c:2347 do_con_write+0x113/0x1de0 drivers/tty/vt/vt.c:2909 con_write+0x22/0xc0 drivers/tty/vt/vt.c:3296 gsmld_write+0xd0/0x150 drivers/tty/n_gsm.c:2720 do_tty_write drivers/tty/tty_io.c:1028 [inline] file_tty_write.constprop.0+0x502/0x8e0 drivers/tty/tty_io.c:1118 call_write_iter include/linux/fs.h:1903 [inline] aio_write+0x355/0x7b0 fs/aio.c:1580 __io_submit_one fs/aio.c:1952 [inline] io_submit_one+0xf45/0x1a90 fs/aio.c:1999 __do_sys_io_submit fs/aio.c:2058 [inline] __se_sys_io_submit fs/aio.c:2028 [inline] __x64_sys_io_submit+0x18c/0x2f0 fs/aio.c:2028 do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46 entry_SYSCALL_64_after_hwframe+0x61/0xc6 The problem happens in the following control flow: gsmld_write(...) spin_lock_irqsave(&gsm->tx_lock, flags) // taken a spinlock on TX data con_write(...) do_con_write(...) console_lock() might_sleep() // -> bug As far as console_lock() might sleep it should not be called with spinlock held. The patch replaces tx_lock spinlock with mutex in order to avoid the problem. Found by Linux Verification Center (linuxtesting.org) with Syzkaller. Fixes: 32dd59f96924 ("tty: n_gsm: fix race condition in gsmld_write()") Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> --- v1->v2: sorry, now adapted patch from 5.10 to upstream v2->v3: replaced a kick_timer with a delayed_work v3->v4: separated kick_timer and tx_mutex into different patches drivers/tty/n_gsm.c | 54 +++++++++++++++++++++------------------------ 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c index c4164c85ffd4..715ae2f1e90e 100644 --- a/drivers/tty/n_gsm.c +++ b/drivers/tty/n_gsm.c @@ -248,7 +248,7 @@ struct gsm_mux { bool constipated; /* Asked by remote to shut up */ bool has_devices; /* Devices were registered */ - spinlock_t tx_lock; + struct mutex tx_mutex; unsigned int tx_bytes; /* TX data outstanding */ #define TX_THRESH_HI 8192 #define TX_THRESH_LO 2048 @@ -680,7 +680,6 @@ static int gsm_send(struct gsm_mux *gsm, int addr, int cr, int control) struct gsm_msg *msg; u8 *dp; int ocr; - unsigned long flags; msg = gsm_data_alloc(gsm, addr, 0, control); if (!msg) @@ -702,10 +701,10 @@ static int gsm_send(struct gsm_mux *gsm, int addr, int cr, int control) gsm_print_packet("Q->", addr, cr, control, NULL, 0); - spin_lock_irqsave(&gsm->tx_lock, flags); + mutex_lock(&gsm->tx_mutex); list_add_tail(&msg->list, &gsm->tx_ctrl_list); gsm->tx_bytes += msg->len; - spin_unlock_irqrestore(&gsm->tx_lock, flags); + mutex_unlock(&gsm->tx_mutex); gsmld_write_trigger(gsm); return 0; @@ -730,7 +729,7 @@ static void gsm_dlci_clear_queues(struct gsm_mux *gsm, struct gsm_dlci *dlci) spin_unlock_irqrestore(&dlci->lock, flags); /* Clear data packets in MUX write queue */ - spin_lock_irqsave(&gsm->tx_lock, flags); + mutex_lock(&gsm->tx_mutex); list_for_each_entry_safe(msg, nmsg, &gsm->tx_data_list, list) { if (msg->addr != addr) continue; @@ -738,7 +737,7 @@ static void gsm_dlci_clear_queues(struct gsm_mux *gsm, struct gsm_dlci *dlci) list_del(&msg->list); kfree(msg); } - spin_unlock_irqrestore(&gsm->tx_lock, flags); + mutex_unlock(&gsm->tx_mutex); } /** @@ -1024,10 +1023,9 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg) static void gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg) { - unsigned long flags; - spin_lock_irqsave(&dlci->gsm->tx_lock, flags); + mutex_lock(&dlci->gsm->tx_mutex); __gsm_data_queue(dlci, msg); - spin_unlock_irqrestore(&dlci->gsm->tx_lock, flags); + mutex_unlock(&dlci->gsm->tx_mutex); } /** @@ -1039,7 +1037,7 @@ static void gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg) * is data. Keep to the MRU of the mux. This path handles the usual tty * interface which is a byte stream with optional modem data. * - * Caller must hold the tx_lock of the mux. + * Caller must hold the tx_mutex of the mux. */ static int gsm_dlci_data_output(struct gsm_mux *gsm, struct gsm_dlci *dlci) @@ -1099,7 +1097,7 @@ static int gsm_dlci_data_output(struct gsm_mux *gsm, struct gsm_dlci *dlci) * is data. Keep to the MRU of the mux. This path handles framed data * queued as skbuffs to the DLCI. * - * Caller must hold the tx_lock of the mux. + * Caller must hold the tx_mutex of the mux. */ static int gsm_dlci_data_output_framed(struct gsm_mux *gsm, @@ -1115,7 +1113,7 @@ static int gsm_dlci_data_output_framed(struct gsm_mux *gsm, if (dlci->adaption == 4) overhead = 1; - /* dlci->skb is locked by tx_lock */ + /* dlci->skb is locked by tx_mutex */ if (dlci->skb == NULL) { dlci->skb = skb_dequeue_tail(&dlci->skb_list); if (dlci->skb == NULL) @@ -1169,7 +1167,7 @@ static int gsm_dlci_data_output_framed(struct gsm_mux *gsm, * Push an empty frame in to the transmit queue to update the modem status * bits and to transmit an optional break. * - * Caller must hold the tx_lock of the mux. + * Caller must hold the tx_mutex of the mux. */ static int gsm_dlci_modem_output(struct gsm_mux *gsm, struct gsm_dlci *dlci, @@ -1283,13 +1281,12 @@ static int gsm_dlci_data_sweep(struct gsm_mux *gsm) static void gsm_dlci_data_kick(struct gsm_dlci *dlci) { - unsigned long flags; int sweep; if (dlci->constipated) return; - spin_lock_irqsave(&dlci->gsm->tx_lock, flags); + mutex_lock(&dlci->gsm->tx_mutex); /* If we have nothing running then we need to fire up */ sweep = (dlci->gsm->tx_bytes < TX_THRESH_LO); if (dlci->gsm->tx_bytes == 0) { @@ -1300,7 +1297,7 @@ static void gsm_dlci_data_kick(struct gsm_dlci *dlci) } if (sweep) gsm_dlci_data_sweep(dlci->gsm); - spin_unlock_irqrestore(&dlci->gsm->tx_lock, flags); + mutex_unlock(&dlci->gsm->tx_mutex); } /* @@ -1994,14 +1991,13 @@ static void gsm_dlci_command(struct gsm_dlci *dlci, const u8 *data, int len) static void gsm_kick_timeout(struct work_struct *work) { struct gsm_mux *gsm = container_of(work, struct gsm_mux, kick_timeout.work); - unsigned long flags; int sent = 0; - spin_lock_irqsave(&gsm->tx_lock, flags); + mutex_lock(&gsm->tx_mutex); /* If we have nothing running then we need to fire up */ if (gsm->tx_bytes < TX_THRESH_LO) sent = gsm_dlci_data_sweep(gsm); - spin_unlock_irqrestore(&gsm->tx_lock, flags); + mutex_unlock(&gsm->tx_mutex); if (sent && debug & 4) pr_info("%s TX queue stalled\n", __func__); @@ -2506,7 +2502,7 @@ static int gsm_activate_mux(struct gsm_mux *gsm) INIT_WORK(&gsm->tx_work, gsmld_write_task); init_waitqueue_head(&gsm->event); spin_lock_init(&gsm->control_lock); - spin_lock_init(&gsm->tx_lock); + mutex_init(&gsm->tx_mutex); if (gsm->encoding == 0) gsm->receive = gsm0_receive; @@ -2538,6 +2534,7 @@ static void gsm_free_mux(struct gsm_mux *gsm) break; } } + mutex_destroy(&gsm->tx_mutex); mutex_destroy(&gsm->mutex); kfree(gsm->txframe); kfree(gsm->buf); @@ -2609,6 +2606,7 @@ static struct gsm_mux *gsm_alloc_mux(void) } spin_lock_init(&gsm->lock); mutex_init(&gsm->mutex); + mutex_init(&gsm->tx_mutex); kref_init(&gsm->ref); INIT_LIST_HEAD(&gsm->tx_ctrl_list); INIT_LIST_HEAD(&gsm->tx_data_list); @@ -2636,6 +2634,7 @@ static struct gsm_mux *gsm_alloc_mux(void) } spin_unlock(&gsm_mux_lock); if (i == MAX_MUX) { + mutex_destroy(&gsm->tx_mutex); mutex_destroy(&gsm->mutex); kfree(gsm->txframe); kfree(gsm->buf); @@ -2791,17 +2790,16 @@ static void gsmld_write_trigger(struct gsm_mux *gsm) static void gsmld_write_task(struct work_struct *work) { struct gsm_mux *gsm = container_of(work, struct gsm_mux, tx_work); - unsigned long flags; int i, ret; /* All outstanding control channel and control messages and one data * frame is sent. */ ret = -ENODEV; - spin_lock_irqsave(&gsm->tx_lock, flags); + mutex_lock(&gsm->tx_mutex); if (gsm->tty) ret = gsm_data_kick(gsm); - spin_unlock_irqrestore(&gsm->tx_lock, flags); + mutex_unlock(&gsm->tx_mutex); if (ret >= 0) for (i = 0; i < NUM_DLCI; i++) @@ -3012,7 +3010,6 @@ static ssize_t gsmld_write(struct tty_struct *tty, struct file *file, const unsigned char *buf, size_t nr) { struct gsm_mux *gsm = tty->disc_data; - unsigned long flags; int space; int ret; @@ -3020,13 +3017,13 @@ static ssize_t gsmld_write(struct tty_struct *tty, struct file *file, return -ENODEV; ret = -ENOBUFS; - spin_lock_irqsave(&gsm->tx_lock, flags); + mutex_lock(&gsm->tx_mutex); space = tty_write_room(tty); if (space >= nr) ret = tty->ops->write(tty, buf, nr); else set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags); - spin_unlock_irqrestore(&gsm->tx_lock, flags); + mutex_unlock(&gsm->tx_mutex); return ret; } @@ -3323,14 +3320,13 @@ static struct tty_ldisc_ops tty_ldisc_packet = { static void gsm_modem_upd_via_data(struct gsm_dlci *dlci, u8 brk) { struct gsm_mux *gsm = dlci->gsm; - unsigned long flags; if (dlci->state != DLCI_OPEN || dlci->adaption != 2) return; - spin_lock_irqsave(&gsm->tx_lock, flags); + mutex_lock(&gsm->tx_mutex); gsm_dlci_modem_output(gsm, dlci, brk); - spin_unlock_irqrestore(&gsm->tx_lock, flags); + mutex_unlock(&gsm->tx_mutex); } /** -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-08-30 12:38 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20220827130349.2738-1-hdanton@sina.com> 2022-08-27 19:55 ` [PATCH v3] tty: n_gsm: avoid call of sleeping functions from atomic context Fedor Pchelkin 2022-08-29 6:59 ` Jiri Slaby 2022-08-29 13:16 ` [PATCH v4 0/2] " Fedor Pchelkin 2022-08-29 13:16 ` [PATCH v4 1/2] tty: n_gsm: replace kicktimer with delayed_work Fedor Pchelkin 2022-08-30 7:30 ` Jiri Slaby 2022-08-30 12:38 ` Greg Kroah-Hartman 2022-08-29 13:16 ` [PATCH v4 2/2] tty: n_gsm: avoid call of sleeping functions from atomic context Fedor Pchelkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).