Re: [PATCH] tty: Fix data race in tty_insert_flip_string_fixed_flag

[Alan Cox <gnomes@lxorguk.ukuu.org.uk>]

On Wed, 25 Apr 2018 22:20:50 +0900
DaeRyong Jeong <threeearcat@gmail.com> wrote:

> tty_insert_flip_string_fixed_flag() copies chars to the buffer indicated
> by th->used and updates tb->used.
> But tty_insert_flip_string_fixed_flag() can be executed concurrently and
> tb->used can be updated improperly.

The tty input layer does not work if it can be executed concurrently. If
that is happening in the pty code then the pty code is buggy and that
needs serializing on the inbound path.


> -static void tty_write_unlock(struct tty_struct *tty)
> +void tty_write_unlock(struct tty_struct *tty, int wakeup)
>  {
>  	mutex_unlock(&tty->atomic_write_lock);
> -	wake_up_interruptible_poll(&tty->write_wait, EPOLLOUT);
> +	if (wakeup) {
> +		wake_up_interruptible_poll(&tty->write_wait, EPOLLOUT);
> +	}

And this may cause deadlocks.

You don't actually need any of the wakeup changes in your code

> diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
> index d9b561d89432..a54ab91aec90 100644
> --- a/drivers/tty/tty_ioctl.c
> +++ b/drivers/tty/tty_ioctl.c
> @@ -911,12 +911,17 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file,
>  			spin_unlock_irq(&tty->flow_lock);
>  			break;
>  		case TCOON:
> +			if (tty_write_lock(tty, 0) < 0)
> +				return -ERESTARTSYS;
> +
>  			spin_lock_irq(&tty->flow_lock);
>  			if (tty->flow_stopped) {
>  				tty->flow_stopped = 0;
>  				__start_tty(tty);
>  			}
>  			spin_unlock_irq(&tty->flow_lock);
> +
> +			tty_write_unlock(tty, 0);

If you just used these unmodified it would be simpler and as good,
however it won't actually fix anything. The pty layer is broken not this
code.

The tty layer rule for all the input buffer handling is that you may not
call *any* of it from multiple threads at once. This works fine for
normal serial because the IRQ layer or the polling logic has that
property.

The bug looks real, your diagnosis looks right, your fix sort of works
but isn't sufficient.

So NAK.

Alan