Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 25 Nov 2010 01:38:12 +0300 | From | Alexander Gordeev <> | Subject | Re: [PATCHv5 05/17] tty: don't allow ldisc dcd_change() after ldisc halt |
| |
В Wed, 24 Nov 2010 16:43:29 +0000 Alan Cox <alan@lxorguk.ukuu.org.uk> пишет:
> On Wed, 24 Nov 2010 19:15:43 +0300 > Alexander Gordeev <lasaine@lvk.cs.msu.su> wrote: > > > There was a possibility that uart_handle_dcd_change() could obtain a > > reference to ldisc while running in parallel with tty_set_ldisc() on > > different CPU but call dcd_change() operation after > > tty_ldisc_close() which is incorrect. > > How can this occur ? > > > > + spin_lock_irqsave(&tty->dcd_change_lock, flags); > > + > > + ld = tty_ldisc_ref(tty); > > What is the expecting lock ordering rule here ? > > > > I don't see why this patch is needed. You've got an ldisc ref from > tty_ldisc_ref, until you drop that ldisc ref you are fine. If for some > reason that is not the case then there is a bug in the ldisc code.
Yes, indeed, it's a bug. Please consider the following example:
CPU1 CPU2 ========================================================= uart_handle_dcd_change() { tty_set_ldisc() { ld = tty_ldisc_ref(...) ... ... tty_ldisc_halt(...) ... ... ... tty_ldisc_close(...) if (ld && ld->ops->dcd_change) ... ld->ops->dcd_change(...); ... ... tty_ldisc_open(...) } }
I think that semantically ldisc ops should never be called before open or after close. This situation is possible because tty_ldisc_halt() only ensures that no more references are taken. This is ok for everything except dcd_change() because it cleans up workqueue and doesn't accept any more data. dcd_change() is a different story because it doesn't use workqueues.
I think tty code is exactly the right place to fix this bug; this is what my patch is for.
-- Alexander [unhandled content-type:application/pgp-signature] | ![\](/images/icornerr.gif) |