linux-kernel - Re: [PATCH 1/2] padata: always acquire cpu_hotplug

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20190815051518.GB24982@gondor.apana.org.au>
Date:   Thu, 15 Aug 2019 15:15:18 +1000
From:   Herbert Xu <herbert@...dor.apana.org.au>
To:     Daniel Jordan <daniel.m.jordan@...cle.com>
Cc:     Steffen Klassert <steffen.klassert@...unet.com>,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] padata: always acquire cpu_hotplug_lock before
 pinst->lock

On Fri, Aug 09, 2019 at 03:28:56PM -0400, Daniel Jordan wrote:
> On a 5.2 kernel, lockdep complains when offlining a CPU and writing to a
> parallel_cpumask sysfs file.
> 
>   echo 0 > /sys/devices/system/cpu/cpu1/online
>   echo ff > /sys/kernel/pcrypt/pencrypt/parallel_cpumask
> 
>   ======================================================
>   WARNING: possible circular locking dependency detected
>   5.2.0-padata-base+ #19 Not tainted
>   ------------------------------------------------------
>   cpuhp/1/13 is trying to acquire lock:
>   ...  (&pinst->lock){+.+.}, at: padata_cpu_prep_down+0x37/0x70
> 
>   but task is already holding lock:
>   ...  (cpuhp_state-down){+.+.}, at: cpuhp_thread_fun+0x34/0x240
> 
>   which lock already depends on the new lock.
> 
> padata doesn't take cpu_hotplug_lock and pinst->lock in a consistent
> order.  Which should be first?  CPU hotplug calls into padata with
> cpu_hotplug_lock already held, so it should have priority.

Yeah this is clearly a bug but I think we need tackle something
else first.
 
> diff --git a/kernel/padata.c b/kernel/padata.c
> index b60cc3dcee58..d056276a96ce 100644
> --- a/kernel/padata.c
> +++ b/kernel/padata.c
> @@ -487,9 +487,7 @@ static void __padata_stop(struct padata_instance *pinst)
>  
>  	synchronize_rcu();
>  
> -	get_online_cpus();
>  	padata_flush_queues(pinst->pd);
> -	put_online_cpus();
>  }

As I pointed earlier, the whole concept of flushing the queues is
suspect.  So we should tackle that first and it may obviate the need
to do get_online_cpus completely if the flush call disappears.

My main worry is that you're adding an extra lock around synchronize_rcu
and that is always something that should be done only after careful
investigation.

Cheers,
-- 
Email: Herbert Xu <herbert@...dor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt