| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 15 Jun 2020 12:37:26 -0400
From: Waiman Long <longman@...hat.com>
To: Boqun Feng <boqun.feng@...il.com>
Cc: Peter Zijlstra <peterz@...radead.org>,
Dmitry Vyukov <dvyukov@...gle.com>,
syzbot <syzbot+a9fb1457d720a55d6dc5@...kaller.appspotmail.com>,
Alexey Dobriyan <adobriyan@...il.com>,
Andrew Morton <akpm@...ux-foundation.org>, allison@...utok.net,
areber@...hat.com, aubrey.li@...ux.intel.com,
Andrei Vagin <avagin@...il.com>,
Bruce Fields <bfields@...ldses.org>,
Christian Brauner <christian@...uner.io>, cyphar@...har.com,
"Eric W. Biederman" <ebiederm@...ssion.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>, guro@...com,
Jeff Layton <jlayton@...nel.org>,
Joel Fernandes <joel@...lfernandes.org>,
Kees Cook <keescook@...omium.org>, linmiaohe@...wei.com,
linux-fsdevel <linux-fsdevel@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Michal Hocko <mhocko@...e.com>, Ingo Molnar <mingo@...nel.org>,
Oleg Nesterov <oleg@...hat.com>, sargun@...gun.me,
syzkaller-bugs <syzkaller-bugs@...glegroups.com>,
Thomas Gleixner <tglx@...utronix.de>,
Al Viro <viro@...iv.linux.org.uk>
Subject: Re: possible deadlock in send_sigio
On 6/12/20 3:01 AM, Boqun Feng wrote:
> On Fri, Jun 12, 2020 at 07:55:26AM +0800, Boqun Feng wrote:
>> Hi Peter and Waiman,
>>
>> On Thu, Jun 11, 2020 at 12:09:59PM -0400, Waiman Long wrote:
>>> On 6/11/20 10:22 AM, Peter Zijlstra wrote:
>>>> On Thu, Jun 11, 2020 at 09:51:29AM -0400, Waiman Long wrote:
>>>>
>>>>> There was an old lockdep patch that I think may address the issue, but was
>>>>> not merged at the time. I will need to dig it out and see if it can be
>>>>> adapted to work in the current kernel. It may take some time.
>>>> Boqun was working on that; I can't remember what happened, but ISTR it
>>>> was shaping up nice.
>>>>
>>> Yes, I am talking about Boqun's patch. However, I think he had moved to
>>> another company and so may not be able to actively work on that again.
>>>
>> I think you are talking about the rescursive read deadlock detection
>> patchset:
>>
>> https://lore.kernel.org/lkml/20180411135110.9217-1-boqun.feng@gmail.com/
>>
>> Let me have a good and send a new version based on today's master of tip
>> tree.
>>
> FWIW, with the following patch, I think we can avoid to the false
> positives. But solely with this patch, we don't have the ability to
> detect deadlocks with recursive locks..
>
> I've managed to rebase my patchset, but need some time to tweak it to
> work properly, in the meantime, Dmitry, could you give this a try?
>
> Regards,
> Boqun
>
> ------------->8
> Subject: [PATCH] locking: More accurate annotations for read_lock()
>
> On the archs using QUEUED_RWLOCKS, read_lock() is not always a recursive
> read lock, actually it's only recursive if in_interrupt() is true. So
> change the annotation accordingly to catch more deadlocks.
>
> Note we used to treat read_lock() as pure recursive read locks in
> lib/locking-seftest.c, and this is useful, especially for the lockdep
> development selftest, so we keep this via a variable to force switching
> lock annotation for read_lock().
>
> Signed-off-by: Boqun Feng <boqun.feng@...il.com>
> ---
> include/linux/lockdep.h | 35 ++++++++++++++++++++++++++++++++++-
> lib/locking-selftest.c | 11 +++++++++++
> 2 files changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 8fce5c98a4b0..50aedbba0812 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -43,6 +43,7 @@ enum lockdep_wait_type {
> #include <linux/list.h>
> #include <linux/debug_locks.h>
> #include <linux/stacktrace.h>
> +#include <linux/preempt.h>
>
> /*
> * We'd rather not expose kernel/lockdep_states.h this wide, but we do need
> @@ -640,6 +641,31 @@ static inline void print_irqtrace_events(struct task_struct *curr)
> }
> #endif
>
> +/* Variable used to make lockdep treat read_lock() as recursive in selftests */
> +#ifdef CONFIG_DEBUG_LOCKING_API_SELFTESTS
> +extern unsigned int force_read_lock_recursive;
> +#else /* CONFIG_DEBUG_LOCKING_API_SELFTESTS */
> +#define force_read_lock_recursive 0
> +#endif /* CONFIG_DEBUG_LOCKING_API_SELFTESTS */
> +
> +#ifdef CONFIG_LOCKDEP
> +/*
> + * read_lock() is recursive if:
> + * 1. We force lockdep think this way in selftests or
> + * 2. The implementation is not queued read/write lock or
> + * 3. The locker is at an in_interrupt() context.
> + */
> +static inline bool read_lock_is_recursive(void)
> +{
> + return force_read_lock_recursive ||
> + !IS_ENABLED(CONFIG_QUEUED_RWLOCKS) ||
> + in_interrupt();
> +}
> +#else /* CONFIG_LOCKDEP */
> +/* If !LOCKDEP, the value is meaningless */
> +#define read_lock_is_recursive() 0
> +#endif
> +
> /*
> * For trivial one-depth nesting of a lock-class, the following
> * global define can be used. (Subsystems with multiple levels
> @@ -661,7 +687,14 @@ static inline void print_irqtrace_events(struct task_struct *curr)
> #define spin_release(l, i) lock_release(l, i)
>
> #define rwlock_acquire(l, s, t, i) lock_acquire_exclusive(l, s, t, NULL, i)
> -#define rwlock_acquire_read(l, s, t, i) lock_acquire_shared_recursive(l, s, t, NULL, i)
> +#define rwlock_acquire_read(l, s, t, i) \
> +do { \
> + if (read_lock_is_recursive()) \
> + lock_acquire_shared_recursive(l, s, t, NULL, i); \
> + else \
> + lock_acquire_shared(l, s, t, NULL, i); \
> +} while (0)
> +
> #define rwlock_release(l, i) lock_release(l, i)
>
> #define seqcount_acquire(l, s, t, i) lock_acquire_exclusive(l, s, t, NULL, i)
> diff --git a/lib/locking-selftest.c b/lib/locking-selftest.c
> index 14f44f59e733..caadc4dd3368 100644
> --- a/lib/locking-selftest.c
> +++ b/lib/locking-selftest.c
> @@ -28,6 +28,7 @@
> * Change this to 1 if you want to see the failure printouts:
> */
> static unsigned int debug_locks_verbose;
> +unsigned int force_read_lock_recursive;
>
> static DEFINE_WD_CLASS(ww_lockdep);
>
> @@ -1978,6 +1979,11 @@ void locking_selftest(void)
> return;
> }
>
> + /*
> + * treats read_lock() as recursive read locks for testing purpose
> + */
> + force_read_lock_recursive = 1;
> +
> /*
> * Run the testsuite:
> */
> @@ -2073,6 +2079,11 @@ void locking_selftest(void)
>
> ww_tests();
>
> + force_read_lock_recursive = 0;
> + /*
> + * queued_read_lock() specific test cases can be put here
> + */
> +
> if (unexpected_testcase_failures) {
> printk("-----------------------------------------------------------------\n");
> debug_locks = 0;
Your patch looks good to me.
Reviewed-by: Waiman Long <longman@...hat.com>
Powered by blists - more mailing lists