linux-kernel - Re: [PATCH RFC] selinux: policydb - convert filename trans hash to rhashtable

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: <f821809b-548d-fd95-6574-7c74c634353e@tycho.nsa.gov>
Date:   Tue, 4 Feb 2020 10:32:20 -0500
From:   Stephen Smalley <sds@...ho.nsa.gov>
To:     Ondrej Mosnacek <omosnace@...hat.com>
Cc:     Lucas Stach <dev@...xeye.de>, Paul Moore <paul@...l-moore.com>,
        SElinux list <selinux@...r.kernel.org>,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>,
        Richard Haines <richard_c_haines@...nternet.com>
Subject: Re: [PATCH RFC] selinux: policydb - convert filename trans hash to
 rhashtable

On 2/4/20 10:01 AM, Ondrej Mosnacek wrote:
> On Fri, Jan 17, 2020 at 8:11 PM Stephen Smalley <sds@...ho.nsa.gov> wrote:
>> On 1/16/20 4:39 PM, Lucas Stach wrote:
>>> The current hash is too small for current usages in, e.g. the Fedora standard
>>> policy. On file creates a considerable amount of CPU time is spent walking the
>>> the hash chains. Increasing the number of hash buckets somewhat mitigates the
>>> issue, but doesn't completely get rid of the long hash chains.
>>>
>>> This patch does take the bit more invasive route by converting the filename
>>> trans hash to a rhashtable to allow this hash to scale with load.
>>>
>>> fs_mark create benchmark on a SSD device, no ramdisk:
>>> Count          Size       Files/sec     App Overhead
>>> before:
>>> 10000          512        512.3           147715
>>> after:
>>> 10000          512        572.3            75141
>>>
>>> filenametr_cmp(), which was the topmost function in the CPU cycle trace before
>>> at ~5% of the overall CPU time, is now down in the noise.
>>
>> Thank you for working on this.  IMHO, Fedora overuses name-based type
>> transitions but that's another topic. I haven't yet investigated the
>> root cause but with your patch applied, I see some test failures related
>> to name-based transitions:
>>
>> [...]
>> #   Failed test at overlay/test line 439.
>> overlay/test ................ 114/119 # Looks like you failed 1 test of 119.
>> [...]
>> filesystem/test ............. 3/70 File context error, expected:
>>          test_filesystem_filenametranscon1_t
>> got:
>>          test_filesystem_filetranscon_t
>>
>> #   Failed test at filesystem/test line 279.
>> File context error, expected:
>>          test_filesystem_filenametranscon2_t
>> got:
>>          test_filesystem_filetranscon_t
>>
>> #   Failed test at filesystem/test line 286.
>> filesystem/test ............. 68/70 # Looks like you failed 2 tests of 70.
>>
>> You can reproduce by cloning the selinux-testsuite from
>> https://github.com/SELinuxProject/selinux-testsuite, applying the
>> filesystem tests patch from
>> https://patchwork.kernel.org/patch/11337659/,
>> and following the README.md instructions.
> 
> I think I figured out what's wrong - see below.
> <snip>
>>> @@ -441,6 +442,39 @@ static int filenametr_cmp(struct hashtab *h, const void *k1, const void *k2)
>>>
>>>    }
>>>
>>> +static const struct rhashtable_params filename_trans_hashparams = {
>>> +     .nelem_hint             = 1024,
>>> +     .head_offset            = offsetof(struct filename_trans, hash_head),
> 
> You need to add:
> 
> +.hashfn = filenametr_hash,
> 
> here so that the key is correctly hashed on lookup. After applying
> this fix, the selinux-testuite passes for me with this patch.

Hmm..does that then make the .obj_hashfn assignment below unnecessary or 
is that required too?  I'm unclear on the difference.

> 
>>> +     .obj_hashfn             = filenametr_hash,
>>> +     .obj_cmpfn              = filenametr_cmp,
>>> +};