diff mbox series

[SRU,X,B,F,G,1/1] tty: hvcs: Don't NULL tty->driver_data until hvcs_cleanup()

Message ID 20200915185649.24706-2-patricia.domingues@canonical.com
State New
Headers show
Series Don't NULL tty->driver_data until hvcs_cleanup() (LP: 1892546) | expand

Commit Message

patricia.domingues@canonical.com Sept. 15, 2020, 6:56 p.m. UTC
From: Tyrel Datwyler <tyreld@linux.ibm.com>

BugLink: https://bugs.launchpad.net/bugs/1892546

The code currently NULLs tty->driver_data in hvcs_close() with the
intent of informing the next call to hvcs_open() that device needs to be
reconfigured. However, when hvcs_cleanup() is called we copy hvcsd from
tty->driver_data which was previoulsy NULLed by hvcs_close() and our
call to tty_port_put(&hvcsd->port) doesn't actually do anything since
&hvcsd->port ends up translating to NULL by chance. This has the side
effect that when hvcs_remove() is called we have one too many port
references preventing hvcs_destuct_port() from ever being called. This
also prevents us from reusing the /dev/hvcsX node in a future
hvcs_probe() and we can eventually run out of /dev/hvcsX devices.

Fix this by waiting to NULL tty->driver_data in hvcs_cleanup().

Fixes: 27bf7c43a19c ("TTY: hvcs, add tty install")
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
Link: https://lore.kernel.org/r/20200820234643.70412-1-tyreld@linux.ibm.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit 63ffcbdad738e3d1c857027789a2273df3337624)
Signed-off-by: Patricia Domingues <patricia.domingues@canonical.com>
---
 drivers/tty/hvc/hvcs.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Kleber Souza Sept. 16, 2020, 9:11 a.m. UTC | #1
On 15.09.20 20:56, patricia.domingues@canonical.com wrote:
> From: Tyrel Datwyler <tyreld@linux.ibm.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1892546
> 
> The code currently NULLs tty->driver_data in hvcs_close() with the
> intent of informing the next call to hvcs_open() that device needs to be
> reconfigured. However, when hvcs_cleanup() is called we copy hvcsd from
> tty->driver_data which was previoulsy NULLed by hvcs_close() and our
> call to tty_port_put(&hvcsd->port) doesn't actually do anything since
> &hvcsd->port ends up translating to NULL by chance. This has the side
> effect that when hvcs_remove() is called we have one too many port
> references preventing hvcs_destuct_port() from ever being called. This
> also prevents us from reusing the /dev/hvcsX node in a future
> hvcs_probe() and we can eventually run out of /dev/hvcsX devices.
> 
> Fix this by waiting to NULL tty->driver_data in hvcs_cleanup().
> 
> Fixes: 27bf7c43a19c ("TTY: hvcs, add tty install")
> Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
> Link: https://lore.kernel.org/r/20200820234643.70412-1-tyreld@linux.ibm.com
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> (cherry picked from commit 63ffcbdad738e3d1c857027789a2273df3337624)
> Signed-off-by: Patricia Domingues <patricia.domingues@canonical.com>

Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

> ---
>  drivers/tty/hvc/hvcs.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
> index 5997b1731111..cba662c50f91 100644
> --- a/drivers/tty/hvc/hvcs.c
> +++ b/drivers/tty/hvc/hvcs.c
> @@ -1232,13 +1232,6 @@ static void hvcs_close(struct tty_struct *tty, struct file *filp)
>  
>  		tty_wait_until_sent(tty, HVCS_CLOSE_WAIT);
>  
> -		/*
> -		 * This line is important because it tells hvcs_open that this
> -		 * device needs to be re-configured the next time hvcs_open is
> -		 * called.
> -		 */
> -		tty->driver_data = NULL;
> -
>  		free_irq(irq, hvcsd);
>  		return;
>  	} else if (hvcsd->port.count < 0) {
> @@ -1254,6 +1247,13 @@ static void hvcs_cleanup(struct tty_struct * tty)
>  {
>  	struct hvcs_struct *hvcsd = tty->driver_data;
>  
> +	/*
> +	 * This line is important because it tells hvcs_open that this
> +	 * device needs to be re-configured the next time hvcs_open is
> +	 * called.
> +	 */
> +	tty->driver_data = NULL;
> +
>  	tty_port_put(&hvcsd->port);
>  }
>  
>
Colin King Sept. 16, 2020, 9:19 a.m. UTC | #2
On 15/09/2020 19:56, patricia.domingues@canonical.com wrote:
> From: Tyrel Datwyler <tyreld@linux.ibm.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1892546
> 
> The code currently NULLs tty->driver_data in hvcs_close() with the
> intent of informing the next call to hvcs_open() that device needs to be
> reconfigured. However, when hvcs_cleanup() is called we copy hvcsd from
> tty->driver_data which was previoulsy NULLed by hvcs_close() and our
> call to tty_port_put(&hvcsd->port) doesn't actually do anything since
> &hvcsd->port ends up translating to NULL by chance. This has the side
> effect that when hvcs_remove() is called we have one too many port
> references preventing hvcs_destuct_port() from ever being called. This
> also prevents us from reusing the /dev/hvcsX node in a future
> hvcs_probe() and we can eventually run out of /dev/hvcsX devices.
> 
> Fix this by waiting to NULL tty->driver_data in hvcs_cleanup().
> 
> Fixes: 27bf7c43a19c ("TTY: hvcs, add tty install")
> Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
> Link: https://lore.kernel.org/r/20200820234643.70412-1-tyreld@linux.ibm.com
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> (cherry picked from commit 63ffcbdad738e3d1c857027789a2273df3337624)
> Signed-off-by: Patricia Domingues <patricia.domingues@canonical.com>
> ---
>  drivers/tty/hvc/hvcs.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
> index 5997b1731111..cba662c50f91 100644
> --- a/drivers/tty/hvc/hvcs.c
> +++ b/drivers/tty/hvc/hvcs.c
> @@ -1232,13 +1232,6 @@ static void hvcs_close(struct tty_struct *tty, struct file *filp)
>  
>  		tty_wait_until_sent(tty, HVCS_CLOSE_WAIT);
>  
> -		/*
> -		 * This line is important because it tells hvcs_open that this
> -		 * device needs to be re-configured the next time hvcs_open is
> -		 * called.
> -		 */
> -		tty->driver_data = NULL;
> -
>  		free_irq(irq, hvcsd);
>  		return;
>  	} else if (hvcsd->port.count < 0) {
> @@ -1254,6 +1247,13 @@ static void hvcs_cleanup(struct tty_struct * tty)
>  {
>  	struct hvcs_struct *hvcsd = tty->driver_data;
>  
> +	/*
> +	 * This line is important because it tells hvcs_open that this
> +	 * device needs to be re-configured the next time hvcs_open is
> +	 * called.
> +	 */
> +	tty->driver_data = NULL;
> +
>  	tty_port_put(&hvcsd->port);
>  }
>  
> 

Sane testing, fix looks good.

The cherry pick is from linux-next though, I can't find that sha in linux.

Colin
Kelsey Skunberg Sept. 17, 2020, 5:41 p.m. UTC | #3
On 2020-09-16 10:19:39 , Colin Ian King wrote:
> On 15/09/2020 19:56, patricia.domingues@canonical.com wrote:
> > From: Tyrel Datwyler <tyreld@linux.ibm.com>
> > 
> > BugLink: https://bugs.launchpad.net/bugs/1892546
> > 
> > The code currently NULLs tty->driver_data in hvcs_close() with the
> > intent of informing the next call to hvcs_open() that device needs to be
> > reconfigured. However, when hvcs_cleanup() is called we copy hvcsd from
> > tty->driver_data which was previoulsy NULLed by hvcs_close() and our
> > call to tty_port_put(&hvcsd->port) doesn't actually do anything since
> > &hvcsd->port ends up translating to NULL by chance. This has the side
> > effect that when hvcs_remove() is called we have one too many port
> > references preventing hvcs_destuct_port() from ever being called. This
> > also prevents us from reusing the /dev/hvcsX node in a future
> > hvcs_probe() and we can eventually run out of /dev/hvcsX devices.
> > 
> > Fix this by waiting to NULL tty->driver_data in hvcs_cleanup().
> > 
> > Fixes: 27bf7c43a19c ("TTY: hvcs, add tty install")
> > Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
> > Link: https://lore.kernel.org/r/20200820234643.70412-1-tyreld@linux.ibm.com
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > (cherry picked from commit 63ffcbdad738e3d1c857027789a2273df3337624)
> > Signed-off-by: Patricia Domingues <patricia.domingues@canonical.com>
> > ---
> >  drivers/tty/hvc/hvcs.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
> > index 5997b1731111..cba662c50f91 100644
> > --- a/drivers/tty/hvc/hvcs.c
> > +++ b/drivers/tty/hvc/hvcs.c
> > @@ -1232,13 +1232,6 @@ static void hvcs_close(struct tty_struct *tty, struct file *filp)
> >  
> >  		tty_wait_until_sent(tty, HVCS_CLOSE_WAIT);
> >  
> > -		/*
> > -		 * This line is important because it tells hvcs_open that this
> > -		 * device needs to be re-configured the next time hvcs_open is
> > -		 * called.
> > -		 */
> > -		tty->driver_data = NULL;
> > -
> >  		free_irq(irq, hvcsd);
> >  		return;
> >  	} else if (hvcsd->port.count < 0) {
> > @@ -1254,6 +1247,13 @@ static void hvcs_cleanup(struct tty_struct * tty)
> >  {
> >  	struct hvcs_struct *hvcsd = tty->driver_data;
> >  
> > +	/*
> > +	 * This line is important because it tells hvcs_open that this
> > +	 * device needs to be re-configured the next time hvcs_open is
> > +	 * called.
> > +	 */
> > +	tty->driver_data = NULL;
> > +
> >  	tty_port_put(&hvcsd->port);
> >  }
> >  
> > 
> 
> Sane testing, fix looks good.
> 
> The cherry pick is from linux-next though, I can't find that sha in linux.
> 
> Colin
>

Hey Colin, 

Is this suppose to be an ACK or just a comment? Pending this is fully
reviewed, I'll change the cherry pick line while applying. Thanks! 

-Kelsey 

> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Colin King Sept. 17, 2020, 5:47 p.m. UTC | #4
On 17/09/2020 18:41, Kelsey Skunberg wrote:
> On 2020-09-16 10:19:39 , Colin Ian King wrote:
>> On 15/09/2020 19:56, patricia.domingues@canonical.com wrote:
>>> From: Tyrel Datwyler <tyreld@linux.ibm.com>
>>>
>>> BugLink: https://bugs.launchpad.net/bugs/1892546
>>>
>>> The code currently NULLs tty->driver_data in hvcs_close() with the
>>> intent of informing the next call to hvcs_open() that device needs to be
>>> reconfigured. However, when hvcs_cleanup() is called we copy hvcsd from
>>> tty->driver_data which was previoulsy NULLed by hvcs_close() and our
>>> call to tty_port_put(&hvcsd->port) doesn't actually do anything since
>>> &hvcsd->port ends up translating to NULL by chance. This has the side
>>> effect that when hvcs_remove() is called we have one too many port
>>> references preventing hvcs_destuct_port() from ever being called. This
>>> also prevents us from reusing the /dev/hvcsX node in a future
>>> hvcs_probe() and we can eventually run out of /dev/hvcsX devices.
>>>
>>> Fix this by waiting to NULL tty->driver_data in hvcs_cleanup().
>>>
>>> Fixes: 27bf7c43a19c ("TTY: hvcs, add tty install")
>>> Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
>>> Link: https://lore.kernel.org/r/20200820234643.70412-1-tyreld@linux.ibm.com
>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> (cherry picked from commit 63ffcbdad738e3d1c857027789a2273df3337624)
>>> Signed-off-by: Patricia Domingues <patricia.domingues@canonical.com>
>>> ---
>>>  drivers/tty/hvc/hvcs.c | 14 +++++++-------
>>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
>>> index 5997b1731111..cba662c50f91 100644
>>> --- a/drivers/tty/hvc/hvcs.c
>>> +++ b/drivers/tty/hvc/hvcs.c
>>> @@ -1232,13 +1232,6 @@ static void hvcs_close(struct tty_struct *tty, struct file *filp)
>>>  
>>>  		tty_wait_until_sent(tty, HVCS_CLOSE_WAIT);
>>>  
>>> -		/*
>>> -		 * This line is important because it tells hvcs_open that this
>>> -		 * device needs to be re-configured the next time hvcs_open is
>>> -		 * called.
>>> -		 */
>>> -		tty->driver_data = NULL;
>>> -
>>>  		free_irq(irq, hvcsd);
>>>  		return;
>>>  	} else if (hvcsd->port.count < 0) {
>>> @@ -1254,6 +1247,13 @@ static void hvcs_cleanup(struct tty_struct * tty)
>>>  {
>>>  	struct hvcs_struct *hvcsd = tty->driver_data;
>>>  
>>> +	/*
>>> +	 * This line is important because it tells hvcs_open that this
>>> +	 * device needs to be re-configured the next time hvcs_open is
>>> +	 * called.
>>> +	 */
>>> +	tty->driver_data = NULL;
>>> +
>>>  	tty_port_put(&hvcsd->port);
>>>  }
>>>  
>>>
>>
>> Sane testing, fix looks good.
>>
>> The cherry pick is from linux-next though, I can't find that sha in linux.
>>
>> Colin
>>
> 
> Hey Colin, 
> 
> Is this suppose to be an ACK or just a comment? Pending this is fully
> reviewed, I'll change the cherry pick line while applying. Thanks! 

It's now an ACK since I know the cherry pick like will be fixed up :-)

thanks,

Colin

> 
> -Kelsey 
> 
>> -- 
>> kernel-team mailing list
>> kernel-team@lists.ubuntu.com
>> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Kelsey Skunberg Sept. 17, 2020, 6:16 p.m. UTC | #5
On 2020-09-17 18:47:27 , Colin Ian King wrote:
> On 17/09/2020 18:41, Kelsey Skunberg wrote:
> > On 2020-09-16 10:19:39 , Colin Ian King wrote:
> >> On 15/09/2020 19:56, patricia.domingues@canonical.com wrote:
> >>> From: Tyrel Datwyler <tyreld@linux.ibm.com>
> >>>
> >>> BugLink: https://bugs.launchpad.net/bugs/1892546
> >>>
> >>> The code currently NULLs tty->driver_data in hvcs_close() with the
> >>> intent of informing the next call to hvcs_open() that device needs to be
> >>> reconfigured. However, when hvcs_cleanup() is called we copy hvcsd from
> >>> tty->driver_data which was previoulsy NULLed by hvcs_close() and our
> >>> call to tty_port_put(&hvcsd->port) doesn't actually do anything since
> >>> &hvcsd->port ends up translating to NULL by chance. This has the side
> >>> effect that when hvcs_remove() is called we have one too many port
> >>> references preventing hvcs_destuct_port() from ever being called. This
> >>> also prevents us from reusing the /dev/hvcsX node in a future
> >>> hvcs_probe() and we can eventually run out of /dev/hvcsX devices.
> >>>
> >>> Fix this by waiting to NULL tty->driver_data in hvcs_cleanup().
> >>>
> >>> Fixes: 27bf7c43a19c ("TTY: hvcs, add tty install")
> >>> Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
> >>> Link: https://lore.kernel.org/r/20200820234643.70412-1-tyreld@linux.ibm.com
> >>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>> (cherry picked from commit 63ffcbdad738e3d1c857027789a2273df3337624)
> >>> Signed-off-by: Patricia Domingues <patricia.domingues@canonical.com>
> >>> ---
> >>>  drivers/tty/hvc/hvcs.c | 14 +++++++-------
> >>>  1 file changed, 7 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
> >>> index 5997b1731111..cba662c50f91 100644
> >>> --- a/drivers/tty/hvc/hvcs.c
> >>> +++ b/drivers/tty/hvc/hvcs.c
> >>> @@ -1232,13 +1232,6 @@ static void hvcs_close(struct tty_struct *tty, struct file *filp)
> >>>  
> >>>  		tty_wait_until_sent(tty, HVCS_CLOSE_WAIT);
> >>>  
> >>> -		/*
> >>> -		 * This line is important because it tells hvcs_open that this
> >>> -		 * device needs to be re-configured the next time hvcs_open is
> >>> -		 * called.
> >>> -		 */
> >>> -		tty->driver_data = NULL;
> >>> -
> >>>  		free_irq(irq, hvcsd);
> >>>  		return;
> >>>  	} else if (hvcsd->port.count < 0) {
> >>> @@ -1254,6 +1247,13 @@ static void hvcs_cleanup(struct tty_struct * tty)
> >>>  {
> >>>  	struct hvcs_struct *hvcsd = tty->driver_data;
> >>>  
> >>> +	/*
> >>> +	 * This line is important because it tells hvcs_open that this
> >>> +	 * device needs to be re-configured the next time hvcs_open is
> >>> +	 * called.
> >>> +	 */
> >>> +	tty->driver_data = NULL;
> >>> +
> >>>  	tty_port_put(&hvcsd->port);
> >>>  }
> >>>  
> >>>
> >>
> >> Sane testing, fix looks good.
> >>
> >> The cherry pick is from linux-next though, I can't find that sha in linux.
> >>
> >> Colin
> >>
> > 
> > Hey Colin, 
> > 
> > Is this suppose to be an ACK or just a comment? Pending this is fully
> > reviewed, I'll change the cherry pick line while applying. Thanks! 
> 
> It's now an ACK since I know the cherry pick like will be fixed up :-)
> 
> thanks,
> 
> Colin
>

sweet, thank you! and will deffinitely do that change. Thanks for catching it!

-Kelsey

> > 
> > -Kelsey 
> > 
> >> -- 
> >> kernel-team mailing list
> >> kernel-team@lists.ubuntu.com
> >> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>
diff mbox series

Patch

diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
index 5997b1731111..cba662c50f91 100644
--- a/drivers/tty/hvc/hvcs.c
+++ b/drivers/tty/hvc/hvcs.c
@@ -1232,13 +1232,6 @@  static void hvcs_close(struct tty_struct *tty, struct file *filp)
 
 		tty_wait_until_sent(tty, HVCS_CLOSE_WAIT);
 
-		/*
-		 * This line is important because it tells hvcs_open that this
-		 * device needs to be re-configured the next time hvcs_open is
-		 * called.
-		 */
-		tty->driver_data = NULL;
-
 		free_irq(irq, hvcsd);
 		return;
 	} else if (hvcsd->port.count < 0) {
@@ -1254,6 +1247,13 @@  static void hvcs_cleanup(struct tty_struct * tty)
 {
 	struct hvcs_struct *hvcsd = tty->driver_data;
 
+	/*
+	 * This line is important because it tells hvcs_open that this
+	 * device needs to be re-configured the next time hvcs_open is
+	 * called.
+	 */
+	tty->driver_data = NULL;
+
 	tty_port_put(&hvcsd->port);
 }