lkml.org 
[lkml]   [2017]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [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);
    \
     
     \ /
      Last update: 2017-07-18 13:30    [W:3.384 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site