* Re: [PATCHv5 3/7] tty: Hold tty_ldisc_lock() during tty_reopen()
[not found] <8c61fd04-969a-cb09-6595-7ee23d214ab1@canonical.com>
@ 2018-10-01 19:01 ` Guilherme G. Piccoli
2018-10-02 21:33 ` Dmitry Safonov
0 siblings, 1 reply; 8+ messages in thread
From: Guilherme G. Piccoli @ 2018-10-01 19:01 UTC (permalink / raw)
To: linux-kernel, dima; +Cc: gpiccoli
On 01/10/2018 15:32, Guilherme G. Piccoli wrote:
> Hi Dmitry, thanks for the patch. It's very promising, we have some
> reports of this issue and I'm building a kernel with this patch in
> order the reporter can test it. But based in the previous feedback,
> this seems to be very mature now and ready to get merged, right?
>
> I'd like to ask you if you did respin the patch with Greg's suggestion
> for the tag - I couldn't find it in LKML heheh
>
> Oh, if you can CC me in future spins of this patch (in case there any),
> I'd really be glad.
>
> Thank in advance,
>
>
> Guilherme
>
Sorry, forgot to CC linux-kernel...
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv5 3/7] tty: Hold tty_ldisc_lock() during tty_reopen()
2018-10-01 19:01 ` [PATCHv5 3/7] tty: Hold tty_ldisc_lock() during tty_reopen() Guilherme G. Piccoli
@ 2018-10-02 21:33 ` Dmitry Safonov
2018-10-03 10:46 ` Guilherme Piccoli
0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Safonov @ 2018-10-02 21:33 UTC (permalink / raw)
To: Guilherme G. Piccoli, linux-kernel
Hi Guilherme,
On Mon, 2018-10-01 at 16:01 -0300, Guilherme G. Piccoli wrote:
> On 01/10/2018 15:32, Guilherme G. Piccoli wrote:
> > Hi Dmitry, thanks for the patch. It's very promising, we have some
> > reports of this issue and I'm building a kernel with this patch in
> > order the reporter can test it. But based in the previous feedback,
> > this seems to be very mature now and ready to get merged, right?
Well, v5 passes 0day, so all previous reports are fixed.
But there is a new one about reboot on parisc platform which takes ~3
mins after the patch with ldisc locked on tty_reopen().
I believe it's related to holding read side for too long..
So, the patches still need another fix, heh.
Unfortunately, I was a bit busy with other bugs hitting more in Arista.
> > I'd like to ask you if you did respin the patch with Greg's
> > suggestion
> > for the tag - I couldn't find it in LKML heheh
> >
> > Oh, if you can CC me in future spins of this patch (in case there
> > any),
> > I'd really be glad.
Sure, will enlarge Cc list ;)
> >
> > Thank in advance,
> >
> >
> > Guilherme
> >
>
>
> Sorry, forgot to CC linux-kernel...
--
Thanks,
Dmitry
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv5 3/7] tty: Hold tty_ldisc_lock() during tty_reopen()
2018-10-02 21:33 ` Dmitry Safonov
@ 2018-10-03 10:46 ` Guilherme Piccoli
2018-10-12 15:57 ` Dmitry Safonov
0 siblings, 1 reply; 8+ messages in thread
From: Guilherme Piccoli @ 2018-10-03 10:46 UTC (permalink / raw)
To: dima; +Cc: linux-kernel
On Tue, Oct 2, 2018 at 6:33 PM Dmitry Safonov <dima@arista.com> wrote:
> [...]
> Well, v5 passes 0day, so all previous reports are fixed.
> But there is a new one about reboot on parisc platform which takes ~3
> mins after the patch with ldisc locked on tty_reopen().
>
> I believe it's related to holding read side for too long..
> So, the patches still need another fix, heh.
>
> Unfortunately, I was a bit busy with other bugs hitting more in Arista.
Hi Dmitry, thank you! Yeah, I saw that reading the whole thread after
sending this email heheh
That's a bummer =/
Unfortunately, I don't have a reproducer to exercise your patch. Do you
have some recipe on how to reproduce it easily?
I'll try more, in order I can validate the reboot behavior here.
> > > I'd like to ask you if you did respin the patch with Greg's
> > > suggestion
> > > for the tag - I couldn't find it in LKML heheh
> > >
> > > Oh, if you can CC me in future spins of this patch (in case there
> > > any),
> > > I'd really be glad.
>
> Sure, will enlarge Cc list ;)
>
Thanks again!
Cheers,
Guilherme
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv5 3/7] tty: Hold tty_ldisc_lock() during tty_reopen()
2018-10-03 10:46 ` Guilherme Piccoli
@ 2018-10-12 15:57 ` Dmitry Safonov
2018-10-15 12:16 ` Guilherme Piccoli
0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Safonov @ 2018-10-12 15:57 UTC (permalink / raw)
To: Guilherme Piccoli; +Cc: linux-kernel
Hi Guilherme,
Just to let you know - I've done with more urgent issues now,
so I'll be back on this patch on Monday, installing qemu-system-hppa
and debugging the root case.
Thanks,
Dmitry
On Wed, 2018-10-03 at 07:46 -0300, Guilherme Piccoli wrote:
> On Tue, Oct 2, 2018 at 6:33 PM Dmitry Safonov <dima@arista.com>
> wrote:
> > [...]
> > Well, v5 passes 0day, so all previous reports are fixed.
> > But there is a new one about reboot on parisc platform which takes
> > ~3
> > mins after the patch with ldisc locked on tty_reopen().
> >
> > I believe it's related to holding read side for too long..
> > So, the patches still need another fix, heh.
> >
> > Unfortunately, I was a bit busy with other bugs hitting more in
> > Arista.
>
> Hi Dmitry, thank you! Yeah, I saw that reading the whole thread after
> sending this email heheh
> That's a bummer =/
>
> Unfortunately, I don't have a reproducer to exercise your patch. Do
> you
> have some recipe on how to reproduce it easily?
> I'll try more, in order I can validate the reboot behavior here.
>
>
> > > > I'd like to ask you if you did respin the patch with Greg's
> > > > suggestion
> > > > for the tag - I couldn't find it in LKML heheh
> > > >
> > > > Oh, if you can CC me in future spins of this patch (in case
> > > > there
> > > > any),
> > > > I'd really be glad.
> >
> > Sure, will enlarge Cc list ;)
> >
>
> Thanks again!
> Cheers,
>
>
> Guilherme
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv5 3/7] tty: Hold tty_ldisc_lock() during tty_reopen()
2018-10-12 15:57 ` Dmitry Safonov
@ 2018-10-15 12:16 ` Guilherme Piccoli
0 siblings, 0 replies; 8+ messages in thread
From: Guilherme Piccoli @ 2018-10-15 12:16 UTC (permalink / raw)
To: dima; +Cc: linux-kernel
On Fri, Oct 12, 2018 at 12:57 PM Dmitry Safonov <dima@arista.com> wrote:
>
> Hi Guilherme,
>
> Just to let you know - I've done with more urgent issues now,
> so I'll be back on this patch on Monday, installing qemu-system-hppa
> and debugging the root case.
>
> Thanks,
> Dmitry
Awesome Dmitry, thanks for the heads-up. I'm willing to help to debug and test
too, I'm still trying to reproduce here.
Cheers,
Guilherme
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv5 3/7] tty: Hold tty_ldisc_lock() during tty_reopen()
2018-09-18 13:47 ` Greg Kroah-Hartman
@ 2018-09-18 14:19 ` Dmitry Safonov
0 siblings, 0 replies; 8+ messages in thread
From: Dmitry Safonov @ 2018-09-18 14:19 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, Dmitry Safonov, Daniel Axtens, Dmitry Vyukov,
Mark Rutland, Michael Neuling, Mikulas Patocka, Nathan March,
Pasi Kärkkäinen, Peter Hurley, Peter Zijlstra, Rong,
Chen, Sergey Senozhatsky, Tan Xiaojun, Tetsuo Handa, Jiri Slaby,
syzbot+3aa9784721dfb90e984d, stable, Jiri Slaby
On Tue, 2018-09-18 at 15:47 +0200, Greg Kroah-Hartman wrote:
> On Tue, Sep 18, 2018 at 12:52:54AM +0100, Dmitry Safonov wrote:
> > tty_ldisc_reinit() doesn't race with neither tty_ldisc_hangup()
> > nor set_ldisc() nor tty_ldisc_release() as they use tty lock.
> > But it races with anyone who expects line discipline to be the same
> > after hoding read semaphore in tty_ldisc_ref().
> >
> > We've seen the following crash on v4.9.108 stable:
> >
> > BUG: unable to handle kernel paging request at 0000000000002260
> > IP: [..] n_tty_receive_buf_common+0x5f/0x86d
> > Workqueue: events_unbound flush_to_ldisc
> > Call Trace:
> > [..] n_tty_receive_buf2
> > [..] tty_ldisc_receive_buf
> > [..] flush_to_ldisc
> > [..] process_one_work
> > [..] worker_thread
> > [..] kthread
> > [..] ret_from_fork
> >
> > tty_ldisc_reinit() should be called with ldisc_sem hold for
> > writing,
> > which will protect any reader against line discipline changes.
> >
> > Backport-first: b027e2298bd5 ("tty: fix data race between
> > tty_init_dev
> > and flush of buf")
>
> What does this mean?
>
> Does this require that patch for a stable patch? If so, just do:
> Cc: stable@vger.kernel.org # b027e2298bd5 ("tty: fix data race
> between tty_init_dev and flush of buf")
> in the signed-off-by area. The stable documentation should describe
> this pretty well. If not, we can modify it to make it more obvious.
>
> can you fix this up for the next resend of this series?
Sure, sorry about that non-obvious tag.
--
Thanks,
Dmitry
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv5 3/7] tty: Hold tty_ldisc_lock() during tty_reopen()
2018-09-17 23:52 ` [PATCHv5 3/7] tty: Hold tty_ldisc_lock() during tty_reopen() Dmitry Safonov
@ 2018-09-18 13:47 ` Greg Kroah-Hartman
2018-09-18 14:19 ` Dmitry Safonov
0 siblings, 1 reply; 8+ messages in thread
From: Greg Kroah-Hartman @ 2018-09-18 13:47 UTC (permalink / raw)
To: Dmitry Safonov
Cc: linux-kernel, Dmitry Safonov, Daniel Axtens, Dmitry Vyukov,
Mark Rutland, Michael Neuling, Mikulas Patocka, Nathan March,
Pasi Kärkkäinen, Peter Hurley, Peter Zijlstra, Rong,
Chen, Sergey Senozhatsky, Tan Xiaojun, Tetsuo Handa, Jiri Slaby,
syzbot+3aa9784721dfb90e984d, stable, Jiri Slaby
On Tue, Sep 18, 2018 at 12:52:54AM +0100, Dmitry Safonov wrote:
> tty_ldisc_reinit() doesn't race with neither tty_ldisc_hangup()
> nor set_ldisc() nor tty_ldisc_release() as they use tty lock.
> But it races with anyone who expects line discipline to be the same
> after hoding read semaphore in tty_ldisc_ref().
>
> We've seen the following crash on v4.9.108 stable:
>
> BUG: unable to handle kernel paging request at 0000000000002260
> IP: [..] n_tty_receive_buf_common+0x5f/0x86d
> Workqueue: events_unbound flush_to_ldisc
> Call Trace:
> [..] n_tty_receive_buf2
> [..] tty_ldisc_receive_buf
> [..] flush_to_ldisc
> [..] process_one_work
> [..] worker_thread
> [..] kthread
> [..] ret_from_fork
>
> tty_ldisc_reinit() should be called with ldisc_sem hold for writing,
> which will protect any reader against line discipline changes.
>
> Backport-first: b027e2298bd5 ("tty: fix data race between tty_init_dev
> and flush of buf")
What does this mean?
Does this require that patch for a stable patch? If so, just do:
Cc: stable@vger.kernel.org # b027e2298bd5 ("tty: fix data race between tty_init_dev and flush of buf")
in the signed-off-by area. The stable documentation should describe
this pretty well. If not, we can modify it to make it more obvious.
can you fix this up for the next resend of this series?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCHv5 3/7] tty: Hold tty_ldisc_lock() during tty_reopen()
2018-09-17 23:52 [PATCHv5 0/7] tty: Hold write ldisc sem in tty_reopen() Dmitry Safonov
@ 2018-09-17 23:52 ` Dmitry Safonov
2018-09-18 13:47 ` Greg Kroah-Hartman
0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Safonov @ 2018-09-17 23:52 UTC (permalink / raw)
To: linux-kernel
Cc: Dmitry Safonov, Dmitry Safonov, Daniel Axtens, Dmitry Vyukov,
Mark Rutland, Michael Neuling, Mikulas Patocka, Nathan March,
Pasi Kärkkäinen, Peter Hurley, Peter Zijlstra, Rong,
Chen, Sergey Senozhatsky, Tan Xiaojun, Tetsuo Handa, Jiri Slaby,
syzbot+3aa9784721dfb90e984d, Tetsuo Handa, stable,
Greg Kroah-Hartman, Jiri Slaby
tty_ldisc_reinit() doesn't race with neither tty_ldisc_hangup()
nor set_ldisc() nor tty_ldisc_release() as they use tty lock.
But it races with anyone who expects line discipline to be the same
after hoding read semaphore in tty_ldisc_ref().
We've seen the following crash on v4.9.108 stable:
BUG: unable to handle kernel paging request at 0000000000002260
IP: [..] n_tty_receive_buf_common+0x5f/0x86d
Workqueue: events_unbound flush_to_ldisc
Call Trace:
[..] n_tty_receive_buf2
[..] tty_ldisc_receive_buf
[..] flush_to_ldisc
[..] process_one_work
[..] worker_thread
[..] kthread
[..] ret_from_fork
tty_ldisc_reinit() should be called with ldisc_sem hold for writing,
which will protect any reader against line discipline changes.
Backport-first: b027e2298bd5 ("tty: fix data race between tty_init_dev
and flush of buf")
Cc: stable@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Reviewed-by: Jiri Slaby <jslaby@suse.cz>
Reported-by: syzbot+3aa9784721dfb90e984d@syzkaller.appspotmail.com
Tested-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
drivers/tty/tty_io.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 5e5da9acaf0a..3ef8b977b167 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1267,15 +1267,20 @@ static int tty_reopen(struct tty_struct *tty)
if (test_bit(TTY_EXCLUSIVE, &tty->flags) && !capable(CAP_SYS_ADMIN))
return -EBUSY;
- tty->count++;
+ retval = tty_ldisc_lock(tty, 5 * HZ);
+ if (retval)
+ return retval;
+ tty->count++;
if (tty->ldisc)
- return 0;
+ goto out_unlock;
retval = tty_ldisc_reinit(tty, tty->termios.c_line);
if (retval)
tty->count--;
+out_unlock:
+ tty_ldisc_unlock(tty);
return retval;
}
--
2.13.6
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-10-15 12:17 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <8c61fd04-969a-cb09-6595-7ee23d214ab1@canonical.com>
2018-10-01 19:01 ` [PATCHv5 3/7] tty: Hold tty_ldisc_lock() during tty_reopen() Guilherme G. Piccoli
2018-10-02 21:33 ` Dmitry Safonov
2018-10-03 10:46 ` Guilherme Piccoli
2018-10-12 15:57 ` Dmitry Safonov
2018-10-15 12:16 ` Guilherme Piccoli
2018-09-17 23:52 [PATCHv5 0/7] tty: Hold write ldisc sem in tty_reopen() Dmitry Safonov
2018-09-17 23:52 ` [PATCHv5 3/7] tty: Hold tty_ldisc_lock() during tty_reopen() Dmitry Safonov
2018-09-18 13:47 ` Greg Kroah-Hartman
2018-09-18 14:19 ` Dmitry Safonov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).