Re: possible deadlock in send_sigio

[Waiman Long]

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@xxxxxxxxx/
> >
> > 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@xxxxxxxxx>
> ---
>   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@xxxxxxxxxx>