Re: linux-next: manual merge of the tty tree with the tty.current tree

[Dmitry Vyukov]

On Mon, Mar 20, 2017 at 3:28 AM, Stephen Rothwell <sfr@xxxxxxxxxxxxxxxx> wrote:
> Hi Greg,
>
> Today's linux-next merge of the tty tree got a conflict in:
>
>   drivers/tty/tty_ldisc.c
>
> between commit:
>
>   5362544bebe8 ("tty: don't panic on OOM in tty_set_ldisc()")
>
> from the tty.current tree and commit:
>
>   71472fa9c52b ("tty: Fix ldisc crash on reopened tty")
>
> from the tty tree.
>
> I fixed it up (see below) and can carry the fix as necessary. This
> is now fixed as far as linux-next is concerned, but any non trivial
> conflicts should be mentioned to your upstream maintainer when your tree
> is submitted for merging.  You may also want to consider cooperating
> with the maintainer of the conflicting tree to minimise any particularly
> complex conflicts.
>
> --
> Cheers,
> Stephen Rothwell
>
> diff --cc drivers/tty/tty_ldisc.c
> index b0500a0a87b8,4ee7742dced3..000000000000
> --- a/drivers/tty/tty_ldisc.c
> +++ b/drivers/tty/tty_ldisc.c
> @@@ -621,14 -669,17 +621,15 @@@ int tty_ldisc_reinit(struct tty_struct
>                 tty_ldisc_put(tty->ldisc);
>         }
>
> -       /* switch the line discipline */
> -       tty->ldisc = ld;
>         tty_set_termios_ldisc(tty, disc);
> -       retval = tty_ldisc_open(tty, tty->ldisc);
> +       retval = tty_ldisc_open(tty, ld);
>         if (retval) {
> -               tty_ldisc_put(tty->ldisc);
> -               tty->ldisc = NULL;
>  -              if (!WARN_ON(disc == N_TTY)) {
>  -                      tty_ldisc_put(ld);
>  -                      ld = NULL;
>  -              }
> ++              tty_ldisc_put(ld);
> ++              ld = NULL;
>         }
> +
> +       /* switch the line discipline */
> +       smp_store_release(&tty->ldisc, ld);
>         return retval;
>   }
>


Peter,

Looking at your patch "tty: Fix ldisc crash on reopened tty", I think
there is a missed barrier in tty_ldisc_ref. A single barrier does not
have any effect, they always need to be in pairs. So I think we also
need at least:

@@ -295,7 +295,8 @@ struct tty_ldisc *tty_ldisc_ref(struct tty_struct *tty)
        struct tty_ldisc *ld = NULL;

        if (ldsem_down_read_trylock(&tty->ldisc_sem)) {
-               ld = tty->ldisc;
+               ld = READ_ONCE(tty->ldisc);
+               read_barrier_depends();
                if (!ld)
                        ldsem_up_read(&tty->ldisc_sem);
        }


Or simply:

@@ -295,7 +295,8 @@ struct tty_ldisc *tty_ldisc_ref(struct tty_struct *tty)
        struct tty_ldisc *ld = NULL;

        if (ldsem_down_read_trylock(&tty->ldisc_sem)) {
-               ld = tty->ldisc;
+               /* pairs with smp_store_release in tty_ldisc_reinit */
+               ld = smp_load_acquire(&tty->ldisc);
                if (!ld)
                        ldsem_up_read(&tty->ldisc_sem);
        }