Re: tty contention resulting from tty_open_by_device export

[Greg Kroah-Hartman]

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