On Thu, Feb 8, 2018 at 8:51 AM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> 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.
I'll pass that along, thanks.
>
>>
>> >
>> > 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
>> > >
>> > >
>>
>>
>>
--
Respectfully,
William C Roberts
