linux-kernel - Re: [RFC PATCH 1/1] lib/stackdepot: replace CONFIG_STACK_HASH

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: <93bf8148-ecc1-75fb-423b-2a76c7252c4e@suse.cz>
Date:   Mon, 20 Jun 2022 14:00:40 +0200
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Dmitry Vyukov <dvyukov@...gle.com>
Cc:     Marco Elver <elver@...gle.com>,
        Alexander Potapenko <glider@...gle.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Andrey Konovalov <andreyknvl@...il.com>,
        kasan-dev@...glegroups.com
Subject: Re: [RFC PATCH 1/1] lib/stackdepot: replace CONFIG_STACK_HASH_ORDER
 with automatic sizing

On 5/27/22 14:02, Dmitry Vyukov wrote:
> On Fri, 27 May 2022 at 13:37, Vlastimil Babka <vbabka@...e.cz> wrote:
>>
>> As Linus explained [1], setting the stackdepot hash table size as a
>> config option is suboptimal, especially as stackdepot becomes a
>> dependency of less specialized subsystems than initially (e.g. DRM,
>> networking, SLUB_DEBUG):
>>
>> : (a) it introduces a new compile-time question that isn't sane to ask
>> : a regular user, but is now exposed to regular users.
>>
>> : (b) this by default uses 1MB of memory for a feature that didn't in
>> : the past, so now if you have small machines you need to make sure you
>> : make a special kernel config for them.
>>
>> Ideally we would employ rhashtable for fully automatic resizing, which
>> should be feasible for many of the new users, but problematic for the
>> original users with restricted context that call __stack_depot_save()
>> with can_alloc == false, i.e. KASAN.
>>
>> However we can easily remove the config option and scale the hash table
>> automatically with system memory. The STACK_HASH_MASK constant becomes
>> stack_hash_mask variable and is used only in one mask operation, so the
>> overhead should be negligible to none. For early allocation we can
>> employ the existing alloc_large_system_hash() function and perform
>> similar scaling for the late allocation.
>>
>> The existing limits of the config option (between 4k and 1M buckets)
>> are preserved, and scaling factor is set to one bucket per 16kB memory
>> so on 64bit the max 1M buckets (8MB memory) is achieved with 16GB
>> system, while a 1GB system will use 512kB.
> 
> Hi Vlastimil,
> 
> We use KASAN with VMs with 2GB of memory.
> If I did the math correctly this will result in 128K entries, while
> currently we have CONFIG_STACK_HASH_ORDER=20 even for arm32.
> I am actually not sure how full the table gets, but we can fuzz a
> large kernel for up to an hour, so we can get lots of stacks (we were
> the only known users who routinely overflowed default LOCKDEP tables
> :)).

Aha, good to know the order of 20 has some real use case then :)

> I am not opposed to this in general. And I understand that KASAN Is
> different from the other users.
> What do you think re allowing CONFIG_STACK_HASH_ORDER=0/is not set
> which will mean auto-size, but keeping ability to set exact size as
> well?
> Or alternatively auto-size if KASAN is not enabled and use a large
> table otherwise? But I am not sure if anybody used
> CONFIG_STACK_HASH_ORDER to reduce the default size with KASAN...

Well if you're unsure and nobody else requested it so far, we could try
setting it to 20 when KASAN is enabled, and autosize otherwise. If somebody
comes up with a use-case for the boot-time parameter override (instead of
CONFIG_), we can add it then?
>> If needed, the automatic scaling could be complemented with a boot-time
>> kernel parameter, but it feels pointless to add it without a specific
>> use case.