Re: tty: fix a possible hang on tty device

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

[-- Attachment #1: Type: text/plain, Size: 4823 bytes --]

On Mon, 30 May 2022, cael wrote:

> Thanks, You are right, barrier is needed here. I changed the patch as follows:
> 1) WRITE_ONCE and READ_ONCE is used to access ldata->no_room since
> n_tty_kick_worker  would be called in kworker and reader cpu;
> 2) smp_mb added in chars_in_buffer as this function will be called in
> reader and kworker, accessing commit_head and read_tail; and to make
> sure that read_tail is not read before setting no_room in
> n_tty_receive_buf_common;
> 3) smp_mb added in n_tty_read to make sure that no_room is not read
> before setting read_tail.

Please include proper changelog to all revised patch submissions, not 
just list of changes you've made (and properly version the submissions 
with [PATCH v2] etc. in the subject).

> ---
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index efc72104c840..3327687da0d3 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -201,8 +201,8 @@ static void n_tty_kick_worker(struct tty_struct *tty)
>         struct n_tty_data *ldata = tty->disc_data;
> 
>         /* Did the input worker stop? Restart it */
> -       if (unlikely(ldata->no_room)) {
> -               ldata->no_room = 0;
> +       if (unlikely(READ_ONCE(ldata->no_room))) {
> +               WRITE_ONCE(ldata->no_room, 0);
>                 WARN_RATELIMIT(tty->port->itty == NULL,
>                                 "scheduling with invalid itty\n");
> @@ -221,6 +221,7 @@ static ssize_t chars_in_buffer(struct tty_struct *tty)
>         struct n_tty_data *ldata = tty->disc_data;
>         ssize_t n = 0;
> 
> +       smp_mb();

You should add the reason in comment for any barriers you add.

>         if (!ldata->icanon)
>                 n = ldata->commit_head - ldata->read_tail;
>         else
> @@ -1632,7 +1633,7 @@ n_tty_receive_buf_common(struct tty_struct *tty,
> const unsigned char *cp,
>                         if (overflow && room < 0)
>                                 ldata->read_head--;
>                         room = overflow;
> -                       ldata->no_room = flow && !room;
> +                       WRITE_ONCE(ldata->no_room, flow && !room);
>                 } else
>                         overflow = 0;
> 
> @@ -1663,6 +1664,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);
> +

Instead of having the barrier in chars_in_buffer() perhaps it would be 
more obvious what's going on here and also scope down to the cases where 
the barrier might be needed in the first place if you'd do:

	if (ldata->no_room) {
		/* ... */
		smp_mb();
		if (!chars_in_buffer(tty))
			n_tty_kick_worker(tty);
	}


-- 
 i.

>         up_read(&tty->termios_rwsem);
> 
>         return rcvd;
> @@ -2180,8 +2184,10 @@ static ssize_t n_tty_read(struct tty_struct
> *tty, struct file *file,
>                 if (time)
>                         timeout = time;
>         }
> -       if (tail != ldata->read_tail)
> +       if (tail != ldata->read_tail) {
> +               smp_mb();
>                 n_tty_kick_worker(tty);
> +       }
>         up_read(&tty->termios_rwsem);
> 
>         remove_wait_queue(&tty->read_wait, &wait);
> --
> 2.27.0
> 
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> 于2022年5月25日周三 19:21写道：
> >
> > On Wed, 25 May 2022, cael wrote:
> >
> > > >Now you switched to an entirely different case, not the one we were
> > > >talking about. ...There is no ldisc->no_room = true race in the case
> > > >you now described.
> > > So, I think we should back to the case ldata->no_room=true as
> > > ldata->no_room=false seems harmless.
> > >
> > > >I'm not worried about the case where both cpus call n_tty_kick_worker but
> > > >the case where producer cpu sees chars_in_buffer() > 0 and consumer cpu
> > > >!no_room.
> > >
> > > As ldata->no_room=true is set before checking chars_in_buffer()
> >
> > Please take a brief look at Documentation/memory-barriers.txt and then
> > tell me if you still find this claim to be true.
> >
> > > if producer
> > > finds chars_in_buffer() > 0, then if reader is currently in n_tty_read,
> >
> > ...Then please do a similar analysis for ldata->read_tail. What guarantees
> > its update is seen by the producer cpu when the reader is already past the
> > point you think it still must be in?
> >
> > > when reader quits n_tty_read, n_tty_kick_worker will be called. If reader
> > > has already exited n_tty_read, which means that reader still has data to read,
> > > next time reader will call n_tty_kick_worker inside n_tty_read too.
> >
> > C-level analysis alone is not going to be very useful here given you're
> > dealing with a concurrency challenge here.
> >
> >
> > --
> >  i.
> >
> >
>