Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752779AbdGHIiI (ORCPT ); Sat, 8 Jul 2017 04:38:08 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:39884 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751927AbdGHIiH (ORCPT ); Sat, 8 Jul 2017 04:38:07 -0400 Date: Sat, 8 Jul 2017 10:38:03 +0200 From: Greg Kroah-Hartman To: Okash Khawaja Cc: Jiri Slaby , Samuel Thibault , Alan Cox , linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org, Kirk Reiser , Chris Brannon , speakup@linux-speakup.org Subject: Re: tty contention resulting from tty_open_by_device export Message-ID: <20170708083803.GA23080@kroah.com> References: <20170707202841.GA4177@sanghar> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170707202841.GA4177@sanghar> User-Agent: Mutt/1.8.3 (2017-05-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5397 Lines: 160 On Fri, Jul 07, 2017 at 09:28:41PM +0100, Okash Khawaja wrote: > Hi, > > The commit 12e84c71b7d4ee (tty: export tty_open_by_driver) exports > tty_open_by_device to allow tty to be opened from inside kernel which > works fine except that it doesn't handle contention with user space or > another kernel-space open of the same tty. For example, opening a tty > from user space while it is kernel opened results in failure and a > kernel log message about mismatch between tty->count and tty's file open > count. > > I suggest we make kernel access to tty exclusive so that if a user > process or kernel opens a kernel opened tty, it gets -EBUSY. Below is > a patch which does this by adding TTY_KOPENED flag to tty->flags. When > this flag is set, tty_open_by_driver returns -EBUSY. Instead of > overlaoding tty_open_by_driver for both kernel and user space, this > patch creates a separate function tty_kopen which closely follows > tty_open_by_driver. > > I am far from an expert on tty and this patch might contain the wrong > approach. But it should convey what I mean. > > Thanks, > Okash > > --- > drivers/staging/speakup/spk_ttyio.c | 2 - > drivers/tty/tty_io.c | 56 ++++++++++++++++++++++++++++++++++-- > include/linux/tty.h | 7 +--- > 3 files changed, 58 insertions(+), 7 deletions(-) > > --- a/drivers/staging/speakup/spk_ttyio.c > +++ b/drivers/staging/speakup/spk_ttyio.c > @@ -164,7 +164,7 @@ static int spk_ttyio_initialise_ldisc(st > if (ret) > return ret; > > - tty = tty_open_by_driver(dev, NULL, NULL); > + tty = tty_kopen(dev); > if (IS_ERR(tty)) > return PTR_ERR(tty); > > --- a/drivers/tty/tty_io.c > +++ b/drivers/tty/tty_io.c > @@ -1786,6 +1786,53 @@ static struct tty_driver *tty_lookup_dri > } > > /** > + * tty_kopen - open a tty device for kernel > + * @device: dev_t of device to open If nothing else, this api call is much simpler overall, as your use above shows :) > + * > + * Opens tty exclusively for kernel. Performs the driver lookup, > + * makes sure it's not already opened and performs the first-time > + * tty initialization. > + * > + * Returns the locked initialized &tty_struct > + * > + * Claims the global tty_mutex to serialize: > + * - concurrent first-time tty initialization > + * - concurrent tty driver removal w/ lookup > + * - concurrent tty removal from driver table > + */ > +struct tty_struct *tty_kopen(dev_t device) > +{ > + struct tty_struct *tty; > + struct tty_driver *driver = NULL; > + int index = -1; > + > + mutex_lock(&tty_mutex); > + driver = tty_lookup_driver(device, NULL, &index); > + if (IS_ERR(driver)) { > + mutex_unlock(&tty_mutex); > + return ERR_CAST(driver); > + } > + > + /* check whether we're reopening an existing tty */ > + tty = tty_driver_lookup_tty(driver, NULL, index); > + if (IS_ERR(tty)) > + goto out; > + > + if (tty) { > + tty_kref_put(tty); /* drop kref from tty_driver_lookup_tty() */ Put the comment above the line? > + tty = ERR_PTR(-EBUSY); > + } else { /* Returns with the tty_lock held for now */ I don't understand this comment. > + tty = tty_init_dev(driver, index); > + set_bit(TTY_KOPENED, &tty->flags); > + } > +out: > + mutex_unlock(&tty_mutex); > + tty_driver_kref_put(driver); > + return tty; > +} > +EXPORT_SYMBOL(tty_kopen); EXPORT_SYMBOL_GPL()? :) > + > +/** > * tty_open_by_driver - open a tty device > * @device: dev_t of device to open > * @inode: inode of device file > @@ -1801,7 +1848,7 @@ static struct tty_driver *tty_lookup_dri > * - concurrent tty driver removal w/ lookup > * - concurrent tty removal from driver table > */ > -struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode, > +static struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode, > struct file *filp) > { > struct tty_struct *tty; > @@ -1824,6 +1871,12 @@ struct tty_struct *tty_open_by_driver(de > } > > if (tty) { > + if (test_bit(TTY_KOPENED, &tty->flags)) { > + tty_kref_put(tty); > + mutex_unlock(&tty_mutex); > + tty = ERR_PTR(-EBUSY); > + goto out; > + } > mutex_unlock(&tty_mutex); > retval = tty_lock_interruptible(tty); > tty_kref_put(tty); /* drop kref from tty_driver_lookup_tty() */ > @@ -1846,7 +1899,6 @@ out: > tty_driver_kref_put(driver); > return tty; > } > -EXPORT_SYMBOL_GPL(tty_open_by_driver); > > /** > * tty_open - open a tty device > --- a/include/linux/tty.h > +++ b/include/linux/tty.h > @@ -362,6 +362,7 @@ struct tty_file_private { > #define TTY_NO_WRITE_SPLIT 17 /* Preserve write boundaries to driver */ > #define TTY_HUPPED 18 /* Post driver->hangup() */ > #define TTY_LDISC_HALTED 22 /* Line discipline is halted */ > +#define TTY_KOPENED 23 /* TTY exclusively opened by kernel */ Minor nit, please use tabs here. Overall, the idea looks sane to me. Keeping userspace from opening a tty that the kernel has opened internally makes sense, hopefully userspace doesn't get too confused when that happens. I don't think we normally return -EBUSY from an open call, have you seen what happens with apps when you do this (like minicom?) thanks, greg k-h