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