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