Re: [PATCH 0/9] SELinux: Improve hash functions and sizing of hash tables

From: Paul Moore <paul@paul-moore.com>
To: siarhei.liakh@concurrent-rt.com
Cc: selinux@vger.kernel.org, colin.king@canonical.com,
	Eric Paris <eparis@parisplace.org>,
	gregkh@linuxfoundation.org, jeffv@google.com,
	omosnace@redhat.com,
	Stephen Smalley <stephen.smalley.work@gmail.com>,
	tglx@linutronix.de
Subject: Re: [PATCH 0/9] SELinux: Improve hash functions and sizing of hash tables
Date: Thu, 9 Apr 2020 09:41:02 -0400	[thread overview]
Message-ID: <CAHC9VhRJ=b-dTVwgTE1TKezqY8KWoGFoHSU1XdfMNjP6uoHQFg@mail.gmail.com> (raw)
In-Reply-To: <20200408182416.30995-1-siarhei.liakh@concurrent-rt.com>

On Wed, Apr 8, 2020 at 2:24 PM <siarhei.liakh@concurrent-rt.com> wrote:
> From: Siarhei Liakh <siarhei.liakh@concurrent-rt.com>
>
> This patch set is the result of an actual customer support case where a client
> of ours observed unacceptable variability of timings while sending UDP packets
> via sendto() with SELinux enabled. The initial investigation centered around
> kernel 3.10.108 in CentOS 6.5 environment. Combination of these patches with
> some substantial tuning of newly added Kconfig options was able to reduce
> *maximum* sendto() latency to about 160us, down from well over 2ms. Worst
> latency was typically observed when a new SSH login was initiated concurrently
> with the test program running a sendto() loop, thus causing an AVC miss with a
> subsequent call to security_compute_av(), which would spend most of its time
> iterating through policydb within context_struct_compute_av().
>
> The original patch set was developed for linux kernel 3.10.108 and later ported
> to newer versions. The patch set presented here is based off Linus' tree as of
> April 7, 2020 and contains only a subset of the changes which are still relevant
> to 5.6+ as many of the issues had already been addressed in different ways.
>
> The patch set consists of 9 patches total and is meant to achieve two goals:
> 1. Replace most local copies of custom hash functions with generic hash
>    functions already available in inlude/linux/*.h.
>
> 2. Replace hard-coded hash table sizing parameters with Kconfig tunables.
>
> "Advanced Hashing" Kconfig option is the only dependency between the patches,
> but other than that and any combination of them can be used.

I haven't yet looked at these patches in detail, but a few quick thoughts:

* I would be very curious to see what the performance improvement is
on a current kernel build from either selinux/next or Linus' tree.
Performance numbers from an extremely old commercial distribution
aren't of much interest to mainline development.

* In general I'm a fan of reducing the number of Kconfig options
whenever possible in favor of the system auto-tuning based on usage
(e.g. the loaded policy).  Obviously this isn't possible in some
cases, but I worry that there is always a risk that if we expose a
build knob there is a risk it will be mis-configured.  My initial
reaction is that this patch set exposes way too many things as Kconfig
knobs.  As an aside, I also worry about runtime tunables, but to a
much lesser extent (the risk is less, the benefits greater, etc.).

* The AVC hash table improvement doesn't exactly look like a
sea-change, have you tried it on multiple policies and work loads?  I
wonder if the small improvement you saw changes on different workloads
and/or policies.

* In general I agree with your statement about using common code, e.g.
hash functions, to improve code quality, maintenance, etc. but the
hashing functions you are replacing are rather trivial and easily
maintained.  Not to mention their performance in the SELinux code is a
well known quantity at this point.

I'll take a closer look at these patches, likely next week after the
merge window closes, but in the meantime if you could provide some
more current performance numbers with a better explanation of the
workloads I think it would be helpful.

Thank you.

-- 
paul moore
www.paul-moore.com