SELinux Archive on lore.kernel.org
 help / color / Atom feed
From: Stephen Smalley <sds@tycho.nsa.gov>
To: Ondrej Mosnacek <omosnace@redhat.com>
Cc: SElinux list <selinux@vger.kernel.org>,
	Paul Moore <paul@paul-moore.com>,
	Michal Sekletar <msekleta@redhat.com>
Subject: Re: [PATCH v2] selinux: cache the SID -> context string translation
Date: Thu, 7 Nov 2019 12:59:55 -0500
Message-ID: <1ea02755-a124-d42f-5437-6c8c582ba420@tycho.nsa.gov> (raw)
In-Reply-To: <CAFqZXNvfovdHTFaPH3QA2dRyibtf8Qhd_3G3HKP2qyex5c+yhg@mail.gmail.com>

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.

      reply index

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-06  8:26 Ondrej Mosnacek
2019-11-06 16:11 ` Stephen Smalley
2019-11-06 19:22   ` Stephen Smalley
2019-11-06 19:47     ` Stephen Smalley
2019-11-07 12:41       ` Ondrej Mosnacek
2019-11-07 17:59         ` Stephen Smalley [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1ea02755-a124-d42f-5437-6c8c582ba420@tycho.nsa.gov \
    --to=sds@tycho.nsa.gov \
    --cc=msekleta@redhat.com \
    --cc=omosnace@redhat.com \
    --cc=paul@paul-moore.com \
    --cc=selinux@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

SELinux Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/selinux/0 selinux/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 selinux selinux/ https://lore.kernel.org/selinux \
		selinux@vger.kernel.org
	public-inbox-index selinux

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.selinux


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git