Messages in this thread | | | Subject | Re: flush_to_ldisc accesses tty after free (was: [PATCH 21/21] TTY: move tty buffers to tty_port) | From | Peter Hurley <> | Date | Sat, 01 Dec 2012 15:06:23 -0500 |
| |
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
| |