linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* 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).