On Thu, 2018-02-08 at 08:34 -0800, William Roberts wrote:
> On Thu, Feb 8, 2018 at 7:47 AM, Stephen Smalley <sds@xxxxxxxxxxxxx>
> wrote:
> > On Thu, 2018-02-08 at 10:20 -0500, Paul Moore wrote:
> > > On Wed, Feb 7, 2018 at 6:46 PM, <william.c.roberts@xxxxxxxxx>
> > > wrote:
> > > > From: William Roberts <william.c.roberts@xxxxxxxxx>
> > > >
> > > > Commit:
> > > > 73ff5fc selinux: cache sidtab_context_to_sid results
> > >
> > > This wouldn't prevent me from merging the patch, but since it is
> > > an
> > > RFC I'll go ahead and provide some nitpickery here ... the
> > > general
> > > recommendation (for the kernel) when referencing previous
> > > comments is
> > > to use the following format:
> > >
> > > <12_char_id> (<subj_in_quotes>)
> > >
> > > ... so the reference in your patch should look like this:
> > >
> > > 73ff5fc0a86b ("selinux: cache sidtab_context_to_sid results")
> > >
> > > .... as generated by the following git command line:
> > >
> > > # git show -s --format="%h (\"%s\")" 73ff5fc
> > > 73ff5fc0a86b ("selinux: cache sidtab_context_to_sid results")
> > >
> > > > Uses a for loop to NULL the sidtab_node cache pointers.
> > > > Use memset, which allows for compiler optimizations
> > > > when present. Note that gcc sometimes sees this loop/set
> > > > pattern and properly optimimizes it.
> > > >
> > > > I sent this as an RFC for 2 reasons:
> > > > 1. NOT TESTED
> > >
> > > So yes, this is a pretty trivial patch, and it is an RFC, but if
> > > you
> > > want me to merge this at some point you need to at least build
> > > and
> > > boot a patched kernel successfully. I try not to be the
> > > grumpiest
> > > maintainer, but one of the things that does really bother me is
> > > when
> > > people submit code without testing and it blows up on me; that
> > > makes
> > > me not like you, which is generally a Bad Thing.
> > >
> > > > 2. Was there some point not clear in doing it via the loop?
> > >
> > > Nothing immediately comes to mind. Although it is worth noting
> > > that
> > > this code will likely only be hit a few times on a normal system
> > > so I
> > > wouldn't really consider it "performance critical" in the
> > > traditional
> > > sense. This doesn't mean we shouldn't improve the code, just
> > > that I
> > > don't think anyone has really looked that carefully at it. It
> > > looks
> > > like there are other loops in ss/sidtab.c that could probably be
> > > memset'd too.
> > >
> > > Thinking out loud, I suppose we could also move the loop/memset
> > > outside the locked region as well since the lock is for the src
> > > sidtab
> > > and not the dst sidtab. The same for clearing the shutdown
> > > field.
> > >
> > > Looking a bit deeper, I'm starting to question how we use
> > > sidtab_set(), especially since it looks like the only caller is
> > > security_load_policy() which takes a rather *creative* approach
> > > to
> > > changing the sidtab on policy (re)load (to be fair, this looks to
> > > be
> > > an effort to limit the work in the locked section). I wonder if
> > > we
> > > are better served by getting rid of sidtab_set() and replacing it
> > > with
> > > a sidtab_replace() function that would release the old state and
> > > replace it with the new. It would be a more work with the policy
> > > write lock held, but that may soon be less of an issue with some
> > > of
> > > the patches being discussed. It would definitely be a bit
> > > cleaner.
> >
> > What is really needed here is that all of the security server state
> > associated with a policy (policydb, sidtab, current_mapping) needs
> > to
> > be accessed through a single pointer that can be atomically swapped
> > from the old policy to the new one upon policy reload. Then the
> > critical section is reduced to that pointer update and we don't
> > need
> > sidtab_set (or _replace) at all.
>
> I would imagine this would help with load times as well. We're
> getting
> beat up on this in auto. Looks like Peter Enderborg's patch sets
> get us started on that path.
No, I don't believe policy load time has much to do with any of this.
Most of the time spent during policy load is the actual allocation and
population of the policydb data structures from the policy image/file.
And that's mostly a function of how large your policy is - the more
rules, the longer it takes. If you really want to improve that (and
can't reduce your rules), then a major overhaul of the policy image
format such that the kernel can just directly use it at runtime and
only allocate a small number of control structures that point into that
memory would likely be more effective than anything else. It is the
parsing of that file, allocation and population of a million little
data structures that is time consuming.
>
> >
> > My selinux namespace patches start down that road but don't quite
> > get
> > there since that wasn't the focus. They do move everything inside
> > the
> > selinux_ss and reference it through a single pointer, but that also
> > includes the lock and the latest_granting, which shouldn't be
> > swapped
> > on a policy reload (they are per-namespace, not per-policy). I
> > guess
> > I'd need to move everything but those two fields down another level
> > of
> > indirection to achieve that goal, or alternatively move those two
> > back
> > to being global and separate from the selinux_ss (don't need to be
> > per-
> > selinux_ss until multiple namespaces exist).
>
> I wish I had the time to work on this. I would be ready to support N
> namespaces
> asap, that's been another area of contention. I told those
> complaining that
> patches are always welcome.
>
> >
> > > > Signed-off-by: William Roberts <william.c.roberts@xxxxxxxxx>
> > > > ---
> > > > security/selinux/ss/sidtab.c | 3 +--
> > > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > diff --git a/security/selinux/ss/sidtab.c
> > > > b/security/selinux/ss/sidtab.c
> > > > index 5be31b7..fb88ef4 100644
> > > > --- a/security/selinux/ss/sidtab.c
> > > > +++ b/security/selinux/ss/sidtab.c
> > > > @@ -292,8 +292,7 @@ void sidtab_set(struct sidtab *dst, struct
> > > > sidtab *src)
> > > > dst->nel = src->nel;
> > > > dst->next_sid = src->next_sid;
> > > > dst->shutdown = 0;
> > > > - for (i = 0; i < SIDTAB_CACHE_LEN; i++)
> > > > - dst->cache[i] = NULL;
> > > > + memset(dst->cache, 0, sizeof(dst->cache));
> > > > spin_unlock_irqrestore(&src->lock, flags);
> > > > }
> > > >
> > > > --
> > > > 2.7.4
> > >
> > >
>
>
>
