Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752298AbdGILvo (ORCPT ); Sun, 9 Jul 2017 07:51:44 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:41452 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752039AbdGILvn (ORCPT ); Sun, 9 Jul 2017 07:51:43 -0400 Date: Sun, 9 Jul 2017 13:51:39 +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 , speakup@linux-speakup.org, Chris Brannon Subject: Re: [patch 1/3] tty: resolve tty contention between kernel and user space Message-ID: <20170709115139.GB14769@kroah.com> References: <20170708083803.GA23080@kroah.com> <20170709114153.157783481@gmail.com> <20170709114426.618570903@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170709114426.618570903@gmail.com> 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: 4669 Lines: 129 On Sun, Jul 09, 2017 at 12:41:54PM +0100, Okash Khawaja wrote: > 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. > > This patch makes kernel access to tty exclusive, so that if a user > process or kernel opens a kernel opened tty, it gets -EBUSY. It 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. > > Returning -EBUSY on tty open is a change in the interface. I have > tested this with minicom, picocom and commands like "echo foo > > /dev/ttyS0". They all correctly report "Device or resource busy" when > the tty is already kernel opened. > > Signed-off-by: Okash Khawaja > > --- > drivers/tty/tty_io.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/tty.h | 4 +++ > 2 files changed, 58 insertions(+) > > --- a/drivers/tty/tty_io.c > +++ b/drivers/tty/tty_io.c > @@ -1786,6 +1786,54 @@ static struct tty_driver *tty_lookup_dri > } > > /** > + * tty_kopen - open a tty device for kernel > + * @device: dev_t of device to open > + * > + * 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) { > + /* drop kref from tty_driver_lookup_tty() */ > + tty_kref_put(tty); > + tty = ERR_PTR(-EBUSY); > + } else { /* tty_init_dev returns tty with the tty_lock held */ > + 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_GPL(tty_kopen); > + > +/** > * tty_open_by_driver - open a tty device > * @device: dev_t of device to open > * @inode: inode of device file > @@ -1824,6 +1872,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() */ > --- 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 */ > > /* Values for tty->flow_change */ > #define TTY_THROTTLE_SAFE 1 > @@ -401,6 +402,7 @@ extern int __init tty_init(void); > extern const char *tty_name(const struct tty_struct *tty); > extern struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode, > struct file *filp); > +extern struct tty_struct *tty_kopen(dev_t device); > extern int tty_dev_name_to_number(const char *name, dev_t *number); > #else > static inline void tty_kref_put(struct tty_struct *tty) > @@ -425,6 +427,8 @@ static inline const char *tty_name(const > static inline struct tty_struct *tty_open_by_driver(dev_t device, > struct inode *inode, struct file *filp) > { return NULL; } > +static inline struct tty_struct *tty_kopen(dev_t device) > +{ return NULL; } Like I said in response to patch 2, maybe this should return an error? thanks, greg k-h