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