lkml.org 
[lkml]   [2017]   [Jul]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: tty contention resulting from tty_open_by_device export
    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

    \
     
     \ /
      Last update: 2017-07-08 10:39    [W:3.141 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site