From: Peter Hurley <peter@hurleysoftware.com>
To: Jiri Slaby <jslaby@suse.cz>, Jiri Slaby <jirislaby@gmail.com>,
alan@linux.intel.com
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
Dave Jones <davej@redhat.com>,
Sasha Levin <levinsasha928@gmail.com>
Subject: Re: flush_to_ldisc accesses tty after free (was: [PATCH 21/21] TTY: move tty buffers to tty_port)
Date: Sat, 01 Dec 2012 15:06:23 -0500 [thread overview]
Message-ID: <1354392383.2531.118.camel@thor> (raw)
In-Reply-To: <1354373995.2531.48.camel@thor>
On Sat, 2012-12-01 at 09:59 -0500, Peter Hurley wrote:
....
> From instrumenting the tty_release() path, it's clear that tty_buffer
> work is still scheduled even after tty_release_ldisc() has run. For
> example, with this patch I get the warning below it.
>
> [Further analysis to follow in subsequent mail...]
[ Please note: this analysis only refers to the pty driver. The
situation with hardware drivers has further complications.]
Firstly, this problem predates Jiri's changes; only because he was
cautious by checking the lifetime of the itty in flush_to_ldisc(), did
he uncover this existing problem.
One example of how it is possible for buffer work to be scheduled even
after tty_release_ldisc() stems from how tty_ldisc_halt() works (or
rather doesn't). (I've snipped out the relevant code from tty_ldisc.c
for annotation below.)
tty_ldisc_halt() has only 2 callers; tty_release_ldisc() and
tty_set_ldisc(). A 3rd code site -- tty_ldisc_hangup() -- has similar
logic.
The idea behind tty_ldisc_halt() is to prevent __future__ use of this
ldisc (since users are required to acquire an ldisc reference via
tty_ldisc_try() -- also below). Annotations are mine.
----------------------------------
static struct tty_ldisc *tty_ldisc_try(struct tty_struct *tty)
{
unsigned long flags;
struct tty_ldisc *ld;
/* this spin_lock is irrelevant to this discussion */
spin_lock_irqsave(&tty_ldisc_lock, flags);
ld = NULL;
/*
* Atomically test if __new__ ldisc references are
* allowed. Please note, there can be any number of
* existing users (ie., outstanding references).
*/
if (test_bit(TTY_LDISC, &tty->flags))
ld = get_ldisc(tty->ldisc);
spin_unlock_irqrestore(&tty_ldisc_lock, flags);
return ld;
}
static int tty_ldisc_halt(struct tty_struct *tty)
{
/* Prevent any __new__ ldisc references from being acquired. */
clear_bit(TTY_LDISC, &tty->flags);
/* Since __existing__ ldisc references can still schedule new
* buffer work (via tty_flip_buffer_push()), the cancellation
* below is pointless. The instant that cancellation completes
* an existing ldisc user can schedule new work.
*
* At a minimum, we must wait for all ldisc references **here**
* rather than **after** cancelling the work.
*/
return cancel_work_sync(&tty->buf.work);
}
**** A special note about locking *****
Locking around tty_ldisc_halt() ...
- does not prevent existing ldisc users from continuing to use the ldisc
- is unnecessary for the 3 'callers' because all 3 are trying to
accomplish the same goal by the same means
- can deadlock. Reference: https://lkml.org/lkml/2012/11/21/267
Regards,
Peter Hurley
next prev parent reply other threads:[~2012-12-01 20:06 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-10-18 20:26 [PATCH 00/21] TTY buffer in tty_port and other stuff Jiri Slaby
2012-10-18 20:26 ` [PATCH 01/21] TTY: devpts, don't care about TTY in devpts_get_tty Jiri Slaby
2012-10-18 20:26 ` [PATCH 02/21] TTY: devpts, return created inode from devpts_pty_new Jiri Slaby
2012-10-18 20:26 ` [PATCH 03/21] TTY: devpts, do not set driver_data Jiri Slaby
2012-10-18 20:26 ` [PATCH 04/21] TTY: devpts, document devpts inode operations Jiri Slaby
2012-10-18 20:26 ` [PATCH 05/21] TTY: move devpts kill to pty Jiri Slaby
2012-10-18 20:26 ` [PATCH 06/21] TTY: vt, fix paste_selection ldisc handling Jiri Slaby
2012-10-18 20:26 ` [PATCH 07/21] TTY: ldisc, wait for idle ldisc in release Jiri Slaby
2012-10-18 20:26 ` [PATCH 08/21] TTY: hci_ldisc, remove invalid check in open Jiri Slaby
2012-10-18 20:47 ` Marcel Holtmann
2012-10-18 20:26 ` [PATCH 09/21] TTY: n_tty, simplify read_buf+echo_buf allocation Jiri Slaby
2012-10-18 20:26 ` [PATCH 10/21] TTY: n_tty, remove bogus checks Jiri Slaby
2012-10-18 20:26 ` [PATCH 11/21] TTY: audit, stop accessing tty->icount Jiri Slaby
2012-10-18 20:26 ` [PATCH 12/21] TTY: n_tty, add ldisc data to n_tty Jiri Slaby
2012-10-18 20:26 ` [PATCH 13/21] TTY: move ldisc data from tty_struct: simple members Jiri Slaby
2012-10-18 20:26 ` [PATCH 14/21] TTY: move ldisc data from tty_struct: bitmaps Jiri Slaby
2012-10-18 20:26 ` [PATCH 15/21] TTY: move ldisc data from tty_struct: read_* and echo_* and canon_* stuff Jiri Slaby
2012-10-18 20:26 ` [PATCH 16/21] TTY: move ldisc data from tty_struct: locks Jiri Slaby
2012-10-18 20:26 ` [PATCH 17/21] TTY: n_tty, propagate n_tty_data Jiri Slaby
2012-10-18 20:26 ` [PATCH 18/21] TTY: move TTY_FLUSH* flags to tty_port Jiri Slaby
2012-10-18 20:26 ` [PATCH 19/21] TTY: tty_buffer, cache pointer to tty->buf Jiri Slaby
2012-10-18 20:26 ` [PATCH 20/21] TTY: add port -> tty link Jiri Slaby
2012-10-18 20:26 ` [PATCH 21/21] TTY: move tty buffers to tty_port Jiri Slaby
2012-10-25 18:02 ` Sasha Levin
2012-10-25 18:08 ` Greg KH
2012-10-31 12:53 ` Jiri Slaby
2012-10-31 15:30 ` Sasha Levin
2012-10-31 15:32 ` Jiri Slaby
2012-10-31 15:59 ` Sasha Levin
2012-11-02 15:51 ` Jiri Slaby
2012-11-02 16:07 ` Sasha Levin
2012-11-02 16:18 ` Jiri Slaby
2012-11-02 16:23 ` Sasha Levin
2012-11-03 2:03 ` Sasha Levin
2012-11-03 15:55 ` Jiri Slaby
2012-11-03 23:06 ` Sasha Levin
2012-11-04 0:53 ` Sasha Levin
2012-11-27 19:57 ` Peter Hurley
2012-11-30 23:52 ` Sasha Levin
2012-12-01 14:59 ` flush_to_ldisc accesses tty after free (was: [PATCH 21/21] TTY: move tty buffers to tty_port) Peter Hurley
2012-12-01 20:06 ` Peter Hurley [this message]
2012-12-02 19:57 ` Peter Hurley
2012-12-04 19:21 ` Ilya Zykov
2012-10-31 20:10 ` [PATCH 21/21] TTY: move tty buffers to tty_port Sasha Levin
2012-10-18 21:12 ` [PATCH 00/21] TTY buffer in tty_port and other stuff Greg KH
2012-10-22 14:57 ` Alan Cox
2012-10-22 23:59 ` Greg KH
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=1354392383.2531.118.camel@thor \
--to=peter@hurleysoftware.com \
--cc=alan@linux.intel.com \
--cc=davej@redhat.com \
--cc=gregkh@linuxfoundation.org \
--cc=jirislaby@gmail.com \
--cc=jslaby@suse.cz \
--cc=levinsasha928@gmail.com \
--cc=linux-kernel@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).