[PATCH 2/2] TTY: ntty, add one more sanoty check

[PATCH 2/2] TTY: ntty, add one more sanoty check

[Jiri Slaby]

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

Re: [PATCH 1/2] TTY: ldisc, do not close until there are readers

[Jiri Slaby]

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

Re: [PATCH 1/2] TTY: ldisc, do not close until there are readers

[Linus Torvalds]

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

[PATCH v2 2/2] TTY: ntty, add one more sanity check

[Jiri Slaby]

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

Re: [PATCH v2 2/2] TTY: ntty, add one more sanity check

[Greg KH]

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

Re: [PATCH v2 2/2] TTY: ntty, add one more sanity check

[Jiri Slaby]

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

Re: [PATCH v2 2/2] TTY: ntty, add one more sanity check

[Greg KH]

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

Re: TTY: ntty, add one more sanity check

[Greg KH]

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);

Re: TTY: ntty, add one more sanity check

[Jiri Slaby]

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

Re: TTY: ntty, add one more sanity check

[Greg KH]

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