Re: [PATCH v2] padata: use smp_mb in padata_reorder to avoid orphaned padata jobs

[Herbert Xu]

On Tue, Jul 16, 2019 at 12:32:53PM -0400, Daniel Jordan wrote:
> Testing padata with the tcrypt module on a 5.2 kernel...
> 
>     # modprobe tcrypt alg="pcrypt(rfc4106(gcm(aes)))" type=3
>     # modprobe tcrypt mode=211 sec=1
> 
> ...produces this splat:
> 
>     INFO: task modprobe:10075 blocked for more than 120 seconds.
>           Not tainted 5.2.0-base+ #16
>     modprobe        D    0 10075  10064 0x80004080
>     Call Trace:
>      ? __schedule+0x4dd/0x610
>      ? ring_buffer_unlock_commit+0x23/0x100
>      schedule+0x6c/0x90
>      schedule_timeout+0x3b/0x320
>      ? trace_buffer_unlock_commit_regs+0x4f/0x1f0
>      wait_for_common+0x160/0x1a0
>      ? wake_up_q+0x80/0x80
>      { crypto_wait_req }             # entries in braces added by hand
>      { do_one_aead_op }
>      { test_aead_jiffies }
>      test_aead_speed.constprop.17+0x681/0xf30 [tcrypt]
>      do_test+0x4053/0x6a2b [tcrypt]
>      ? 0xffffffffa00f4000
>      tcrypt_mod_init+0x50/0x1000 [tcrypt]
>      ...
> 
> The second modprobe command never finishes because in padata_reorder,
> CPU0's load of reorder_objects is executed before the unlocking store in
> spin_unlock_bh(pd->lock), causing CPU0 to miss CPU1's increment:
> 
> CPU0                                 CPU1
> 
> padata_reorder                       padata_do_serial
>   LOAD reorder_objects  // 0
>                                        INC reorder_objects  // 1
>                                        padata_reorder
>                                          TRYLOCK pd->lock   // failed
>   UNLOCK pd->lock
> 
> CPU0 deletes the timer before returning from padata_reorder and since no
> other job is submitted to padata, modprobe waits indefinitely.
> 
> Add a pair of full barriers to guarantee proper ordering:
> 
> CPU0                                 CPU1
> 
> padata_reorder                       padata_do_serial
>   UNLOCK pd->lock
>   smp_mb()
>   LOAD reorder_objects
>                                        INC reorder_objects
>                                        smp_mb__after_atomic()
>                                        padata_reorder
>                                          TRYLOCK pd->lock
> 
> smp_mb__after_atomic is needed so the read part of the trylock operation
> comes after the INC, as Andrea points out.   Thanks also to Andrea for
> help with writing a litmus test.
> 
> Fixes: 16295bec6398 ("padata: Generic parallelization/serialization interface")
> Signed-off-by: Daniel Jordan <daniel.m.jordan@xxxxxxxxxx>
> Cc: Andrea Parri <andrea.parri@xxxxxxxxxxxxxxxxxxxx>
> Cc: Boqun Feng <boqun.feng@xxxxxxxxx>
> Cc: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> Cc: Paul E. McKenney <paulmck@xxxxxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Steffen Klassert <steffen.klassert@xxxxxxxxxxx>
> Cc: linux-arch@xxxxxxxxxxxxxxx
> Cc: linux-crypto@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---
>  kernel/padata.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt