On Tue, Feb 18, 2020 at 5:44 PM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> On 2/18/20 11:18 AM, Stephen Smalley wrote:
> > On 2/18/20 10:21 AM, Ondrej Mosnacek wrote:
> >> On Tue, Feb 18, 2020 at 3:59 PM Stephen Smalley <sds@xxxxxxxxxxxxx>
> >> wrote:
> >>> On 2/17/20 6:49 AM, Ondrej Mosnacek wrote:
> >>>> Instead allocate hash tables with just the right size based on the
> >>>> actual number of elements (which is almost always known beforehand, we
> >>>> just need to defer the hashtab allocation to the right time). The only
> >>>> case when we don't know the size (with the current policy format) is
> >>>> the
> >>>> new filename transitions hashtable. Here I just left the existing
> >>>> value.
> >>>>
> >>>> After this patch, the time to load Fedora policy on x86_64 decreases
> >>>> from 950 ms to 220 ms. If the unconfined module is removed, it
> >>>> decreases
> >>>> from 870 ms to 170 ms. It is also likely that other operations are
> >>>> going
> >>>> to be faster, mainly string_to_context_struct() or mls_compute_sid(),
> >>>> but I didn't try to quantify that.
> >>>>
> >>>> The memory usage increases a bit after this patch, but only by ~1-2 MB
> >>>> (it is hard to measure precisely). I believe it is a small price to pay
> >>>> for the increased performance.
> >>>>
> >>>> Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> >>>> ---
> >>>> security/selinux/ss/hashtab.c | 21 ++++++++++++--
> >>>> security/selinux/ss/hashtab.h | 2 +-
> >>>> security/selinux/ss/policydb.c | 53
> >>>> +++++++++++++---------------------
> >>>> security/selinux/ss/policydb.h | 2 --
> >>>> 4 files changed, 40 insertions(+), 38 deletions(-)
> >>>>
> >>>> diff --git a/security/selinux/ss/hashtab.c
> >>>> b/security/selinux/ss/hashtab.c
> >>>> index ebfdaa31ee32..554a91ef3f06 100644
> >>>> --- a/security/selinux/ss/hashtab.c
> >>>> +++ b/security/selinux/ss/hashtab.c
> >>>> @@ -27,6 +41,9 @@ struct hashtab *hashtab_create(u32
> >>>> (*hash_value)(struct hashtab *h, const void *
> >>>> p->nel = 0;
> >>>> p->hash_value = hash_value;
> >>>> p->keycmp = keycmp;
> >>>> + if (!size)
> >>>> + return p;
> >>>> +
> >>>> p->htable = kmalloc_array(size, sizeof(*p->htable), GFP_KERNEL);
> >>>> if (!p->htable) {
> >>>> kfree(p);
> >>>
> >>> Thanks, this looks promising. However, I was wondering: if we end up
> >>> with size == 0 (e.g. policy happens to have an empty table), does the
> >>> rest of the hashtab_* code always correctly handle the fact that
> >>> ->htable could be NULL? Doesn't look obviously safe to me on a quick
> >>> look.
> >>
> >> Hm... it seems I didn't think this through when I was trying to handle
> >> this case. I was rebasing this patch all over the place as I was
> >> working on other changes in parallel, so maybe I just lost the safety
> >> somewhere along the way... I think I will just clamp the minimum size
> >> to 1, as that seems both safer and simpler. The extra 8-byte
> >> allocation shouldn't cost much (there are only O(number of classes +
> >> commons) hash tables and these make no sense to have 0 entries).
> >
> > Hmm...on booting this kernel, I am seeing a number of calls to
> > hashtab_create() with nel_hint==0 leading to size == 0 and nothing is
> > obviously breaking, so maybe this is safe?
>
> I guess this happens for any class that doesn't define any of its own
> permissions (beyond the inherited common ones). Probably could just add
> some simple checks to hashtab_* where absent to ensure we don't ever
> dereference ->htable if NULL.
Yeah, OK. I only needed to add a check to hashtab_insert() and
hashtab_search(), the rest will already do the sane thing as-is.
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
