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