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  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.

Powered by blists - more mailing lists