Re: tty: fix a possible hang on tty device

[cael <juanfengpy@gmail.com>]

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.
---
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();
        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);
+
        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.
>
>