Re: [PATCH v2 2/3] selinux: eliminate the redundant policycap array

From: Ondrej Mosnacek <omosnace@redhat.com>
To: Stephen Smalley <stephen.smalley.work@gmail.com>
Cc: SElinux list <selinux@vger.kernel.org>, Paul Moore <paul@paul-moore.com>
Subject: Re: [PATCH v2 2/3] selinux: eliminate the redundant policycap array
Date: Thu, 27 Aug 2020 09:40:27 +0200	[thread overview]
Message-ID: <CAFqZXNvQdzPCrPSd+eO9FS0iBy9DaA6QS=DVDGVCStSYeGzOUQ@mail.gmail.com> (raw)
In-Reply-To: <CAEjxPJ7V3XW+wCVpNUitD_cc2GoVjM0mm-zWrDWcxLd7wcv_Eg@mail.gmail.com>

On Wed, Aug 26, 2020 at 4:12 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Wed, Aug 26, 2020 at 9:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > The policycap array in struct selinux_state is redundant and can be
> > substituted by calling security_policycap_supported().
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
>
> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index 7cc2f7486c18f..e82a2cfe171f3 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -2113,30 +2113,6 @@ bad:
> >         return 0;
> >  }
> >
> > -static void security_load_policycaps(struct selinux_state *state,
> > -                               struct selinux_policy *policy)
> > -{
> > -       struct policydb *p;
> > -       unsigned int i;
> > -       struct ebitmap_node *node;
> > -
> > -       p = &policy->policydb;
> > -
> > -       for (i = 0; i < ARRAY_SIZE(state->policycap); i++)
> > -               state->policycap[i] = ebitmap_get_bit(&p->policycaps, i);
> > -
> > -       for (i = 0; i < ARRAY_SIZE(selinux_policycap_names); i++)
> > -               pr_info("SELinux:  policy capability %s=%d\n",
> > -                       selinux_policycap_names[i],
> > -                       ebitmap_get_bit(&p->policycaps, i));
> > -
> > -       ebitmap_for_each_positive_bit(&p->policycaps, node, i) {
> > -               if (i >= ARRAY_SIZE(selinux_policycap_names))
> > -                       pr_info("SELinux:  unknown policy capability %u\n",
> > -                               i);
> > -       }
> > -}
> > -
>
> Two requests:
> 1. Can you do a little benchmarking to confirm that calling
> security_policycap_supported() each time doesn't cause any significant
> overheads?  Networking benchmark might be of interest.

I tried to sample a simple `ping -f -i 0 -c 5000000 127.0.0.1` with
perf and indeed security_policycap_supported() now makes up about half
the time spent in some hooks (selinux_socket_sock_rcv_skb(),
selinux_ip_postroute()), mainly because of ebitmap_get_bit() it seems.
I'll try moving the array to policydb and using it in
security_policycap_supported() instead of the bitmap.

>
> 2. Can you retain the logging of the policy capability values?  Just
> drop the first part of the function and rename it e.g.
> security_log_policycaps().

Sure, somehow I failed to notice those prints...

-- 
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.