[Xenial][PATCH 0/7] Fix rare tty_ldisc_reinit panic

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Xenial][PATCH 0/7] Fix rare tty_ldisc_reinit panic

Kamal Mostafa-2
BugLink: http://bugs.launchpad.net/bugs/1709126

The mainline commit 892d1fa7ea "tty: Destroy ldisc instance on hangup" (circa
v4.6) fixes a rare panic-inducing race in the tty driver.  A handful of
prerequisite mainline cherry-picks are required to apply this to v4.4.

Positive test results in the original bug:
  https://bugs.launchpad.net/cpc-gce/+bug/1707089

 -Kamal


Peter Hurley (7):
  tty: Simplify tty_set_ldisc() exit handling
  tty: Reset c_line from driver's init_termios
  tty: Handle NULL tty->ldisc
  tty: Move tty_ldisc_kill()
  tty: Use 'disc' for line discipline index name
  tty: Refactor tty_ldisc_reinit() for reuse
  tty: Destroy ldisc instance on hangup

 drivers/tty/tty_io.c    |  23 +++---
 drivers/tty/tty_ldisc.c | 182 ++++++++++++++++++++++++------------------------
 include/linux/tty.h     |   5 +-
 3 files changed, 105 insertions(+), 105 deletions(-)

--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Xenial][PATCH 1/7] tty: Simplify tty_set_ldisc() exit handling

Kamal Mostafa-2
From: Peter Hurley <[hidden email]>

BugLink: http://bugs.launchpad.net/bugs/1709126

Perform common exit for both successful and error exit handling
in tty_set_ldisc(). Fixes unlikely possibility of failing to restart
input kworker when switching to the same line discipline (noop case).

Signed-off-by: Peter Hurley <[hidden email]>
Signed-off-by: Greg Kroah-Hartman <[hidden email]>
(cherry picked from commit 63d8cb3f19dabb409a09b4f2b8827934ab9365a3)
Signed-off-by: Kamal Mostafa <[hidden email]>
---
 drivers/tty/tty_ldisc.c | 42 +++++++++++++-----------------------------
 1 file changed, 13 insertions(+), 29 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 9bee25c..6f59f14 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -536,34 +536,21 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 
  tty_lock(tty);
  retval = tty_ldisc_lock(tty, 5 * HZ);
- if (retval) {
- tty_ldisc_put(new_ldisc);
- tty_unlock(tty);
- return retval;
- }
+ if (retval)
+ goto err;
 
- /*
- * Check the no-op case
- */
+ /* Check the no-op case */
+ if (tty->ldisc->ops->num == ldisc)
+ goto out;
 
- if (tty->ldisc->ops->num == ldisc) {
- tty_ldisc_unlock(tty);
- tty_ldisc_put(new_ldisc);
- tty_unlock(tty);
- return 0;
+ if (test_bit(TTY_HUPPED, &tty->flags)) {
+ /* We were raced by hangup */
+ retval = -EIO;
+ goto out;
  }
 
  old_ldisc = tty->ldisc;
 
- if (test_bit(TTY_HUPPED, &tty->flags)) {
- /* We were raced by the hangup method. It will have stomped
-   the ldisc data and closed the ldisc down */
- tty_ldisc_unlock(tty);
- tty_ldisc_put(new_ldisc);
- tty_unlock(tty);
- return -EIO;
- }
-
  /* Shutdown the old discipline. */
  tty_ldisc_close(tty, old_ldisc);
 
@@ -589,18 +576,15 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
    the old ldisc (if it was restored as part of error cleanup
    above). In either case, releasing a single reference from
    the old ldisc is correct. */
-
- tty_ldisc_put(old_ldisc);
-
- /*
- * Allow ldisc referencing to occur again
- */
+ new_ldisc = old_ldisc;
+out:
  tty_ldisc_unlock(tty);
 
  /* Restart the work queue in case no characters kick it off. Safe if
    already running */
  tty_buffer_restart_work(tty->port);
-
+err:
+ tty_ldisc_put(new_ldisc); /* drop the extra reference */
  tty_unlock(tty);
  return retval;
 }
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Xenial][PATCH 2/7] tty: Reset c_line from driver's init_termios

Kamal Mostafa-2
In reply to this post by Kamal Mostafa-2
From: Peter Hurley <[hidden email]>

BugLink: http://bugs.launchpad.net/bugs/1709126

After the ldisc is released, but before the tty is destroyed, the termios
is saved (in tty_free_termios()); this termios is restored if a new
tty is created on next open(). However, the line discipline is always
reset, which is not obvious in the current method. Instead, reset
as part of the restore.

Restore the original line discipline, which may not have been N_TTY.

Signed-off-by: Peter Hurley <[hidden email]>
Signed-off-by: Greg Kroah-Hartman <[hidden email]>
(cherry picked from commit ece53405a1f8ddf60b78e1365addcad521b2c93f)
Signed-off-by: Kamal Mostafa <[hidden email]>
---
 drivers/tty/tty_io.c    | 5 +++--
 drivers/tty/tty_ldisc.c | 3 ---
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 1bb629a..02f78c0 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1387,9 +1387,10 @@ int tty_init_termios(struct tty_struct *tty)
  else {
  /* Check for lazy saved data */
  tp = tty->driver->termios[idx];
- if (tp != NULL)
+ if (tp != NULL) {
  tty->termios = *tp;
- else
+ tty->termios.c_line  = tty->driver->init_termios.c_line;
+ } else
  tty->termios = tty->driver->init_termios;
  }
  /* Compatibility until drivers always set this */
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 6f59f14..e24983d 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -745,9 +745,6 @@ static void tty_ldisc_kill(struct tty_struct *tty)
  tty_ldisc_put(tty->ldisc);
  /* Force an oops if we mess this up */
  tty->ldisc = NULL;
-
- /* Ensure the next open requests the N_TTY ldisc */
- tty_set_termios_ldisc(tty, N_TTY);
 }
 
 /**
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Xenial][PATCH 3/7] tty: Handle NULL tty->ldisc

Kamal Mostafa-2
In reply to this post by Kamal Mostafa-2
From: Peter Hurley <[hidden email]>

BugLink: http://bugs.launchpad.net/bugs/1709126

In preparation of destroying line discipline on hangup, fix
ldisc core operations to properly handle when the tty's ldisc is
NULL.

Signed-off-by: Peter Hurley <[hidden email]>
Signed-off-by: Greg Kroah-Hartman <[hidden email]>
(cherry picked from commit a570a49abd343102ce681bbf8273897c3c9fd2d1)
Signed-off-by: Kamal Mostafa <[hidden email]>
---
 drivers/tty/tty_ldisc.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index e24983d..0598ba8 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -262,7 +262,8 @@ const struct file_operations tty_ldiscs_proc_fops = {
 struct tty_ldisc *tty_ldisc_ref_wait(struct tty_struct *tty)
 {
  ldsem_down_read(&tty->ldisc_sem, MAX_SCHEDULE_TIMEOUT);
- WARN_ON(!tty->ldisc);
+ if (!tty->ldisc)
+ ldsem_up_read(&tty->ldisc_sem);
  return tty->ldisc;
 }
 EXPORT_SYMBOL_GPL(tty_ldisc_ref_wait);
@@ -455,7 +456,7 @@ static int tty_ldisc_open(struct tty_struct *tty, struct tty_ldisc *ld)
  if (ret)
  clear_bit(TTY_LDISC_OPEN, &tty->flags);
 
- tty_ldisc_debug(tty, "%p: opened\n", tty->ldisc);
+ tty_ldisc_debug(tty, "%p: opened\n", ld);
  return ret;
  }
  return 0;
@@ -476,7 +477,7 @@ static void tty_ldisc_close(struct tty_struct *tty, struct tty_ldisc *ld)
  clear_bit(TTY_LDISC_OPEN, &tty->flags);
  if (ld->ops->close)
  ld->ops->close(tty);
- tty_ldisc_debug(tty, "%p: closed\n", tty->ldisc);
+ tty_ldisc_debug(tty, "%p: closed\n", ld);
 }
 
 /**
@@ -539,6 +540,11 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
  if (retval)
  goto err;
 
+ if (!tty->ldisc) {
+ retval = -EIO;
+ goto out;
+ }
+
  /* Check the no-op case */
  if (tty->ldisc->ops->num == ldisc)
  goto out;
@@ -654,7 +660,7 @@ void tty_ldisc_hangup(struct tty_struct *tty)
  int reset = tty->driver->flags & TTY_DRIVER_RESET_TERMIOS;
  int err = 0;
 
- tty_ldisc_debug(tty, "%p: closing\n", tty->ldisc);
+ tty_ldisc_debug(tty, "%p: hangup\n", tty->ldisc);
 
  ld = tty_ldisc_ref(tty);
  if (ld != NULL) {
@@ -738,6 +744,8 @@ int tty_ldisc_setup(struct tty_struct *tty, struct tty_struct *o_tty)
 
 static void tty_ldisc_kill(struct tty_struct *tty)
 {
+ if (!tty->ldisc)
+ return;
  /*
  * Now kill off the ldisc
  */
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Xenial][PATCH 4/7] tty: Move tty_ldisc_kill()

Kamal Mostafa-2
In reply to this post by Kamal Mostafa-2
From: Peter Hurley <[hidden email]>

BugLink: http://bugs.launchpad.net/bugs/1709126

In preparation for destroying the line discipline instance on hangup,
move tty_ldisc_kill() to eliminate needless forward declarations.
No functional change.

Signed-off-by: Peter Hurley <[hidden email]>
Signed-off-by: Greg Kroah-Hartman <[hidden email]>
(cherry picked from commit 6ffeb4b2782b31f3d7158795a451ad371955e8a2)
Signed-off-by: Kamal Mostafa <[hidden email]>
---
 drivers/tty/tty_ldisc.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 0598ba8..44b2e0b 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -596,6 +596,25 @@ err:
 }
 
 /**
+ * tty_ldisc_kill - teardown ldisc
+ * @tty: tty being released
+ *
+ * Perform final close of the ldisc and reset tty->ldisc
+ */
+static void tty_ldisc_kill(struct tty_struct *tty)
+{
+ if (!tty->ldisc)
+ return;
+ /*
+ * Now kill off the ldisc
+ */
+ tty_ldisc_close(tty, tty->ldisc);
+ tty_ldisc_put(tty->ldisc);
+ /* Force an oops if we mess this up */
+ tty->ldisc = NULL;
+}
+
+/**
  * tty_reset_termios - reset terminal state
  * @tty: tty to reset
  *
@@ -742,19 +761,6 @@ int tty_ldisc_setup(struct tty_struct *tty, struct tty_struct *o_tty)
  return 0;
 }
 
-static void tty_ldisc_kill(struct tty_struct *tty)
-{
- if (!tty->ldisc)
- return;
- /*
- * Now kill off the ldisc
- */
- tty_ldisc_close(tty, tty->ldisc);
- tty_ldisc_put(tty->ldisc);
- /* Force an oops if we mess this up */
- tty->ldisc = NULL;
-}
-
 /**
  * tty_ldisc_release - release line discipline
  * @tty: tty being shut down (or one end of pty pair)
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Xenial][PATCH 5/7] tty: Use 'disc' for line discipline index name

Kamal Mostafa-2
In reply to this post by Kamal Mostafa-2
From: Peter Hurley <[hidden email]>

BugLink: http://bugs.launchpad.net/bugs/1709126

tty->ldisc is a ptr to struct tty_ldisc, but unfortunately 'ldisc' is
also used as a parameter or local name to refer to the line discipline
index value (ie, N_TTY, N_GSM, etc.); instead prefer the name used
by the line discipline registration/ref counting functions.

Signed-off-by: Peter Hurley <[hidden email]>
Signed-off-by: Greg Kroah-Hartman <[hidden email]>
(cherry picked from commit c12da96f801a3f45b0634c966b9e7cda307daa72)
Signed-off-by: Kamal Mostafa <[hidden email]>
---
 drivers/tty/tty_io.c    |  6 +++---
 drivers/tty/tty_ldisc.c | 22 +++++++++++-----------
 include/linux/tty.h     |  2 +-
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 02f78c0..bec360c 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2651,13 +2651,13 @@ static int tiocgsid(struct tty_struct *tty, struct tty_struct *real_tty, pid_t _
 
 static int tiocsetd(struct tty_struct *tty, int __user *p)
 {
- int ldisc;
+ int disc;
  int ret;
 
- if (get_user(ldisc, p))
+ if (get_user(disc, p))
  return -EFAULT;
 
- ret = tty_set_ldisc(tty, ldisc);
+ ret = tty_set_ldisc(tty, disc);
 
  return ret;
 }
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 44b2e0b..9d12812 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -412,7 +412,7 @@ EXPORT_SYMBOL_GPL(tty_ldisc_flush);
 /**
  * tty_set_termios_ldisc - set ldisc field
  * @tty: tty structure
- * @num: line discipline number
+ * @disc: line discipline number
  *
  * This is probably overkill for real world processors but
  * they are not on hot paths so a little discipline won't do
@@ -425,10 +425,10 @@ EXPORT_SYMBOL_GPL(tty_ldisc_flush);
  * Locking: takes termios_rwsem
  */
 
-static void tty_set_termios_ldisc(struct tty_struct *tty, int num)
+static void tty_set_termios_ldisc(struct tty_struct *tty, int disc)
 {
  down_write(&tty->termios_rwsem);
- tty->termios.c_line = num;
+ tty->termios.c_line = disc;
  up_write(&tty->termios_rwsem);
 
  tty->disc_data = NULL;
@@ -526,12 +526,12 @@ static void tty_ldisc_restore(struct tty_struct *tty, struct tty_ldisc *old)
  * the close of one side of a tty/pty pair, and eventually hangup.
  */
 
-int tty_set_ldisc(struct tty_struct *tty, int ldisc)
+int tty_set_ldisc(struct tty_struct *tty, int disc)
 {
  int retval;
  struct tty_ldisc *old_ldisc, *new_ldisc;
 
- new_ldisc = tty_ldisc_get(tty, ldisc);
+ new_ldisc = tty_ldisc_get(tty, disc);
  if (IS_ERR(new_ldisc))
  return PTR_ERR(new_ldisc);
 
@@ -546,7 +546,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
  }
 
  /* Check the no-op case */
- if (tty->ldisc->ops->num == ldisc)
+ if (tty->ldisc->ops->num == disc)
  goto out;
 
  if (test_bit(TTY_HUPPED, &tty->flags)) {
@@ -562,7 +562,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 
  /* Now set up the new line discipline. */
  tty->ldisc = new_ldisc;
- tty_set_termios_ldisc(tty, ldisc);
+ tty_set_termios_ldisc(tty, disc);
 
  retval = tty_ldisc_open(tty, new_ldisc);
  if (retval < 0) {
@@ -634,15 +634,15 @@ static void tty_reset_termios(struct tty_struct *tty)
 /**
  * tty_ldisc_reinit - reinitialise the tty ldisc
  * @tty: tty to reinit
- * @ldisc: line discipline to reinitialize
+ * @disc: line discipline to reinitialize
  *
  * Switch the tty to a line discipline and leave the ldisc
  * state closed
  */
 
-static int tty_ldisc_reinit(struct tty_struct *tty, int ldisc)
+static int tty_ldisc_reinit(struct tty_struct *tty, int disc)
 {
- struct tty_ldisc *ld = tty_ldisc_get(tty, ldisc);
+ struct tty_ldisc *ld = tty_ldisc_get(tty, disc);
 
  if (IS_ERR(ld))
  return -1;
@@ -653,7 +653,7 @@ static int tty_ldisc_reinit(struct tty_struct *tty, int ldisc)
  * Switch the line discipline back
  */
  tty->ldisc = ld;
- tty_set_termios_ldisc(tty, ldisc);
+ tty_set_termios_ldisc(tty, disc);
 
  return 0;
 }
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 83b264c..c35f713 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -580,7 +580,7 @@ static inline int tty_port_users(struct tty_port *port)
 
 extern int tty_register_ldisc(int disc, struct tty_ldisc_ops *new_ldisc);
 extern int tty_unregister_ldisc(int disc);
-extern int tty_set_ldisc(struct tty_struct *tty, int ldisc);
+extern int tty_set_ldisc(struct tty_struct *tty, int disc);
 extern int tty_ldisc_setup(struct tty_struct *tty, struct tty_struct *o_tty);
 extern void tty_ldisc_release(struct tty_struct *tty);
 extern void tty_ldisc_init(struct tty_struct *tty);
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Xenial][PATCH 6/7] tty: Refactor tty_ldisc_reinit() for reuse

Kamal Mostafa-2
In reply to this post by Kamal Mostafa-2
From: Peter Hurley <[hidden email]>

BugLink: http://bugs.launchpad.net/bugs/1709126

At tty hangup, the line discipline instance is reinitialized by
closing the current ldisc instance and opening a new instance.
This operation is complicated by error recovery: if the attempt
to reinit the current line discipline fails, the line discipline
is reset to N_TTY (which should not but can fail).

Re-purpose tty_ldisc_reinit() to return a valid, open line discipline
instance, or otherwise, an error.

Signed-off-by: Peter Hurley <[hidden email]>
Signed-off-by: Greg Kroah-Hartman <[hidden email]>
(cherry picked from commit 7896f30d6fc602f02198999acca4840620288990)
Signed-off-by: Kamal Mostafa <[hidden email]>
---
 drivers/tty/tty_ldisc.c | 53 +++++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 22 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 9d12812..1dd1758 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -636,26 +636,41 @@ static void tty_reset_termios(struct tty_struct *tty)
  * @tty: tty to reinit
  * @disc: line discipline to reinitialize
  *
- * Switch the tty to a line discipline and leave the ldisc
- * state closed
+ * Completely reinitialize the line discipline state, by closing the
+ * current instance and opening a new instance. If an error occurs opening
+ * the new non-N_TTY instance, the instance is dropped and tty->ldisc reset
+ * to NULL. The caller can then retry with N_TTY instead.
+ *
+ * Returns 0 if successful, otherwise error code < 0
  */
 
 static int tty_ldisc_reinit(struct tty_struct *tty, int disc)
 {
- struct tty_ldisc *ld = tty_ldisc_get(tty, disc);
+ struct tty_ldisc *ld;
+ int retval;
 
- if (IS_ERR(ld))
- return -1;
+ ld = tty_ldisc_get(tty, disc);
+ if (IS_ERR(ld)) {
+ BUG_ON(disc == N_TTY);
+ return PTR_ERR(ld);
+ }
 
- tty_ldisc_close(tty, tty->ldisc);
- tty_ldisc_put(tty->ldisc);
- /*
- * Switch the line discipline back
- */
+ if (tty->ldisc) {
+ tty_ldisc_close(tty, tty->ldisc);
+ tty_ldisc_put(tty->ldisc);
+ }
+
+ /* switch the line discipline */
  tty->ldisc = ld;
  tty_set_termios_ldisc(tty, disc);
-
- return 0;
+ retval = tty_ldisc_open(tty, tty->ldisc);
+ if (retval) {
+ if (!WARN_ON(disc == N_TTY)) {
+ tty_ldisc_put(tty->ldisc);
+ tty->ldisc = NULL;
+ }
+ }
+ return retval;
 }
 
 /**
@@ -711,19 +726,13 @@ void tty_ldisc_hangup(struct tty_struct *tty)
    reopen a new ldisc. We could defer the reopen to the next
    open but it means auditing a lot of other paths so this is
    a FIXME */
- if (reset == 0) {
+ if (reset == 0)
+ err = tty_ldisc_reinit(tty, tty->termios.c_line);
 
- if (!tty_ldisc_reinit(tty, tty->termios.c_line))
- err = tty_ldisc_open(tty, tty->ldisc);
- else
- err = 1;
- }
  /* If the re-open fails or we reset then go to N_TTY. The
    N_TTY open cannot fail */
- if (reset || err) {
- BUG_ON(tty_ldisc_reinit(tty, N_TTY));
- WARN_ON(tty_ldisc_open(tty, tty->ldisc));
- }
+ if (reset || err < 0)
+ tty_ldisc_reinit(tty, N_TTY);
  }
  tty_ldisc_unlock(tty);
  if (reset)
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Xenial][PATCH 7/7] tty: Destroy ldisc instance on hangup

Kamal Mostafa-2
In reply to this post by Kamal Mostafa-2
From: Peter Hurley <[hidden email]>

BugLink: http://bugs.launchpad.net/bugs/1709126

Currently, when the tty is hungup, the ldisc is re-instanced; ie., the
current instance is destroyed and a new instance is created. The purpose
of this design was to guarantee a valid, open ldisc for the lifetime of
the tty.

However, now that tty buffers are owned by and have lifetime equivalent
to the tty_port (since v3.10), any data received immediately after the
ldisc is re-instanced may cause continued driver i/o operations
concurrently with the driver's hangup() operation. For drivers that
shutdown h/w on hangup, this is unexpected and usually bad. For example,
the serial core may free the xmit buffer page concurrently with an
in-progress write() operation (triggered by echo).

With the existing stable and robust ldisc reference handling, the
cleaned-up tty_reopen(), the straggling unsafe ldisc use cleaned up, and
the preparation to properly handle a NULL tty->ldisc, the ldisc instance
can be destroyed and only re-instanced when the tty is re-opened.

If the tty was opened as /dev/console or /dev/tty0, the original behavior
of re-instancing the ldisc is retained (the 'reinit' parameter to
tty_ldisc_hangup() is true). This is required since those file descriptors
are never hungup.

This patch has neglible impact on userspace; the tty file_operations ptr
is changed to point to the hungup file operations _before_ the ldisc
instance is destroyed, so only racing file operations might now retrieve
a NULL ldisc reference (which is simply handled as if the hungup file
operation had been called instead -- see "tty: Prepare for destroying
line discipline on hangup").

This resolves a long-standing FIXME and several crash reports.

Signed-off-by: Peter Hurley <[hidden email]>
Signed-off-by: Greg Kroah-Hartman <[hidden email]>
(cherry picked from commit 892d1fa7eaaed9d3c04954cb140c34ebc3393932)
Signed-off-by: Kamal Mostafa <[hidden email]>
---
 drivers/tty/tty_io.c    | 12 ++++++------
 drivers/tty/tty_ldisc.c | 40 +++++++++++++++++-----------------------
 include/linux/tty.h     |  3 ++-
 3 files changed, 25 insertions(+), 30 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index bec360c..8c0f013 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -726,7 +726,7 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
  while (refs--)
  tty_kref_put(tty);
 
- tty_ldisc_hangup(tty);
+ tty_ldisc_hangup(tty, cons_filp != NULL);
 
  spin_lock_irq(&tty->ctrl_lock);
  clear_bit(TTY_THROTTLED, &tty->flags);
@@ -751,10 +751,9 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
  } else if (tty->ops->hangup)
  tty->ops->hangup(tty);
  /*
- * We don't want to have driver/ldisc interactions beyond
- * the ones we did here. The driver layer expects no
- * calls after ->hangup() from the ldisc side. However we
- * can't yet guarantee all that.
+ * We don't want to have driver/ldisc interactions beyond the ones
+ * we did here. The driver layer expects no calls after ->hangup()
+ * from the ldisc side, which is now guaranteed.
  */
  set_bit(TTY_HUPPED, &tty->flags);
  tty_unlock(tty);
@@ -1475,7 +1474,8 @@ static int tty_reopen(struct tty_struct *tty)
 
  tty->count++;
 
- WARN_ON(!tty->ldisc);
+ if (!tty->ldisc)
+ return tty_ldisc_reinit(tty, tty->termios.c_line);
 
  return 0;
 }
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 1dd1758..539dc3c 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -250,6 +250,9 @@ const struct file_operations tty_ldiscs_proc_fops = {
  * reference to it. If the line discipline is in flux then
  * wait patiently until it changes.
  *
+ * Returns: NULL if the tty has been hungup and not re-opened with
+ * a new file descriptor, otherwise valid ldisc reference
+ *
  * Note: Must not be called from an IRQ/timer context. The caller
  * must also be careful not to hold other locks that will deadlock
  * against a discipline change, such as an existing ldisc reference
@@ -637,14 +640,15 @@ static void tty_reset_termios(struct tty_struct *tty)
  * @disc: line discipline to reinitialize
  *
  * Completely reinitialize the line discipline state, by closing the
- * current instance and opening a new instance. If an error occurs opening
- * the new non-N_TTY instance, the instance is dropped and tty->ldisc reset
- * to NULL. The caller can then retry with N_TTY instead.
+ * current instance, if there is one, and opening a new instance. If
+ * an error occurs opening the new non-N_TTY instance, the instance
+ * is dropped and tty->ldisc reset to NULL. The caller can then retry
+ * with N_TTY instead.
  *
  * Returns 0 if successful, otherwise error code < 0
  */
 
-static int tty_ldisc_reinit(struct tty_struct *tty, int disc)
+int tty_ldisc_reinit(struct tty_struct *tty, int disc)
 {
  struct tty_ldisc *ld;
  int retval;
@@ -688,11 +692,9 @@ static int tty_ldisc_reinit(struct tty_struct *tty, int disc)
  * tty itself so we must be careful about locking rules.
  */
 
-void tty_ldisc_hangup(struct tty_struct *tty)
+void tty_ldisc_hangup(struct tty_struct *tty, bool reinit)
 {
  struct tty_ldisc *ld;
- int reset = tty->driver->flags & TTY_DRIVER_RESET_TERMIOS;
- int err = 0;
 
  tty_ldisc_debug(tty, "%p: hangup\n", tty->ldisc);
 
@@ -720,25 +722,17 @@ void tty_ldisc_hangup(struct tty_struct *tty)
  */
  tty_ldisc_lock(tty, MAX_SCHEDULE_TIMEOUT);
 
- if (tty->ldisc) {
+ if (tty->driver->flags & TTY_DRIVER_RESET_TERMIOS)
+ tty_reset_termios(tty);
 
- /* At this point we have a halted ldisc; we want to close it and
-   reopen a new ldisc. We could defer the reopen to the next
-   open but it means auditing a lot of other paths so this is
-   a FIXME */
- if (reset == 0)
- err = tty_ldisc_reinit(tty, tty->termios.c_line);
-
- /* If the re-open fails or we reset then go to N_TTY. The
-   N_TTY open cannot fail */
- if (reset || err < 0)
- tty_ldisc_reinit(tty, N_TTY);
+ if (tty->ldisc) {
+ if (reinit) {
+ if (tty_ldisc_reinit(tty, tty->termios.c_line) < 0)
+ tty_ldisc_reinit(tty, N_TTY);
+ } else
+ tty_ldisc_kill(tty);
  }
  tty_ldisc_unlock(tty);
- if (reset)
- tty_reset_termios(tty);
-
- tty_ldisc_debug(tty, "%p: re-opened\n", tty->ldisc);
 }
 
 /**
diff --git a/include/linux/tty.h b/include/linux/tty.h
index c35f713..0964e9d 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -498,7 +498,8 @@ extern int tty_set_termios(struct tty_struct *tty, struct ktermios *kt);
 extern struct tty_ldisc *tty_ldisc_ref(struct tty_struct *);
 extern void tty_ldisc_deref(struct tty_ldisc *);
 extern struct tty_ldisc *tty_ldisc_ref_wait(struct tty_struct *);
-extern void tty_ldisc_hangup(struct tty_struct *tty);
+extern void tty_ldisc_hangup(struct tty_struct *tty, bool reset);
+extern int tty_ldisc_reinit(struct tty_struct *tty, int disc);
 extern const struct file_operations tty_ldiscs_proc_fops;
 
 extern void tty_wakeup(struct tty_struct *tty);
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

ACK: [Xenial][PATCH 0/7] Fix rare tty_ldisc_reinit panic

Marcelo Henrique Cerri
In reply to this post by Kamal Mostafa-2
Clean cherry-pick with reasonable dependencies picked. It seems fine to me.

Acked-by: Marcelo Henrique Cerri <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (484 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

ACK: Re: [Xenial][PATCH 0/7] Fix rare tty_ldisc_reinit panic

Benjamin M. Romer
In reply to this post by Kamal Mostafa-2
Acked-by: Benjamin M Romer <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

APPLIED: [Xenial][PATCH 0/7] Fix rare tty_ldisc_reinit panic

Kleber Souza
In reply to this post by Kamal Mostafa-2
Applied on xenial/master-next branch. Thanks.

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Loading...