From: Paul Moore <paul@paul-moore.com>
To: Ondrej Mosnacek <omosnace@redhat.com>
Cc: SElinux list <selinux@vger.kernel.org>,
Stephen Smalley <sds@tycho.nsa.gov>,
rcu@vger.kernel.org, "Paul E. McKenney" <paulmck@kernel.org>,
Michal Sekletar <msekleta@redhat.com>
Subject: Re: [PATCH v4] selinux: cache the SID -> context string translation
Date: Fri, 15 Nov 2019 10:36:52 -0500 [thread overview]
Message-ID: <CAHC9VhTGYWs+k2vRYFnHU7-mUXfDDop7CGaKAthv4Gds0A54zw@mail.gmail.com> (raw)
In-Reply-To: <CAFqZXNvKD16h_T+w8OhAsxJTdr_gLv8B0=LjSmKGKkOLjam3DQ@mail.gmail.com>
On Fri, Nov 15, 2019 at 9:50 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Fri, Nov 15, 2019 at 1:42 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Mon, Nov 11, 2019 at 10:40 AM Ondrej Mosnacek <omosnace@redhat.com> 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>
> > > ---
> > >
> > > Changes in v4:
> > > - use rcu_dereference_protected() instead of rcu_dereference_raw() in
> > > sidtab_sid2str_put()
> > > - fix typo in comment
> > > - remove unnecessary rcu_head_init() call
> > >
> > > Changes in v3:
> > > - add rcu@vger.kernel.org and Paul McKenney to Cc for review of the RCU
> > > logic
> > > - add __rcu annotation to the cache entry pointer (sidtab.c now passes
> > > sparse checks with C=1)
> > >
> > > Changes in v2:
> > > - skip sidtab_sid2str_put() when in non-task context to prevent
> > > deadlock while avoiding the need to lock the spinlock with
> > > irqsave/-restore (which is slower)
> > >
> > > security/selinux/Kconfig | 11 ++
> > > security/selinux/ss/services.c | 138 +++++++++++++++----------
> > > security/selinux/ss/sidtab.c | 179 +++++++++++++++++++++++++++------
> > > security/selinux/ss/sidtab.h | 58 +++++++++--
> > > 4 files changed, 294 insertions(+), 92 deletions(-)
> >
> > ...
> >
> > > diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
> > > index 7d49994e8d5f..6d6ce1c43b49 100644
> > > --- a/security/selinux/ss/sidtab.c
> > > +++ b/security/selinux/ss/sidtab.c
> > > @@ -492,3 +528,88 @@ void sidtab_destroy(struct sidtab *s)
> > >
> > > sidtab_destroy_tree(s->roots[level], level);
> > > }
> > > +
> > > +#if CONFIG_SECURITY_SELINUX_SID2STR_CACHE_SIZE > 0
> > > +
> > > +void sidtab_sid2str_put(struct sidtab *s, struct sidtab_entry *entry,
> > > + const char *str, u32 str_len)
> > > +{
> > > + struct sidtab_str_cache *cache, *victim;
> > > +
> > > + /* do not cache invalid contexts */
> > > + if (entry->context.len)
> > > + return;
> > > +
> > > + /*
> > > + * Skip the put operation when in non-task context to avoid the need
> > > + * to disable interrupts while holding s->cache_lock.
> > > + */
> > > + if (!in_task())
> > > + return;
> > > +
> > > + spin_lock(&s->cache_lock);
> > > +
> > > + cache = rcu_dereference_protected(entry->cache,
> > > + lockdep_is_held(&s->cache_lock));
> > > + if (cache) {
> > > + /* entry in cache - just bump to the head of LRU list */
> > > + list_move(&cache->lru_member, &s->cache_lru_list);
> > > + goto out_unlock;
> > > + }
> > > +
> > > + cache = kmalloc(sizeof(struct sidtab_str_cache) + str_len, GFP_ATOMIC);
> > > + if (!cache)
> > > + goto out_unlock;
> > > +
> > > + if (s->cache_free_slots == 0) {
> > > + /* pop a cache entry from the tail and free it */
> > > + victim = container_of(s->cache_lru_list.prev,
> > > + struct sidtab_str_cache, lru_member);
> > > + list_del(&victim->lru_member);
> > > + kfree_rcu(victim, rcu_member);
> >
> > We could move the kfree_rcu() down to after we drop the spinlock,
> > right? It's likely not a big deal, but since the whole point of this
> > patch is performance improvements it seems like it might be nice. ;)
>
> I could be wrong, but I think kfree_rcu() just (always?) appends the
> object to the RCU list and defers the deallocation for later (and that
> should be pretty quick). But actually... since the kfree_rcu() is not
> called under RCU read lock here, I should at least move it below the
> next line, which still dereferences "victim". And at that point I
> could move it all the way after spin_unlock() as you suggest...
Yes, the bulk of the work is handled later once it is safe to free the
memory, but that doesn't mean work doesn't still happen :)
It's definitely a nitpicky thing, but since we are already at -rc7 and
this isn't a bug-fix, this was always going to land in selinux/next
after the upcoming merge window so we've got time for a respin.
--
paul moore
www.paul-moore.com
prev parent reply other threads:[~2019-11-15 15:37 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-11 15:40 [PATCH v4] selinux: cache the SID -> context string translation Ondrej Mosnacek
2019-11-12 15:23 ` Stephen Smalley
2019-11-13 14:29 ` Paul E. McKenney
2019-11-15 0:42 ` Paul Moore
2019-11-15 14:50 ` Ondrej Mosnacek
2019-11-15 15:36 ` Paul Moore [this message]
Reply instructions:
You may reply publicly 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=CAHC9VhTGYWs+k2vRYFnHU7-mUXfDDop7CGaKAthv4Gds0A54zw@mail.gmail.com \
--to=paul@paul-moore.com \
--cc=msekleta@redhat.com \
--cc=omosnace@redhat.com \
--cc=paulmck@kernel.org \
--cc=rcu@vger.kernel.org \
--cc=sds@tycho.nsa.gov \
--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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).