lkml.org 
[lkml]   [2022]   [Sep]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2] tty: n_gsm: avoid call of sleeping functions from atomic context

    > 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.
    >

    Do you have any hints why this might be correct?

    Have you tested it?

    Locking around line disciplines is spinlock_irqsave elsewhere in the
    kernel.

    Best regards,
    Pavel

    > 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>
    > ---
    > v2->v1: sorry, now adapted patch from 5.10 to upstream
    >
    > drivers/tty/n_gsm.c | 46 +++++++++++++++++++++------------------------
    > 1 file changed, 21 insertions(+), 25 deletions(-)
    >
    > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
    > index caa5c14ed57f..be62c601058d 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);
    > }
    >
    > /**
    > @@ -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_timer(struct timer_list *t)
    > {
    > struct gsm_mux *gsm = from_timer(gsm, t, kick_timer);
    > - 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

    --
    People of Russia, stop Putin before his war on Ukraine escalates.
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2022-09-19 13:33    [W:6.135 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site