On 7/30/2019 7:46 AM, Paul Moore wrote: > On Tue, Jul 30, 2019 at 8:48 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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@xxxxxxxxxx> >> Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx> > 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). >
