lkml.org 
[lkml]   [2010]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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]
\
 
 \ /
  Last update: 2010-11-24 23:41    [W:0.137 / U:4.132 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site