Re: [PATCH v2] tty: n_gsm: avoid call of sleeping functions from atomic context

[Fedor Pchelkin]

On 05.10.2022 13:47, Daniel Starke wrote:
> This patch breaks packet retransmission. Basically tx_lock and now 
tx_mutex
> protects the transmission packet queue. This works fine as long as 
packets
> are transmitted in a context that allows sleep. However, the 
retransmission
> timer T2 is called from soft IRQ context and spans an additional atomic
> context via control_lock within gsm_control_retransmit(). The call path
> looks like this:
> gsm_control_retransmit()
>    spin_lock_irqsave(&gsm->control_lock, flags)
>      gsm_control_transmit()
>        gsm_data_queue()
>          mutex_lock(&gsm->tx_mutex) // -> sleep in atomic context

As far as switching to tx_mutex turns out to have its own problems,
we suggest to revert it and to find another solution for the original
issue.

As it is described in commit 32dd59f ("tty: n_gsm: fix race condition in 
gsmld_write()"), the issue is that gsmld_write() may be used by the user 
directly and also by the n_gsm internal functions. But the proposed 
solution to add a spinlock around the low side tty write is not suitable 
since the tty write may sleep:

  gsmld_write(...)
   spin_lock_irqsave(&gsm->tx_lock, flags)
    tty->ops->write(...);
     con_write(...)
      do_con_write(...)
       console_lock()
        might_sleep() // -> bug

So let's consider alternative approaches to avoid the race condition.

We have found the only potential concurrency place:
gsm->tty->ops->write() in gsmld_output() and tty->ops->write() in
gsmld_write().

Is that right? Or there are some other cases?

On 05.10.2022 13:47, Daniel Starke wrote:
> Long story short: The patch via mutex does not solve the issue. It is 
only
> shifted to another function. I suggest splitting the TX lock into packet
> queue lock and underlying tty write mutex.
>
> I would have implemented the patch if I had means to verify it.

Probably splitting the TX lock would be rather complex as there is
gsm_data_kick() which in this way has to be protected by packet queue
spinlock and at the same time it contains gsmld_output() (via
gsm_send_packet()) that would require mutex protection.