linux-kernel - Re: [PATCH 2/3] z3fold: don't fail kernel build if z3fold

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: <CALZtONDxQHOT6fZE7L-C+JhHZYOr=Ff3RSd_+5H5JufKijhEig@mail.gmail.com>
Date:   Fri, 25 Nov 2016 13:33:44 -0500
From:   Dan Streetman <ddstreet@...e.org>
To:     Vitaly Wool <vitalywool@...il.com>
Cc:     Linux-MM <linux-mm@...ck.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH 2/3] z3fold: don't fail kernel build if z3fold_header is
 too big

On Fri, Nov 25, 2016 at 11:25 AM, Vitaly Wool <vitalywool@...il.com> wrote:
> On Fri, Nov 25, 2016 at 4:59 PM, Dan Streetman <ddstreet@...e.org> wrote:
>> On Tue, Nov 15, 2016 at 11:00 AM, Vitaly Wool <vitalywool@...il.com> wrote:
>>> Currently the whole kernel build will be stopped if the size of
>>> struct z3fold_header is greater than the size of one chunk, which
>>> is 64 bytes by default. This may stand in the way of automated
>>> test/debug builds so let's remove that and just fail the z3fold
>>> initialization in such case instead.
>>>
>>> Signed-off-by: Vitaly Wool <vitalywool@...il.com>
>>> ---
>>>  mm/z3fold.c | 11 ++++++++---
>>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/mm/z3fold.c b/mm/z3fold.c
>>> index 7ad70fa..ffd9353 100644
>>> --- a/mm/z3fold.c
>>> +++ b/mm/z3fold.c
>>> @@ -870,10 +870,15 @@ MODULE_ALIAS("zpool-z3fold");
>>>
>>>  static int __init init_z3fold(void)
>>>  {
>>> -       /* Make sure the z3fold header will fit in one chunk */
>>> -       BUILD_BUG_ON(sizeof(struct z3fold_header) > ZHDR_SIZE_ALIGNED);
>>
>> Nak.  this is the wrong way to handle this.  The build bug is there to
>> indicate to you that your patch makes the header too large, not as a
>> runtime check to disable everything.
>
> Okay, let's agree to drop it.
>
>> The right way to handle it is to change the hardcoded assumption that
>> the header fits into a single chunk; e.g.:
>>
>> #define ZHDR_SIZE_ALIGNED round_up(sizeof(struct z3fold_header), CHUNK_SIZE)
>> #define ZHDR_CHUNKS (ZHDR_SIZE_ALIGNED >> CHUNK_SHIFT)
>>
>> then use ZHDR_CHUNKS in all places where it's currently assumed the
>> header is 1 chunk, e.g. in num_free_chunks:
>>
>>   if (zhdr->middle_chunks != 0) {
>>     int nfree_before = zhdr->first_chunks ?
>> -      0 : zhdr->start_middle - 1;
>> +      0 : zhdr->start_middle - ZHDR_CHUNKS;
>>
>> after changing all needed places like that, the build bug isn't needed
>> anymore (unless we want to make sure the header isn't larger than some
>> arbitrary number N chunks)
>
> That sounds overly complicated to me. I would rather use bit_spin_lock
> as Arnd suggested. What would you say?

using the correctly-calculated header size instead of a hardcoded
value is overly complicated?  i don't agree with that...i'd say it
should have been done in the first place ;-)

bit spin locks are hard to debug and slower and should only be used
where space really is absolutely required to be minimal, which
definitely isn't the case here.  this should use regular spin locks
and change the hardcoded assumption of zhdr size < chunk size (which
always was a bad assumption) to calculate it correctly.  it's really
not that hard; there are only a few places where the offset position
of the chunks is calculated.


>
> ~vitaly