lkml.org 
[lkml]   [2017]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: linux-next: manual merge of the tty tree with the tty.current tree
On Mon, Mar 20, 2017 at 10:21 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Mon, Mar 20, 2017 at 3:28 AM, Stephen Rothwell <sfr@canb.auug.org.au> 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);
> }




I am also surprised that callers of tty_ldisc_reinit don't hold
ldisc_sem. I thought that ldisc_sem is what's supposed to protect
changes to ldisc. That would also auto fix the crash without any
tricky barriers as flush_to_ldisc uses tty_ldisc_ref.

\
 
 \ /
  Last update: 2017-03-20 10:28    [W:0.081 / U:104.832 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site