On Thu, Apr 16, 2020 at 9:20 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> On Thu, Apr 16, 2020 at 5:51 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> > On Thu, Apr 16, 2020 at 3:41 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > On Tue, Apr 7, 2020 at 2:29 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> > > >
> > > > Currently, they are stored in a linked list, which adds significant
> > > > overhead to security_transition_sid(). On Fedora, with 428 role
> > > > transitions in policy, converting this list to a hash table cuts down
> > > > its run time by about 50%. This was measured by running 'stress-ng --msg
> > > > 1 --msg-ops 100000' under perf with and without this patch.
> > > >
> > > > Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> > > > ---
> > > >
> > > > v2:
> > > > - fix typo scontext->tcontext in security_compute_sid()
> > > > - suggest a better command for testing in the commit msg
> > > >
> > > > security/selinux/ss/policydb.c | 138 ++++++++++++++++++++++-----------
> > > > security/selinux/ss/policydb.h | 8 +-
> > > > security/selinux/ss/services.c | 21 +++--
> > > > 3 files changed, 107 insertions(+), 60 deletions(-)
> > >
> > > ...
> > >
> > > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> > > > index 70ecdc78efbd..4f0cfffd008d 100644
> > > > --- a/security/selinux/ss/policydb.c
> > > > +++ b/security/selinux/ss/policydb.c
> > > > @@ -458,6 +465,30 @@ static int rangetr_cmp(struct hashtab *h, const void *k1, const void *k2)
> > > > return v;
> > > > }
> > > >
> > > > +static u32 role_trans_hash(struct hashtab *h, const void *k)
> > > > +{
> > > > + const struct role_trans_key *key = k;
> > > > +
> > > > + return (key->role + (key->type << 3) + (key->tclass << 5)) &
> > > > + (h->size - 1);
> > > > +}
> > > > +
> > > > +static int role_trans_cmp(struct hashtab *h, const void *k1, const void *k2)
> > > > +{
> > > > + const struct role_trans_key *key1 = k1, *key2 = k2;
> > > > + int v;
> > > > +
> > > > + v = key1->role - key2->role;
> > > > + if (v)
> > > > + return v;
> > > > +
> > > > + v = key1->type - key2->type;
> > > > + if (v)
> > > > + return v;
> > > > +
> > > > + return key1->tclass - key2->tclass;
> > >
> > > Why just a simple boolean statement?
> > >
> > > return key1->role != key2->role || \
> > > key1->type != key2->type || \
> > > key1->tclass != key2->tclass;
> >
> > Because hashtab sorts the entries in each bucket, so it needs a proper
> > sort function. Other such functions (rangetr_cmp(), filenametr_cmp())
> > do a similar thing.
>
> Ooops, yep, of course you're correct. Sorry about the noise :)
>
> I'll send a note later today when it's merged, but this looks good to me.
A day later than expected, but I just merged this into selinux/next, thanks.
--
paul moore
www.paul-moore.com
