Re: tty: fix a possible hang on tty device

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Tue, 24 May 2022, cael wrote:

> We have met a hang on pty device, the reader was blocking at
>  epoll on master side, the writer was sleeping at wait_woken inside
>  n_tty_write on slave side ,and the write buffer on tty_port was full, we

Space after comma. It would be also useful to tone down usage of "we" in 
the changelog.

>  found that the reader and writer would never be woken again and block
>  forever.
> 
> We thought the problem was caused as a race between reader and
> kworker as follows:
> n_tty_read(reader)| n_tty_receive_buf_common(kworker)
>                   |room = N_TTY_BUF_SIZE - (ldata->read_head - tail)
>                   |room <= 0
> copy_from_read_buf|
> n_tty_kick_worker |
>                   |ldata->no_room = true
>
> After writing to slave device, writer wakes up kworker to flush
> data on tty_port to reader, and the kworker finds that reader
> has no room to store data so room <= 0 is met. At this moment,
> reader consumes all the data on reader buffer and call
> n_tty_kick_worker to check ldata->no_room and finds that there
> is no need to call tty_buffer_restart_work to flush data to reader
> and reader quits reading. Then kworker sets ldata->no_room=true
> and quits too.
> 
> If write buffer is not full, writer will wake kworker to flush data
> again after following writes, but if writer buffer is full and writer
> goes to sleep, kworker will never be woken again and tty device is
> blocked.
> 
> We think this problem can be solved with a check for read buffer
> inside function n_tty_receive_buf_common, if read buffer is empty and
> ldata->no_room is true, this means that kworker has more data to flush
> to read buffer, so a call to n_tty_kick_worker is necessary.
> 
> Signed-off-by: cael <juanfengpy@gmail.com>
> ---
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index efc72104c840..36c7bc033c78 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -1663,6 +1663,9 @@ n_tty_receive_buf_common(struct tty_struct *tty,
> const unsigned char *cp,
>         } else
>                 n_tty_check_throttle(tty);
> 
> +       if (!chars_in_buffer(tty))
> +               n_tty_kick_worker(tty);
> +

chars_in_buffer() accesses ldata->read_tail in producer context so this 
probably just moves the race there?


-- 
 i.