[ +cc Paul Mackerras, linux-ppp, netdev ] I neglected to cc the proper folks. Sorry about that.
Regards, Peter Hurley On Mon, 2013-03-11 at 16:44 -0400, Peter Hurley wrote: > tty_perform_flush() can deadlock when called while holding > a line discipline reference. By definition, all ldisc drivers > hold a ldisc reference, so calls originating from ldisc drivers > must not block for a ldisc reference. > > The deadlock can occur when: > CPU 0 | CPU 1 > | > tty_ldisc_ref(tty) | > .... | <line discipline halted> > tty_ldisc_ref_wait(tty) | > | > > CPU 0 cannot progess because it cannot obtain an ldisc reference > with the line discipline has been halted (thus no new references > are granted). > CPU 1 cannot progress because an outstanding ldisc reference > has not been released. > > An in-tree call-tree audit of tty_perform_flush() [1] shows 5 > ldisc drivers calling tty_perform_flush() indirectly via > n_tty_ioctl_helper() and 2 ldisc drivers calling directly. > A single tty driver safely uses the function. > > [1] > Recursive usage: > > /* These functions are line discipline ioctls and thus > * recursive wrt line discipline references */ > > tty_perform_flush() - ./drivers/tty/tty_ioctl.c > n_tty_ioctl_helper() > hci_uart_tty_ioctl(default) - drivers/bluetooth/hci_ldisc.c (N_HCI) > n_hdlc_tty_ioctl(default) - drivers/tty/n_hdlc.c (N_HDLC) > gsmld_ioctl(default) - drivers/tty/n_gsm.c (N_GSM0710) > n_tty_ioctl(default) - drivers/tty/n_tty.c (N_TTY) > gigaset_tty_ioctl(default) - drivers/isdn/gigaset/ser-gigaset.c > (N_GIGASET_M101) > ppp_synctty_ioctl(TCFLSH) - drivers/net/ppp/pps_synctty.c > ppp_asynctty_ioctl(TCFLSH) - drivers/net/ppp/ppp_async.c > > Non-recursive use: > > tty_perform_flush() - drivers/tty/tty_ioctl.c > ipw_ioctl(TCFLSH) - drivers/tty/ipwireless/tty.c > /* This function is a tty i/o ioctl method, which > * is invoked by tty_ioctl() */ > > Signed-off-by: Peter Hurley <[email protected]> > --- > drivers/net/ppp/ppp_async.c | 2 +- > drivers/net/ppp/ppp_synctty.c | 2 +- > drivers/tty/tty_ioctl.c | 28 +++++++++++++++++++--------- > 3 files changed, 21 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/ppp/ppp_async.c b/drivers/net/ppp/ppp_async.c > index a031f6b..9c889e0 100644 > --- a/drivers/net/ppp/ppp_async.c > +++ b/drivers/net/ppp/ppp_async.c > @@ -314,7 +314,7 @@ ppp_asynctty_ioctl(struct tty_struct *tty, struct file > *file, > /* flush our buffers and the serial port's buffer */ > if (arg == TCIOFLUSH || arg == TCOFLUSH) > ppp_async_flush_output(ap); > - err = tty_perform_flush(tty, arg); > + err = n_tty_ioctl_helper(tty, file, cmd, arg); > break; > > case FIONREAD: > diff --git a/drivers/net/ppp/ppp_synctty.c b/drivers/net/ppp/ppp_synctty.c > index 1a12033..bdf3b13 100644 > --- a/drivers/net/ppp/ppp_synctty.c > +++ b/drivers/net/ppp/ppp_synctty.c > @@ -355,7 +355,7 @@ ppp_synctty_ioctl(struct tty_struct *tty, struct file > *file, > /* flush our buffers and the serial port's buffer */ > if (arg == TCIOFLUSH || arg == TCOFLUSH) > ppp_sync_flush_output(ap); > - err = tty_perform_flush(tty, arg); > + err = n_tty_ioctl_helper(tty, file, cmd, arg); > break; > > case FIONREAD: > diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c > index d58b92c..935b032 100644 > --- a/drivers/tty/tty_ioctl.c > +++ b/drivers/tty/tty_ioctl.c > @@ -1086,14 +1086,12 @@ int tty_mode_ioctl(struct tty_struct *tty, struct > file *file, > } > EXPORT_SYMBOL_GPL(tty_mode_ioctl); > > -int tty_perform_flush(struct tty_struct *tty, unsigned long arg) > + > +/* Caller guarantees ldisc reference is held */ > +static int __tty_perform_flush(struct tty_struct *tty, unsigned long arg) > { > - struct tty_ldisc *ld; > - int retval = tty_check_change(tty); > - if (retval) > - return retval; > + struct tty_ldisc *ld = tty->ldisc; > > - ld = tty_ldisc_ref_wait(tty); > switch (arg) { > case TCIFLUSH: > if (ld && ld->ops->flush_buffer) { > @@ -1111,12 +1109,24 @@ int tty_perform_flush(struct tty_struct *tty, > unsigned long arg) > tty_driver_flush_buffer(tty); > break; > default: > - tty_ldisc_deref(ld); > return -EINVAL; > } > - tty_ldisc_deref(ld); > return 0; > } > + > +int tty_perform_flush(struct tty_struct *tty, unsigned long arg) > +{ > + struct tty_ldisc *ld; > + int retval = tty_check_change(tty); > + if (retval) > + return retval; > + > + ld = tty_ldisc_ref_wait(tty); > + retval = __tty_perform_flush(tty, arg); > + if (ld) > + tty_ldisc_deref(ld); > + return retval; > +} > EXPORT_SYMBOL_GPL(tty_perform_flush); > > int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file, > @@ -1155,7 +1165,7 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct > file *file, > } > return 0; > case TCFLSH: > - return tty_perform_flush(tty, arg); > + return __tty_perform_flush(tty, arg); > default: > /* Try the mode commands */ > return tty_mode_ioctl(tty, file, cmd, arg); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
