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

[DaeRyong Jeong]

On Wed, Apr 25, 2018 at 03:39:48PM +0100, Alan Cox wrote:
> On Wed, 25 Apr 2018 22:20:50 +0900
> DaeRyong Jeong <threeearcat@xxxxxxxxx> 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.

Yes. I thought the wrong way to resolve the issue.

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

With your comments, now I know that this patch is incorrect.
Since the bug is real, I will send a new patch with considering all your
comments above.

Thank you a lot for all your comments.

DaeRyong Jeong