linux-kernel - Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature

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]

Message-ID: <20170816050506.GR20323@X58A-UD3R>
Date:   Wed, 16 Aug 2017 14:05:06 +0900
From:   Byungchul Park <byungchul.park@....com>
To:     Boqun Feng <boqun.feng@...il.com>
Cc:     Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>, peterz@...radead.org,
        walken@...gle.com, kirill@...temov.name,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        akpm@...ux-foundation.org, willy@...radead.org, npiggin@...il.com,
        kernel-team@....com
Subject: Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature

On Wed, Aug 16, 2017 at 12:05:31PM +0800, Boqun Feng wrote:
> > I see...
> > 
> > Worker A : acquired of wfc.work -> wait for cpu_hotplug_lock to be released
> > Task   B : acquired of cpu_hotplug_lock -> wait for lock#3 to be released
> > Task   C : acquired of lock#3 -> wait for completion of barr->done
> 
> >From the stack trace below, this barr->done is for flush_work() in
> lru_add_drain_all_cpuslocked(), i.e. for work "per_cpu(lru_add_drain_work)"
> 
> > Worker D : wait for wfc.work to be released -> will complete barr->done
> 
> and this barr->done is for work "wfc.work".
> 
> So those two barr->done could not be the same instance, IIUC. Therefore
> the deadlock case is not possible.
> 
> The problem here is all barr->done instances are initialized at
> insert_wq_barrier() and they belongs to the same lock class, to fix

I'm not sure this caused the lockdep warning but, if they belongs to the
same class even though they couldn't be the same instance as you said, I
also think that is another problem and should be fixed.

> this, we need to differ barr->done with different lock classes based on
> the corresponding works.
> 
> How about the this(only compilation test):
> 
> ----------------->8
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index e86733a8b344..d14067942088 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -2431,6 +2431,27 @@ struct wq_barrier {
>  	struct task_struct	*task;	/* purely informational */
>  };
>  
> +#ifdef CONFIG_LOCKDEP_COMPLETE
> +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
> +do {										\
> +	INIT_WORK_ONSTACK(&(barr)->work, func);					\
> +	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
> +	lockdep_init_map_crosslock((struct lockdep_map *)&(barr)->done.map,	\
> +				   "(complete)" #barr,				\
> +				   (target)->lockdep_map.key, 1); 		\
> +	__init_completion(&barr->done);						\
> +	barr->task = current;							\
> +} while (0)
> +#else
> +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
> +do {										\
> +	INIT_WORK_ONSTACK(&(barr)->work, func);					\
> +	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
> +	init_completion(&barr->done);						\
> +	barr->task = current;							\
> +} while (0)
> +#endif
> +
>  static void wq_barrier_func(struct work_struct *work)
>  {
>  	struct wq_barrier *barr = container_of(work, struct wq_barrier, work);
> @@ -2474,10 +2495,7 @@ static void insert_wq_barrier(struct pool_workqueue *pwq,
>  	 * checks and call back into the fixup functions where we
>  	 * might deadlock.
>  	 */
> -	INIT_WORK_ONSTACK(&barr->work, wq_barrier_func);
> -	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&barr->work));
> -	init_completion(&barr->done);
> -	barr->task = current;
> +	INIT_WQ_BARRIER_ONSTACK(barr, wq_barrier_func, target);
>  
>  	/*
>  	 * If @target is currently being executed, schedule the