tty contention resulting from tty_open_by_device export

PATCH Superseded Okash Khawaja 5 participants 37 messages Jul 7, 2017 – Jul 20, 2017

Subsystems (2)

Staging TTY layer and serial drivers

lore

Share & tools

lore

Okash Khawaja

author

2017-07-07 20:28 UTC #0 lore raw

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

drivers/staging/speakup/spk_ttyio.c +1 -1

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

drivers/tty/tty_io.c +54 -2

80 lines not on this page · raw message

include/linux/tty.h +3 -4

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

Greg Kroah-Hartman

maintainer

2017-07-08 08:38 UTC #1 lore raw

Quoted 44 lines · Okash Khawaja

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(-)
drivers/staging/speakup/spk_ttyio.c +1 -1
@@ -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);
 

drivers/tty/tty_io.c static struct tty_driver *tty_lookup_dri · line 1790

Quoted 5 lines

drivers/tty/tty_io.c +2 -0

@@ -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 :)

Quoted 22 lines

drivers/tty/tty_io.c +22 -0

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

drivers/tty/tty_io.c static struct tty_driver *tty_lookup_dri · line 1822

Quoted 10 lines

drivers/tty/tty_io.c +10 -0

+                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?

drivers/tty/tty_io.c static struct tty_driver *tty_lookup_dri · line 1824

Quoted 2 lines

drivers/tty/tty_io.c +2 -0

+                tty = ERR_PTR(-EBUSY);
+        } else { /* Returns with the tty_lock held for now */

I don't understand this comment.

drivers/tty/tty_io.c static struct tty_driver *tty_lookup_dri · line 1833

Quoted 9 lines

drivers/tty/tty_io.c +9 -0

+                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()? :)

Quoted 38 lines

drivers/tty/tty_io.c +9 -2

+
+/**
  *	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

include/linux/tty.h struct tty_file_private { · line 365

Quoted 4 lines

include/linux/tty.h +1 -0

@@ -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

Okash Khawaja

author

2017-07-08 09:01 UTC #2 lore raw

Quoted 6 lines · Greg Kroah-Hartman

drivers/tty/tty_io.c static struct tty_driver *tty_lookup_dri · line 1822
Quoted 3 lines
drivers/tty/tty_io.c +3 -0
+
+        if (tty) {
+                tty_kref_put(tty);  /* drop kref from tty_driver_lookup_tty() */
Put the comment above the line?

Sure

Quoted 5 lines

drivers/tty/tty_io.c static struct tty_driver *tty_lookup_dri · line 1824
Quoted 2 lines
drivers/tty/tty_io.c +2 -0
+                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.

Quoted 12 lines

drivers/tty/tty_io.c static struct tty_driver *tty_lookup_dri · line 1833

Quoted 9 lines

drivers/tty/tty_io.c +9 -0

+                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

Quoted 12 lines

include/linux/tty.h struct tty_file_private { · line 365

Quoted 9 lines

/**
* tty_open - open a tty device

include/linux/tty.h +1 -0

@@ -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

Quoted 6 lines

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

Okash Khawaja

author

2017-07-09 11:44 UTC #3 lore raw

In reply to Greg Kroah-Hartman

Quoted 6 lines · Greg Kroah-Hartman

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

Okash Khawaja

author

2017-07-09 11:44 UTC #4 lore raw

PATCH v2 1/3 tty: resolve tty contention between kernel and user space

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 <[email protected]>

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

drivers/tty/tty_io.c +54 -0

@@ -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() */

include/linux/tty.h +4 -0

@@ -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

Greg Kroah-Hartman

maintainer

2017-07-09 11:51 UTC #5 lore raw

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

Quoted 117 lines · Okash Khawaja

Signed-off-by Okash Khawaja <[email protected]>

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

drivers/tty/tty_io.c +54 -0

@@ -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() */

include/linux/tty.h +2 -0

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

include/linux/tty.h static inline const char *tty_name(const · line 431

Quoted 5 lines

include/linux/tty.h +2 -0

@@ -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

Andy Shevchenko

2017-07-09 15:04 UTC #6 lore raw

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

In reply to Okash Khawaja

drivers/tty/tty_io.c static struct tty_driver *tty_lookup_dri · line 1813

Quoted 12 lines · Okash Khawaja

drivers/tty/tty_io.c +11 -0

+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.

Quoted 7 lines

drivers/tty/tty_io.c +7 -0

+       }
+
+       /* check whether we're reopening an existing tty */
+       tty = tty_driver_lookup_tty(driver, NULL, index);
+       if (IS_ERR(tty))
+               goto out;
+

drivers/tty/tty_io.c static struct tty_driver *tty_lookup_dri · line 1829

Quoted 10 lines

drivers/tty/tty_io.c +9 -0

+       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: ?

Review

Quoted 2 lines

(quoted diff) +2 -0

+       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?

Quoted 2 lines

drivers/tty/tty_io.c +2 -0
+       return tty;
+}

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Okash Khawaja

author

2017-07-09 19:08 UTC #7 lore raw

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

Hi,

Quoted 8 lines · Andy Shevchenko

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

Quoted 6 lines

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

Okash Khawaja

author

2017-07-10 08:31 UTC #8 lore raw

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

In reply to Andy Shevchenko

Quoted 15 lines · Andy Shevchenko

drivers/tty/tty_io.c static struct tty_driver *tty_lookup_dri · line 1813

Quoted 12 lines · Okash Khawaja

drivers/tty/tty_io.c +11 -0

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

Quoted 7 lines

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

Andy Shevchenko

2017-07-10 15:21 UTC #9 lore raw

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

Quoted 18 lines · Okash Khawaja

Quoted 17 lines · Andy Shevchenko

drivers/tty/tty_io.c static struct tty_driver *tty_lookup_dri · line 1813

Quoted 12 lines · Okash Khawaja

drivers/tty/tty_io.c +11 -0

+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

Quoted 8 lines

Quoted 5 lines
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.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Okash Khawaja

author

2017-07-10 16:12 UTC #10 lore raw

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

Quoted 22 lines · Andy Shevchenko

Quoted 21 lines · Okash Khawaja
Quoted 20 lines · Andy Shevchenko
drivers/tty/tty_io.c static struct tty_driver *tty_lookup_dri · line 1813
Quoted 12 lines · Okash Khawaja
drivers/tty/tty_io.c +11 -0
+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;

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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

Okash Khawaja

author

2017-07-09 11:44 UTC #11 lore raw

PATCH v2 2/3 staging: speakup: use tty_kopen instead of tty_open_by_driver

In reply to Okash Khawaja

This patch replaces call to tty_open_by_driver with a tty_kopen.

Signed-off-by Okash Khawaja <[email protected]>

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

drivers/staging/speakup/spk_ttyio.c +1 -1

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

Greg Kroah-Hartman

maintainer

2017-07-09 11:51 UTC #12 lore raw

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

Quoted 11 lines · Okash Khawaja

This patch replaces call to tty_open_by_driver with a tty_kopen.
Signed-off-by Okash Khawaja <[email protected]>
---
 drivers/staging/speakup/spk_ttyio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/speakup/spk_ttyio.c static int spk_ttyio_initialise_ldisc(st · line 168

Quoted 6 lines

drivers/staging/speakup/spk_ttyio.c +1 -1

@@ -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

Okash Khawaja

author

2017-07-09 12:28 UTC #13 lore raw

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

Quoted 15 lines · Greg Kroah-Hartman

Quoted 14 lines · Okash Khawaja
This patch replaces call to tty_open_by_driver with a tty_kopen.
Signed-off-by Okash Khawaja <[email protected]>
---
 drivers/staging/speakup/spk_ttyio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/speakup/spk_ttyio.c +0 -0
@@ -164,7 +164,7 @@ static int spk_ttyio_initialise_ldisc(st
 	if (ret)
 		return ret;
 

Quoted 7 lines

drivers/staging/speakup/spk_ttyio.c static int spk_ttyio_initialise_ldisc(st · line 168
Quoted 3 lines
drivers/staging/speakup/spk_ttyio.c +1 -1
-	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

Okash Khawaja

author

2017-07-09 11:44 UTC #14 lore raw

PATCH v2 3/3 tty: undo export of tty_open_by_driver

In reply to Okash Khawaja

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 <[email protected]>

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

drivers/tty/tty_io.c +1 -2

@@ -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

include/linux/tty.h +0 -5

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

Greg Kroah-Hartman

maintainer

2017-07-09 11:57 UTC #15 lore raw

In reply to Okash Khawaja

Quoted 10 lines · Okash Khawaja

Quoted 6 lines · Greg Kroah-Hartman
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".

Quoted 7 lines

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

Okash Khawaja

author

2017-07-09 12:32 UTC #16 lore raw

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

In reply to Okash Khawaja

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 <[email protected]>

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

include/linux/tty.h +1 -1

@@ -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

Alan Cox

2017-07-10 11:53 UTC #17 lore raw

In reply to Okash Khawaja

Quoted 7 lines · On Sun, 09 Jul 2017 12:41:53 +0100 Okash Khawaja <[email protected]>…

Quoted 6 lines · Greg Kroah-Hartman
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?)

Quoted 5 lines

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

Okash Khawaja

author

2017-07-10 12:33 UTC #18 lore raw

Quoted 13 lines · Alan Cox

Quoted 10 lines · On Sun, 09 Jul 2017 12:41:53 +0100 Okash Khawaja <[email protected]>…
Quoted 6 lines · Greg Kroah-Hartman
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".

Quoted 8 lines

Quoted 2 lines
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?

Quoted 3 lines

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

Okash Khawaja

author

2017-07-10 16:22 UTC #19 lore raw

Quoted 10 lines · Okash Khawaja

Quoted 5 lines
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

Alan Cox

2017-07-12 18:21 UTC #20 lore raw

In reply to Okash Khawaja

Quoted 6 lines

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?

Your kernel reference is the same as having a file open reference so I think this actually needs addressing in the maths. In other words count the number of kernel references and also add that into the test for check_tty_count (kernel references + #fds == count).

I'd really like to keep this right because that check has a long history of catching really nasty race conditions in the tty code. The open/close/hangup code is really fragile so worth the debugability.

Quoted 3 lines

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?

It only tells you whether the port is currently active for some purpose, not which. If you still want to implement exclusivity between kernel and user then it needs another flag, but I think that flag should be in port->flags as it is a property of the physical interface.

(Take a look at tty_port_open for example)

Alan

Okash Khawaja

author

2017-07-13 11:30 UTC #21 lore raw

Quoted 7 lines · Alan Cox

Quoted 6 lines
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?

Quoted 8 lines

Your kernel reference is the same as having a file open reference so I think this actually needs addressing in the maths. In other words count the number of kernel references and also add that into the test for check_tty_count (kernel references + #fds == count).
I'd really like to keep this right because that check has a long history of catching really nasty race conditions in the tty code. The open/close/hangup code is really fragile so worth the debugability.

I see. Okay based this, check_tty_count can be easily updated to take into account kernel references.

Quoted 11 lines

Quoted 4 lines
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?
It only tells you whether the port is currently active for some purpose, not which. If you still want to implement exclusivity between kernel and user then it needs another flag, but I think that flag should be in port->flags as it is a property of the physical interface.
(Take a look at tty_port_open for example)

Okay I can add TTY_PORT_KOPENED to port->flags and that should work too.

However, can you please help me understand this:
Our use case requires kernel access to tty_struct and accordingly tty_kopen returns tty_struct. The exclusivity between user and kernel space is also meant to prevent one side from opening tty_struct while another has it opened. In all this, it is tty_struct and not tty_port which is the key resource we are concerned with. So shouldn't the exclusivity flag belong to tty_struct?

Adding a the flag to port->flags but controlling it from code for opening and closing tty will also mean we have tty_port_kopened, tty_port_set_kopen etc inside tty open/close code.

Am I viewing this problem incorrectly?

Thanks,
Okash

Greg Kroah-Hartman

maintainer

2017-07-17 12:31 UTC #22 lore raw

Quoted 13 lines · Okash Khawaja

Quoted 12 lines · Alan Cox
Quoted 6 lines
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?
Your kernel reference is the same as having a file open reference so I think this actually needs addressing in the maths. In other words count the number of kernel references and also add that into the test for check_tty_count (kernel references + #fds == count).

Quoted 6 lines

Quoted 3 lines
I'd really like to keep this right because that check has a long history of catching really nasty race conditions in the tty code. The open/close/hangup code is really fragile so worth the debugability.
I see. Okay based this, check_tty_count can be easily updated to take into account kernel references.

Ok, I'll drop this series from my "to-apply" queue and wait for you to redo it.

thanks,

greg k-h

Okash Khawaja

author

2017-07-17 13:23 UTC #23 lore raw

Quoted 22 lines · Greg Kroah-Hartman

Quoted 19 lines · Okash Khawaja
Quoted 16 lines · Alan Cox
Quoted 6 lines
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?
Your kernel reference is the same as having a file open reference so I think this actually needs addressing in the maths. In other words count the number of kernel references and also add that into the test for check_tty_count (kernel references + #fds == count).
I'd really like to keep this right because that check has a long history of catching really nasty race conditions in the tty code. The open/close/hangup code is really fragile so worth the debugability.

Quoted 5 lines

Quoted 2 lines
I see. Okay based this, check_tty_count can be easily updated to take into account kernel references.
Ok, I'll drop this series from my "to-apply" queue and wait for you to redo it.

Sure. I can fix the tty->count mismatch based on Alan's suggestion. However I don't understand why the exclusivity flag should belong to tty_port and not tty_struct. It will be good to know why.

Thanks,
Okash

Alan Cox

2017-07-17 22:05 UTC #24 lore raw

Quoted 2 lines

Sure. I can fix the tty->count mismatch based on Alan's suggestion. However I don't understand why the exclusivity flag should belong to tty_port and not tty_struct. It will be good to know why.

We are trying to move all the flags that we can and structs into the tty_port, except any that are used internally within the struct tty level code. The main reason for this is to make the object lifetimes and locking simpler - because the tty_port lasts for the time the hardware is present.

Alan

Okash Khawaja

author

2017-07-18 11:30 UTC #25 lore raw

Quoted 8 lines · Alan Cox

Quoted 2 lines
Sure. I can fix the tty->count mismatch based on Alan's suggestion. However I don't understand why the exclusivity flag should belong to tty_port and not tty_struct. It will be good to know why.
We are trying to move all the flags that we can and structs into the tty_port, except any that are used internally within the struct tty level code. The main reason for this is to make the object lifetimes and locking simpler - because the tty_port lasts for the time the hardware is present.

I see. That does make sense. I have appended a version of the patch which adds the flag to tty_port and uses that inside tty_kopen.

Quoted 1 line

From readability point of view however, I think adding the flag to

tty_struct looks more intuitive. At least for now - until we move other things from tty_struct to tty_port.

The patch is untested but I can work on this if that fits in with the plans for tty.

Thanks,
Okash

---
 drivers/tty/tty_io.c |   67 +++++++++++++++++++++++++++++++++++++++++++++++----
 include/linux/tty.h  |   17 ++++++++++++
 2 files changed, 79 insertions(+), 5 deletions(-)

drivers/tty/tty_io.c +62 -5

@@ -280,7 +280,7 @@ static int check_tty_count(struct tty_st
 {
 #ifdef CHECK_TTY_COUNT
        struct list_head *p;
-       int count = 0;
+       int count = 0, kopen_count = 0;
 
        spin_lock(&tty->files_lock);
        list_for_each(p, &tty->tty_files) {

@@ -291,10 +291,12 @@ static int check_tty_count(struct tty_st
            tty->driver->subtype == PTY_TYPE_SLAVE &&
            tty->link && tty->link->count)
                count++;
-       if (tty->count != count) {
-               tty_warn(tty, "%s: tty->count(%d) != #fd's(%d)\n",
-                        routine, tty->count, count);
-               return count;
+       if (tty_port_kopened(tty->port))
+               kopen_count++;
+       if (tty->count != (count + kopen_count)) {
+               tty_warn(tty, "%s: tty->count(%d) != (#fd's(%d) + #kopen's(%d))\n",
+                        routine, tty->count, count, kopen_count);
+               return (count + kopen_count);
        }
 #endif
        return 0;

@@ -1522,6 +1524,7 @@ static int tty_release_checks(struct tty
  */
 void tty_release_struct(struct tty_struct *tty, int idx)
 {
+       // TODO: reset TTY_PORT_KOPENED on tty->port
        /*
         * Ask the line discipline code to release its structures
         */

@@ -1786,6 +1789,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);
+               tty_port_set_kopened(tty->port, 1);
+       }
+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 +1875,12 @@ struct tty_struct *tty_open_by_driver(de
        }
 
        if (tty) {
+               if (tty_port_kopened(tty->port)) {
+                       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() */

include/linux/tty.h +17 -0

@@ -261,6 +261,7 @@ struct tty_port {
  */
 #define TTY_PORT_CTS_FLOW      3       /* h/w flow control enabled */
 #define TTY_PORT_CHECK_CD      4       /* carrier detect enabled */
+#define TTY_PORT_KOPENED       5       /* device exclusively opened by kernel */
 
 /*
  * Where all of the state associated with a tty is kept while the tty

@@ -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

@@ -652,6 +656,19 @@ static inline void tty_port_set_initiali
                clear_bit(TTY_PORT_INITIALIZED, &port->iflags);
 }
 
+static inline bool tty_port_kopened(struct tty_port *port)
+{
+       return test_bit(TTY_PORT_KOPENED, &port->iflags);
+}
+
+static inline void tty_port_set_kopened(struct tty_port *port, bool val)
+{
+       if (val)
+               set_bit(TTY_PORT_KOPENED, &port->iflags);
+       else
+               clear_bit(TTY_PORT_KOPENED, &port->iflags);
+}
+
 extern struct tty_struct *tty_port_tty_get(struct tty_port *port);
 extern void tty_port_tty_set(struct tty_port *port, struct tty_struct *tty);
 extern int tty_port_carrier_raised(struct tty_port *port);

Dan Carpenter

2017-07-18 12:28 UTC #26 lore raw

Quoted 15 lines · Okash Khawaja

drivers/tty/tty_io.c +15 -0

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

drivers/tty/tty_io.c static struct tty_driver *tty_lookup_dri · line 1830

Quoted 10 lines

drivers/tty/tty_io.c +10 -0

+       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);
+               tty_port_set_kopened(tty->port, 1);

                                       ^^^^^^^^^

tty_init_dev() can fail leading to an error pointer dereference here.

drivers/tty/tty_io.c static struct tty_driver *tty_lookup_dri · line 1837

Quoted 7 lines

drivers/tty/tty_io.c +7 -0

+       }
+out:
+       mutex_unlock(&tty_mutex);
+       tty_driver_kref_put(driver);
+       return tty;
+}
+EXPORT_SYMBOL_GPL(tty_kopen);

regards,
dan carpenter

Okash Khawaja

author

2017-07-18 19:23 UTC #27 lore raw

Quoted 10 lines · Dan Carpenter

drivers/tty/tty_io.c static struct tty_driver *tty_lookup_dri · line 1830

Quoted 7 lines

drivers/tty/tty_io.c +7 -0

+       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);
+               tty_port_set_kopened(tty->port, 1);

                                       ^^^^^^^^^

tty_init_dev() can fail leading to an error pointer dereference here.

Thanks very much. I will check for error pointer here.

Okash

Alan Cox

2017-07-18 13:50 UTC #28 lore raw

In reply to Okash Khawaja

Quoted 2 lines

The patch is untested but I can work on this if that fits in with the plans for tty.

I think this is the right direction.

Alan

Okash Khawaja

author

2017-07-20 07:29 UTC #29 lore raw

v3 cover tty contention resulting from tty_open_by_driver export

Hi,

I have updated the patches so that the exclusivity flag is in tty_port. When closing the struct - by calling tty_release_struct - we also need to reset the flag. One way to do that is to reset the flag inside tty_release_struct function, regardless of whether the tty was opened through tty_kopen or not. In order to keep the code clean, I have instead created a separate function called tty_kclose which is the same as tty_release_struct except that it also resets the exclusivity flag. As a result, any changes to tty_release_struct won't be in tty_kclose untless manually added. Please advise on this.

Here is a summary of changes when compared to v2:

- Patch 1 uses TTY_PORT_KOPENED flag instead of TTY_KOPENED and as a result adds helper functions to read and change the flag
- Patch 1 adds tty_kclose function to close tty opened by tty_kopen
- Patch 2 calls tty_kclose instead of tty_release_struct

Thanks,
Okash

Okash Khawaja

author

2017-07-20 07:29 UTC #30 lore raw

PATCH v3 1/3 tty: resolve tty contention between kernel and user space

The commit 12e84c71b7d4 ("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 overloading 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. tty_kclose closes the tty opened by tty_kopen.

To address the mismatch between tty->count and #fd's, this patch adds #kopen's to the count before comparing it with tty->count. That way check_tty_count reflects correct usage count.

Signed-off-by Okash Khawaja <[email protected]>

---
 drivers/tty/tty_io.c |  100 ++++++++++++++++++++++++++++++++++++++++++++++++---
 include/linux/tty.h  |   21 ++++++++++
 2 files changed, 116 insertions(+), 5 deletions(-)

drivers/tty/tty_io.c +95 -5

@@ -280,7 +280,7 @@ static int check_tty_count(struct tty_st
 {
 #ifdef CHECK_TTY_COUNT
 	struct list_head *p;
-	int count = 0;
+	int count = 0, kopen_count = 0;
 
 	spin_lock(&tty->files_lock);
 	list_for_each(p, &tty->tty_files) {

@@ -291,10 +291,12 @@ static int check_tty_count(struct tty_st
 	    tty->driver->subtype == PTY_TYPE_SLAVE &&
 	    tty->link && tty->link->count)
 		count++;
-	if (tty->count != count) {
-		tty_warn(tty, "%s: tty->count(%d) != #fd's(%d)\n",
-			 routine, tty->count, count);
-		return count;
+	if (tty_port_kopened(tty->port))
+		kopen_count++;
+	if (tty->count != (count + kopen_count)) {
+		tty_warn(tty, "%s: tty->count(%d) != (#fd's(%d) + #kopen's(%d))\n",
+			 routine, tty->count, count, kopen_count);
+		return (count + kopen_count);
 	}
 #endif
 	return 0;

@@ -1513,6 +1515,38 @@ static int tty_release_checks(struct tty
 }
 
 /**
+ *      tty_kclose      -       closes tty opened by tty_kopen
+ *      @tty: tty device
+ *
+ *      Performs the final steps to release and free a tty device. It is the
+ *      same as tty_release_struct except that it also resets TTY_PORT_KOPENED
+ *      flag on tty->port.
+ */
+void tty_kclose(struct tty_struct *tty)
+{
+	/*
+	 * Ask the line discipline code to release its structures
+	 */
+	tty_ldisc_release(tty);
+
+	/* Wait for pending work before tty destruction commmences */
+	tty_flush_works(tty);
+
+	tty_debug_hangup(tty, "freeing structure\n");
+	/*
+	 * The release_tty function takes care of the details of clearing
+	 * the slots and preserving the termios structure. The tty_unlock_pair
+	 * should be safe as we keep a kref while the tty is locked (so the
+	 * unlock never unlocks a freed tty).
+	 */
+	mutex_lock(&tty_mutex);
+	tty_port_set_kopened(tty->port, 0);
+	release_tty(tty, tty->index);
+	mutex_unlock(&tty_mutex);
+}
+EXPORT_SYMBOL_GPL(tty_kclose);
+
+/**
  *	tty_release_struct	-	release a tty struct
  *	@tty: tty device
  *	@idx: index of the tty

@@ -1786,6 +1820,56 @@ 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);
+		if (IS_ERR(tty))
+			goto out;
+		tty_port_set_kopened(tty->port, 1);
+	}
+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 +1908,12 @@ struct tty_struct *tty_open_by_driver(de
 	}
 
 	if (tty) {
+		if (tty_port_kopened(tty->port)) {
+			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() */

include/linux/tty.h +21 -0

@@ -261,6 +261,8 @@ struct tty_port {
  */
 #define TTY_PORT_CTS_FLOW	3	/* h/w flow control enabled */
 #define TTY_PORT_CHECK_CD	4	/* carrier detect enabled */
+#define TTY_PORT_KOPENED	5	/* device exclusively opened by
+					   kernel */
 
 /*
  * Where all of the state associated with a tty is kept while the tty

@@ -401,6 +403,8 @@ 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 void tty_kclose(struct tty_struct *tty);
 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 +429,10 @@ 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 ERR_PTR(-ENODEV); }
+static inline void tty_kclose(struct tty_struct *tty)
+{ }
 static inline int tty_dev_name_to_number(const char *name, dev_t *number)
 { return -ENOTSUPP; }
 #endif

@@ -652,6 +660,19 @@ static inline void tty_port_set_initiali
 		clear_bit(TTY_PORT_INITIALIZED, &port->iflags);
 }
 
+static inline bool tty_port_kopened(struct tty_port *port)
+{
+	return test_bit(TTY_PORT_KOPENED, &port->iflags);
+}
+
+static inline void tty_port_set_kopened(struct tty_port *port, bool val)
+{
+	if (val)
+		set_bit(TTY_PORT_KOPENED, &port->iflags);
+	else
+		clear_bit(TTY_PORT_KOPENED, &port->iflags);
+}
+
 extern struct tty_struct *tty_port_tty_get(struct tty_port *port);
 extern void tty_port_tty_set(struct tty_port *port, struct tty_struct *tty);
 extern int tty_port_carrier_raised(struct tty_port *port);

Okash Khawaja

author

2017-07-20 07:29 UTC #31 lore raw

PATCH v3 2/3 staging: speakup: use tty_kopen and tty_kclose

In reply to Okash Khawaja

This patch replaces call to tty_open_by_driver with a tty_kopen and uses tty_kclose instead of tty_release_struct to close it.

Signed-off-by Okash Khawaja <[email protected]>

---
 drivers/staging/speakup/spk_ttyio.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/staging/speakup/spk_ttyio.c +2 -2

@@ -158,7 +158,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);

@@ -308,7 +308,7 @@ void spk_ttyio_release(void)
 
 	tty_ldisc_flush(speakup_tty);
 	tty_unlock(speakup_tty);
-	tty_release_struct(speakup_tty, speakup_tty->index);
+	tty_kclose(speakup_tty);
 }
 EXPORT_SYMBOL_GPL(spk_ttyio_release);

Okash Khawaja

author

2017-07-20 07:29 UTC #32 lore raw

PATCH v3 3/3 tty: undo export of tty_open_by_driver

In reply to Okash Khawaja

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 <[email protected]>

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

drivers/tty/tty_io.c +1 -2

@@ -1885,7 +1885,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;

@@ -1936,7 +1936,6 @@ out:
 	tty_driver_kref_put(driver);
 	return tty;
 }
-EXPORT_SYMBOL_GPL(tty_open_by_driver);
 
 /**
  *	tty_open		-	open a tty device

include/linux/tty.h +0 -5

@@ -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 void tty_kclose(struct tty_struct *tty);
 extern int tty_dev_name_to_number(const char *name, dev_t *number);

@@ -425,9 +423,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 ERR_PTR(-ENODEV); }
 static inline void tty_kclose(struct tty_struct *tty)

Okash Khawaja

author

2017-07-17 21:08 UTC #33 lore raw

v2 cover tty contention resulting from tty_open_by_driver export

In reply to Greg Kroah-Hartman

Hi,

I have reworked the previous patch set. These are the changes:

1. Patch 1 fixes tty->count mismatch reported by check_tty_count when a tty is kopened.
2. Patch 1 incorporates patch 4 in the previous patch set - it returns -ENODEV when tty is not configured.

Thanks,
Okash

Okash Khawaja

author

2017-07-17 21:08 UTC #34 lore raw

PATCH v2 1/3 tty: resolve tty contention between kernel and user space

To address the mismatch between tty->count and #fd's, this patch adds #kopen's to the count before comparing it with tty->count. That way check_tty_count reflects correct usage count.

Signed-off-by Okash Khawaja <[email protected]>

---
 drivers/tty/tty_io.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++----
 include/linux/tty.h  |    4 +++
 2 files changed, 65 insertions(+), 5 deletions(-)

drivers/tty/tty_io.c +61 -5

@@ -280,7 +280,7 @@ static int check_tty_count(struct tty_st
 {
 #ifdef CHECK_TTY_COUNT
 	struct list_head *p;
-	int count = 0;
+	int count = 0, kopen_count = 0;
 
 	spin_lock(&tty->files_lock);
 	list_for_each(p, &tty->tty_files) {

@@ -291,10 +291,12 @@ static int check_tty_count(struct tty_st
 	    tty->driver->subtype == PTY_TYPE_SLAVE &&
 	    tty->link && tty->link->count)
 		count++;
-	if (tty->count != count) {
-		tty_warn(tty, "%s: tty->count(%d) != #fd's(%d)\n",
-			 routine, tty->count, count);
-		return count;
+	if (test_bit(TTY_KOPENED, &tty->flags))
+		kopen_count++;
+	if (tty->count != (count + kopen_count)) {
+		tty_warn(tty, "%s: tty->count(%d) != (#fd's(%d) + #kopen's(%d)\n",
+			 routine, tty->count, count, kopen_count);
+		return (count + kopen_count);
 	}
 #endif
 	return 0;

@@ -1786,6 +1788,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 +1874,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() */

include/linux/tty.h +4 -0

@@ -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 ERR_PTR(-ENODEV); }
 static inline int tty_dev_name_to_number(const char *name, dev_t *number)
 { return -ENOTSUPP; }
 #endif

Okash Khawaja

author

2017-07-17 21:08 UTC #35 lore raw

PATCH v2 2/3 staging: speakup: use tty_kopen instead of tty_open_by_driver

In reply to Okash Khawaja

This patch replaces call to tty_open_by_driver with a tty_kopen.

Signed-off-by Okash Khawaja <[email protected]>

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

drivers/staging/speakup/spk_ttyio.c +1 -1

@@ -158,7 +158,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);

Okash Khawaja

author

2017-07-17 21:08 UTC #36 lore raw

PATCH v2 3/3 tty: undo export of tty_open_by_driver

In reply to Okash Khawaja

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 <[email protected]>

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

drivers/tty/tty_io.c +1 -2

@@ -1851,7 +1851,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;

@@ -1902,7 +1902,6 @@ out:
 	tty_driver_kref_put(driver);
 	return tty;
 }
-EXPORT_SYMBOL_GPL(tty_open_by_driver);
 
 /**
  *	tty_open		-	open a tty device

include/linux/tty.h +0 -5

@@ -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 ERR_PTR(-ENODEV); }
 static inline int tty_dev_name_to_number(const char *name, dev_t *number)