[PATCH] [RFC] sidtab: use memset vs loop for init

Post by w***@intel.com
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")

Post by w***@intel.com
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.
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.

Post by w***@intel.com
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.

Post by w***@intel.com
---
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

Stephen Smalley

2018-02-08 15:47:19 UTC

Post by w***@intel.com
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
<12_char_id> (<subj_in_quotes>)
73ff5fc0a86b ("selinux: cache sidtab_context_to_sid results")
# git show -s --format="%h (\"%s\")" 73ff5fc
73ff5fc0a86b ("selinux: cache sidtab_context_to_sid results")

Post by w***@intel.com
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).

Post by w***@intel.com
---
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

William Roberts

2018-02-08 16:34:36 UTC

Post by w***@intel.com
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
<12_char_id> (<subj_in_quotes>)
73ff5fc0a86b ("selinux: cache sidtab_context_to_sid results")
# git show -s --format="%h (\"%s\")" 73ff5fc
73ff5fc0a86b ("selinux: cache sidtab_context_to_sid results")

Post by w***@intel.com
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.

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.

Post by Stephen Smalley
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.

Post by w***@intel.com
---
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

Stephen Smalley

2018-02-08 16:51:41 UTC

Post by w***@intel.com
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
<12_char_id> (<subj_in_quotes>)
73ff5fc0a86b ("selinux: cache sidtab_context_to_sid results")
# git show -s --format="%h (\"%s\")" 73ff5fc
73ff5fc0a86b ("selinux: cache sidtab_context_to_sid results")

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.

Post by w***@intel.com
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.

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

Post by w***@intel.com
---
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

William Roberts

2018-02-08 17:30:53 UTC

Post by w***@intel.com
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
<12_char_id> (<subj_in_quotes>)
73ff5fc0a86b ("selinux: cache sidtab_context_to_sid results")
# git show -s --format="%h (\"%s\")" 73ff5fc
73ff5fc0a86b ("selinux: cache sidtab_context_to_sid results")

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.

Post by w***@intel.com
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.

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.

I'll pass that along, thanks.

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.

Post by w***@intel.com
---
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

William Roberts

2018-02-08 16:31:09 UTC

Post by w***@intel.com
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
<12_char_id> (<subj_in_quotes>)
73ff5fc0a86b ("selinux: cache sidtab_context_to_sid results")
# git show -s --format="%h (\"%s\")" 73ff5fc
73ff5fc0a86b ("selinux: cache sidtab_context_to_sid results")

Good to know.

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

Post by Paul Moore
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.

Post by w***@intel.com
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.

--
paul moore
www.paul-moore.com

--
Respectfully,

William C Roberts

Paul Moore

2018-02-08 21:58:22 UTC

On Thu, Feb 8, 2018 at 11:31 AM, William Roberts
...