linux-kernel - Re: [PATCH] padata: Fix possible divide-by-0 panic in padata_mt

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: <0d8c956b-9397-4268-830b-2abe19ec3066@redhat.com>
Date: Mon, 19 Aug 2024 20:07:45 -0400
From: Waiman Long <longman@...hat.com>
To: Daniel Jordan <daniel.m.jordan@...cle.com>,
 Herbert Xu <herbert@...dor.apana.org.au>
Cc: steffen.klassert@...unet.com, akpm@...ux-foundation.org,
 linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] padata: Fix possible divide-by-0 panic in
 padata_mt_helper()

On 8/19/24 18:29, Daniel Jordan wrote:
> On Sat, Aug 17, 2024 at 03:12:37PM GMT, Herbert Xu wrote:
>> On Mon, Aug 12, 2024 at 10:04:07AM -0400, Waiman Long wrote:
>>> Anyway, using DIV_ROUND_UP() is a slight change in behavior as chunk_size
>>> will be increased by 1 in most cases. I am a bit hesitant to make this
>>> change without looking into more detail about the rationale behind the
>>> current code.
>> I don't think it matters much.  Just look at the two lines after
>> the division, they're both rounding the value up.  So clearly this
>> is expected to handle the case where work gets bunched up into the
>> first N CPUs, potentially leaving some CPUs unused.
> Yeah, the caller is supposed to use min_chunk as a hint for what a
> reasonable amount of work is per thread and so avoid wasteful amounts of
> threads.
>
>> But Daniel wrote the code so he can have the last say of whether
>> we should round up after the division or after the other two ops.
> I think either way works fine with the three existing users and how they
> choose job->min_chunk and job->size.
>
> The DIV_ROUND_UP approach reads a bit nicer to me, but I can imagine
> oddball cases where rounding up is undesirable (say, near-zero values
> for size, min_chunk, and align; padata_work_alloc_mt returns many fewer
> works than requested; and a single unit of work is very expensive) so
> that rounding up makes a bigger difference.  So, the way it now is seems
> ok.
>
>
> By the way, this bug must've happened coming from
> hugetlb_pages_alloc_boot(), right, Waiman?  Because the other padata
> users have hardcoded min_chunk.  I guess it was a case of
>
>      h->max_huge_pages < num_node_state(N_MEMORY) * 2
>
Yes, I guess the hugetlbfs caller is the cause of this div-by-0 problem. 
This is likely a bug that needs to be fixed. The current patch does 
guarantee that padata won't crash like that even with rogue caller.

Cheers,
Longman