Re: [PATCH 1/3] selinux: fix cond_list corruption when changing booleans

From: Paul Moore <paul@paul-moore.com>
To: Ondrej Mosnacek <omosnace@redhat.com>
Cc: SElinux list <selinux@vger.kernel.org>,
	Stephen Smalley <stephen.smalley.work@gmail.com>
Subject: Re: [PATCH 1/3] selinux: fix cond_list corruption when changing booleans
Date: Wed, 31 Mar 2021 19:19:45 -0400	[thread overview]
Message-ID: <CAHC9VhRD=SNev8Ptk7idauX+0gzQysKDLBhGkfpD1CETB2TNrA@mail.gmail.com> (raw)
In-Reply-To: <CAFqZXNsus-5PGNMT2t=T4WQQMrzgBSJDu6=qr0KmtDgKCR=hfQ@mail.gmail.com>

On Wed, Mar 31, 2021 at 5:12 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Wed, Mar 31, 2021 at 4:04 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Mar 30, 2021 at 9:16 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:

...

> > > -int avtab_duplicate(struct avtab *new, struct avtab *orig)
> > > +int avtab_alloc(struct avtab *h, u32 nrules)
> > >  {
> > > -       int i;
> > > -       struct avtab_node *node, *tmp, *tail;
> > > -
> > > -       memset(new, 0, sizeof(*new));
> > > +       int rc;
> > > +       u32 shift = 0;
> > > +       u32 work = nrules;
> > > +       u32 nslot = 0;
> > >
> > > -       new->htable = kvcalloc(orig->nslot, sizeof(void *), GFP_KERNEL);
> > > -       if (!new->htable)
> > > -               return -ENOMEM;
> > > -       new->nslot = orig->nslot;
> > > -       new->mask = orig->mask;
> > > -
> > > -       for (i = 0; i < orig->nslot; i++) {
> > > -               tail = NULL;
> > > -               for (node = orig->htable[i]; node; node = node->next) {
> > > -                       tmp = kmem_cache_zalloc(avtab_node_cachep, GFP_KERNEL);
> > > -                       if (!tmp)
> > > -                               goto error;
> > > -                       tmp->key = node->key;
> > > -                       if (tmp->key.specified & AVTAB_XPERMS) {
> > > -                               tmp->datum.u.xperms =
> > > -                                       kmem_cache_zalloc(avtab_xperms_cachep,
> > > -                                                       GFP_KERNEL);
> > > -                               if (!tmp->datum.u.xperms) {
> > > -                                       kmem_cache_free(avtab_node_cachep, tmp);
> > > -                                       goto error;
> > > -                               }
> > > -                               tmp->datum.u.xperms = node->datum.u.xperms;
> > > -                       } else
> > > -                               tmp->datum.u.data = node->datum.u.data;
> > > -
> > > -                       if (tail)
> > > -                               tail->next = tmp;
> > > -                       else
> > > -                               new->htable[i] = tmp;
> > > -
> > > -                       tail = tmp;
> > > -                       new->nel++;
> > > +       if (nrules != 0) {
> > > +               while (work) {
> > > +                       work  = work >> 1;
> >
> > Extra  horizontal  spaces  are  awkward  and  bad.
> >
> > > +                       shift++;
> > >                 }
> > > +               if (shift > 2)
> > > +                       shift = shift - 2;
> >
> > Since we are getting nit-picky with this code, why not make the
> > loop/if a bit more elegant?  How about something like below?
> >
> >   u32 shift = 2;
> >   u32 work = nrules >> 4;
> >   while (work) {
> >     work >>= 1;
> >     shift++;
> >   }
>
> I think you meant:
> u32 shift = 0;
> u32 work = nrules >> 2;
> while (work) {
>     work >>= 1;
>     shift++;
> }
>
> ...which is equivalent to the current code and yes, I'll use that :)

Well, no, not really, but that's okay as looking at it now we both got
it wrong :)

Basically I wanted to avoid the odd problem where the current code has
a dip in the number of slots/buckets when nrules is equal to 4, 5, 6,
or 7.  While the code I proposed yesterday did that, it inflated the
number of buckets beyond the current code; your suggestion had
problems with low numbers resulting in zero buckets.

I think what we really want is this:

  shift = 2;
  work = nrules >> 4;
  while (work) {
    work >>= 1;
    shift++;
  }

... it avoids any dips in the bucket count and it results in similar
bucket counts as the existing code for larger numbers of rules.

-- 
paul moore
www.paul-moore.com