On Thu, Apr 16, 2020 at 2:56 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> On Thu, Apr 9, 2020 at 4:12 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> >
> > Always hashing the string representation is inefficient. Just hash the
> > contents of the structure directly (using jhash). If the context is
> > invalid (str & len are set), then hash the string as before, otherwise
> > hash the structured data. Any context that is valid under the given
> > policy should always be structured, and also any context that is invalid
> > should be never structured, so the hashes should always match for the
> > same context. The fact that context_cmp() also follows this logic
> > further reinforces this assumption.
> >
> > Since the context hashing function is now faster (about 10 times), this
> > patch decreases the overhead of security_transition_sid(), which is
> > called from many hooks.
> >
> > The jhash function seemed as a good choice, since it is used as the
> > default hashing algorithm in rhashtable.
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> > ---
> > security/selinux/Makefile | 2 +-
> > security/selinux/ss/context.c | 28 ++++++++++++
> > security/selinux/ss/context.h | 6 ++-
> > security/selinux/ss/ebitmap.c | 14 ++++++
> > security/selinux/ss/ebitmap.h | 1 +
> > security/selinux/ss/policydb.c | 7 +--
> > security/selinux/ss/services.c | 80 ++++++++++++++--------------------
> > security/selinux/ss/services.h | 3 --
> > 8 files changed, 82 insertions(+), 59 deletions(-)
> > create mode 100644 security/selinux/ss/context.c
>
> ...
>
> > diff --git a/security/selinux/ss/context.c b/security/selinux/ss/context.c
> > new file mode 100644
> > index 000000000000..7ca32683056d
> > --- /dev/null
> > +++ b/security/selinux/ss/context.c
> > @@ -0,0 +1,28 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Implementations of the security context functions.
> > + *
> > + * Author: Ondrej Mosnacek <omosnacek@xxxxxxxxx>
> > + * Copyright (C) 2018 Red Hat, Inc.
>
> I think your clock is a bit off ... ;)
>
> Joking aside, copyright dates are important so please fix this (also
> because it is a copyright related issue, it isn't something I want to
> fix during the merge).
Ah, I copy-pasted the boilerplate from elsewhere and forgot to fix the
year... No problem, I'll send a fixed patch.
>
> > + */
> > +
> > +#include <linux/jhash.h>
> > +
> > +#include "context.h"
> > +#include "mls.h"
> > +
> > +u32 context_compute_hash(const struct context *c)
> > +{
> > + u32 hash = 0;
> > +
> > + if (c->len)
> > + return full_name_hash(NULL, c->str, c->len);
> > +
> > + hash = jhash_3words(c->user, c->role, c->type, hash);
> > + hash = jhash_2words(c->range.level[0].sens,
> > + c->range.level[1].sens, hash);
> > + hash = ebitmap_hash(&c->range.level[0].cat, hash);
> > + hash = ebitmap_hash(&c->range.level[1].cat, hash);
>
> Most other places we try to abstract away the mls_range details by
> having an associated mls_XXX(...) function, it seems like that would
> be a good idea here too. How about adding a mls_context_hash(), or
> similarly named function and calling it here.
Good point... I'm a little wary of adding yet another nested function
call to something that should be fast (and that will do only a few
operations on its own), but I suppose I can just make it an inline
function.
>
> > + return hash;
> > +}
[...]
> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index 8ad34fd031d1..2099355e9a7d 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -1490,38 +1490,11 @@ out:
> > return rc;
> > }
> >
> > -int context_add_hash(struct policydb *policydb,
> > - struct context *context)
> > +static int context_struct_to_sid(struct sidtab *sidtab, struct context *context,
> > + u32 *sid)
>
> Since you need to respin this patchset anyway, and patch 2/2 deals
> with the move to sidtab, I think it might be better to keep the
> context_struct_to_sid() arguments the same and not swap the first
> argument for the sidtab. If you keep the first argument as the state
> it makes this patch much more focused on the change at hand and leaves
> patch 2/2 dedicated to just the sidtab move.
Also a good point. I wanted to emphasize that the computation no
longer depends on policydb, but I agree that it adds an unnecessary
churn to this patch. I'll amend it as you suggest.
Thanks,
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
