Re: [PATCHv5 05/17] tty: don't allow ldisc dcd_change() after ldisc halt

Alexander Gordeev
2010-11-24T22:38:35Z

[-- Attachment #1: Type: text/plain, Size: 1798 bytes --]

В 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 490 bytes --]