Re: [PATCH] selinux: optimize MLS context to string conversion

[Casey Schaufler <casey@schaufler-ca.com>]

On 7/30/2019 7:46 AM, Paul Moore wrote:
> On Tue, Jul 30, 2019 at 8:48 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>> When mls_compute_context_len() or mls_sid_to_context() encounters a
>> context with large category ranges, it behaves suboptimally - it
>> traverses each positive bit of the category bitmap, each time calling
>> find_next_bit() again.
>>
>> This has a large performance impact on UNIX datagram sockets with
>> SO_PASSSEC set, since here the peer context needs to be converted to
>> string for each recieved datagram. See [1] for more information.
>>
>> This patch introduces a new helper for ebitmap traversal, which allows
>> to efficiently iterate over positive ranges instead of bits -
>> ebitmap_for_each_positive_range() - and converts the two mls_*()
>> functions to leverage it.
>>
>> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1733259
>>
>> Reported-by: Michal Sekletar <msekleta@redhat.com>
>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> My current opinion is that this isn't the right way to solve the
> problem, a SELinux sid->secctx cache would be a better choice.

I implemented secctx retention a while ago (can't locate the
thread to determine exactly when) and the performance improvement
was rather impressive. A cache with proper lifecycle management
would be a tremendous win for audit by reducing not only the time
required to generate a secctx, but the frequency at which they would
need to be copied. The downside would be on small systems with
sophisticated policies, where a small cache would be subject to
thrashing. If the cached values aren't considered when doing
secctx->sid conversions I would think aliasing issues wouldn't
apply, although I have missed subtleties in the past.

I'm rooting for the cache solution.


>   With
> that in mind, and the understanding that this patch is an optimization
> and not a bug fix, I'm going to hold-off on doing anything with this
> patch until there is a cache option we can use for comparison.
>
> As Stephen mentioned in the RH BZ (linked in the description), there
> are a couple of reasons why the code doesn't currently store the
> string translations.  Ignoring the issue of aliasing for a moment, I
> do want to stress that I agree with Stephen on the issue of memory
> pressure and that to keep translated strings around indefinitely in
> the kernel is going to be a problem (there are other issues beyond
> just memory use).  I imagine any cache implementation would need to be
> built to only store a subset of all the translations and there would
> need to be a way to age-out those translations (as well as purge them
> completely on events such as a policy load).
>