Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751574AbdGRLaA (ORCPT ); Tue, 18 Jul 2017 07:30:00 -0400 Received: from mail-wr0-f195.google.com ([209.85.128.195]:35857 "EHLO mail-wr0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751423AbdGRL36 (ORCPT ); Tue, 18 Jul 2017 07:29:58 -0400 Date: Tue, 18 Jul 2017 12:29:52 +0100 From: Okash Khawaja To: Alan Cox Cc: Greg Kroah-Hartman , Jiri Slaby , Samuel Thibault , "linux-kernel@vger.kernel.org" , William Hubbs , Chris Brannon , Kirk Reiser , "speakup@linux-speakup.org" , "devel@driverdev.osuosl.org" Subject: Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device export Message-ID: <20170718112952.GA564@sanghar> References: <20170708083803.GA23080@kroah.com> <20170709114153.157783481@gmail.com> <20170710125233.2006733e@alans-desktop> <20170710123307.GA777@sanghar> <20170712192028.70bc0d54@alans-desktop> <20170713112954.GA665@sanghar> <20170717123145.GE24503@kroah.com> <3E107A3D-D8E2-43E5-8DCB-F9DE2F5AAAEA@gmail.com> <20170717230438.5c3bd397@alans-desktop> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170717230438.5c3bd397@alans-desktop> 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: 6637 Lines: 180 On Mon, Jul 17, 2017 at 11:04:38PM +0100, Alan Cox wrote: > > > Sure. I can fix the tty->count mismatch based on Alan's suggestion. However I don't understand why the exclusivity flag should belong to tty_port and not tty_struct. It will be good to know why. > > We are trying to move all the flags that we can and structs into the > tty_port, except any that are used internally within the struct tty > level code. The main reason for this is to make the object lifetimes and > locking simpler - because the tty_port lasts for the time the hardware is > present. I see. That does make sense. I have appended a version of the patch which adds the flag to tty_port and uses that inside tty_kopen. >From readability point of view however, I think adding the flag to tty_struct looks more intuitive. At least for now - until we move other things from tty_struct to tty_port. The patch is untested but I can work on this if that fits in with the plans for tty. Thanks, Okash --- drivers/tty/tty_io.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++---- include/linux/tty.h | 17 ++++++++++++ 2 files changed, 79 insertions(+), 5 deletions(-) --- a/drivers/tty/tty_io.c +++ b/drivers/tty/tty_io.c @@ -280,7 +280,7 @@ static int check_tty_count(struct tty_st { #ifdef CHECK_TTY_COUNT struct list_head *p; - int count = 0; + int count = 0, kopen_count = 0; spin_lock(&tty->files_lock); list_for_each(p, &tty->tty_files) { @@ -291,10 +291,12 @@ static int check_tty_count(struct tty_st tty->driver->subtype == PTY_TYPE_SLAVE && tty->link && tty->link->count) count++; - if (tty->count != count) { - tty_warn(tty, "%s: tty->count(%d) != #fd's(%d)\n", - routine, tty->count, count); - return count; + if (tty_port_kopened(tty->port)) + kopen_count++; + if (tty->count != (count + kopen_count)) { + tty_warn(tty, "%s: tty->count(%d) != (#fd's(%d) + #kopen's(%d))\n", + routine, tty->count, count, kopen_count); + return (count + kopen_count); } #endif return 0; @@ -1522,6 +1524,7 @@ static int tty_release_checks(struct tty */ void tty_release_struct(struct tty_struct *tty, int idx) { + // TODO: reset TTY_PORT_KOPENED on tty->port /* * Ask the line discipline code to release its structures */ @@ -1786,6 +1789,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); + tty_port_set_kopened(tty->port, 1); + } +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 +1875,12 @@ struct tty_struct *tty_open_by_driver(de } if (tty) { + if (tty_port_kopened(tty->port)) { + 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 @@ -261,6 +261,7 @@ struct tty_port { */ #define TTY_PORT_CTS_FLOW 3 /* h/w flow control enabled */ #define TTY_PORT_CHECK_CD 4 /* carrier detect enabled */ +#define TTY_PORT_KOPENED 5 /* device exclusively opened by kernel */ /* * Where all of the state associated with a tty is kept while the tty @@ -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; } static inline int tty_dev_name_to_number(const char *name, dev_t *number) { return -ENOTSUPP; } #endif @@ -652,6 +656,19 @@ static inline void tty_port_set_initiali clear_bit(TTY_PORT_INITIALIZED, &port->iflags); } +static inline bool tty_port_kopened(struct tty_port *port) +{ + return test_bit(TTY_PORT_KOPENED, &port->iflags); +} + +static inline void tty_port_set_kopened(struct tty_port *port, bool val) +{ + if (val) + set_bit(TTY_PORT_KOPENED, &port->iflags); + else + clear_bit(TTY_PORT_KOPENED, &port->iflags); +} + extern struct tty_struct *tty_port_tty_get(struct tty_port *port); extern void tty_port_tty_set(struct tty_port *port, struct tty_struct *tty); extern int tty_port_carrier_raised(struct tty_port *port);