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).
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
