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

[Okash Khawaja <okash.khawaja@gmail.com>]

On Mon, Jul 17, 2017 at 11:04:38PM +0100, Alan Cox wrote:
> 
> > 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.

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

--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -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() */
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -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);