Re: [PATCH 5/6] n_tty: Fix stuck write wakeup

From: Peter Hurley <peter@hurleysoftware.com>
To: Johannes Stezenbach <js@sig21.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jslaby@suse.cz>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 5/6] n_tty: Fix stuck write wakeup
Date: Sun, 13 Dec 2015 10:38:02 -0800	[thread overview]
Message-ID: <566DBB0A.5070108@hurleysoftware.com> (raw)
In-Reply-To: <20151213151823.GB10204@sig21.net>

On 12/13/2015 07:18 AM, Johannes Stezenbach wrote:
> Hi Peter,
> 
> On Sat, Dec 12, 2015 at 02:16:38PM -0800, Peter Hurley wrote:
>> If signal-driven i/o is disabled while write wakeup is pending (ie.,
>> n_tty_write() has set TTY_DO_WRITE_WAKEUP but then signal-driven i/o
>> is disabled), the TTY_DO_WRITE_WAKEUP bit will never be cleared and
>> will cause tty_wakeup() to always call n_tty_write_wakeup.
>>
>> Unconditionally clear the write wakeup, and since kill_fasync()
>> already checks if the fasync ptr is null, call kill_fasync()
>> unconditionally as well.
> ...
>> @@ -230,8 +230,8 @@ static ssize_t chars_in_buffer(struct tty_struct *tty)
>>  
>>  static void n_tty_write_wakeup(struct tty_struct *tty)
>>  {
>> -	if (tty->fasync && test_and_clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags))
>> -		kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
>> +	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
>> +	kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
>>  }
> 
> There is a related bug that I meant to send a patch, but I
> never got around because the issue was found with proprietary
> userspace and ancient kernel.  Maybe you could take care of it?
> The patch might not apply cleanly after your recent changes
> or might even be invalid now, please check.

Thanks for the patch, Johannes!

Yes, the patch below is still required to prevent excessive SIGIO
(and to prevent missed SIGIO when the amount actually copied just
happens to be exactly the amount left to be copied).

I made some comments in the patch; can you re-submit with those
changes and the patch title in the subject? Or I'd happy to re-work
it and send it to Greg if you'd prefer; just let me know.

Regards,
Peter Hurley

> ---
> tty: n_tty: fix SIGIO for output
> 
> According to fcntl(2), "a SIGIO signal is sent whenever input
> or output becomes possible on that file descriptor", i.e.
> after the output buffer was full and now has space for new data.
> But in fact SIGIO is sent after every write.
> 
> n_tty_write() should set TTY_DO_WRITE_WAKEUP only when
> not all data could be written to the buffer.
> 
> Signed-off-by: Johannes Stezenbach <js@sig21.net>
> 
> --- drivers/char/n_tty.c.orig   2015-11-02 22:26:04.124227148 +0100
> +++ drivers/char/n_tty.c        2015-11-02 22:26:10.644212115 +0100
> @@ -1925,6 +1925,7 @@ static ssize_t n_tty_write(struct tty_st
>         DECLARE_WAITQUEUE(wait, current);
>         int c;
>         ssize_t retval = 0;
> +       size_t count = nr;

'count' isn't required because after exiting the write loop, 'nr' will
be the remainder still to write so ...

> 
>         /* Job control check -- must be done at start (POSIX.1 7.1.1.4). */
>         if (L_TOSTOP(tty) && file->f_op->write != redirected_tty_write) {
> @@ -1991,7 +1992,7 @@ static ssize_t n_tty_write(struct tty_st
>  break_out:
>         __set_current_state(TASK_RUNNING);
>         remove_wait_queue(&tty->write_wait, &wait);
> -       if (b - buf != nr && tty->fasync)
> +       if (b - buf != count && tty->fasync)

... this can be

	if (nr && tty->fasync)
		set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);

>                 set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
>         return (b - buf) ? b - buf : retval;
>  }
> 
> 