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
                      Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device  export Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-18 13:40 +0200
                        Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device  export Dan Carpenter <dan.carpenter@oracle.com> - 2017-07-18 14:30 +0200
                          Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device  export Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-18 21: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 16:00 +0200
                          [patch v3 3/3] tty: undo export of tty_open_by_driver Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-20 09:30 +0200
                          [patch v3 1/3] tty: resolve tty contention between kernel and user space Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-20 09:30 +0200
                          [patch v3 0/3] tty contention resulting from tty_open_by_driver export Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-20 09:30 +0200
                            [patch v3 2/3] staging: speakup: use tty_kopen and tty_kclose Okash Khawaja <okash.khawaja@gmail.com> - 2017-07-20 09:40 +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 2 of 2 — ← Prev page 1 [2]

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

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

> 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

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

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

On Wed, Jul 12, 2017 at 07:20:28PM +0100, Alan Cox wrote:
> 
> > 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.

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

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

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

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

On Thu, Jul 13, 2017 at 12:29:54PM +0100, Okash Khawaja wrote:
> On Wed, Jul 12, 2017 at 07:20:28PM +0100, Alan Cox wrote:
> > 
> > > 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.
> 
> 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

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

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

> On 17 Jul 2017, at 13:31, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
>> On Thu, Jul 13, 2017 at 12:29:54PM +0100, Okash Khawaja wrote:
>>> On Wed, Jul 12, 2017 at 07:20:28PM +0100, Alan Cox wrote:
>>> 
>>>> 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.
>> 
>> 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

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

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

> 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

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

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

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

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

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

On Tue, Jul 18, 2017 at 12:29:52PM +0100, Okash Khawaja 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);
> +       }
> +
> +       /* 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);
                                       ^^^^^^^^^

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

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

regards,
dan carpenter

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

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

On Tue, Jul 18, 2017 at 03:26:37PM +0300, Dan Carpenter wrote:
> > +       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

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

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

> 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

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

#1692489 — [patch v3 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
@@ -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
--- 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 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)

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

#1692490 — [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.

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 |  100 ++++++++++++++++++++++++++++++++++++++++++++++++---
 include/linux/tty.h  |   21 ++++++++++
 2 files changed, 116 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;
@@ -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() */
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -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);

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

#1692497 — [patch v3 0/3] 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

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

#1692512 — [patch v3 2/3] staging: speakup: use tty_kopen and tty_kclose

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 <okash.khawaja@gmail.com>

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

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

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

#1689471 — [patch v2 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.

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. 

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 |   66 +++++++++++++++++++++++++++++++++++++++++++++++----
 include/linux/tty.h  |    4 +++
 2 files changed, 65 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 (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() */
--- 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 ERR_PTR(-ENODEV); }
 static inline int tty_dev_name_to_number(const char *name, dev_t *number)
 { return -ENOTSUPP; }
 #endif

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

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

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

#1689479 — [patch v2 0/3] tty contention resulting from tty_open_by_driver export

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

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

#1689484 — [patch v2 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
@@ -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);
 

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web