Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1683422 > unrolled thread

tty contention resulting from tty_open_by_device export

Back to article view | Back to linux.kernel

Contents

  tty contention resulting from tty_open_by_device export Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-07 22:30 +0200
    Re: tty contention resulting from tty_open_by_device export Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-07-08 10:40 +0200
      Re: tty contention resulting from tty_open_by_device export Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-08 11:10 +0200
      [patch 3/3] tty: undo export of tty_open_by_driver Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-09 13:50 +0200
      [patch 0/3] Re: tty contention resulting from tty_open_by_device export Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-09 13:50 +0200
        [patch 2/3] staging: speakup: use tty_kopen instead of tty_open_by_driver Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-09 13:50 +0200
          Re: [patch 2/3] staging: speakup: use tty_kopen instead of  tty_open_by_driver Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-07-09 14:00 +0200
            Re: [patch 2/3] staging: speakup: use tty_kopen instead of  tty_open_by_driver Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-09 14:30 +0200
        [patch 1/3] tty: resolve tty contention between kernel and user space Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-09 13:50 +0200
          Re: [patch 1/3] tty: resolve tty contention between kernel and user  space Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-07-09 14:00 +0200
          Re: [patch 1/3] tty: resolve tty contention between kernel and user space Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-07-09 17:10 +0200
            Re: [patch 1/3] tty: resolve tty contention between kernel and user  space Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-09 21:10 +0200
            Re: [patch 1/3] tty: resolve tty contention between kernel and user  space Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-10 10:40 +0200
              Re: [patch 1/3] tty: resolve tty contention between kernel and user space Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-07-10 17:30 +0200
                Re: [patch 1/3] tty: resolve tty contention between kernel and user  space Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-10 18:20 +0200
        Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device  export Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-07-09 14:00 +0200
        [patch 4/3] tty: make tty_kopen return ENODEV in case of no TTY Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-09 14:40 +0200
        Re: [patch 0/3] Re: tty contention resulting from  tty_open_by_device export Alan Cox <gnomes@lxorguk.ukuu.org.uk> - 2017-07-10 14:00 +0200
          Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device  export Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-10 14:40 +0200
            Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device  export Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-10 18:30 +0200
            Re: [patch 0/3] Re: tty contention resulting from  tty_open_by_device export Alan Cox <gnomes@lxorguk.ukuu.org.uk> - 2017-07-12 20:30 +0200
              Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device  export Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-13 13:40 +0200
                Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device  export Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-07-17 14:40 +0200
                  Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device export Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-17 15:30 +0200
                    Re: [patch 0/3] Re: tty contention resulting from  tty_open_by_device export Alan Cox <gnomes@lxorguk.ukuu.org.uk> - 2017-07-18 00:10 +0200
                  [patch v2 1/3] tty: resolve tty contention between kernel and user space Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-17 23:10 +0200
                  [patch v2 3/3] tty: undo export of tty_open_by_driver Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-17 23:10 +0200
                  [patch v2 0/3] tty contention resulting from tty_open_by_driver export Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-17 23:10 +0200
                    [patch v2 2/3] staging: speakup: use tty_kopen instead of tty_open_by_driver Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-17 23:10 +0200

Page 1 of 2  [1] 2  Next page →

#1683422 — tty contention resulting from tty_open_by_device export

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; }

[toc] | [next] | [standalone]

#1683561

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

[toc] | [prev] | [next] | [standalone]

#1683569

On Sat, Jul 08, 2017 at 10:38:03AM +0200, Greg Kroah-Hartman wrote:

> > +
> > +        if (tty) {
> > +                tty_kref_put(tty);  /* drop kref from tty_driver_lookup_tty() */
> 
> Put the comment above the line?
> 
Sure

> > +                tty = ERR_PTR(-EBUSY);
> > +        } else { /* Returns with the tty_lock held for now */
> 
> I don't understand this comment.
> 
This is same comment in tty_open_by_driver. Basically, tty_init_dev
returns tty locked so that it doesn't dissapear from underneath the
caller. I am not sure about the "for now" part of the comment. I'll
remove it.

> > +                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()?  :)
> 
Of course, will update
> >  /**
> >   *	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.
> 
Will do

> 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?)
> 
Yes, returning -EBUSY is a change in the interface. I will test against
applications like minicom before resubmitting this.

Thanks,
Okash

[toc] | [prev] | [next] | [standalone]

#1683723 — [patch 3/3] tty: undo export of tty_open_by_driver

Since we have tty_kopen, we no longer need to export tty_open_by_driver.
This patch makes this function static.

Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>

---
 drivers/tty/tty_io.c |    3 +--
 include/linux/tty.h  |    5 -----
 2 files changed, 1 insertion(+), 7 deletions(-)

--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1849,7 +1849,7 @@ EXPORT_SYMBOL_GPL(tty_kopen);
  *	  - 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;
@@ -1900,7 +1900,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
@@ -400,8 +400,6 @@ 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
@@ -424,9 +422,6 @@ 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)
-{ 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)

[toc] | [prev] | [next] | [standalone]

#1683725 — [patch 0/3] Re: tty contention resulting from tty_open_by_device export

On Sat, Jul 08, 2017 at 10:38:03AM +0200, Greg Kroah-Hartman wrote:
> 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?)
>
I tested this wil minincom, picocom and commands like "echo foo >
/dev/ttyS0". They all correctly report "Device or resource busy".

I have addressed all the comments you made. I have also split the patch
into three. Following is summary of each.

Patch 1: introduces the tty_kopen function and checks for TTY_KOPENED
Patch 2: updates speakup code to use tty_kopen instead of
        tty_open_by_driver
Patch 3: reverses the export of tty_open_by_driver

Thanks,
Okash

[toc] | [prev] | [next] | [standalone]

#1683730 — [patch 2/3] staging: speakup: use tty_kopen instead of tty_open_by_driver

This patch replaces call to tty_open_by_driver with a tty_kopen.

Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>

---
 drivers/staging/speakup/spk_ttyio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- 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);
 

[toc] | [prev] | [next] | [standalone]

#1683735 — Re: [patch 2/3] staging: speakup: use tty_kopen instead of tty_open_by_driver

On Sun, Jul 09, 2017 at 12:41:55PM +0100, Okash Khawaja wrote:
> This patch replaces call to tty_open_by_driver with a tty_kopen.
> 
> Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>
> 
> ---
>  drivers/staging/speakup/spk_ttyio.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- 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))

Hm, the "no tty layer" return value for this will be NULL.  I doubt
that's really a big deal, but perhaps just have that return
PTR_ERR(-ENODEV) or something?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1683738 — Re: [patch 2/3] staging: speakup: use tty_kopen instead of tty_open_by_driver

On Sun, Jul 09, 2017 at 01:50:55PM +0200, Greg Kroah-Hartman wrote:
> On Sun, Jul 09, 2017 at 12:41:55PM +0100, Okash Khawaja wrote:
> > This patch replaces call to tty_open_by_driver with a tty_kopen.
> > 
> > Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>
> > 
> > ---
> >  drivers/staging/speakup/spk_ttyio.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > --- 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))
> 
> Hm, the "no tty layer" return value for this will be NULL.  I doubt
> that's really a big deal, but perhaps just have that return
> PTR_ERR(-ENODEV) or something?
Good point, thanks. Will send a follow up patch

Okash

[toc] | [prev] | [next] | [standalone]

#1683731 — [patch 1/3] tty: resolve tty contention between kernel and user space

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.

This patch makes kernel access to tty exclusive, so that if a user
process or kernel opens a kernel opened tty, it gets -EBUSY. It 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.

Returning -EBUSY on tty open is a change in the interface. I have
tested this with minicom, picocom and commands like "echo foo >
/dev/ttyS0". They all correctly report "Device or resource busy" when
the tty is already kernel opened.

Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>

---
 drivers/tty/tty_io.c |   54 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/tty.h  |    4 +++
 2 files changed, 58 insertions(+)

--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1786,6 +1786,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);
+		set_bit(TTY_KOPENED, &tty->flags);
+	}
+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 +1872,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() */
--- 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
@@ -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

[toc] | [prev] | [next] | [standalone]

#1683733 — Re: [patch 1/3] tty: resolve tty contention between kernel and user space

On Sun, Jul 09, 2017 at 12:41:54PM +0100, Okash Khawaja wrote:
> 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.
> 
> This patch makes kernel access to tty exclusive, so that if a user
> process or kernel opens a kernel opened tty, it gets -EBUSY. It 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.
> 
> Returning -EBUSY on tty open is a change in the interface. I have
> tested this with minicom, picocom and commands like "echo foo >
> /dev/ttyS0". They all correctly report "Device or resource busy" when
> the tty is already kernel opened.
> 
> Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>
> 
> ---
>  drivers/tty/tty_io.c |   54 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/tty.h  |    4 +++
>  2 files changed, 58 insertions(+)
> 
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -1786,6 +1786,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);
> +		set_bit(TTY_KOPENED, &tty->flags);
> +	}
> +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 +1872,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() */
> --- 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
> @@ -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; }

Like I said in response to patch 2, maybe this should return an error?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1683759 — Re: [patch 1/3] tty: resolve tty contention between kernel and user space

On Sun, Jul 9, 2017 at 2:41 PM, Okash Khawaja <okash.khawaja@gmail.com> wrote:

> +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);

Hmm... perhaps

tty = ERR_CAST(driver);
goto out_unlock;

See below for further details.

> +       }
> +
> +       /* 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);
> +               set_bit(TTY_KOPENED, &tty->flags);
> +       }

> +out:

out_unlock: ?

> +       mutex_unlock(&tty_mutex);
> +       tty_driver_kref_put(driver);

I'm not sure I understand locking model here:

Above
1. take mutex
2. take reference

Here:
1. give mutex back
2. give reference back

I think we usually see symmetrical  calls, i.e.

1. give reference back
2. give mutex back

So, what did I miss?

> +       return tty;
> +}



-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1683824 — Re: [patch 1/3] tty: resolve tty contention between kernel and user space

Hi,

On Sun, Jul 09, 2017 at 06:04:17PM +0300, Andy Shevchenko wrote:
 
> Above
> 1. take mutex
> 2. take reference
> 
> Here:
> 1. give mutex back
> 2. give reference back
> 
> I think we usually see symmetrical  calls, i.e.
> 
> 1. give reference back
> 2. give mutex back
> 
> So, what did I miss?

After we hold the kref, driver won't disappear from underneath us so we
don't need tty_mutex just for decrementing the refcount.

Thanks,
Okash

[toc] | [prev] | [next] | [standalone]

#1684050 — Re: [patch 1/3] tty: resolve tty contention between kernel and user space

On Sun, Jul 09, 2017 at 06:04:17PM +0300, Andy Shevchenko wrote:
> On Sun, Jul 9, 2017 at 2:41 PM, Okash Khawaja <okash.khawaja@gmail.com> wrote:
> 
> > +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);
> 
> Hmm... perhaps
> 
> tty = ERR_CAST(driver);
> goto out_unlock;
> 
> See below for further details.
> 
Sorry missed this one out. Since tty_lookup_driver has failed, we don't
need to down the refcount on driver. So we can return here, without
going to out_unlock.

Thanks,
Okash

[toc] | [prev] | [next] | [standalone]

#1684347 — Re: [patch 1/3] tty: resolve tty contention between kernel and user space

On Mon, Jul 10, 2017 at 11:31 AM, Okash Khawaja <okash.khawaja@gmail.com> wrote:
> On Sun, Jul 09, 2017 at 06:04:17PM +0300, Andy Shevchenko wrote:
>> On Sun, Jul 9, 2017 at 2:41 PM, Okash Khawaja <okash.khawaja@gmail.com> wrote:
>>
>> > +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);
>>
>> Hmm... perhaps
>>
>> tty = ERR_CAST(driver);
>> goto out_unlock;
>>
>> See below for further details.
>>
> Sorry missed this one out. Since tty_lookup_driver has failed, we don't
> need to down the refcount on driver. So we can return here, without
> going to out_unlock.

Yeah, and my point is to use goto with the symmetric giveups of lock
and reference.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1684400 — Re: [patch 1/3] tty: resolve tty contention between kernel and user space

On Mon, Jul 10, 2017 at 06:21:37PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 10, 2017 at 11:31 AM, Okash Khawaja <okash.khawaja@gmail.com> wrote:
> > On Sun, Jul 09, 2017 at 06:04:17PM +0300, Andy Shevchenko wrote:
> >> On Sun, Jul 9, 2017 at 2:41 PM, Okash Khawaja <okash.khawaja@gmail.com> wrote:
> >>
> >> > +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);
> >>
> >> Hmm... perhaps
> >>
> >> tty = ERR_CAST(driver);
> >> goto out_unlock;
> >>
> >> See below for further details.
> >>
> > Sorry missed this one out. Since tty_lookup_driver has failed, we don't
> > need to down the refcount on driver. So we can return here, without
> > going to out_unlock.
> 
> Yeah, and my point is to use goto with the symmetric giveups of lock
> and reference.
Ah okay I see your point. Sure, I don't mind either way. However, this
code closely follows tty_open_by_driver, so I have tried to keep the
same pattern for sake of consistency :)

Thanks,
Okash

[toc] | [prev] | [next] | [standalone]

#1683734 — Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device export

On Sun, Jul 09, 2017 at 12:41:53PM +0100, Okash Khawaja wrote:
> On Sat, Jul 08, 2017 at 10:38:03AM +0200, Greg Kroah-Hartman wrote:
> > 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?)
> >
> I tested this wil minincom, picocom and commands like "echo foo >
> /dev/ttyS0". They all correctly report "Device or resource busy".
> 
> I have addressed all the comments you made. I have also split the patch
> into three. Following is summary of each.
> 
> Patch 1: introduces the tty_kopen function and checks for TTY_KOPENED
> Patch 2: updates speakup code to use tty_kopen instead of
>         tty_open_by_driver
> Patch 3: reverses the export of tty_open_by_driver

Looks great, only one tiny comment about the return value from me, and
really, it's not a big deal at all, you can send a patch 4/3 to change
that up if you want.  No one really runs without the tty layer enabled :)

If there are no other objections, I'll queue this up after 4.13-rc1 is
out, nice job.

greg k-h

[toc] | [prev] | [next] | [standalone]

#1683745 — [patch 4/3] tty: make tty_kopen return ENODEV in case of no TTY

When TTY is not built, tty_kopen should return an error. This way
calling code remains consistent, regardless of whether tty is built or
not. This patch returns -ENODEV when there is no tty.

Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>

---
 include/linux/tty.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -423,7 +423,7 @@ static inline int __init tty_init(void)
 static inline const char *tty_name(const struct tty_struct *tty)
 { return "(none)"; }
 static inline struct tty_struct *tty_kopen(dev_t device)
-{ return NULL; }
+{ return ERR_PTR(-ENODEV); }
 static inline int tty_dev_name_to_number(const char *name, dev_t *number)
 { return -ENOTSUPP; }
 #endif

[toc] | [prev] | [next] | [standalone]

#1684168 — Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device export

On Sun, 09 Jul 2017 12:41:53 +0100
Okash Khawaja <okash.khawaja@gmail.com> wrote:

> On Sat, Jul 08, 2017 at 10:38:03AM +0200, Greg Kroah-Hartman wrote:
> > 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?)
> >  
> I tested this wil minincom, picocom and commands like "echo foo >
> /dev/ttyS0". They all correctly report "Device or resource busy".
> 
> I have addressed all the comments you made. I have also split the patch
> into three. Following is summary of each.

If the tty counts are being misreported then it would be better to fix
the code to actually manage the counts properly. The core tty code is
telling you that the tty is not in a valid state. While this is of
itself a good API to have, the underlying reference miscounting ought
IMHO to be fixed as well.

Also you don't need a new TTY_KOPENED flag as far as I can see. All tty's
have a usage count and active bit flags already. Use those.

Alan

[toc] | [prev] | [next] | [standalone]

#1684196 — Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device export

On Mon, Jul 10, 2017 at 12:52:33PM +0100, Alan Cox wrote:
> On Sun, 09 Jul 2017 12:41:53 +0100
> Okash Khawaja <okash.khawaja@gmail.com> wrote:
> 
> > On Sat, Jul 08, 2017 at 10:38:03AM +0200, Greg Kroah-Hartman wrote:
> > > 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?)
> > >  
> > I tested this wil minincom, picocom and commands like "echo foo >
> > /dev/ttyS0". They all correctly report "Device or resource busy".
> > 
> > I have addressed all the comments you made. I have also split the patch
> > into three. Following is summary of each.
> 
> If the tty counts are being misreported then it would be better to fix
> the code to actually manage the counts properly. The core tty code is
> telling you that the tty is not in a valid state. While this is of
> itself a good API to have, the underlying reference miscounting ought
> IMHO to be fixed as well.
When opening from kernel, we don't use file pointer. The count mismatch
is between tty->count and #fd's. So opening from kernel leads to #fd's
being less than tty->count. I thought this difference is relevant to
user-space opening of tty, and not to kernel opening of tty. Can you
suggest how to address this mismatch?

> 
> Also you don't need a new TTY_KOPENED flag as far as I can see. All tty's
> have a usage count and active bit flags already. Use those.
Ah may be I didn't notice the active bit. Is it one of the #defines in
tty.h? Can usage count and active bit be used to differentiate between
whether the tty was opened by kernel or user?

Thanks,
Okash

[toc] | [prev] | [next] | [standalone]

#1684401 — Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device export

On Mon, Jul 10, 2017 at 01:33:07PM +0100, Okash Khawaja wrote:
> > If the tty counts are being misreported then it would be better to fix
> > the code to actually manage the counts properly. The core tty code is
> > telling you that the tty is not in a valid state. While this is of
> > itself a good API to have, the underlying reference miscounting ought
> > IMHO to be fixed as well.
> When opening from kernel, we don't use file pointer. The count mismatch
> is between tty->count and #fd's. So opening from kernel leads to #fd's
> being less than tty->count. I thought this difference is relevant to
> user-space opening of tty, and not to kernel opening of tty. Can you
> suggest how to address this mismatch?
Idea is tty_kopen only ever returns a newly initialised tty - returned
by tty_init_dev. Since the access is exclusive, tty->count shouldn't
matter. When the caller is done with it, it calls tty_release_struct
which frees up the tty itself.

But yes, all the while a tty is kopened, its tty->count and #fds will be
unequal.

Thanks,
Okash

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web