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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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(_mutex); > + driver = tty_lookup_driver(device, NULL, ); > + if (IS_ERR(driver)) { > + mutex_unlock(_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(_mutex); > + tty_driver_kref_put(driver); > + return tty; > +} > +EXPORT_SYMBOL_GPL(tty_kopen); regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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(>files_lock); list_for_each(p, >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 _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(_mutex); + driver = tty_lookup_driver(device, NULL, ); + if (IS_ERR(driver)) { + mutex_unlock(_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(_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(_mutex); + tty = ERR_PTR(-EBUSY); + goto out; + } mutex_unlock(_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
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device export
> On 17 Jul 2017, at 13:31, Greg Kroah-Hartman> 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device export
On Mon, Jul 10, 2017 at 01:33:07PM +0100, Okash Khawaja wrote: > > 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device export
On Mon, Jul 10, 2017 at 12:52:33PM +0100, Alan Cox wrote: > On Sun, 09 Jul 2017 12:41:53 +0100 > Okash Khawajawrote: > > > On Sat, Jul 08, 2017 at 10:38:03AM +0200, Greg Kroah-Hartman wrote: > > > 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. > > 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? > > 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device export
On Sun, 09 Jul 2017 12:41:53 +0100 Okash Khawajawrote: > On Sat, Jul 08, 2017 at 10:38:03AM +0200, Greg Kroah-Hartman wrote: > > 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. 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [patch 0/3] Re: tty contention resulting from tty_open_by_device export
On Sun, Jul 09, 2017 at 12:41:53PM +0100, Okash Khawaja wrote: > On Sat, Jul 08, 2017 at 10:38:03AM +0200, Greg Kroah-Hartman wrote: > > 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 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel