Re: [PATCH v2] selinux: cache the SID -> context string translation

[Stephen Smalley <sds@tycho.nsa.gov>]

On 11/7/19 7:41 AM, Ondrej Mosnacek wrote:
> On Wed, Nov 6, 2019 at 8:48 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 11/6/19 2:22 PM, Stephen Smalley wrote:
>>> On 11/6/19 11:11 AM, Stephen Smalley wrote:
>>>> On 11/6/19 3:26 AM, Ondrej Mosnacek wrote:
>>>>> Translating a context struct to string can be quite slow, especially if
>>>>> the context has a lot of category bits set. This can cause quite
>>>>> noticeable performance impact in situations where the translation needs
>>>>> to be done repeatedly. A common example is a UNIX datagram socket with
>>>>> the SO_PASSSEC option enabled, which is used e.g. by systemd-journald
>>>>> when receiving log messages via datagram socket. This scenario can be
>>>>> reproduced with:
>>>>>
>>>>>       cat /dev/urandom | base64 | logger &
>>>>>       timeout 30s perf record -p $(pidof systemd-journald) -a -g
>>>>>       kill %1
>>>>>       perf report -g none --pretty raw | grep security_secid_to_secctx
>>>>>
>>>>> Before the caching introduced by this patch, computing the context
>>>>> string (security_secid_to_secctx() function) takes up ~65% of
>>>>> systemd-journald's CPU time (assuming a context with 1024 categories
>>>>> set and Fedora x86_64 release kernel configs). After this patch
>>>>> (assuming near-perfect cache hit ratio) this overhead is reduced to just
>>>>> ~2%.
>>>>>
>>>>> This patch addresses the issue by caching a certain number (compile-time
>>>>> configurable) of recently used context strings to speed up repeated
>>>>> translations of the same context, while using only a small amount of
>>>>> memory.
>>>>>
>>>>> The cache is integrated into the existing sidtab table by adding a field
>>>>> to each entry, which when not NULL contains an RCU-protected pointer to
>>>>> a cache entry containing the cached string. The cache entries are kept
>>>>> in a linked list sorted according to how recently they were used. On a
>>>>> cache miss when the cache is full, the least recently used entry is
>>>>> removed to make space for the new entry.
>>>>>
>>>>> The patch migrates security_sid_to_context_core() to use the cache (also
>>>>> a few other functions where it was possible without too much fuss, but
>>>>> these mostly use the translation for logging in case of error, which is
>>>>> rare).
>>>>>
>>>>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1733259
>>>>> Cc: Michal Sekletar <msekleta@redhat.com>
>>>>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>>>>
>>>> This looks good to me and didn't trigger any issues during testing.
>>>> Might want to run it by the RCU wizards, e.g. Paul McKenney, to
>>>> confirm that it is correct and optimal usage of RCU.  I didn't see
>>>> anywhere near the same degree of performance improvement in running
>>>> your reproducer above but I also have all the kernel debugging options
>>>> enabled so perhaps those are creating noise or perhaps the reproducer
>>>> doesn't yield stable results.
>>>
>>> Rebuilding with the stock Fedora x86_64 kernel config was closer to your
>>> results albeit still different, ~35% before and ~2% after.
>>
>> Ah, I can reproduce your ~65% result on the 5.3-based Fedora kernel, but
>> not with mainline 5.4.0-rc1.  Only SELinux change between those two that
>> seems potentially relevant is your "selinux: avoid atomic_t usage in
>> sidtab" patch.
> 
> Hm... did you use the stock Fedora kernel RPM as the baseline? If so,
> this could be because on stable Fedora releases the kernel package is
> built with release config and kernel-debug with debug config, while on
> Rawhide there is only one kernel package, which is built with debug
> config. Under debug Fedora config the numbers are completely different
> due to the overhead of various debug checks. I don't remember the
> "after" value that I got when testing the patched Rawhide kernel with
> the default debug config, but on stock Rawhide I got 43%, and on
> Rawhide kernel rebuild with release config I got 65% again.

I first built selinux/next (based on 5.4-rc1) before and after your 
patch using a config with many debug options enabled (including KASAN), 
which yielded wildly different percentages both before and after - don't 
remember the exact values (but still improved by your patch).

Then I built the same sources using the stock Fedora 31 release kernel 
config and got the 35% versus 2% figures, likewise an improvement.

Then I ran your reproducer just on the stock Fedora 31 release kernel, 
which happened to be 5.3-based, and got the 65% versus 2% figures that 
matched your results.  So I'm not sure what accounts for the difference 
in the before results between the stock Fedora 31 release kernel and 
selinux/next using the same config.  Could be a 5.3 vs 5.4-rc1 change, a 
change in selinux/next on top of 5.4-rc1, or something different in the 
build toolchain/environment used for the stock Fedora kernel versus my 
build host (which was running Fedora 31).

Regardless, I do see a significant improvement in all cases from the 
patch, only the degree of improvement differs.  So I'm fine with it. 
Might want to get a 2nd opinion from Paul McKenney all the same on the 
RCU bits.