Patchwork TTY: hvc_console, fix port reference count going to zero prematurely

login
register
mail settings
Submitter Paul Mackerras
Date Nov. 14, 2012, 8:15 a.m.
Message ID <20121114081547.GA573@bloggs.ozlabs.ibm.com>
Download mbox | patch
Permalink /patch/198832/
State Accepted, archived
Headers show

Comments

Paul Mackerras - Nov. 14, 2012, 8:15 a.m.
Commit bdb498c20040 "TTY: hvc_console, add tty install" took the port
refcounting out of hvc_open()/hvc_close(), but failed to remove the
kref_put() and tty_kref_put() calls in hvc_hangup() that were there to
remove the extra references that hvc_open() had taken.

The result was that doing a vhangup() when the current terminal was
a hvc_console, then closing the current terminal, would end up calling
destroy_hvc_struct() and making the port disappear entirely.  This
meant that Fedora 17 systems would boot up but then not display the
login prompt on the console, and attempts to open /dev/hvc0 would
give a "No such device" error.

This fixes it by removing the extra kref_put() and tty_kref_put() calls.

Signed-off-by: Paul Mackerras <paulus@samba.org>
Cc: stable@vger.kernel.org
---
 drivers/tty/hvc/hvc_console.c |    7 -------
 1 file changed, 7 deletions(-)
Jiri Slaby - Nov. 14, 2012, 9:45 a.m.
On 11/14/2012 09:15 AM, Paul Mackerras wrote:
> Commit bdb498c20040 "TTY: hvc_console, add tty install" took the port
> refcounting out of hvc_open()/hvc_close(), but failed to remove the
> kref_put() and tty_kref_put() calls in hvc_hangup() that were there to
> remove the extra references that hvc_open() had taken.
> 
> The result was that doing a vhangup() when the current terminal was
> a hvc_console, then closing the current terminal, would end up calling
> destroy_hvc_struct() and making the port disappear entirely.  This
> meant that Fedora 17 systems would boot up but then not display the
> login prompt on the console, and attempts to open /dev/hvc0 would
> give a "No such device" error.
> 
> This fixes it by removing the extra kref_put() and tty_kref_put() calls.

Oh yeah. Thanks.

Acked-by: Jiri Slaby <jslaby@suse.cz>

> Signed-off-by: Paul Mackerras <paulus@samba.org>
> Cc: stable@vger.kernel.org
> ---
>  drivers/tty/hvc/hvc_console.c |    7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
> index a5dec1c..13ee53b 100644
> --- a/drivers/tty/hvc/hvc_console.c
> +++ b/drivers/tty/hvc/hvc_console.c
> @@ -424,7 +424,6 @@ static void hvc_hangup(struct tty_struct *tty)
>  {
>  	struct hvc_struct *hp = tty->driver_data;
>  	unsigned long flags;
> -	int temp_open_count;
>  
>  	if (!hp)
>  		return;
> @@ -444,7 +443,6 @@ static void hvc_hangup(struct tty_struct *tty)
>  		return;
>  	}
>  
> -	temp_open_count = hp->port.count;
>  	hp->port.count = 0;
>  	spin_unlock_irqrestore(&hp->port.lock, flags);
>  	tty_port_tty_set(&hp->port, NULL);
> @@ -453,11 +451,6 @@ static void hvc_hangup(struct tty_struct *tty)
>  
>  	if (hp->ops->notifier_hangup)
>  		hp->ops->notifier_hangup(hp, hp->data);
> -
> -	while(temp_open_count) {
> -		--temp_open_count;
> -		tty_port_put(&hp->port);
> -	}
>  }
>  
>  /*
>
Benjamin Herrenschmidt - Nov. 14, 2012, 12:47 p.m.
On Wed, 2012-11-14 at 10:45 +0100, Jiri Slaby wrote:

> > This fixes it by removing the extra kref_put() and tty_kref_put() calls.
> 
> Oh yeah. Thanks.
> 
> Acked-by: Jiri Slaby <jslaby@suse.cz>

So who's merging it ?

Cheers,
Ben.

> > Signed-off-by: Paul Mackerras <paulus@samba.org>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/tty/hvc/hvc_console.c |    7 -------
> >  1 file changed, 7 deletions(-)
> > 
> > diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
> > index a5dec1c..13ee53b 100644
> > --- a/drivers/tty/hvc/hvc_console.c
> > +++ b/drivers/tty/hvc/hvc_console.c
> > @@ -424,7 +424,6 @@ static void hvc_hangup(struct tty_struct *tty)
> >  {
> >  	struct hvc_struct *hp = tty->driver_data;
> >  	unsigned long flags;
> > -	int temp_open_count;
> >  
> >  	if (!hp)
> >  		return;
> > @@ -444,7 +443,6 @@ static void hvc_hangup(struct tty_struct *tty)
> >  		return;
> >  	}
> >  
> > -	temp_open_count = hp->port.count;
> >  	hp->port.count = 0;
> >  	spin_unlock_irqrestore(&hp->port.lock, flags);
> >  	tty_port_tty_set(&hp->port, NULL);
> > @@ -453,11 +451,6 @@ static void hvc_hangup(struct tty_struct *tty)
> >  
> >  	if (hp->ops->notifier_hangup)
> >  		hp->ops->notifier_hangup(hp, hp->data);
> > -
> > -	while(temp_open_count) {
> > -		--temp_open_count;
> > -		tty_port_put(&hp->port);
> > -	}
> >  }
> >  
> >  /*
> > 
> 
>
Greg KH - Nov. 14, 2012, 3:09 p.m.
On Wed, Nov 14, 2012 at 11:47:59PM +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2012-11-14 at 10:45 +0100, Jiri Slaby wrote:
> 
> > > This fixes it by removing the extra kref_put() and tty_kref_put() calls.
> > 
> > Oh yeah. Thanks.
> > 
> > Acked-by: Jiri Slaby <jslaby@suse.cz>
> 
> So who's merging it ?

Give me a chance to at least wake up please :)

I will.

greg k-h
Benjamin Herrenschmidt - Nov. 14, 2012, 8:48 p.m.
On Wed, 2012-11-14 at 07:09 -0800, Greg KH wrote:
> > So who's merging it ?
> 
> Give me a chance to at least wake up please :)

Sure ;-) Just asking since I'm about to cook up a powerpc batch :-)

> I will.

Thanks ! It should go into stable 3.6 as well.

Cheers,
Ben.
Greg KH - Nov. 14, 2012, 8:54 p.m.
On Thu, Nov 15, 2012 at 07:48:17AM +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2012-11-14 at 07:09 -0800, Greg KH wrote:
> > > So who's merging it ?
> > 
> > Give me a chance to at least wake up please :)
> 
> Sure ;-) Just asking since I'm about to cook up a powerpc batch :-)

It's now in my tree, and will get to Linus in time for 3.7.

> > I will.
> 
> Thanks ! It should go into stable 3.6 as well.

Why?  The offending patch didn't show up until 3.7-rc1.

thanks,

greg k-h
Benjamin Herrenschmidt - Nov. 14, 2012, 8:57 p.m.
On Wed, 2012-11-14 at 12:54 -0800, Greg KH wrote:
> > Thanks ! It should go into stable 3.6 as well.
> 
> Why?  The offending patch didn't show up until 3.7-rc1.

Ah, my bad, I though Paulus had observed the problem with 3.6 as well
but it looks like you are right. So 3.7 then.

Thanks.

Cheers,
Ben.
Paul Mackerras - Nov. 14, 2012, 11:02 p.m.
On Wed, Nov 14, 2012 at 12:54:07PM -0800, Greg KH wrote:
> On Thu, Nov 15, 2012 at 07:48:17AM +1100, Benjamin Herrenschmidt wrote:
> > On Wed, 2012-11-14 at 07:09 -0800, Greg KH wrote:
> > > > So who's merging it ?
> > > 
> > > Give me a chance to at least wake up please :)
> > 
> > Sure ;-) Just asking since I'm about to cook up a powerpc batch :-)
> 
> It's now in my tree, and will get to Linus in time for 3.7.

Turns out I stuffed up the commit message a bit, I talk about kref_put
and tty_kref_put when I should be talking about tty_port_put.  If
there is still a chance to update the commit message, that would be
good.  The patch itself is correct.

> > > I will.
> > 
> > Thanks ! It should go into stable 3.6 as well.
> 
> Why?  The offending patch didn't show up until 3.7-rc1.

Yes.  I had thought it was in 3.6 but it isn't.

Paul.

Patch

diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index a5dec1c..13ee53b 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -424,7 +424,6 @@  static void hvc_hangup(struct tty_struct *tty)
 {
 	struct hvc_struct *hp = tty->driver_data;
 	unsigned long flags;
-	int temp_open_count;
 
 	if (!hp)
 		return;
@@ -444,7 +443,6 @@  static void hvc_hangup(struct tty_struct *tty)
 		return;
 	}
 
-	temp_open_count = hp->port.count;
 	hp->port.count = 0;
 	spin_unlock_irqrestore(&hp->port.lock, flags);
 	tty_port_tty_set(&hp->port, NULL);
@@ -453,11 +451,6 @@  static void hvc_hangup(struct tty_struct *tty)
 
 	if (hp->ops->notifier_hangup)
 		hp->ops->notifier_hangup(hp, hp->data);
-
-	while(temp_open_count) {
-		--temp_open_count;
-		tty_port_put(&hp->port);
-	}
 }
 
 /*