On Thu, Feb 8, 2018 at 7:20 AM, Paul Moore <paul@xxxxxxxxxxxxxx> 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")
Good to know.
>
>> 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.
Well yes, I would never send a patch that I intended for merge
without thorough testing. This is why it is clearly marked and such.
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.
The reason for the RFC patch was for the above discussion. I was
just looking at things briefly yesterday and noticed this odd loop sticking
out. The reason I was looking at things, is that their is some performance
concerns during load, which likely couples back into the patches that
Peter Enderborg is working on. Which IIUC should swap out those
memcpy's into an atomic policy pointer switch. Which then harkens
to Stephen's response to this as well.
>
>> 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
>
> --
> paul moore
> www.paul-moore.com
>
--
Respectfully,
William C Roberts
