Mailing List Archive: Linux: Kernel

[PATCH] tty: add lockdep annotations

1 2

View All

Index | Next | Previous | View Threaded

eric.dumazet at gmail

May 31, 2012, 2:35 AM

Post #1 of 27 (149 views)
Permalink

[PATCH] tty: add lockdep annotations

From: Eric Dumazet <edumazet [at] google>

tty_lock_pair() do the right thing to avoid deadlocks, but should
instruct LOCKDEP of this to avoid a splat.

Signed-off-by: Eric Dumazet <edumazet [at] google>
---
Latest Linus tree still hang on my machine, at least this patch removes
the lockdep problem...

drivers/tty/tty_mutex.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
index 69adc80..67feac9 100644
--- a/drivers/tty/tty_mutex.c
+++ b/drivers/tty/tty_mutex.c
@@ -6,11 +6,17 @@

/* Legacy tty mutex glue */

+enum {
+ TTY_MUTEX_NORMAL,
+ TTY_MUTEX_NESTED,
+};
+
/*
* Getting the big tty mutex.
*/

-void __lockfunc tty_lock(struct tty_struct *tty)
+static void __lockfunc tty_lock_nested(struct tty_struct *tty,
+ unsigned int subclass)
{
if (tty->magic != TTY_MAGIC) {
printk(KERN_ERR "L Bad %p\n", tty);
@@ -18,7 +24,12 @@ void __lockfunc tty_lock(struct tty_struct *tty)
return;
}
tty_kref_get(tty);
- mutex_lock(&tty->legacy_mutex);
+ mutex_lock_nested(&tty->legacy_mutex, subclass);
+}
+
+void __lockfunc tty_lock(struct tty_struct *tty)
+{
+ return tty_lock_nested(tty, TTY_MUTEX_NORMAL);
}
EXPORT_SYMBOL(tty_lock);

@@ -43,11 +54,11 @@ void __lockfunc tty_lock_pair(struct tty_struct *tty,
{
if (tty < tty2) {
tty_lock(tty);
- tty_lock(tty2);
+ tty_lock_nested(tty2, TTY_MUTEX_NESTED);
} else {
if (tty2 && tty2 != tty)
tty_lock(tty2);
- tty_lock(tty);
+ tty_lock_nested(tty, TTY_MUTEX_NESTED);
}
}
EXPORT_SYMBOL(tty_lock_pair);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

jkosina at suse

May 31, 2012, 4:45 AM

Post #2 of 27 (152 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

On Thu, 31 May 2012, Eric Dumazet wrote:

> From: Eric Dumazet <edumazet [at] google>
>
> tty_lock_pair() do the right thing to avoid deadlocks, but should
> instruct LOCKDEP of this to avoid a splat.

I have submitted very similar patch to Greg/Alan already 3 days ago[1],
but have been told that this is already fixed. Apparently it wasn't?

[1] https://lkml.org/lkml/2012/5/28/226

>
> Signed-off-by: Eric Dumazet <edumazet [at] google>
> ---
> Latest Linus tree still hang on my machine, at least this patch removes
> the lockdep problem...
>
> drivers/tty/tty_mutex.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
> index 69adc80..67feac9 100644
> --- a/drivers/tty/tty_mutex.c
> +++ b/drivers/tty/tty_mutex.c
> @@ -6,11 +6,17 @@
>
> /* Legacy tty mutex glue */
>
> +enum {
> + TTY_MUTEX_NORMAL,
> + TTY_MUTEX_NESTED,
> +};
> +
> /*
> * Getting the big tty mutex.
> */
>
> -void __lockfunc tty_lock(struct tty_struct *tty)
> +static void __lockfunc tty_lock_nested(struct tty_struct *tty,
> + unsigned int subclass)
> {
> if (tty->magic != TTY_MAGIC) {
> printk(KERN_ERR "L Bad %p\n", tty);
> @@ -18,7 +24,12 @@ void __lockfunc tty_lock(struct tty_struct *tty)
> return;
> }
> tty_kref_get(tty);
> - mutex_lock(&tty->legacy_mutex);
> + mutex_lock_nested(&tty->legacy_mutex, subclass);
> +}
> +
> +void __lockfunc tty_lock(struct tty_struct *tty)
> +{
> + return tty_lock_nested(tty, TTY_MUTEX_NORMAL);
> }
> EXPORT_SYMBOL(tty_lock);
>
> @@ -43,11 +54,11 @@ void __lockfunc tty_lock_pair(struct tty_struct *tty,
> {
> if (tty < tty2) {
> tty_lock(tty);
> - tty_lock(tty2);
> + tty_lock_nested(tty2, TTY_MUTEX_NESTED);
> } else {
> if (tty2 && tty2 != tty)
> tty_lock(tty2);
> - tty_lock(tty);
> + tty_lock_nested(tty, TTY_MUTEX_NESTED);
> }
> }
> EXPORT_SYMBOL(tty_lock_pair);
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo [at] vger
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

--
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

eric.dumazet at gmail

May 31, 2012, 4:59 AM

Post #3 of 27 (150 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

On Thu, 2012-05-31 at 13:45 +0200, Jiri Kosina wrote:
> On Thu, 31 May 2012, Eric Dumazet wrote:
>
> > From: Eric Dumazet <edumazet [at] google>
> >
> > tty_lock_pair() do the right thing to avoid deadlocks, but should
> > instruct LOCKDEP of this to avoid a splat.
>
> I have submitted very similar patch to Greg/Alan already 3 days ago[1],
> but have been told that this is already fixed. Apparently it wasn't?
>
> [1] https://lkml.org/lkml/2012/5/28/226

I saw that right now.

Not sure your patch is the right fix with hard coded 1 and 2 values ?

0 and 1 should be enough.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

eric.dumazet at gmail

Jun 1, 2012, 11:17 AM

Post #4 of 27 (150 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

On Thu, 2012-05-31 at 11:35 +0200, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet [at] google>
>
> tty_lock_pair() do the right thing to avoid deadlocks, but should
> instruct LOCKDEP of this to avoid a splat.
>
> Signed-off-by: Eric Dumazet <edumazet [at] google>
> ---
> Latest Linus tree still hang on my machine, at least this patch removes
> the lockdep problem...

About 10% of boots on my machine, and this looks like (hand written)

general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
...
RIP : tty_shutdown+0x15/)x70

Call Trace:

con_shutdown+0x3e/0x50
queue_release_one_tty+0x24/0x70
tty_kref_put+0x28/0x30
tty_unlock+0x2a/0x60
tty_open+0x4d8/0x620
chrdev_open+0xb1/0x1a0
...

Sometime stack is :
memset()
alloc_tty_struct
pty_unix98_install
tty_init_dev
ptmx_open
chrdev_open

I suspect a refcounting problem, but am not familiar enough with tty
code....

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

torvalds at linux-foundation

Jun 1, 2012, 11:51 AM

Post #5 of 27 (150 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

On Fri, Jun 1, 2012 at 11:17 AM, Eric Dumazet <eric.dumazet [at] gmail> wrote:
>
> About 10% of boots on my machine, and this looks like (hand written)
>
> general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
> ...
> RIP : tty_shutdown+0x15/)x70

Ok, DEBUG_PAGEALLOC and with that offset fairly early in tty_shutdown,
it's almost certainly one of the accesses in the

tty->driver->ops->remove

chain when it does the (inlined)

tty_driver_remove_tty(tty->driver, tty);

you could check which one it is at that offset 0x15, but I think both
the ops and the driver structures should be statically allocated, so I
suspect it's the "tty" itself that is already freed.

Odd. But yes, smells very much like a refcount issue, probably due to
broken locking. Does the problem go away if you revert commits
d29f3ef39be4 ("tty_lock: Localise the lock") and 3af502b96649
("tty_lock: undo the old tty_lock use on the ctty")?

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

alan at linux

Jun 1, 2012, 1:38 PM

Post #6 of 27 (148 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

On Fri, 1 Jun 2012 11:51:13 -0700
Linus Torvalds <torvalds [at] linux-foundation> wrote:

> On Fri, Jun 1, 2012 at 11:17 AM, Eric Dumazet
> <eric.dumazet [at] gmail> wrote:
> >
> > About 10% of boots on my machine, and this looks like (hand written)
> >
> > general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
> > ...
> > RIP : tty_shutdown+0x15/)x70
>
> Ok, DEBUG_PAGEALLOC and with that offset fairly early in tty_shutdown,
> it's almost certainly one of the accesses in the
>
> tty->driver->ops->remove
>
> chain when it does the (inlined)
>
> tty_driver_remove_tty(tty->driver, tty);
>
> you could check which one it is at that offset 0x15, but I think both
> the ops and the driver structures should be statically allocated, so I
> suspect it's the "tty" itself that is already freed.

Might be worth checking tty->magic in queue_release_one_tty and the
entry to release_one_tty

You can also turn on TTY_PARANOIA_CHECK defines which may show more but
can also be mistriggered in a few cases.

What distro setup is this and is it booting to run level 3 or 5. I'm
just wondering which paths you hit. Some of the distros do weird stuff
(notably Fedora is totally broken on the console management) and that
might be hiding a race or two in saner cases where you get more console
hangups.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

eric.dumazet at gmail

Jun 1, 2012, 1:44 PM

Post #7 of 27 (148 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

On Fri, 2012-06-01 at 11:51 -0700, Linus Torvalds wrote:
> On Fri, Jun 1, 2012 at 11:17 AM, Eric Dumazet <eric.dumazet [at] gmail> wrote:
> >
> > About 10% of boots on my machine, and this looks like (hand written)
> >
> > general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC
> > ...
> > RIP : tty_shutdown+0x15/)x70
>
> Ok, DEBUG_PAGEALLOC and with that offset fairly early in tty_shutdown,
> it's almost certainly one of the accesses in the
>
> tty->driver->ops->remove
>

Yes, tty->driver deref is ok (tty points to valid memory), but crash is
on tty->driver->ops (driver points to freed/illegal memory)

using slub_debug=FZPU, I can indeed see RDI=6b6b6b6b6b6b6b6b

Typical use after free...

> chain when it does the (inlined)
>
> tty_driver_remove_tty(tty->driver, tty);
>
> you could check which one it is at that offset 0x15, but I think both
> the ops and the driver structures should be statically allocated, so I
> suspect it's the "tty" itself that is already freed.
>
> Odd. But yes, smells very much like a refcount issue, probably due to
> broken locking. Does the problem go away if you revert commits
> d29f3ef39be4 ("tty_lock: Localise the lock") and 3af502b96649
> ("tty_lock: undo the old tty_lock use on the ctty")?

Tried this but seems not straightforward, and its pretty late here in
France, week end starting ;)

By the way, release_one_tty() uses the following racy code :

tty_driver_kref_put(driver);
module_put(driver->owner);

I would use following patch to make sure bad things cant happen...

Thanks
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 9e930c0..b8faf40 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1479,13 +1479,14 @@ static void release_one_tty(struct work_struct *work)
struct tty_struct *tty =
container_of(work, struct tty_struct, hangup_work);
struct tty_driver *driver = tty->driver;
+ struct module *module = driver->owner;

if (tty->ops->cleanup)
tty->ops->cleanup(tty);

tty->magic = 0;
tty_driver_kref_put(driver);
- module_put(driver->owner);
+ module_put(module);

spin_lock(&tty_files_lock);
list_del_init(&tty->tty_files);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

eric.dumazet at gmail

Jun 1, 2012, 1:46 PM

Post #8 of 27 (148 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

On Fri, 2012-06-01 at 21:38 +0100, Alan Cox wrote:

> Might be worth checking tty->magic in queue_release_one_tty and the
> entry to release_one_tty
>
> You can also turn on TTY_PARANOIA_CHECK defines which may show more but
> can also be mistriggered in a few cases.
>
> What distro setup is this and is it booting to run level 3 or 5. I'm
> just wondering which paths you hit. Some of the distros do weird stuff
> (notably Fedora is totally broken on the console management) and that
> might be hiding a race or two in saner cases where you get more console
> hangups.

Thanks I'll try this.

Distro is Ubuntu 12.04, up2date...

I believe it claims a run level 2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

alan at lxorguk

Jun 1, 2012, 1:56 PM

Post #9 of 27 (149 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

> Yes, tty->driver deref is ok (tty points to valid memory), but crash is
> on tty->driver->ops (driver points to freed/illegal memory)
>
> using slub_debug=FZPU, I can indeed see RDI=6b6b6b6b6b6b6b6b

driver and driver->ops is basically const and it's not what you'd expect
from a tty refcount bug. The driver side puts shouldn't have changed but
I'll take a look over that patch and the error paths closely again just
in case.

It could be that tty->driver is pointing at a valid but bogus location
but again its not something I'd expect.

> By the way, release_one_tty() uses the following racy code :
>
> tty_driver_kref_put(driver);
> module_put(driver->owner);
>
> I would use following patch to make sure bad things cant happen...

Agreed. Although we wouldn't be unloading console or pty so it's alas not
the real cause.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

eric.dumazet at gmail

Jun 1, 2012, 1:59 PM

Post #10 of 27 (150 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

On Fri, 2012-06-01 at 21:56 +0100, Alan Cox wrote:
> > Yes, tty->driver deref is ok (tty points to valid memory), but crash is
> > on tty->driver->ops (driver points to freed/illegal memory)
> >
> > using slub_debug=FZPU, I can indeed see RDI=6b6b6b6b6b6b6b6b
>
> driver and driver->ops is basically const and it's not what you'd expect
> from a tty refcount bug. The driver side puts shouldn't have changed but
> I'll take a look over that patch and the error paths closely again just
> in case.

right

The code looks like :
...
call mcount
mov %rdi,%rbx
mov 0x10(%rdi),%rdi tty->driver
<> mov 0xf8(%rdi),%rax CRASH

So tty was freed an tty->driver contains 6b6b6b6b6b6b6b6b

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

eric.dumazet at gmail

Jun 2, 2012, 12:17 AM

Post #11 of 27 (138 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

On Fri, 2012-06-01 at 22:59 +0200, Eric Dumazet wrote:
> On Fri, 2012-06-01 at 21:56 +0100, Alan Cox wrote:
> > > Yes, tty->driver deref is ok (tty points to valid memory), but crash is
> > > on tty->driver->ops (driver points to freed/illegal memory)
> > >
> > > using slub_debug=FZPU, I can indeed see RDI=6b6b6b6b6b6b6b6b
> >
> > driver and driver->ops is basically const and it's not what you'd expect
> > from a tty refcount bug. The driver side puts shouldn't have changed but
> > I'll take a look over that patch and the error paths closely again just
> > in case.
>
> right
>
> The code looks like :
> ...
> call mcount
> mov %rdi,%rbx
> mov 0x10(%rdi),%rdi tty->driver
> <> mov 0xf8(%rdi),%rax CRASH
>
> So tty was freed an tty->driver contains 6b6b6b6b6b6b6b6b
>
>

Here is the patch I am currently testing (need to boot the machine ~50
times to make sure it is the right fix)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 9e930c0..128a95b 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1479,13 +1479,14 @@ static void release_one_tty(struct work_struct *work)
struct tty_struct *tty =
container_of(work, struct tty_struct, hangup_work);
struct tty_driver *driver = tty->driver;
+ struct module *module = driver->owner;

if (tty->ops->cleanup)
tty->ops->cleanup(tty);

tty->magic = 0;
tty_driver_kref_put(driver);
- module_put(driver->owner);
+ module_put(module);

spin_lock(&tty_files_lock);
list_del_init(&tty->tty_files);
@@ -2005,11 +2006,15 @@ retry_open:
filp->f_op = &tty_fops;
goto retry_open;
}
- tty_unlock(tty);
-

+ /*
+ * Must acquire both mutexes in right order,
+ * and keep a reference on tty, so dont call tty_unlock() !
+ */
+ mutex_unlock(&tty->legacy_mutex);
mutex_lock(&tty_mutex);
- tty_lock(tty);
+ mutex_lock(&tty->legacy_mutex);
+
spin_lock_irq(&current->sighand->siglock);
if (!noctty &&
current->signal->leader &&

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

eric.dumazet at gmail

Jun 2, 2012, 12:55 AM

Post #12 of 27 (137 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

From: Eric Dumazet <edumazet [at] google>

On Sat, 2012-06-02 at 09:17 +0200, Eric Dumazet wrote:

>
> Here is the patch I am currently testing (need to boot the machine ~50
> times to make sure it is the right fix)

Seems good, but am still doing reboots to make sure.

We should add a debugging code to kref to catch poisoned refcounts...

Something like :

[PATCH] kref: detects poisoned refcounts

Catch kref_get() done on freed memory, if slub/slab are poisoning the
freed area.

Signed-off-by: Eric Dumazet <edumazet [at] google>
---
include/linux/kref.h | 2 ++
include/linux/poison.h | 3 +++
2 files changed, 5 insertions(+)

diff --git a/include/linux/kref.h b/include/linux/kref.h
index 9c07dce..d8e90a1 100644
--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -18,6 +18,7 @@
#include <linux/bug.h>
#include <linux/atomic.h>
#include <linux/kernel.h>
+#include <linux/poison.h>

struct kref {
atomic_t refcount;
@@ -39,6 +40,7 @@ static inline void kref_init(struct kref *kref)
static inline void kref_get(struct kref *kref)
{
WARN_ON(!atomic_read(&kref->refcount));
+ WARN_ON(atomic_read(&kref->refcount) == POISON_FREE_INT);
atomic_inc(&kref->refcount);
}

diff --git a/include/linux/poison.h b/include/linux/poison.h
index 2110a81..2947582 100644
--- a/include/linux/poison.h
+++ b/include/linux/poison.h
@@ -48,6 +48,9 @@
#define POISON_FREE 0x6b /* for use-after-free poisoning */
#define POISON_END 0xa5 /* end-byte of poisoning */

+#define POISON_FREE_SHORT ((POISON_FREE << 8) + POISON_FREE)
+#define POISON_FREE_INT ((POISON_FREE_SHORT << 16) + POISON_FREE_SHORT)
+
/********** arch/$ARCH/mm/init.c **********/
#define POISON_FREE_INITMEM 0xcc

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

eric.dumazet at gmail

Jun 2, 2012, 1:01 AM

Post #13 of 27 (137 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

On Sat, 2012-06-02 at 09:55 +0200, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet [at] google>
>
> On Sat, 2012-06-02 at 09:17 +0200, Eric Dumazet wrote:
>
> >
> > Here is the patch I am currently testing (need to boot the machine ~50
> > times to make sure it is the right fix)
>
> Seems good, but am still doing reboots to make sure.

Nope, still get a crash at 15th boot.

I have to run.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

alan at lxorguk

Jun 2, 2012, 4:57 AM

Post #14 of 27 (133 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

On Sat, 02 Jun 2012 10:01:42 +0200
Eric Dumazet <eric.dumazet [at] gmail> wrote:

> On Sat, 2012-06-02 at 09:55 +0200, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet [at] google>
> >
> > On Sat, 2012-06-02 at 09:17 +0200, Eric Dumazet wrote:
> >
> > >
> > > Here is the patch I am currently testing (need to boot the machine ~50
> > > times to make sure it is the right fix)
> >
> > Seems good, but am still doing reboots to make sure.
>
> Nope, still get a crash at 15th boot.
>
> I have to run.

I'll have a look at this ASAP - I think the driver ref counting side is a
red herring but I'll build an Ubuntu 12 setup and try and replicate the
crash once I'm back at work (its a national holiday here Sat-Tue for the
English queens jubilee)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

eric.dumazet at gmail

Jun 2, 2012, 5:30 AM

Post #15 of 27 (133 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

On Sat, 2012-06-02 at 12:57 +0100, Alan Cox wrote:

> I'll have a look at this ASAP - I think the driver ref counting side is a
> red herring but I'll build an Ubuntu 12 setup and try and replicate the
> crash once I'm back at work (its a national holiday here Sat-Tue for the
> English queens jubilee)

With following debugging patch I now have crashes at every boot, so it
might be more easy to find the bug :

stack trace :
tty_lock (BUG_ON in kref_get())
tty_open
chrdev_open
do_dentry_open
nameidata_to_filp

diff --git a/include/linux/kref.h b/include/linux/kref.h
index 9c07dce..49a9acf 100644
--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -18,6 +18,7 @@
#include <linux/bug.h>
#include <linux/atomic.h>
#include <linux/kernel.h>
+#include <linux/poison.h>

struct kref {
atomic_t refcount;
@@ -39,6 +40,7 @@ static inline void kref_init(struct kref *kref)
static inline void kref_get(struct kref *kref)
{
WARN_ON(!atomic_read(&kref->refcount));
+ BUG_ON(atomic_read(&kref->refcount) == POISON_FREE_INT);
atomic_inc(&kref->refcount);
}

@@ -65,7 +67,9 @@ static inline int kref_sub(struct kref *kref, unsigned int count,
{
WARN_ON(release == NULL);

+ BUG_ON(atomic_read(&kref->refcount) == POISON_FREE_INT);
if (atomic_sub_and_test((int) count, &kref->refcount)) {
+ atomic_set(&kref->refcount, POISON_FREE_INT);
release(kref);
return 1;
}
diff --git a/include/linux/poison.h b/include/linux/poison.h
index 2110a81..2947582 100644
--- a/include/linux/poison.h
+++ b/include/linux/poison.h
@@ -48,6 +48,9 @@
#define POISON_FREE 0x6b /* for use-after-free poisoning */
#define POISON_END 0xa5 /* end-byte of poisoning */

+#define POISON_FREE_SHORT ((POISON_FREE << 8) + POISON_FREE)
+#define POISON_FREE_INT ((POISON_FREE_SHORT << 16) + POISON_FREE_SHORT)
+
/********** arch/$ARCH/mm/init.c **********/
#define POISON_FREE_INITMEM 0xcc

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

torvalds at linux-foundation

Jun 2, 2012, 11:38 AM

Post #16 of 27 (133 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

On Sat, Jun 2, 2012 at 5:30 AM, Eric Dumazet <eric.dumazet [at] gmail> wrote:
>
> With following debugging patch I now have crashes at every boot, so it
> might be more easy to find the bug :

I'm seeing *something* odd.

We call tty_shutdown() without holding the tty_mutex as far as I can
tell. Which means that we remove the tty from the tty lists with *no*
serialization with looking them up.

The fact that we lock the tty *after* we have found it is kind of
irrelevant. By then it's too late.

And I think this is the fundamental bug that was introduced in commit
d29f3ef39be4 ("tty_lock: Localise the lock"). We *used* to do
tty_lock() before the lookup - early in tty_open(). And we held it
until the very end of tty_release(), so the tty lock actually
protected the lookup of the tty.

That's simply not true any more. The comments for
tty_driver_remove_tty() say "Locking: tty_mutex for now", but that's
just garbage and wishful thinking. The callers don't actually hold
tty_mutex, and never have. Sure, some of them may do so (there's a lot
of potential callers through tty_kref_put()) but most of them
definitely don't. Look at tty_release(), most of the final refcounts
will be dropped after the mutex_unlock(&tty_mutex) afaik.

I'm pretty sure this is the bug. And there's no way we can make
'tty_mutex' protect every tty_kref_put(). So I think we have two
options:

- revert all the tty locking changes

- make a new global lock that protects just driver->ops->lookup(),
driver->ttys[idx], and driver->ops->remove()

Hmm?

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

torvalds at linux-foundation

Jun 2, 2012, 12:59 PM

Post #17 of 27 (130 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

On Sat, Jun 2, 2012 at 11:38 AM, Linus Torvalds
<torvalds [at] linux-foundation> wrote:
>
> I'm pretty sure this is the bug. And there's no way we can make
> 'tty_mutex' protect every tty_kref_put(). So I think we have two
> options:
>
> �- revert all the tty locking changes
>
> �- make a new global lock that protects just driver->ops->lookup(),
> driver->ttys[idx], and driver->ops->remove()
>
> Hmm?

Ok, here's the second option. THIS PATCH IS TOTALLY UNTESTED!

Basic concepts:

- the tty driver lookup is protected with "tty_lookup_mutex",
similarly to how the "current task mutex" is protected by the signal
lock.

- At lookup, we now always increment the kref (which in the case of
tty_open_current_tty() means that we do *not* drop the kref), and in
the case of a tty driver lookup, we use "atomic_inc_not_zero()" to
make sure that we get a tty that is not on its way out.

I *think* this makes sense. But it has has had absolutely zero
testing. It compiles in my config, and the locking is at least
sensible and well-localized, but maybe I'm missing something.

Comments?

Linus

Attachments:

tty-lookup-locking.patch (3.96 KB)

alan at lxorguk

Jun 2, 2012, 1:00 PM

Post #18 of 27 (129 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

> 'tty_mutex' protect every tty_kref_put(). So I think we have two
> options:
>
> - revert all the tty locking changes
>
> - make a new global lock that protects just driver->ops->lookup(),
> driver->ttys[idx], and driver->ops->remove()

I've been working on the latter as a later step already. It triggers a
whole set of other horrible problems about synchronization between
driver->ttys[] and driver->termios.

Can we just punt the tty localisation patches for a release (the last
console one before it is fine).

This isn't urgent stuff, its gradual cleaning the kernel up stuff and if
we keep it in -next for another cycle while fixing up these cases there
will be no hardware not supported, nobody unable to use their system, no
regressions. Better to get it right first because its ugly and
complicated as hell.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

torvalds at linux-foundation

Jun 2, 2012, 1:01 PM

Post #19 of 27 (133 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

On Sat, Jun 2, 2012 at 12:59 PM, Linus Torvalds
<torvalds [at] linux-foundation> wrote:
>
> I *think* this makes sense. But it has has had absolutely zero
> testing. It compiles in my config, and the locking is at least
> sensible and well-localized, but maybe I'm missing something.

Ok, it boots for me. For whatever that is worth.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

alan at lxorguk

Jun 2, 2012, 1:19 PM

Post #20 of 27 (128 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

> I *think* this makes sense. But it has has had absolutely zero
> testing. It compiles in my config, and the locking is at least
> sensible and well-localized, but maybe I'm missing something.
>
> Comments?

Yep. Been that way 8)

tty_driver_remove_tty is called from tty_shutdown is called from the
final tty_kref_put can be called from IRQ context.

If you move the tty_driver_remove_tty into the queued part of the code
path then driver->termios[] races appear between the destructor and the
next allocation of the same tty.

Really tty_driver_remove_tty should occur at the end of tty_release, and
tty->termios should simply be a struct ktermios (its a fairly small
structure) and copied back to the drivers array on drivers that need it.
That cleans up pty as well.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

torvalds at linux-foundation

Jun 2, 2012, 3:25 PM

Post #21 of 27 (130 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

On Sat, Jun 2, 2012 at 1:19 PM, Alan Cox <alan [at] lxorguk> wrote:
>
> Yep. Been that way 8)
>
> tty_driver_remove_tty is called from tty_shutdown is called from the
> final tty_kref_put can be called from IRQ context.

Hmm. Ok. Looking at it, the ".shutdown" and ".remove" functions are
all very limited, so I suspect we could just make the rule be that the
install/lookup functions are serialized against each other by the
pty_mutex (true today), and then we just add a small spinlock for the
actual driver array insert/lookup.

So my patch could probably be munged to do something like that.

But since I want to close the merge window and release -rc1 today, it
does feel too late to start mucking with fundamental locking things.
So reverting is likely the right thing to do.

Can somebody double-check the attached revert commit? Did I miss some
commit that depended on the tty locking changes?

Eric, does this make things work for you again? When/if we get the
locking sorted out for the tty install/lookup/remove, we can revert
this revert and do the locking fixes on top of that.

Alan, please double-check my revert. I've done an allmodconfig build
with it, and I've looked at the patch and grepped that I didn't miss
any 'tty_[un]lock()' cases, but that's the limit of my
sanity-checking. I did check that this revert means that the main tty
files are now back in the same exact state they were before that
"tty_lock: Localise the lock" commit. I left the per-ldisc waitqueue
change in there, though, that one seemed independent.

Linus

Attachments:

tty-locking-revert.patch (27.3 KB)

alan at lxorguk

Jun 2, 2012, 4:02 PM

Post #22 of 27 (130 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

> Alan, please double-check my revert. I've done an allmodconfig build
> with it, and I've looked at the patch and grepped that I didn't miss
> any 'tty_[un]lock()' cases, but that's the limit of my
> sanity-checking. I did check that this revert means that the main tty
> files are now back in the same exact state they were before that
> "tty_lock: Localise the lock" commit. I left the per-ldisc waitqueue
> change in there, though, that one seemed independent.

per ldisc waitqueue is indeed independent. The rest I'll try and check
but may be Wednesday for a serious check. You could also I guess narrow
the check by leaving it all passing tty in to the lock function but just
using the global lock. Might keep the noise down for the 3.6 work too.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

torvalds at linux-foundation

Jun 2, 2012, 4:14 PM

Post #23 of 27 (130 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

On Sat, Jun 2, 2012 at 4:02 PM, Alan Cox <alan [at] lxorguk> wrote:
>
> per ldisc waitqueue is indeed independent. The rest I'll try and check
> but may be Wednesday for a serious check. You could also I guess narrow
> the check by leaving it all passing tty in to the lock function but just
> using the global lock. Might keep the noise down for the 3.6 work too.

Not really possible. The bug fundamentally comes from how the per-tty
locking locks too late - after the lookup - rather than before.

And fixing that to lock in the right place is not possible with the
"pass in the tty" model. Because we don't know what the tty will be.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

torvalds at linux-foundation

Jun 2, 2012, 6:37 PM

Post #24 of 27 (123 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

On Sat, Jun 2, 2012 at 3:25 PM, Linus Torvalds
<torvalds [at] linux-foundation> wrote:
>
> Hmm. Ok. Looking at it, the ".shutdown" and ".remove" functions are
> all very limited, so I suspect we could just make the rule be that the
> install/lookup functions are serialized against each other by the
> pty_mutex (true today), and then we just add a small spinlock for the
> actual driver array insert/lookup.

Actually, I think we could probably make it really trivial by forcing
the free'ing of the tty itself to be RCU-delayed.

Then shutdown/remove would remove the entry with no locking
what-so-ever (which is really nice if you're in an interrupt - because
now *other* users don't need to use those annoying irq-safe versions),
and simply just clear the ttys[] array index.

The lookup side would need to just do a RCU read lock, read the
->ttys[index] thing using ACCESS_ONCE, and then just do the
atomic_inc_not_zero() dance I already did to validate that the thing
is still alive.

Voila - very cheap locking, and the part that could possibly happen
from interrupts (shutdown/remove) needs no locking at all.

Making the tty freeing be rcu-delayed sounds pretty dang simple too.

What do you think?

Anyway, I'm closing the merge window now (doing the tagging, booting
and checking that allmodconfig/allyesconfig/allnoconfigs all compile
fine) so it's 3.6 material, but it doesn't sound bad.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

alan at lxorguk

Jun 3, 2012, 6:06 AM

Post #25 of 27 (120 views)
Permalink

Re: [PATCH] tty: add lockdep annotations [In reply to]

> Actually, I think we could probably make it really trivial by forcing
> the free'ing of the tty itself to be RCU-delayed.

It doesn't solve the problem of the synchronization between ->ttys[] and
->termios[]. Thats two objects, and the root of the problem. Given the
size of struct ktermios it appears to be a win anyway to put it in the
tty.

At that point tty_release can do what it should do.

> What do you think?

Second complication is some ttys have their own private ->shutdown.

It's possibly a useful optimisation and way of doing it *once* the
termios tidy is done, but I think once that is done then ->shutdown goes
away or becomes an non IRQ path event in tty_release.

> Anyway, I'm closing the merge window now (doing the tagging, booting
> and checking that allmodconfig/allyesconfig/allnoconfigs all compile
> fine) so it's 3.6 material, but it doesn't sound bad.

I've been gradually putting in places the bits leading up to the tty_lock
fix. I hadn't realised that one needed to be ahead or was exposing the
termios/tty array race.

I'll just go back and put the termios stuff further ahead and get that
chunk right first. After that tty locking, and then we can worry about
the real nasty which is that if we fix the open/close paths for console
devices to be secure Fedora breaks because their userspace appears to be
completely broken in this area.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

1 2

View All

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads