Re: [PATCH 2/4] tty: Hold tty_ldisc_lock() during tty_reopen()

From: Sergey Senozhatsky
Date: Wed Aug 29 2018 - 00:34:44 EST


Hi,

Cc-ing Benjamin on this.

On (08/29/18 03:23), Dmitry Safonov wrote:
> BUG: unable to handle kernel paging request at 0000000000002260
> IP: [..] n_tty_receive_buf_common+0x5f/0x86d
> Workqueue: events_unbound flush_to_ldisc
> Call Trace:
> [..] n_tty_receive_buf2
> [..] tty_ldisc_receive_buf
> [..] flush_to_ldisc
> [..] process_one_work
> [..] worker_thread
> [..] kthread
> [..] ret_from_fork

Seems that you are not the first one to hit this NULL deref.

> I think, tty_ldisc_reinit() should be called with ldisc_sem hold for
> writing, which will protect any reader against line discipline changes.

Per https://lore.kernel.org/patchwork/patch/777220/

: Note that we noticed one path that called reinit without the ldisc lock
: held for writing, we added that, but it didn't fix the problem.

And I guess that Ben meant the same reinit path which you patched:

> @@ -1267,15 +1267,20 @@ static int tty_reopen(struct tty_struct *tty)
> if (test_bit(TTY_EXCLUSIVE, &tty->flags) && !capable(CAP_SYS_ADMIN))
> return -EBUSY;
>
> - tty->count++;
> + retval = tty_ldisc_lock(tty, 5 * HZ);
> + if (retval)
> + return retval;
>
> + tty->count++;
> if (tty->ldisc)
> - return 0;
> + goto out_unlock;
>
> retval = tty_ldisc_reinit(tty, tty->termios.c_line);
> if (retval)
> tty->count--;
>
> +out_unlock:
> + tty_ldisc_unlock(tty);
> return retval;
> }

-ss