* [PATCH 2/2] TTY: ntty, add one more sanoty check [not found] <1307106096-6461-1-git-send-email-jslaby@suse.cz> @ 2011-06-03 13:01 ` Jiri Slaby 2011-06-03 13:06 ` [PATCH 1/2] TTY: ldisc, do not close until there are readers Jiri Slaby 2011-06-03 21:52 ` Linus Torvalds 2 siblings, 0 replies; 10+ messages in thread From: Jiri Slaby @ 2011-06-03 13:01 UTC (permalink / raw) To: gregkh; +Cc: jirislaby, linux-kernel, Jiri Slaby, Alan Cox With the previous patch, we fixed another bug where read_buf was freed while we still was in n_tty_read. We currently check whether read_buf is NULL at the start of the function. Add one more check after we wake up from waiting for input. Signed-off-by: Jiri Slaby <jslaby@suse.cz> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk> --- drivers/tty/n_tty.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index 95d0a9c..c62c856 100644 --- a/drivers/tty/n_tty.c +++ b/drivers/tty/n_tty.c @@ -1785,6 +1785,7 @@ do_it_again: break; } timeout = schedule_timeout(timeout); + BUG_ON(!tty->read_buf); continue; } __set_current_state(TASK_RUNNING); -- 1.7.5.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] TTY: ldisc, do not close until there are readers [not found] <1307106096-6461-1-git-send-email-jslaby@suse.cz> 2011-06-03 13:01 ` [PATCH 2/2] TTY: ntty, add one more sanoty check Jiri Slaby @ 2011-06-03 13:06 ` Jiri Slaby 2011-06-03 21:52 ` Linus Torvalds 2 siblings, 0 replies; 10+ messages in thread From: Jiri Slaby @ 2011-06-03 13:06 UTC (permalink / raw) To: Jiri Slaby; +Cc: gregkh, linux-kernel, Alan Cox, stable, Linus Torvalds On 06/03/2011 03:01 PM, Jiri Slaby wrote: > We restored tty_ldisc_wait_idle in 100eeae2c5c (TTY: restore > tty_ldisc_wait_idle). We used it in the ldisc changing path to fix the > case where there are tasks in n_tty_read waiting for data and somebody > tries to change ldisc. > > Similar to the case above, there may be also tasks waiting in > n_tty_read. As 65b770468e98 (tty-ldisc: turn ldisc user count into a > proper refcount) removed the wait-until-idle from all paths, hangup > path won't wait for them to disappear either. So add it back even to > the hangup path. > > There is a difference, we need uninterruptible sleep as there is > obviously HUP signal pending. So tty_ldisc_wait_idle now takes > interruptible parameter to decide. I'm not sure whether it's worth the > complexity. Instead uninterruptible waiting may fit even for the ldisc > change code. It wouldn't be possible to interrupted the change, but > there is 5s timeout anyway. I've detypoed the text locally, but will wait if there are any comments on the code before resending. > Before 65b770468e98 tty_ldisc_release was called also from > tty_ldisc_release. It is called from tty_release, so I don't think we > need to restore that one. > > This is nicely reproducible after constify the timing as follows: > --- a/drivers/tty/n_tty.c > +++ b/drivers/tty/n_tty.c > @@ -1548,6 +1549,7 @@ static int n_tty_open(struct tty_struct *tty) > > /* These are ugly. Currently a malloc failure here can panic */ > if (!tty->read_buf) { > + msleep(100); > tty->read_buf = kzalloc(N_TTY_BUF_SIZE, GFP_KERNEL); > if (!tty->read_buf) > return -ENOMEM; > @@ -1741,6 +1743,7 @@ do_it_again: > > add_wait_queue(&tty->read_wait, &wait); > while (nr) { > + WARN_ON(!tty->read_buf); > /* First test for status change. */ > if (packet && tty->link->ctrl_status) { > unsigned char cs; > @@ -1785,6 +1788,7 @@ do_it_again: > break; > } > timeout = schedule_timeout(timeout); > + msleep(20); > continue; > } > __set_current_state(TASK_RUNNING); > > ================================== > > With a process: > while (1) { > int fd = open(argv[1], O_RDWR); > read(fd, buf, sizeof(buf)); > close(fd); > } > ===== and its child: ===== > setsid(); > while (1) { > int fd = open(tty, O_RDWR|O_NOCTTY); > ioctl(fd, TIOCSCTTY, 1); > vhangup(); > close(fd); > usleep(100 * (10 + random() % 1000)); > } > > References: https://bugzilla.novell.com/show_bug.cgi?id=693374 > References: https://bugzilla.novell.com/show_bug.cgi?id=694509 > Signed-off-by: Jiri Slaby <jslaby@suse.cz> > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk> > Cc: stable@kernel.org [32, 33, 34, 39] > Cc: Linus Torvalds <torvalds@linux-foundation.org> > --- > drivers/tty/tty_ldisc.c | 34 +++++++++++++++++++++++++++------- > 1 files changed, 27 insertions(+), 7 deletions(-) > > diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c > index 5d01d32..3c01bf0 100644 > --- a/drivers/tty/tty_ldisc.c > +++ b/drivers/tty/tty_ldisc.c > @@ -552,14 +552,32 @@ static void tty_ldisc_flush_works(struct tty_struct *tty) > * Wait for the line discipline to become idle. The discipline must > * have been halted for this to guarantee it remains idle. > */ > -static int tty_ldisc_wait_idle(struct tty_struct *tty) > +static int tty_ldisc_wait_idle(struct tty_struct *tty, bool interruptible) > { > + DEFINE_WAIT(wait); > + long timeout = 5 * HZ; > + int state = interruptible ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE; > int ret; > - ret = wait_event_interruptible_timeout(tty_ldisc_idle, > - atomic_read(&tty->ldisc->users) == 1, 5 * HZ); > - if (ret < 0) > - return ret; > - return ret > 0 ? 0 : -EBUSY; > + > + for (;;) { > + prepare_to_wait(&tty_ldisc_idle, &wait, state); > + if (atomic_read(&tty->ldisc->users) == 1) { > + ret = 0; > + break; > + } > + if (interruptible && signal_pending(current)) { > + ret = -ERESTARTSYS; > + break; > + } > + > + timeout = schedule_timeout(timeout); > + if (!timeout) { > + ret = -EBUSY; > + break; > + } > + } > + finish_wait(&tty_ldisc_idle, &wait); > + return ret; > } > > /** > @@ -666,7 +684,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc) > > tty_ldisc_flush_works(tty); > > - retval = tty_ldisc_wait_idle(tty); > + retval = tty_ldisc_wait_idle(tty, 1); > > tty_lock(); > mutex_lock(&tty->ldisc_mutex); > @@ -763,6 +781,8 @@ static int tty_ldisc_reinit(struct tty_struct *tty, int ldisc) > if (IS_ERR(ld)) > return -1; > > + WARN_ON_ONCE(tty_ldisc_wait_idle(tty, 0)); > + > tty_ldisc_close(tty, tty->ldisc); > tty_ldisc_put(tty->ldisc); > tty->ldisc = NULL; -- js ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] TTY: ldisc, do not close until there are readers [not found] <1307106096-6461-1-git-send-email-jslaby@suse.cz> 2011-06-03 13:01 ` [PATCH 2/2] TTY: ntty, add one more sanoty check Jiri Slaby 2011-06-03 13:06 ` [PATCH 1/2] TTY: ldisc, do not close until there are readers Jiri Slaby @ 2011-06-03 21:52 ` Linus Torvalds [not found] ` <1307276177-20957-1-git-send-email-jslaby@suse.cz> 2 siblings, 1 reply; 10+ messages in thread From: Linus Torvalds @ 2011-06-03 21:52 UTC (permalink / raw) To: Jiri Slaby; +Cc: gregkh, jirislaby, linux-kernel, Alan Cox, stable On Fri, Jun 3, 2011 at 10:01 PM, Jiri Slaby <jslaby@suse.cz> wrote: > > There is a difference, we need uninterruptible sleep as there is > obviously HUP signal pending. So tty_ldisc_wait_idle now takes > interruptible parameter to decide. I'm not sure whether it's worth the > complexity. Instead uninterruptible waiting may fit even for the ldisc > change code. It wouldn't be possible to interrupted the change, but > there is 5s timeout anyway. Ugh. I'd much rather not see these kinds of random flags. > This is nicely reproducible after constify the timing as follows: > --- a/drivers/tty/n_tty.c > +++ b/drivers/tty/n_tty.c And PLEASE don't do things like this: you just introduced a patch inside the email that shows the bug, and now any sane email application tool will just assume that THIS patch is the one you want to apply. So rather than applying the fix, it will apply the "descriptive" patch to intentionally introduce the timing delays to show the bug! So don't include actual real patches like this without indenting them or doing something to make it really really obvious that the patch is not the one to be applied, but just a description of the problem! Linus ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <1307276177-20957-1-git-send-email-jslaby@suse.cz>]
* [PATCH v2 2/2] TTY: ntty, add one more sanity check [not found] ` <1307276177-20957-1-git-send-email-jslaby@suse.cz> @ 2011-06-05 12:16 ` Jiri Slaby 2011-06-07 16:44 ` Greg KH 2011-06-07 17:20 ` Greg KH 0 siblings, 2 replies; 10+ messages in thread From: Jiri Slaby @ 2011-06-05 12:16 UTC (permalink / raw) To: gregkh; +Cc: jirislaby, linux-kernel, Jiri Slaby, Alan Cox With the previous patch, we fixed another bug where read_buf was freed while we still was in n_tty_read. We currently check whether read_buf is NULL at the start of the function. Add one more check after we wake up from waiting for input. Signed-off-by: Jiri Slaby <jslaby@suse.cz> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk> --- drivers/tty/n_tty.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index 95d0a9c..c62c856 100644 --- a/drivers/tty/n_tty.c +++ b/drivers/tty/n_tty.c @@ -1785,6 +1785,7 @@ do_it_again: break; } timeout = schedule_timeout(timeout); + BUG_ON(!tty->read_buf); continue; } __set_current_state(TASK_RUNNING); -- 1.7.5.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] TTY: ntty, add one more sanity check 2011-06-05 12:16 ` [PATCH v2 2/2] TTY: ntty, add one more sanity check Jiri Slaby @ 2011-06-07 16:44 ` Greg KH 2011-06-07 16:57 ` Jiri Slaby 2011-06-07 17:20 ` Greg KH 1 sibling, 1 reply; 10+ messages in thread From: Greg KH @ 2011-06-07 16:44 UTC (permalink / raw) To: Jiri Slaby; +Cc: gregkh, jirislaby, linux-kernel, Alan Cox On Sun, Jun 05, 2011 at 02:16:17PM +0200, Jiri Slaby wrote: > With the previous patch, we fixed another bug where read_buf was freed > while we still was in n_tty_read. We currently check whether read_buf > is NULL at the start of the function. Add one more check after we wake > up from waiting for input. > > Signed-off-by: Jiri Slaby <jslaby@suse.cz> > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk> > --- > drivers/tty/n_tty.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c > index 95d0a9c..c62c856 100644 > --- a/drivers/tty/n_tty.c > +++ b/drivers/tty/n_tty.c > @@ -1785,6 +1785,7 @@ do_it_again: > break; > } > timeout = schedule_timeout(timeout); > + BUG_ON(!tty->read_buf); So, if we ever hit this, what are we going to do with this crash? I really don't want to add more BUG_ON() calls to the kernel if at all possible. Or is it the case that we will crash if this case is true soon afterward anyway? thanks, greg k-h ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] TTY: ntty, add one more sanity check 2011-06-07 16:44 ` Greg KH @ 2011-06-07 16:57 ` Jiri Slaby 2011-06-07 17:10 ` Greg KH 0 siblings, 1 reply; 10+ messages in thread From: Jiri Slaby @ 2011-06-07 16:57 UTC (permalink / raw) To: Greg KH; +Cc: gregkh, jirislaby, linux-kernel, Alan Cox On 06/07/2011 06:44 PM, Greg KH wrote: > On Sun, Jun 05, 2011 at 02:16:17PM +0200, Jiri Slaby wrote: >> With the previous patch, we fixed another bug where read_buf was freed >> while we still was in n_tty_read. We currently check whether read_buf >> is NULL at the start of the function. Add one more check after we wake >> up from waiting for input. >> >> Signed-off-by: Jiri Slaby <jslaby@suse.cz> >> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk> >> --- >> drivers/tty/n_tty.c | 1 + >> 1 files changed, 1 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c >> index 95d0a9c..c62c856 100644 >> --- a/drivers/tty/n_tty.c >> +++ b/drivers/tty/n_tty.c >> @@ -1785,6 +1785,7 @@ do_it_again: >> break; >> } >> timeout = schedule_timeout(timeout); >> + BUG_ON(!tty->read_buf); > > So, if we ever hit this, what are we going to do with this crash? > > I really don't want to add more BUG_ON() calls to the kernel if at all > possible. Or is it the case that we will crash if this case is true > soon afterward anyway? Yeah, it will crash something like 10 lines below. The pointer is dereferenced there. thanks, -- js suse labs ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] TTY: ntty, add one more sanity check 2011-06-07 16:57 ` Jiri Slaby @ 2011-06-07 17:10 ` Greg KH 0 siblings, 0 replies; 10+ messages in thread From: Greg KH @ 2011-06-07 17:10 UTC (permalink / raw) To: Jiri Slaby; +Cc: gregkh, jirislaby, linux-kernel, Alan Cox On Tue, Jun 07, 2011 at 06:57:50PM +0200, Jiri Slaby wrote: > On 06/07/2011 06:44 PM, Greg KH wrote: > > On Sun, Jun 05, 2011 at 02:16:17PM +0200, Jiri Slaby wrote: > >> With the previous patch, we fixed another bug where read_buf was freed > >> while we still was in n_tty_read. We currently check whether read_buf > >> is NULL at the start of the function. Add one more check after we wake > >> up from waiting for input. > >> > >> Signed-off-by: Jiri Slaby <jslaby@suse.cz> > >> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk> > >> --- > >> drivers/tty/n_tty.c | 1 + > >> 1 files changed, 1 insertions(+), 0 deletions(-) > >> > >> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c > >> index 95d0a9c..c62c856 100644 > >> --- a/drivers/tty/n_tty.c > >> +++ b/drivers/tty/n_tty.c > >> @@ -1785,6 +1785,7 @@ do_it_again: > >> break; > >> } > >> timeout = schedule_timeout(timeout); > >> + BUG_ON(!tty->read_buf); > > > > So, if we ever hit this, what are we going to do with this crash? > > > > I really don't want to add more BUG_ON() calls to the kernel if at all > > possible. Or is it the case that we will crash if this case is true > > soon afterward anyway? > > Yeah, it will crash something like 10 lines below. The pointer is > dereferenced there. Ok, fair enough, thanks. greg k-h ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: TTY: ntty, add one more sanity check 2011-06-05 12:16 ` [PATCH v2 2/2] TTY: ntty, add one more sanity check Jiri Slaby 2011-06-07 16:44 ` Greg KH @ 2011-06-07 17:20 ` Greg KH 2011-06-07 17:30 ` Jiri Slaby 1 sibling, 1 reply; 10+ messages in thread From: Greg KH @ 2011-06-07 17:20 UTC (permalink / raw) To: Jiri Slaby; +Cc: gregkh, jirislaby, linux-kernel, Alan Cox On Sun, Jun 05, 2011 at 02:16:17PM +0200, Jiri Slaby wrote: > With the previous patch, we fixed another bug where read_buf was freed > while we still was in n_tty_read. We currently check whether read_buf > is NULL at the start of the function. Add one more check after we wake > up from waiting for input. > > Signed-off-by: Jiri Slaby <jslaby@suse.cz> > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk> > --- > drivers/tty/n_tty.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c > index 95d0a9c..c62c856 100644 > --- a/drivers/tty/n_tty.c > +++ b/drivers/tty/n_tty.c > @@ -1785,6 +1785,7 @@ do_it_again: > break; > } > timeout = schedule_timeout(timeout); > + BUG_ON(!tty->read_buf); > continue; > } > __set_current_state(TASK_RUNNING); This doesn't apply anymore without some fuzz as stuff has changed in this area in Linus's tree. Can you refresh it and resend it so that I know it's correct? Or, just verify that the diff below is correct, and I'll take that one. thanks, greg k-h diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index 0ad3288..c3954fb 100644 --- a/drivers/tty/n_tty.c +++ b/drivers/tty/n_tty.c @@ -1815,6 +1815,7 @@ do_it_again: /* FIXME: does n_tty_set_room need locking ? */ n_tty_set_room(tty); timeout = schedule_timeout(timeout); + BUG_ON(!tty->read_buf); continue; } __set_current_state(TASK_RUNNING); ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: TTY: ntty, add one more sanity check 2011-06-07 17:20 ` Greg KH @ 2011-06-07 17:30 ` Jiri Slaby 2011-06-07 17:37 ` Greg KH 0 siblings, 1 reply; 10+ messages in thread From: Jiri Slaby @ 2011-06-07 17:30 UTC (permalink / raw) To: Greg KH; +Cc: gregkh, jirislaby, linux-kernel, Alan Cox On 06/07/2011 07:20 PM, Greg KH wrote: > On Sun, Jun 05, 2011 at 02:16:17PM +0200, Jiri Slaby wrote: >> With the previous patch, we fixed another bug where read_buf was freed >> while we still was in n_tty_read. We currently check whether read_buf >> is NULL at the start of the function. Add one more check after we wake >> up from waiting for input. >> >> Signed-off-by: Jiri Slaby <jslaby@suse.cz> >> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk> >> --- >> drivers/tty/n_tty.c | 1 + >> 1 files changed, 1 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c >> index 95d0a9c..c62c856 100644 >> --- a/drivers/tty/n_tty.c >> +++ b/drivers/tty/n_tty.c >> @@ -1785,6 +1785,7 @@ do_it_again: >> break; >> } >> timeout = schedule_timeout(timeout); >> + BUG_ON(!tty->read_buf); >> continue; >> } >> __set_current_state(TASK_RUNNING); > > This doesn't apply anymore without some fuzz as stuff has changed in > this area in Linus's tree. > > Can you refresh it and resend it so that I know it's correct? > > Or, just verify that the diff below is correct, and I'll take that one. Yes, it's correct. > thanks, > > greg k-h > > diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c > index 0ad3288..c3954fb 100644 > --- a/drivers/tty/n_tty.c > +++ b/drivers/tty/n_tty.c > @@ -1815,6 +1815,7 @@ do_it_again: > /* FIXME: does n_tty_set_room need locking ? */ > n_tty_set_room(tty); > timeout = schedule_timeout(timeout); > + BUG_ON(!tty->read_buf); > continue; > } > __set_current_state(TASK_RUNNING); thanks, -- js suse labs ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: TTY: ntty, add one more sanity check 2011-06-07 17:30 ` Jiri Slaby @ 2011-06-07 17:37 ` Greg KH 0 siblings, 0 replies; 10+ messages in thread From: Greg KH @ 2011-06-07 17:37 UTC (permalink / raw) To: Jiri Slaby; +Cc: gregkh, jirislaby, linux-kernel, Alan Cox On Tue, Jun 07, 2011 at 07:30:31PM +0200, Jiri Slaby wrote: > On 06/07/2011 07:20 PM, Greg KH wrote: > > On Sun, Jun 05, 2011 at 02:16:17PM +0200, Jiri Slaby wrote: > >> With the previous patch, we fixed another bug where read_buf was freed > >> while we still was in n_tty_read. We currently check whether read_buf > >> is NULL at the start of the function. Add one more check after we wake > >> up from waiting for input. > >> > >> Signed-off-by: Jiri Slaby <jslaby@suse.cz> > >> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk> > >> --- > >> drivers/tty/n_tty.c | 1 + > >> 1 files changed, 1 insertions(+), 0 deletions(-) > >> > >> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c > >> index 95d0a9c..c62c856 100644 > >> --- a/drivers/tty/n_tty.c > >> +++ b/drivers/tty/n_tty.c > >> @@ -1785,6 +1785,7 @@ do_it_again: > >> break; > >> } > >> timeout = schedule_timeout(timeout); > >> + BUG_ON(!tty->read_buf); > >> continue; > >> } > >> __set_current_state(TASK_RUNNING); > > > > This doesn't apply anymore without some fuzz as stuff has changed in > > this area in Linus's tree. > > > > Can you refresh it and resend it so that I know it's correct? > > > > Or, just verify that the diff below is correct, and I'll take that one. > > Yes, it's correct. Thanks for confirming, now applied. greg k-h ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-06-07 17:37 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1307106096-6461-1-git-send-email-jslaby@suse.cz>
2011-06-03 13:01 ` [PATCH 2/2] TTY: ntty, add one more sanoty check Jiri Slaby
2011-06-03 13:06 ` [PATCH 1/2] TTY: ldisc, do not close until there are readers Jiri Slaby
2011-06-03 21:52 ` Linus Torvalds
[not found] ` <1307276177-20957-1-git-send-email-jslaby@suse.cz>
2011-06-05 12:16 ` [PATCH v2 2/2] TTY: ntty, add one more sanity check Jiri Slaby
2011-06-07 16:44 ` Greg KH
2011-06-07 16:57 ` Jiri Slaby
2011-06-07 17:10 ` Greg KH
2011-06-07 17:20 ` Greg KH
2011-06-07 17:30 ` Jiri Slaby
2011-06-07 17:37 ` Greg KH
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).