Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753448AbdDDLxZ (ORCPT ); Tue, 4 Apr 2017 07:53:25 -0400 Received: from frisell.zx2c4.com ([192.95.5.64]:56193 "EHLO frisell.zx2c4.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752932AbdDDLxX (ORCPT ); Tue, 4 Apr 2017 07:53:23 -0400 MIME-Version: 1.0 In-Reply-To: <20170324141608.GA15370@gondor.apana.org.au> References: <20170323112443.30843-1-Jason@zx2c4.com> <20170324141608.GA15370@gondor.apana.org.au> From: "Jason A. Donenfeld" Date: Tue, 4 Apr 2017 13:53:15 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH] padata: avoid race in reordering To: stable@vger.kernel.org Cc: Steffen Klassert , Linux Crypto Mailing List , LKML , Herbert Xu Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2730 Lines: 65 Herbert applied this to his tree. It's probably a good stable candidate, since it's a two line change to fix a race condition. On Fri, Mar 24, 2017 at 3:16 PM, Herbert Xu wrote: > Jason A. Donenfeld wrote: >> Under extremely heavy uses of padata, crashes occur, and with list >> debugging turned on, this happens instead: >> >> [87487.298728] WARNING: CPU: 1 PID: 882 at lib/list_debug.c:33 >> __list_add+0xae/0x130 >> [87487.301868] list_add corruption. prev->next should be next >> (ffffb17abfc043d0), but was ffff8dba70872c80. (prev=ffff8dba70872b00). >> [87487.339011] [] dump_stack+0x68/0xa3 >> [87487.342198] [] ? console_unlock+0x281/0x6d0 >> [87487.345364] [] __warn+0xff/0x140 >> [87487.348513] [] warn_slowpath_fmt+0x4a/0x50 >> [87487.351659] [] __list_add+0xae/0x130 >> [87487.354772] [] ? _raw_spin_lock+0x64/0x70 >> [87487.357915] [] padata_reorder+0x1e6/0x420 >> [87487.361084] [] padata_do_serial+0xa5/0x120 >> >> padata_reorder calls list_add_tail with the list to which its adding >> locked, which seems correct: >> >> spin_lock(&squeue->serial.lock); >> list_add_tail(&padata->list, &squeue->serial.list); >> spin_unlock(&squeue->serial.lock); >> >> This therefore leaves only place where such inconsistency could occur: >> if padata->list is added at the same time on two different threads. >> This pdata pointer comes from the function call to >> padata_get_next(pd), which has in it the following block: >> >> next_queue = per_cpu_ptr(pd->pqueue, cpu); >> padata = NULL; >> reorder = &next_queue->reorder; >> if (!list_empty(&reorder->list)) { >> padata = list_entry(reorder->list.next, >> struct padata_priv, list); >> spin_lock(&reorder->lock); >> list_del_init(&padata->list); >> atomic_dec(&pd->reorder_objects); >> spin_unlock(&reorder->lock); >> >> pd->processed++; >> >> goto out; >> } >> out: >> return padata; >> >> I strongly suspect that the problem here is that two threads can race >> on reorder list. Even though the deletion is locked, call to >> list_entry is not locked, which means it's feasible that two threads >> pick up the same padata object and subsequently call list_add_tail on >> them at the same time. The fix is thus be hoist that lock outside of >> that block. >> >> Signed-off-by: Jason A. Donenfeld > > Patch applied. Thanks. > -- > Email: Herbert Xu > Home Page: http://gondor.apana.org.au/~herbert/ > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt