linux-kernel - Re: [PATCH] padata: avoid race in reordering

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]

Date:   Tue, 4 Apr 2017 13:53:15 +0200
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     stable@...r.kernel.org
Cc:     Steffen Klassert <steffen.klassert@...unet.com>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Herbert Xu <herbert@...dor.apana.org.au>
Subject: Re: [PATCH] padata: avoid race in reordering

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 <herbert@...dor.apana.org.au> wrote:
> Jason A. Donenfeld <Jason@...c4.com> 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]  [<ffffffff9a53d075>] dump_stack+0x68/0xa3
>> [87487.342198]  [<ffffffff99e119a1>] ? console_unlock+0x281/0x6d0
>> [87487.345364]  [<ffffffff99d6b91f>] __warn+0xff/0x140
>> [87487.348513]  [<ffffffff99d6b9aa>] warn_slowpath_fmt+0x4a/0x50
>> [87487.351659]  [<ffffffff9a58b5de>] __list_add+0xae/0x130
>> [87487.354772]  [<ffffffff9add5094>] ? _raw_spin_lock+0x64/0x70
>> [87487.357915]  [<ffffffff99eefd66>] padata_reorder+0x1e6/0x420
>> [87487.361084]  [<ffffffff99ef0055>] 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 <Jason@...c4.com>
>
> Patch applied.  Thanks.
> --
> 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