linux-kernel - Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 30 Aug 2017 11:09:53 +0900
From:   Byungchul Park <byungchul.park@....com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     mingo@...nel.org, tj@...nel.org, boqun.feng@...il.com,
        david@...morbit.com, johannes@...solutions.net, oleg@...hat.com,
        linux-kernel@...r.kernel.org, kernel-team@....com
Subject: Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation

On Tue, Aug 29, 2017 at 10:59:39AM +0200, Peter Zijlstra wrote:
> Subject: lockdep: Untangle xhlock history save/restore from task independence
> 
> Where XHLOCK_{SOFT,HARD} are save/restore points in the xhlocks[] to
> ensure the temporal IRQ events don't interact with task state, the
> XHLOCK_PROC is a fundament different beast that just happens to share
> the interface.
> 
> The purpose of XHLOCK_PROC is to annotate independent execution inside
> one task. For example workqueues, each work should appear to run in its
> own 'pristine' 'task'.
> 
> Remove XHLOCK_PROC in favour of its own interface to avoid confusion.

Much better to me than the patch you did previously, but, see blow.

> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index c0331891dec1..ab3c0dc8c7ed 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -2107,14 +2107,14 @@ __acquires(&pool->lock)
>  	 * Which would create W1->C->W1 dependencies, even though there is no
>  	 * actual deadlock possible. There are two solutions, using a
>  	 * read-recursive acquire on the work(queue) 'locks', but this will then
> -	 * hit the lockdep limitation on recursive locks, or simly discard
> +	 * hit the lockdep limitation on recursive locks, or simply discard
>  	 * these locks.
>  	 *
>  	 * AFAICT there is no possible deadlock scenario between the
>  	 * flush_work() and complete() primitives (except for single-threaded
>  	 * workqueues), so hiding them isn't a problem.
>  	 */
> -	crossrelease_hist_start(XHLOCK_PROC, true);
> +	lockdep_invariant_state(true);

This is what I am always curious about. It would be ok if you agree with
removing this work-around after fixing acquire things in wq. But, you
keep to say this is essencial.

You should focus on what dependencies actually are, than saparating
contexts unnecessarily. Of course, we have to do it for each work, _BUT_
not between outside of work and each work since there might be
dependencies between them certainly.