The commit 12e84c71b7d4ee (tty: export tty_open_by_driver) exports
tty_open_by_device to allow tty to be opened from inside kernel which
works fine except that it doesn't handle contention with user space or
another kernel-space open of the same tty. For example, opening a tty
from user space while it is kernel opened results in failure and a
kernel log message about mismatch between tty->count and tty's file
open count.
This patch makes kernel access to tty exclusive, so that if a user
process or kernel opens a kernel opened tty, it gets -EBUSY. It does
this by adding TTY_KOPENED flag to tty->flags. When this flag is set,
tty_open_by_driver returns -EBUSY. Instead of overlaoding
tty_open_by_driver for both kernel and user space, this
patch creates a separate function tty_kopen which closely follows
tty_open_by_driver.
Returning -EBUSY on tty open is a change in the interface. I have
tested this with minicom, picocom and commands like "echo foo >
/dev/ttyS0". They all correctly report "Device or resource busy" when
the tty is already kernel opened.
Signed-off-by: Okash Khawaja <[email protected]>
---
drivers/tty/tty_io.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/tty.h | 4 +++
2 files changed, 58 insertions(+)
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1786,6 +1786,54 @@ static struct tty_driver *tty_lookup_dri
}
/**
+ * tty_kopen - open a tty device for kernel
+ * @device: dev_t of device to open
+ *
+ * Opens tty exclusively for kernel. Performs the driver lookup,
+ * makes sure it's not already opened and performs the first-time
+ * tty initialization.
+ *
+ * Returns the locked initialized &tty_struct
+ *
+ * Claims the global tty_mutex to serialize:
+ * - concurrent first-time tty initialization
+ * - concurrent tty driver removal w/ lookup
+ * - concurrent tty removal from driver table
+ */
+struct tty_struct *tty_kopen(dev_t device)
+{
+ struct tty_struct *tty;
+ struct tty_driver *driver = NULL;
+ int index = -1;
+
+ mutex_lock(&tty_mutex);
+ driver = tty_lookup_driver(device, NULL, &index);
+ if (IS_ERR(driver)) {
+ mutex_unlock(&tty_mutex);
+ return ERR_CAST(driver);
+ }
+
+ /* check whether we're reopening an existing tty */
+ tty = tty_driver_lookup_tty(driver, NULL, index);
+ if (IS_ERR(tty))
+ goto out;
+
+ if (tty) {
+ /* drop kref from tty_driver_lookup_tty() */
+ tty_kref_put(tty);
+ tty = ERR_PTR(-EBUSY);
+ } else { /* tty_init_dev returns tty with the tty_lock held */
+ tty = tty_init_dev(driver, index);
+ set_bit(TTY_KOPENED, &tty->flags);
+ }
+out:
+ mutex_unlock(&tty_mutex);
+ tty_driver_kref_put(driver);
+ return tty;
+}
+EXPORT_SYMBOL_GPL(tty_kopen);
+
+/**
* tty_open_by_driver - open a tty device
* @device: dev_t of device to open
* @inode: inode of device file
@@ -1824,6 +1872,12 @@ struct tty_struct *tty_open_by_driver(de
}
if (tty) {
+ if (test_bit(TTY_KOPENED, &tty->flags)) {
+ tty_kref_put(tty);
+ mutex_unlock(&tty_mutex);
+ tty = ERR_PTR(-EBUSY);
+ goto out;
+ }
mutex_unlock(&tty_mutex);
retval = tty_lock_interruptible(tty);
tty_kref_put(tty); /* drop kref from tty_driver_lookup_tty() */
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -362,6 +362,7 @@ struct tty_file_private {
#define TTY_NO_WRITE_SPLIT 17 /* Preserve write boundaries to driver */
#define TTY_HUPPED 18 /* Post driver->hangup() */
#define TTY_LDISC_HALTED 22 /* Line discipline is halted */
+#define TTY_KOPENED 23 /* TTY exclusively opened by kernel */
/* Values for tty->flow_change */
#define TTY_THROTTLE_SAFE 1
@@ -401,6 +402,7 @@ extern int __init tty_init(void);
extern const char *tty_name(const struct tty_struct *tty);
extern struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
struct file *filp);
+extern struct tty_struct *tty_kopen(dev_t device);
extern int tty_dev_name_to_number(const char *name, dev_t *number);
#else
static inline void tty_kref_put(struct tty_struct *tty)
@@ -425,6 +427,8 @@ static inline const char *tty_name(const
static inline struct tty_struct *tty_open_by_driver(dev_t device,
struct inode *inode, struct file *filp)
{ return NULL; }
+static inline struct tty_struct *tty_kopen(dev_t device)
+{ return NULL; }
static inline int tty_dev_name_to_number(const char *name, dev_t *number)
{ return -ENOTSUPP; }
#endif
On Sun, Jul 09, 2017 at 12:41:54PM +0100, Okash Khawaja wrote:
> The commit 12e84c71b7d4ee (tty: export tty_open_by_driver) exports
> tty_open_by_device to allow tty to be opened from inside kernel which
> works fine except that it doesn't handle contention with user space or
> another kernel-space open of the same tty. For example, opening a tty
> from user space while it is kernel opened results in failure and a
> kernel log message about mismatch between tty->count and tty's file
> open count.
>
> This patch makes kernel access to tty exclusive, so that if a user
> process or kernel opens a kernel opened tty, it gets -EBUSY. It does
> this by adding TTY_KOPENED flag to tty->flags. When this flag is set,
> tty_open_by_driver returns -EBUSY. Instead of overlaoding
> tty_open_by_driver for both kernel and user space, this
> patch creates a separate function tty_kopen which closely follows
> tty_open_by_driver.
>
> Returning -EBUSY on tty open is a change in the interface. I have
> tested this with minicom, picocom and commands like "echo foo >
> /dev/ttyS0". They all correctly report "Device or resource busy" when
> the tty is already kernel opened.
>
> Signed-off-by: Okash Khawaja <[email protected]>
>
> ---
> drivers/tty/tty_io.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/tty.h | 4 +++
> 2 files changed, 58 insertions(+)
>
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -1786,6 +1786,54 @@ static struct tty_driver *tty_lookup_dri
> }
>
> /**
> + * tty_kopen - open a tty device for kernel
> + * @device: dev_t of device to open
> + *
> + * Opens tty exclusively for kernel. Performs the driver lookup,
> + * makes sure it's not already opened and performs the first-time
> + * tty initialization.
> + *
> + * Returns the locked initialized &tty_struct
> + *
> + * Claims the global tty_mutex to serialize:
> + * - concurrent first-time tty initialization
> + * - concurrent tty driver removal w/ lookup
> + * - concurrent tty removal from driver table
> + */
> +struct tty_struct *tty_kopen(dev_t device)
> +{
> + struct tty_struct *tty;
> + struct tty_driver *driver = NULL;
> + int index = -1;
> +
> + mutex_lock(&tty_mutex);
> + driver = tty_lookup_driver(device, NULL, &index);
> + if (IS_ERR(driver)) {
> + mutex_unlock(&tty_mutex);
> + return ERR_CAST(driver);
> + }
> +
> + /* check whether we're reopening an existing tty */
> + tty = tty_driver_lookup_tty(driver, NULL, index);
> + if (IS_ERR(tty))
> + goto out;
> +
> + if (tty) {
> + /* drop kref from tty_driver_lookup_tty() */
> + tty_kref_put(tty);
> + tty = ERR_PTR(-EBUSY);
> + } else { /* tty_init_dev returns tty with the tty_lock held */
> + tty = tty_init_dev(driver, index);
> + set_bit(TTY_KOPENED, &tty->flags);
> + }
> +out:
> + mutex_unlock(&tty_mutex);
> + tty_driver_kref_put(driver);
> + return tty;
> +}
> +EXPORT_SYMBOL_GPL(tty_kopen);
> +
> +/**
> * tty_open_by_driver - open a tty device
> * @device: dev_t of device to open
> * @inode: inode of device file
> @@ -1824,6 +1872,12 @@ struct tty_struct *tty_open_by_driver(de
> }
>
> if (tty) {
> + if (test_bit(TTY_KOPENED, &tty->flags)) {
> + tty_kref_put(tty);
> + mutex_unlock(&tty_mutex);
> + tty = ERR_PTR(-EBUSY);
> + goto out;
> + }
> mutex_unlock(&tty_mutex);
> retval = tty_lock_interruptible(tty);
> tty_kref_put(tty); /* drop kref from tty_driver_lookup_tty() */
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -362,6 +362,7 @@ struct tty_file_private {
> #define TTY_NO_WRITE_SPLIT 17 /* Preserve write boundaries to driver */
> #define TTY_HUPPED 18 /* Post driver->hangup() */
> #define TTY_LDISC_HALTED 22 /* Line discipline is halted */
> +#define TTY_KOPENED 23 /* TTY exclusively opened by kernel */
>
> /* Values for tty->flow_change */
> #define TTY_THROTTLE_SAFE 1
> @@ -401,6 +402,7 @@ extern int __init tty_init(void);
> extern const char *tty_name(const struct tty_struct *tty);
> extern struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
> struct file *filp);
> +extern struct tty_struct *tty_kopen(dev_t device);
> extern int tty_dev_name_to_number(const char *name, dev_t *number);
> #else
> static inline void tty_kref_put(struct tty_struct *tty)
> @@ -425,6 +427,8 @@ static inline const char *tty_name(const
> static inline struct tty_struct *tty_open_by_driver(dev_t device,
> struct inode *inode, struct file *filp)
> { return NULL; }
> +static inline struct tty_struct *tty_kopen(dev_t device)
> +{ return NULL; }
Like I said in response to patch 2, maybe this should return an error?
thanks,
greg k-h
On Sun, Jul 9, 2017 at 2:41 PM, Okash Khawaja <[email protected]> wrote:
> +struct tty_struct *tty_kopen(dev_t device)
> +{
> + struct tty_struct *tty;
> + struct tty_driver *driver = NULL;
> + int index = -1;
> +
> + mutex_lock(&tty_mutex);
> + driver = tty_lookup_driver(device, NULL, &index);
> + if (IS_ERR(driver)) {
> + mutex_unlock(&tty_mutex);
> + return ERR_CAST(driver);
Hmm... perhaps
tty = ERR_CAST(driver);
goto out_unlock;
See below for further details.
> + }
> +
> + /* check whether we're reopening an existing tty */
> + tty = tty_driver_lookup_tty(driver, NULL, index);
> + if (IS_ERR(tty))
> + goto out;
> +
> + if (tty) {
> + /* drop kref from tty_driver_lookup_tty() */
> + tty_kref_put(tty);
> + tty = ERR_PTR(-EBUSY);
> + } else { /* tty_init_dev returns tty with the tty_lock held */
> + tty = tty_init_dev(driver, index);
> + set_bit(TTY_KOPENED, &tty->flags);
> + }
> +out:
out_unlock: ?
> + mutex_unlock(&tty_mutex);
> + tty_driver_kref_put(driver);
I'm not sure I understand locking model here:
Above
1. take mutex
2. take reference
Here:
1. give mutex back
2. give reference back
I think we usually see symmetrical calls, i.e.
1. give reference back
2. give mutex back
So, what did I miss?
> + return tty;
> +}
--
With Best Regards,
Andy Shevchenko
Hi,
On Sun, Jul 09, 2017 at 06:04:17PM +0300, Andy Shevchenko wrote:
> Above
> 1. take mutex
> 2. take reference
>
> Here:
> 1. give mutex back
> 2. give reference back
>
> I think we usually see symmetrical calls, i.e.
>
> 1. give reference back
> 2. give mutex back
>
> So, what did I miss?
After we hold the kref, driver won't disappear from underneath us so we
don't need tty_mutex just for decrementing the refcount.
Thanks,
Okash
On Sun, Jul 09, 2017 at 06:04:17PM +0300, Andy Shevchenko wrote:
> On Sun, Jul 9, 2017 at 2:41 PM, Okash Khawaja <[email protected]> wrote:
>
> > +struct tty_struct *tty_kopen(dev_t device)
> > +{
> > + struct tty_struct *tty;
> > + struct tty_driver *driver = NULL;
> > + int index = -1;
> > +
> > + mutex_lock(&tty_mutex);
> > + driver = tty_lookup_driver(device, NULL, &index);
> > + if (IS_ERR(driver)) {
>
> > + mutex_unlock(&tty_mutex);
> > + return ERR_CAST(driver);
>
> Hmm... perhaps
>
> tty = ERR_CAST(driver);
> goto out_unlock;
>
> See below for further details.
>
Sorry missed this one out. Since tty_lookup_driver has failed, we don't
need to down the refcount on driver. So we can return here, without
going to out_unlock.
Thanks,
Okash
On Mon, Jul 10, 2017 at 11:31 AM, Okash Khawaja <[email protected]> wrote:
> On Sun, Jul 09, 2017 at 06:04:17PM +0300, Andy Shevchenko wrote:
>> On Sun, Jul 9, 2017 at 2:41 PM, Okash Khawaja <[email protected]> wrote:
>>
>> > +struct tty_struct *tty_kopen(dev_t device)
>> > +{
>> > + struct tty_struct *tty;
>> > + struct tty_driver *driver = NULL;
>> > + int index = -1;
>> > +
>> > + mutex_lock(&tty_mutex);
>> > + driver = tty_lookup_driver(device, NULL, &index);
>> > + if (IS_ERR(driver)) {
>>
>> > + mutex_unlock(&tty_mutex);
>> > + return ERR_CAST(driver);
>>
>> Hmm... perhaps
>>
>> tty = ERR_CAST(driver);
>> goto out_unlock;
>>
>> See below for further details.
>>
> Sorry missed this one out. Since tty_lookup_driver has failed, we don't
> need to down the refcount on driver. So we can return here, without
> going to out_unlock.
Yeah, and my point is to use goto with the symmetric giveups of lock
and reference.
--
With Best Regards,
Andy Shevchenko
On Mon, Jul 10, 2017 at 06:21:37PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 10, 2017 at 11:31 AM, Okash Khawaja <[email protected]> wrote:
> > On Sun, Jul 09, 2017 at 06:04:17PM +0300, Andy Shevchenko wrote:
> >> On Sun, Jul 9, 2017 at 2:41 PM, Okash Khawaja <[email protected]> wrote:
> >>
> >> > +struct tty_struct *tty_kopen(dev_t device)
> >> > +{
> >> > + struct tty_struct *tty;
> >> > + struct tty_driver *driver = NULL;
> >> > + int index = -1;
> >> > +
> >> > + mutex_lock(&tty_mutex);
> >> > + driver = tty_lookup_driver(device, NULL, &index);
> >> > + if (IS_ERR(driver)) {
> >>
> >> > + mutex_unlock(&tty_mutex);
> >> > + return ERR_CAST(driver);
> >>
> >> Hmm... perhaps
> >>
> >> tty = ERR_CAST(driver);
> >> goto out_unlock;
> >>
> >> See below for further details.
> >>
> > Sorry missed this one out. Since tty_lookup_driver has failed, we don't
> > need to down the refcount on driver. So we can return here, without
> > going to out_unlock.
>
> Yeah, and my point is to use goto with the symmetric giveups of lock
> and reference.
Ah okay I see your point. Sure, I don't mind either way. However, this
code closely follows tty_open_by_driver, so I have tried to keep the
same pattern for sake of consistency :)
Thanks,
Okash