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.
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).
> > 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
>
>
