Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1305415 > unrolled thread

[PATCH v2 17/19] tty: Destroy ldisc instance on hangup

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 17/19] tty: Destroy ldisc instance on hangup Peter Hurley <peter@hurleysoftware.com> - 2016-01-10 05:50 +0100
    Re: [PATCH v2 17/19] tty: Destroy ldisc instance on hangup kbuild test robot <lkp@intel.com> - 2016-01-10 07:30 +0100
    [PATCH] tty: fix badzero.cocci warnings kbuild test robot <lkp@intel.com> - 2016-01-10 07:30 +0100
      Re: [PATCH] tty: fix badzero.cocci warnings Peter Hurley <peter@hurleysoftware.com> - 2016-01-10 08:10 +0100

#1305415 — [PATCH v2 17/19] tty: Destroy ldisc instance on hangup

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 <peter@hurleysoftware.com>
---
 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 f6f559c..fea8318 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -728,7 +728,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 != 0);
 
 	spin_lock_irq(&tty->ctrl_lock);
 	clear_bit(TTY_THROTTLED, &tty->flags);
@@ -753,10 +753,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);
@@ -1480,7 +1479,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 527fc5b..199e4b4 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -257,6 +257,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
@@ -644,14 +647,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;
@@ -695,11 +699,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);
 
@@ -727,25 +729,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 ee73220..56d1133 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -493,7 +493,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.0

[toc] | [next] | [standalone]

#1305479

Hi Peter,

[auto build test WARNING on tty/tty-testing]
[cannot apply to v4.4-rc8 next-20160108]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Peter-Hurley/Fix-driver-crashes-on-hangup/20160110-124954
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing


coccinelle warnings: (new ones prefixed by >>)

>> drivers/tty/tty_io.c:731:36-37: WARNING comparing pointer to 0

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1305480 — [PATCH] tty: fix badzero.cocci warnings

drivers/tty/tty_io.c:731:36-37: WARNING comparing pointer to 0

 Compare pointer-typed values to NULL rather than 0

Semantic patch information:
 This makes an effort to choose between !x and x == NULL.  !x is used
 if it has previously been used with the function used to initialize x.
 This relies on type information.  More type information can be obtained
 using the option -all_includes and the option -I to specify an
 include path.

Generated by: scripts/coccinelle/null/badzero.cocci

CC: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 tty_io.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -728,7 +728,7 @@ static void __tty_hangup(struct tty_stru
 	while (refs--)
 		tty_kref_put(tty);
 
-	tty_ldisc_hangup(tty, cons_filp != 0);
+	tty_ldisc_hangup(tty, cons_filp != NULL);
 
 	spin_lock_irq(&tty->ctrl_lock);
 	clear_bit(TTY_THROTTLED, &tty->flags);

[toc] | [prev] | [next] | [standalone]

#1305498 — Re: [PATCH] tty: fix badzero.cocci warnings

On 01/09/2016 10:24 PM, kbuild test robot wrote:
> drivers/tty/tty_io.c:731:36-37: WARNING comparing pointer to 0
> 
>  Compare pointer-typed values to NULL rather than 0
> 
> Semantic patch information:
>  This makes an effort to choose between !x and x == NULL.  !x is used
>  if it has previously been used with the function used to initialize x.
>  This relies on type information.  More type information can be obtained
>  using the option -all_includes and the option -I to specify an
>  include path.
> 
> Generated by: scripts/coccinelle/null/badzero.cocci

Not sure what possessed me to write 'cons_filp != 0'. Not my style, for sure.
Thanks.

Regards,
Peter Hurley

> CC: Peter Hurley <peter@hurleysoftware.com>
> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
> ---
> 
>  tty_io.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -728,7 +728,7 @@ static void __tty_hangup(struct tty_stru
>  	while (refs--)
>  		tty_kref_put(tty);
>  
> -	tty_ldisc_hangup(tty, cons_filp != 0);
> +	tty_ldisc_hangup(tty, cons_filp != NULL);
>  
>  	spin_lock_irq(&tty->ctrl_lock);
>  	clear_bit(TTY_THROTTLED, &tty->flags);
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web