Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752399AbdGGU2r (ORCPT ); Fri, 7 Jul 2017 16:28:47 -0400 Received: from mail-wr0-f172.google.com ([209.85.128.172]:33681 "EHLO mail-wr0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750941AbdGGU2q (ORCPT ); Fri, 7 Jul 2017 16:28:46 -0400 Date: Fri, 7 Jul 2017 21:28:41 +0100 From: Okash Khawaja To: Greg Kroah-Hartman , Jiri Slaby , Samuel Thibault , Alan Cox , linux-kernel@vger.kernel.org Cc: William Hubbs , Chris Brannon , Kirk Reiser , speakup@linux-speakup.org, devel@driverdev.osuosl.org Subject: tty contention resulting from tty_open_by_device export Message-ID: <20170707202841.GA4177@sanghar> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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: 5486 Lines: 157 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 + * + * 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() */ + tty = ERR_PTR(-EBUSY); + } else { /* Returns with the tty_lock held for now */ + 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); + +/** * 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 */ /* Values for tty->flow_change */ #define TTY_THROTTLE_SAFE 1 @@ -399,8 +400,7 @@ extern struct tty_struct *get_current_tt /* tty_io.c */ 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) @@ -422,8 +422,7 @@ static inline int __init tty_init(void) { return 0; } static inline const char *tty_name(const struct tty_struct *tty) { return "(none)"; } -static inline struct tty_struct *tty_open_by_driver(dev_t device, - struct inode *inode, struct file *filp) +static inline struct tty_struct *tty_kopen(dev_t device) { return NULL; } static inline int tty_dev_name_to_number(const char *name, dev_t *number) { return -ENOTSUPP; }