All of lore.kernel.org
 help / color / mirror / Atom feed
From: Peter Hurley <peter@hurleysoftware.com>
To: Alan Cox <alan@linux.intel.com>, Jiri Slaby <jslaby@suse.cz>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
	Peter Hurley <peter@hurleysoftware.com>
Subject: [PATCH -next 0/9] tty: Fix buffer work access-after-free
Date: Tue,  4 Dec 2012 02:07:36 -0500	[thread overview]
Message-ID: <1354604865-10278-1-git-send-email-peter@hurleysoftware.com> (raw)

This patch series addresses the causes of flush_to_ldisc accessing
the tty after freeing.

The most common cause stems from the n_tty_close() path spuriously
scheduling buffer work, when the ldisc has already been halted.
This is fixed in 'tty: Don't reschedule buffer work while closing'

The other causes have a central theme: incorrect order-of-operations
when halting a line discipline. In general, to prevent buffer work
from being scheduled requires:
  1. Disallowing further ldisc references
  2. Waiting for all existing ldisc references to be released
  3. Cancelling existing buffer work
If the wait takes place _after_ cancellation, then new work
can be scheduled by existing holder(s) of ldisc reference(s). That's
bad.

Halting the line discipline is performed when,
 - hanging up the tty (tty_ldisc_hangup())
 - TIOCSETD ioctl (tty_set_ldisc())
 - finally closing the tty (pair) (tty_ldisc_release())

Concurrent halts are governed by the following requirements:
1. tty_ldisc_release is not concurrent with the other two and so
   does not need lock or state protection during the ldiscs halt.
2. Accesses through tty->ldisc must be protected by the ldisc_mutex.
   The wait operation checks the user count (ldisc references)
   in tty->ldisc->users.
3. tty_set_ldisc() reschedules buffer work that was pending when
   the ldiscs were halted. That must be an atomic operation in
   conjunction with re-enabling the ldisc -- which necessitates
   locking concurrent halts (tty_ldisc_release is exempt here)
4. The legacy mutex cannot be held while waiting for ldisc
   reference(s) release -or- for cancelling buffer work.
5. Because of #4, the legacy mutex must be dropped prior to or
   during the halt. Which means reacquiring after the halt. But
   to preserve lock order the ldisc_mutex must be dropped and
   reacquired after reacquiring the legacy mutex.
6. Because of #5, the ldisc state may have changed while the
   ldisc mutex was dropped.

Note: this series does not include the lock correction initially
reported on by Sebastian Andrzej Siewior <bigeasy@linutronix.de> here
https://lkml.org/lkml/2012/11/21/267 . I commented on the latest
version here https://lkml.org/lkml/2012/12/3/362

This series also does not include Jiri's debug patch here
https://lkml.org/lkml/2012/11/2/278 for identifying which itty cleanup
path is used. I think it may be helpful to include this in -next.

Peter Hurley (9):
  tty: WARN if buffer work racing with tty free
  tty: Add diagnostic for halted line discipline
  tty: Don't reschedule buffer work while closing
  tty: Refactor wait for ldisc refs out of tty_ldisc_hangup()
  tty: Remove unnecessary re-test of ldisc ref count
  tty: Fix ldisc halt sequence on hangup
  tty: Strengthen no-subsequent-use guarantee of tty_ldisc_halt()
  tty: Remove unnecessary buffer work flush
  tty: Halt both ldiscs concurrently

 drivers/tty/n_tty.c      |   8 ++-
 drivers/tty/tty_buffer.c |   3 +
 drivers/tty/tty_io.c     |   2 +
 drivers/tty/tty_ldisc.c  | 171 +++++++++++++++++++++++++++++------------------
 include/linux/tty.h      |   1 +
 5 files changed, 119 insertions(+), 66 deletions(-)

-- 
1.8.0


             reply	other threads:[~2012-12-04  7:08 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-12-04  7:07 Peter Hurley [this message]
2012-12-04  7:07 ` [PATCH -next 1/9] tty: WARN if buffer work racing with tty free Peter Hurley
2012-12-04  7:07 ` [PATCH -next 2/9] tty: Add diagnostic for halted line discipline Peter Hurley
2012-12-04  7:07 ` [PATCH -next 3/9] tty: Don't reschedule buffer work while closing Peter Hurley
2012-12-04  7:07 ` [PATCH -next 4/9] tty: Refactor wait for ldisc refs out of tty_ldisc_hangup() Peter Hurley
2012-12-04  7:07 ` [PATCH -next 5/9] tty: Remove unnecessary re-test of ldisc ref count Peter Hurley
2012-12-04  7:07 ` [PATCH -next 6/9] tty: Fix ldisc halt sequence on hangup Peter Hurley
2012-12-04  7:07 ` [PATCH -next 7/9] tty: Strengthen no-subsequent-use guarantee of tty_ldisc_halt() Peter Hurley
2012-12-04  7:07 ` [PATCH -next 8/9] tty: Remove unnecessary buffer work flush Peter Hurley
2012-12-04  7:07 ` [PATCH -next 9/9] tty: Halt both ldiscs concurrently Peter Hurley
2012-12-04  7:40 ` [PATCH -next 0/9] tty: Fix buffer work access-after-free Ilya Zykov
2012-12-04  8:54 ` Alan Cox
2012-12-04 13:58   ` Peter Hurley
2012-12-04 14:30     ` Alan Cox
2012-12-04  9:38 ` Jiri Slaby
2012-12-07  0:57 ` Peter Hurley
2012-12-10 19:00 ` Ilya Zykov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1354604865-10278-1-git-send-email-peter@hurleysoftware.com \
    --to=peter@hurleysoftware.com \
    --cc=alan@linux.intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jslaby@suse.cz \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.