Re: [PATCH RESEND] ttyprintk: fix a potential sleeping in interrupt context issue

[Greg Kroah-Hartman]

On Fri, Jan 03, 2020 at 11:45:41AM +0800, Zhenzhong Duan wrote:
> Google syzbot reports:
> BUG: sleeping function called from invalid context at
> kernel/locking/mutex.c:938
> in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0, name: swapper/1
> 1 lock held by swapper/1/0:
> ...
> Call Trace:
>   <IRQ>
>   dump_stack+0x197/0x210
>   ___might_sleep.cold+0x1fb/0x23e
>   __might_sleep+0x95/0x190
>   __mutex_lock+0xc5/0x13c0
>   mutex_lock_nested+0x16/0x20
>   tpk_write+0x5d/0x340
>   resync_tnc+0x1b6/0x320
>   call_timer_fn+0x1ac/0x780
>   run_timer_softirq+0x6c3/0x1790
>   __do_softirq+0x262/0x98c
>   irq_exit+0x19b/0x1e0
>   smp_apic_timer_interrupt+0x1a3/0x610
>   apic_timer_interrupt+0xf/0x20
>   </IRQ>
> 
> Fix it by using spinlock in process context instead of mutex and having
> interrupt disabled in critical section.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@xxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/char/ttyprintk.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)

Why was this resent?  What differs from the first version that required
it to be resent?

Always give us a clue here please :)


> 
> diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
> index 4f24e46ebe7c..56db949a7b70 100644
> --- a/drivers/char/ttyprintk.c
> +++ b/drivers/char/ttyprintk.c
> @@ -15,10 +15,11 @@
>  #include <linux/serial.h>
>  #include <linux/tty.h>
>  #include <linux/module.h>
> +#include <linux/spinlock.h>
>  
>  struct ttyprintk_port {
>  	struct tty_port port;
> -	struct mutex port_write_mutex;
> +	spinlock_t spinlock;
>  };
>  
>  static struct ttyprintk_port tpk_port;
> @@ -99,11 +100,12 @@ static int tpk_open(struct tty_struct *tty, struct file *filp)
>  static void tpk_close(struct tty_struct *tty, struct file *filp)
>  {
>  	struct ttyprintk_port *tpkp = tty->driver_data;
> +	unsigned long flags;
>  
> -	mutex_lock(&tpkp->port_write_mutex);
> +	spin_lock_irqsave(&tpkp->spinlock, flags);
>  	/* flush tpk_printk buffer */
>  	tpk_printk(NULL, 0);

Are you sure you can call this with a spinlock held?

Doesn't your trace above show the opposite?

What is wrong with sleeping during the mutex you currently have?  How is
syzbot reporting this error, is there a reproducer somewhere?

thanks,

greg k-h