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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
