linux-kernel - Re: [PATCH 1/2] pcpcntr: add group allocation/free

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: <CAGudoHFj2fPUAJvW6=yB9=XjMcwpQ=rcd77=-+4_o_EP3aYdZw@mail.gmail.com>
Date:   Tue, 22 Aug 2023 16:06:22 +0200
From:   Mateusz Guzik <mjguzik@...il.com>
To:     Vegard Nossum <vegard.nossum@...cle.com>
Cc:     linux-kernel@...r.kernel.org, dennis@...nel.org, tj@...nel.org,
        cl@...ux.com, akpm@...ux-foundation.org, shakeelb@...gle.com,
        linux-mm@...ck.org
Subject: Re: [PATCH 1/2] pcpcntr: add group allocation/free

On 8/22/23, Vegard Nossum <vegard.nossum@...cle.com> wrote:
>
> Testing out a review style with very detailed comments. Let me know if
> you hate it. Review notes:
>

I do, very noisy and I don't think it adds any value. ;)

If something like this becomes the norm I'm confident people are going
to start skimming responses to their mail, as opposed to reading them.

But then again, I had serious disagreement with review folk in the past, so...

> On 8/21/23 22:28, Mateusz Guzik wrote:
>> +	counters = __alloc_percpu_gfp(sizeof(*counters) * count,
>> +				      sizeof(*counters), gfp);
>
> The second argument here is the alignment. I see other callers using
> __alignof__(type), which is what alloc_percpu_gfp() does as well. In
> practice I think it shouldn't matter, but for clarity/consistency maybe
> this should be __alignof__ as well?
>

Ye, I neglected to patch it up after a whipping out a PoC.

> Presumably multiplication overflow is not an issue here as it is with
> kmalloc and friends since the count can't be controlled by userspace.
>

I wanted to assert on the count being sensible to begin with, but
there is no general "only assert with debug enabled" macro. Perhaps a
warn_once + bail out would be preferred?

>> +	if (!counters) {
>> +		fbc[0].counters = NULL;
>>   		return -ENOMEM;
>> +	}
>
> Checked that __alloc_percpu_gfp() returns NULL on failure.
>
> Checked that nothing else before this in the function needs cleanup.
>
> In the old code, fbc->count would have gotten initialized but it
> shouldn't matter here, I think, as long as the counter is never activated.
>
> I'm not sure why only fbc[0].counters is set to NULL, should this happen
> for all the "count" members? [PS: percpu_counter_destroy_many() below
> has a check for fbc[0].counters]
>

Consumers looked fishy to me with zeroing the counter prior to calling
the init func.

I added the NULL assignment so that a call to destroy does nothing.

> In summary, my only slight concern is sizeof(*counters) being passed as
> the alignment to __alloc_percpu_gfp() when maybe it would be more
> appropriate to pass __alignof__() -- not that it makes a difference at
> runtime since both are 4 for s32.
>

Agreed, will patch later.

> One other thing: I find it a bit odd that the "amount" parameter
> (initial value) is s64 when the counters themselves are s32. Maybe just
> a leftover from an old version?
>

I don't know the reasoning by the authors, but seems a clear case to
me that the per-CPU counts were left int-sized to reduce memory usage
and reduce deviation between the central counter and the real state.

-- 
Mateusz Guzik <mjguzik gmail.com>