From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by befuddled.reisers.ca (Postfix, from userid 65534) id 410E41EFD80; Fri, 7 Jul 2017 16:28:50 -0400 (EDT) Received: from mail-wr0-x233.google.com (mail-wr0-x233.google.com [IPv6:2a00:1450:400c:c0c::233]) by befuddled.reisers.ca (Postfix) with ESMTPS id DA46A1EFD72 for ; Fri, 7 Jul 2017 16:28:45 -0400 (EDT) Received: by mail-wr0-x233.google.com with SMTP id k67so61458928wrc.2 for ; Fri, 07 Jul 2017 13:28:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:mime-version:content-disposition :user-agent; bh=eUyWtsCDnia6qbOp+cRMZtOTX9KkrePKoAAawyxr1lc=; b=Vjr4dXYLr+FxQKDxNQ+vAS5x+7cvpt0WcUsuTqUxagYUs+rSiJress2GHsrPPgzp9y nBeSxrcpV5K6aV+XBnQ2uFH8+UMDcybAlVvLqKkIDCBtP+v+N8M0bVE9lEDoEVkhUR+z jDsFALVvQCApLGMAnaaXUrm8Y4hXO8bMO8ePLFiKPzDXHH6tZd7+yJfulS7/SMX5Sf/0 mikbTgoc1iuV3n+QnMsqEyolT4B8sd5st0IRwAHfb7kd2xIEav34w8KTQVaon9uvjx1g P4mBzqlFmWklVFDDf2a0rGIukEloNlNaGE6RT8OJILcXc1u3rBvlQYNq8a089RRzkK9x 7EYg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:mime-version :content-disposition:user-agent; bh=eUyWtsCDnia6qbOp+cRMZtOTX9KkrePKoAAawyxr1lc=; b=NvLSgDObJe5EI0WJlWZypxmTj/cr86VzNLUlO23jWUnULteW06a+aidNAyoTO+0qaN EP6FTsqfKmmoH4JmT8AGjEkC6/d2LX4/CneVgzOF3hcY3wcKHobMGzuu+oN+jZ3w4n0/ ++gUBvFlluQxtSCsAu0K7MFxj4xd6sLWu4ubCYJoXom9gWwsxq16dECX3dxm4/l+B7La Pjo9zQ0u1N3fu+Z/j+taSwbyvljqZCHCIzs8earybbkY5yDd9fSOhJ8d7aUQTQduXx7k 2J/Uett3EK865kYl0U10KdoKB+NCmIVs/39zFe1YNEdUKb5V9x2iNIPwMATJdhS92Xjr myVA== X-Gm-Message-State: AIVw110K+I/JnJRpUGdwPHMlABItvJ7c0ryZ9huu4/WcLplE/YafW9Cp OSToy6bGh6pdOA== X-Received: by 10.28.91.141 with SMTP id p135mr359263wmb.19.1499459324822; Fri, 07 Jul 2017 13:28:44 -0700 (PDT) Received: from sanghar ([2a00:23c4:7320:5900:224:d6ff:fe76:7136]) by smtp.gmail.com with ESMTPSA id 63sm441466wmi.8.2017.07.07.13.28.43 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 07 Jul 2017 13:28:44 -0700 (PDT) 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) X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 X-BeenThere: speakup@linux-speakup.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Speakup is a screen review system for Linux." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 07 Jul 2017 20:28:50 -0000 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; }