2012-05-24 15:35:59

by Alan

Subject: [PATCH] tty: fix ldisc lock inversion trace

From: Alan Cox <[email protected]>

This is caused by tty_release using tty_lock_pair to lock both
sides of the pty/tty pair, and then tty_ldisc_release dropping
and relocking one side only. We can drop both fine, so drop both
to avoid any lock ordering concerns.

Signed-off-by: Alan Cox <[email protected]>
---

drivers/tty/tty_ldisc.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)


diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 173a900..833e851 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -912,10 +912,13 @@ void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
* race with the set_ldisc code path.
*/

- tty_unlock(tty);
+ /* We don't give a monkeys' about the o_tty lock here but we
+ must do both to avoid lock inversions against another single
+ locking case */
+ tty_unlock_pair(tty, o_tty);
tty_ldisc_halt(tty);
tty_ldisc_flush_works(tty);
- tty_lock(tty);
+ tty_lock_pair(tty, o_tty);

mutex_lock(&tty->ldisc_mutex);
/*

2012-05-25 09:23:39

by Ming Lei

Subject: Re: [PATCH] tty: fix ldisc lock inversion trace

On Thu, May 24, 2012 at 11:51 PM, Alan Cox <[email protected]> wrote:
> From: Alan Cox <[email protected]>
>
> This is caused by tty_release using tty_lock_pair to lock both
> sides of the pty/tty pair, and then tty_ldisc_release dropping
> and relocking one side only. We can drop both fine, so drop both
> to avoid any lock ordering concerns.
>
> Signed-off-by: Alan Cox <[email protected]>
> ---
>
> ?drivers/tty/tty_ldisc.c | ? ?7 +++++--
> ?1 files changed, 5 insertions(+), 2 deletions(-)
>
>
> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
> index 173a900..833e851 100644
> --- a/drivers/tty/tty_ldisc.c
> +++ b/drivers/tty/tty_ldisc.c
> @@ -912,10 +912,13 @@ void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
> ? ? ? ? * race with the set_ldisc code path.
> ? ? ? ? */
>
> - ? ? ? tty_unlock(tty);
> + ? ? ? /* We don't give a monkeys' about the o_tty lock here but we
> + ? ? ? ? ?must do both to avoid lock inversions against another single
> + ? ? ? ? ?locking case */
> + ? ? ? tty_unlock_pair(tty, o_tty);
> ? ? ? ?tty_ldisc_halt(tty);
> ? ? ? ?tty_ldisc_flush_works(tty);
> - ? ? ? tty_lock(tty);
> + ? ? ? tty_lock_pair(tty, o_tty);

This still may cause lockdep warning by the following line:

if (o_tty)
tty_ldisc_release(o_tty, NULL);

The tty_unlock_pair in 'tty_ldisc_release(o_tty, NULL)' may
not match the previous tty_lock_pair. If tty > o_tty, then lockdep
warning will be triggered since the lock to be released is not
at the top of current lock stack.

Thanks,
--
Ming Lei