Re: [PATCHv5 05/17] tty: don't allow ldisc dcd_change() after ldischalt
From: Alexander Gordeev
Date: Wed Nov 24 2010 - 17:38:39 EST
Ð Wed, 24 Nov 2010 16:43:29 +0000
Alan Cox <alan@xxxxxxxxxxxxxxxxxxx> ÐÐÑÐÑ:
> On Wed, 24 Nov 2010 19:15:43 +0300
> Alexander Gordeev <lasaine@xxxxxxxxxxxxx> 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:
signature.asc
Description: PGP signature