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

[Alexander Gordeev]

Ð 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