[PATCH] locking/lockdep: Add debug_locks check in __lock_downgrade()

[PATCH] locking/lockdep: Add debug_locks check in __lock_downgrade()

[Waiman Long]

Tetsuo Handa had reported he saw an incorrect "downgrading a read lock"
warning right after a previous lockdep warning. It is likely that the
previous warning turned off lock debugging causing the lockdep to have
inconsistency states leading to the lock downgrade warning.

Fix that by add a check for debug_locks at the beginning of
__lock_downgrade().

Signed-off-by: Waiman Long <longman@redhat.com>
Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
---
 kernel/locking/lockdep.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 9593233..e805fe3 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -3535,6 +3535,9 @@ static int __lock_downgrade(struct lockdep_map *lock, unsigned long ip)
 	unsigned int depth;
 	int i;
 
+	if (unlikely(!debug_locks))
+		return 0;
+
 	depth = curr->lockdep_depth;
 	/*
 	 * This function is about (re)setting the class of a held lock,
-- 
1.8.3.1

Re: [PATCH] locking/lockdep: Add debug_locks check in __lock_downgrade()

[Dmitry Vyukov]

On Thu, Jan 10, 2019 at 5:04 AM Waiman Long <longman@redhat.com> wrote:
>
> Tetsuo Handa had reported he saw an incorrect "downgrading a read lock"
> warning right after a previous lockdep warning. It is likely that the
> previous warning turned off lock debugging causing the lockdep to have
> inconsistency states leading to the lock downgrade warning.
>
> Fix that by add a check for debug_locks at the beginning of
> __lock_downgrade().
>
> Signed-off-by: Waiman Long <longman@redhat.com>
> Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>

Please also add:

Reported-by: syzbot+53383ae265fb161ef488@syzkaller.appspotmail.com

for tracking purposes. But Tetsuo deserves lots of credit for debugging it.

> ---
>  kernel/locking/lockdep.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 9593233..e805fe3 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -3535,6 +3535,9 @@ static int __lock_downgrade(struct lockdep_map *lock, unsigned long ip)
>         unsigned int depth;
>         int i;
>
> +       if (unlikely(!debug_locks))
> +               return 0;
> +

Are we sure this resolves the problem rather than makes the
inconsistency window smaller?
I don't understand all surrounding code, but looking just at this
function it looks like it may just pepper over the problem. Say, we
pass this check when lockdep was still turned on. Then this thread is
preempted for some time (e.g. a virtual CPU), then another thread
started reporting a warning, turned lockdep off, some information
wasn't collected, and this this task resumes and reports a false
warning.
Or we are holding the mutex here, and the fact that we are holding it
ensures that no other task will take it and no information will be
lost?
Quite a tricky moment, perhaps deserves a comment.




>         depth = curr->lockdep_depth;
>         /*
>          * This function is about (re)setting the class of a held lock,
> --
> 1.8.3.1
>

Re: [PATCH] locking/lockdep: Add debug_locks check in __lock_downgrade()

[Tetsuo Handa]

On 2019/01/10 19:21, Dmitry Vyukov wrote:
>> @@ -3535,6 +3535,9 @@ static int __lock_downgrade(struct lockdep_map *lock, unsigned long ip)
>>         unsigned int depth;
>>         int i;
>>
>> +       if (unlikely(!debug_locks))
>> +               return 0;
>> +
> 
> Are we sure this resolves the problem rather than makes the
> inconsistency window smaller?

As far as I know, this should resolve the problem.

> I don't understand all surrounding code, but looking just at this
> function it looks like it may just pepper over the problem. Say, we
> pass this check when lockdep was still turned on. Then this thread is
> preempted for some time (e.g. a virtual CPU), then another thread
> started reporting a warning, turned lockdep off, some information
> wasn't collected, and this this task resumes and reports a false
> warning.

What this function checks is whether current thread is holding rw_semaphore
for write. Since the information of held locks are per "struct task_struct"
record, if lockdep is still enabled as of entry of this function, there must
be a lockdep record that current thread is holding rw_semaphore for write
if current thread is actually holding rw_semaphore for write. Therefore,
preemption/interrupts can't erase the lockdep record that current thread is
holding rw_semaphore, even if lockdep is turned off after passing this check.

> Or we are holding the mutex here, and the fact that we are holding it
> ensures that no other task will take it and no information will be
> lost?
> Quite a tricky moment, perhaps deserves a comment.

I think many other functions check debug_locks upon entry of functions.

> 
>>         depth = curr->lockdep_depth;
>>         /*
>>          * This function is about (re)setting the class of a held lock,

Re: [PATCH] locking/lockdep: Add debug_locks check in __lock_downgrade()

[Peter Zijlstra]

On Thu, Jan 10, 2019 at 11:21:13AM +0100, Dmitry Vyukov wrote:
> On Thu, Jan 10, 2019 at 5:04 AM Waiman Long <longman@redhat.com> wrote:
> >
> > Tetsuo Handa had reported he saw an incorrect "downgrading a read lock"
> > warning right after a previous lockdep warning. It is likely that the
> > previous warning turned off lock debugging causing the lockdep to have
> > inconsistency states leading to the lock downgrade warning.
> >
> > Fix that by add a check for debug_locks at the beginning of
> > __lock_downgrade().
> >
> > Signed-off-by: Waiman Long <longman@redhat.com>
> > Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> 
> Please also add:
> 
> Reported-by: syzbot+53383ae265fb161ef488@syzkaller.appspotmail.com
> 
> for tracking purposes. But Tetsuo deserves lots of credit for debugging it.

I made that:

Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Debugged-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Reported-by: syzbot+53383ae265fb161ef488@syzkaller.appspotmail.com

> > index 9593233..e805fe3 100644
> > --- a/kernel/locking/lockdep.c
> > +++ b/kernel/locking/lockdep.c
> > @@ -3535,6 +3535,9 @@ static int __lock_downgrade(struct lockdep_map *lock, unsigned long ip)
> >         unsigned int depth;
> >         int i;
> >
> > +       if (unlikely(!debug_locks))
> > +               return 0;
> > +
> 
> Are we sure this resolves the problem rather than makes the
> inconsistency window smaller?
> I don't understand all surrounding code, but looking just at this
> function it looks like it may just pepper over the problem. Say, we
> pass this check when lockdep was still turned on. Then this thread is
> preempted for some time (e.g. a virtual CPU), then another thread
> started reporting a warning, turned lockdep off, some information
> wasn't collected, and this this task resumes and reports a false
> warning.

Theoretically possible I suppose; but this is analogous to many of the
other lockdep hooks.

> Or we are holding the mutex here, and the fact that we are holding it
> ensures that no other task will take it and no information will be
> lost?

There is no lock here; for performance reasons we prefer not to acquire
a global spinlock on every lockdep hook, that would be horrific.

Re: [PATCH] locking/lockdep: Add debug_locks check in __lock_downgrade()

[Dmitry Vyukov]

On Mon, Jan 14, 2019 at 2:37 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Jan 10, 2019 at 11:21:13AM +0100, Dmitry Vyukov wrote:
> > On Thu, Jan 10, 2019 at 5:04 AM Waiman Long <longman@redhat.com> wrote:
> > >
> > > Tetsuo Handa had reported he saw an incorrect "downgrading a read lock"
> > > warning right after a previous lockdep warning. It is likely that the
> > > previous warning turned off lock debugging causing the lockdep to have
> > > inconsistency states leading to the lock downgrade warning.
> > >
> > > Fix that by add a check for debug_locks at the beginning of
> > > __lock_downgrade().
> > >
> > > Signed-off-by: Waiman Long <longman@redhat.com>
> > > Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> >
> > Please also add:
> >
> > Reported-by: syzbot+53383ae265fb161ef488@syzkaller.appspotmail.com
> >
> > for tracking purposes. But Tetsuo deserves lots of credit for debugging it.
>
> I made that:
>
> Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> Debugged-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> Reported-by: syzbot+53383ae265fb161ef488@syzkaller.appspotmail.com
>
> > > index 9593233..e805fe3 100644
> > > --- a/kernel/locking/lockdep.c
> > > +++ b/kernel/locking/lockdep.c
> > > @@ -3535,6 +3535,9 @@ static int __lock_downgrade(struct lockdep_map *lock, unsigned long ip)
> > >         unsigned int depth;
> > >         int i;
> > >
> > > +       if (unlikely(!debug_locks))
> > > +               return 0;
> > > +
> >
> > Are we sure this resolves the problem rather than makes the
> > inconsistency window smaller?
> > I don't understand all surrounding code, but looking just at this
> > function it looks like it may just pepper over the problem. Say, we
> > pass this check when lockdep was still turned on. Then this thread is
> > preempted for some time (e.g. a virtual CPU), then another thread
> > started reporting a warning, turned lockdep off, some information
> > wasn't collected, and this this task resumes and reports a false
> > warning.
>
> Theoretically possible I suppose; but this is analogous to many of the
> other lockdep hooks.
>
> > Or we are holding the mutex here, and the fact that we are holding it
> > ensures that no other task will take it and no information will be
> > lost?
>
> There is no lock here; for performance reasons we prefer not to acquire
> a global spinlock on every lockdep hook, that would be horrific.

I mean the user mutex itself. Some invariants may hold while we are
holding it as Tetsuo noted.

Re: [PATCH] locking/lockdep: Add debug_locks check in __lock_downgrade()

[Tetsuo Handa]

Peter, will this fix be sent to linux.git shortly?
syzbot is generating many corrupted reports due to this race.

On 2019/01/14 22:36, Peter Zijlstra wrote:
> On Thu, Jan 10, 2019 at 11:21:13AM +0100, Dmitry Vyukov wrote:
>> On Thu, Jan 10, 2019 at 5:04 AM Waiman Long <longman@redhat.com> wrote:
>>>
>>> Tetsuo Handa had reported he saw an incorrect "downgrading a read lock"
>>> warning right after a previous lockdep warning. It is likely that the
>>> previous warning turned off lock debugging causing the lockdep to have
>>> inconsistency states leading to the lock downgrade warning.
>>>
>>> Fix that by add a check for debug_locks at the beginning of
>>> __lock_downgrade().
>>>
>>> Signed-off-by: Waiman Long <longman@redhat.com>
>>> Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
>>
>> Please also add:
>>
>> Reported-by: syzbot+53383ae265fb161ef488@syzkaller.appspotmail.com
>>
>> for tracking purposes. But Tetsuo deserves lots of credit for debugging it.
> 
> I made that:
> 
> Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> Debugged-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> Reported-by: syzbot+53383ae265fb161ef488@syzkaller.appspotmail.com

[tip:locking/core] locking/lockdep: Add debug_locks check in __lock_downgrade()

[tip-bot for Waiman Long]

Commit-ID:  71492580571467fb7177aade19c18ce7486267f5
Gitweb:     https://git.kernel.org/tip/71492580571467fb7177aade19c18ce7486267f5
Author:     Waiman Long <longman@redhat.com>
AuthorDate: Wed, 9 Jan 2019 23:03:25 -0500
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 21 Jan 2019 11:18:51 +0100

locking/lockdep: Add debug_locks check in __lock_downgrade()

Tetsuo Handa had reported he saw an incorrect "downgrading a read lock"
warning right after a previous lockdep warning. It is likely that the
previous warning turned off lock debugging causing the lockdep to have
inconsistency states leading to the lock downgrade warning.

Fix that by add a check for debug_locks at the beginning of
__lock_downgrade().

Debugged-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Reported-by: syzbot+53383ae265fb161ef488@syzkaller.appspotmail.com
Signed-off-by: Waiman Long <longman@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Link: https://lkml.kernel.org/r/1547093005-26085-1-git-send-email-longman@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/locking/lockdep.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 95932333a48b..e805fe3bf87f 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -3535,6 +3535,9 @@ static int __lock_downgrade(struct lockdep_map *lock, unsigned long ip)
 	unsigned int depth;
 	int i;
 
+	if (unlikely(!debug_locks))
+		return 0;
+
 	depth = curr->lockdep_depth;
 	/*
 	 * This function is about (re)setting the class of a held lock,

[tip:locking/core] locking/lockdep: Add debug_locks check in __lock_downgrade()

[tip-bot for Waiman Long]

Commit-ID:  513e1073d52e55b8024b4f238a48de7587c64ccf
Gitweb:     https://git.kernel.org/tip/513e1073d52e55b8024b4f238a48de7587c64ccf
Author:     Waiman Long <longman@redhat.com>
AuthorDate: Wed, 9 Jan 2019 23:03:25 -0500
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 4 Feb 2019 09:03:27 +0100

locking/lockdep: Add debug_locks check in __lock_downgrade()

Tetsuo Handa had reported he saw an incorrect "downgrading a read lock"
warning right after a previous lockdep warning. It is likely that the
previous warning turned off lock debugging causing the lockdep to have
inconsistency states leading to the lock downgrade warning.

Fix that by add a check for debug_locks at the beginning of
__lock_downgrade().

Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Reported-by: syzbot+53383ae265fb161ef488@syzkaller.appspotmail.com
Signed-off-by: Waiman Long <longman@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Link: https://lkml.kernel.org/r/1547093005-26085-1-git-send-email-longman@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/locking/lockdep.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 608f74ed8bb9..7f7db23fc002 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -3479,6 +3479,9 @@ __lock_set_class(struct lockdep_map *lock, const char *name,
 	unsigned int depth;
 	int i;
 
+	if (unlikely(!debug_locks))
+		return 0;
+
 	depth = curr->lockdep_depth;
 	/*
 	 * This function is about (re)setting the class of a held lock,