There are many WARNINGs like the following reported nowadays:
WARNING: at drivers/tty/tty_io.c:1331 tty_open+0x2a2/0x49a()
Hardware name: Latitude E6500
Modules linked in:
Pid: 1207, comm: plymouthd Not tainted 2.6.37-rc3-mmotm1123 #3
Call Trace:
[<ffffffff8103b189>] warn_slowpath_common+0x80/0x98
[<ffffffff8103b1b6>] warn_slowpath_null+0x15/0x17
[<ffffffff8128a3ab>] tty_open+0x2a2/0x49a
[<ffffffff810fd53f>] chrdev_open+0x11d/0x146
...
This means tty_reopen is called without TTY_LDISC set. For further
considerations, note tty_lock is held in tty_open. TTY_LDISC is cleared in:
1) __tty_hangup from tty_ldisc_hangup to tty_ldisc_enable. During this
section tty_lock is held. However tty_lock is temporarily dropped in
the middle of the function by tty_ldisc_hangup.
2) tty_release via tty_ldisc_release till the end of tty existence. If
tty->count <= 1, tty_lock is taken, TTY_CLOSING bit set and then
tty_ldisc_release called. tty_reopen checks TTY_CLOSING before checking
TTY_LDISC.
3) tty_set_ldisc from tty_ldisc_halt to tty_ldisc_enable. We:
* take tty_lock, set TTY_LDISC_CHANGING, put tty_lock
* call tty_ldisc_halt (clear TTY_LDISC), tty_lock is _not_ held
* do some other work
* take tty_lock, call tty_ldisc_enable (set TTY_LDISC), put
tty_lock
I cannot see how 2) can be a problem, as there I see no race. OTOH, 1)
and 3) can happen without problems. This patch the case 3) by checking
TTY_LDISC_CHANGING along with TTY_CLOSING in tty_reopen. 1) will be
fixed in the following patch.
Nicely reproducible with two processes:
while (1) {
fd = open("/dev/ttyS1", O_RDWR);
if (fd < 0) {
warn("open");
continue;
}
close(fd);
}
--------
while (1) {
fd = open("/dev/ttyS1", O_RDWR);
ld1 = 0; ld2 = 2;
while (1) {
ioctl(fd, TIOCSETD, &ld1);
ioctl(fd, TIOCSETD, &ld2);
}
close(fd);
}
Signed-off-by: Jiri Slaby <[email protected]>
Reported-by: <[email protected]>
Cc: Kyle McMartin <[email protected]>
Cc: Alan Cox <[email protected]>
---
drivers/tty/tty_io.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index c05c5af..878f6d6 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1310,7 +1310,8 @@ static int tty_reopen(struct tty_struct *tty)
{
struct tty_driver *driver = tty->driver;
- if (test_bit(TTY_CLOSING, &tty->flags))
+ if (test_bit(TTY_CLOSING, &tty->flags) ||
+ test_bit(TTY_LDISC_CHANGING, &tty->flags))
return -EIO;
if (driver->type == TTY_DRIVER_TYPE_PTY &&
--
1.7.3.1
Like in the "TTY: don't allow reopen when ldisc is changing" patch,
this one fixes a TTY WARNING as described in the option 1) there:
1) __tty_hangup from tty_ldisc_hangup to tty_ldisc_enable. During this
section tty_lock is held. However tty_lock is temporarily dropped in
the middle of the function by tty_ldisc_hangup.
The fix is to introduce a new flag which we set during the unlocked
window and check it in tty_reopen too. The flag is TTY_HUPPING and is
cleared after TTY_HUPPED is set.
While at it, remove duplicate TTY_HUPPED set_bit. The one after
calling ops->hangup seems to be more correct. But anyway, we hold
tty_lock, so there should be no difference.
Also document the function it does that kind of crap.
Nicely reproducible with two forked children:
static void do_work(const char *tty)
{
if (signal(SIGHUP, SIG_IGN) == SIG_ERR) exit(1);
setsid();
while (1) {
int fd = open(tty, O_RDWR|O_NOCTTY);
if (fd < 0) continue;
if (ioctl(fd, TIOCSCTTY)) continue;
if (vhangup()) continue;
close(fd);
}
exit(0);
}
Signed-off-by: Jiri Slaby <[email protected]>
Reported-by: <[email protected]>
Reported-by: Kyle McMartin <[email protected]>
Cc: Alan Cox <[email protected]>
---
I don't see a better way how to fix it, so I promoted the
proof-of-concept to a real patch.
drivers/tty/tty_io.c | 10 +++++++++-
include/linux/tty.h | 1 +
2 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 878f6d6..35480dd 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -559,6 +559,9 @@ void __tty_hangup(struct tty_struct *tty)
tty_lock();
+ /* some functions below drop BTM, so we need this bit */
+ set_bit(TTY_HUPPING, &tty->flags);
+
/* inuse_filps is protected by the single tty lock,
this really needs to change if we want to flush the
workqueue with the lock held */
@@ -578,6 +581,10 @@ void __tty_hangup(struct tty_struct *tty)
}
spin_unlock(&tty_files_lock);
+ /*
+ * it drops BTM and thus races with reopen
+ * we protect the race by TTY_HUPPING
+ */
tty_ldisc_hangup(tty);
read_lock(&tasklist_lock);
@@ -615,7 +622,6 @@ void __tty_hangup(struct tty_struct *tty)
tty->session = NULL;
tty->pgrp = NULL;
tty->ctrl_status = 0;
- set_bit(TTY_HUPPED, &tty->flags);
spin_unlock_irqrestore(&tty->ctrl_lock, flags);
/* Account for the p->signal references we killed */
@@ -641,6 +647,7 @@ void __tty_hangup(struct tty_struct *tty)
* can't yet guarantee all that.
*/
set_bit(TTY_HUPPED, &tty->flags);
+ clear_bit(TTY_HUPPING, &tty->flags);
tty_ldisc_enable(tty);
tty_unlock();
@@ -1311,6 +1318,7 @@ static int tty_reopen(struct tty_struct *tty)
struct tty_driver *driver = tty->driver;
if (test_bit(TTY_CLOSING, &tty->flags) ||
+ test_bit(TTY_HUPPING, &tty->flags) ||
test_bit(TTY_LDISC_CHANGING, &tty->flags))
return -EIO;
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 032d79f..54e4eaa 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -366,6 +366,7 @@ struct tty_file_private {
#define TTY_HUPPED 18 /* Post driver->hangup() */
#define TTY_FLUSHING 19 /* Flushing to ldisc in progress */
#define TTY_FLUSHPENDING 20 /* Queued buffer flush pending */
+#define TTY_HUPPING 21 /* ->hangup() in progress */
#define TTY_WRITE_FLUSH(tty) tty_write_flush((tty))
--
1.7.3.1
On Mon, Nov 29, 2010 at 10:16:53AM +0100, Jiri Slaby wrote:
> There are many WARNINGs like the following reported nowadays:
> WARNING: at drivers/tty/tty_io.c:1331 tty_open+0x2a2/0x49a()
> Hardware name: Latitude E6500
> Modules linked in:
> Pid: 1207, comm: plymouthd Not tainted 2.6.37-rc3-mmotm1123 #3
> Call Trace:
> [<ffffffff8103b189>] warn_slowpath_common+0x80/0x98
> [<ffffffff8103b1b6>] warn_slowpath_null+0x15/0x17
> [<ffffffff8128a3ab>] tty_open+0x2a2/0x49a
> [<ffffffff810fd53f>] chrdev_open+0x11d/0x146
> ...
>
Are these two patches also for the .36 kernel? Kyle, you were seeing
the warnings show up there, right?
thanks,
greg k-h
On Mon, Nov 29, 2010 at 01:48:37PM -0800, Greg KH wrote:
> > [<ffffffff8103b189>] warn_slowpath_common+0x80/0x98
> > [<ffffffff8103b1b6>] warn_slowpath_null+0x15/0x17
> > [<ffffffff8128a3ab>] tty_open+0x2a2/0x49a
> > [<ffffffff810fd53f>] chrdev_open+0x11d/0x146
>
> Are these two patches also for the .36 kernel? Kyle, you were seeing
> the warnings show up there, right?
>
Yup, I guess they turned up when the BKL went away. (I reverted the BKL
removal patches on top of 2.6.36, and never saw this WARN_ON trigger at
least.)
--Kyle
On Mon, Nov 29, 2010 at 05:23:42PM -0500, Kyle McMartin wrote:
> On Mon, Nov 29, 2010 at 01:48:37PM -0800, Greg KH wrote:
> > > [<ffffffff8103b189>] warn_slowpath_common+0x80/0x98
> > > [<ffffffff8103b1b6>] warn_slowpath_null+0x15/0x17
> > > [<ffffffff8128a3ab>] tty_open+0x2a2/0x49a
> > > [<ffffffff810fd53f>] chrdev_open+0x11d/0x146
> >
> > Are these two patches also for the .36 kernel? Kyle, you were seeing
> > the warnings show up there, right?
> >
>
> Yup, I guess they turned up when the BKL went away. (I reverted the BKL
> removal patches on top of 2.6.36, and never saw this WARN_ON trigger at
> least.)
Ok, thanks, that helps.
greg k-h