| Message ID | 1518047169-9046-1-git-send-email-william.c.roberts@intel.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Wed, Feb 7, 2018 at 6:46 PM, <william.c.roberts@intel.com> wrote: > From: William Roberts <william.c.roberts@intel.com> > > 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@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
On Thu, 2018-02-08 at 10:20 -0500, Paul Moore wrote: > On Wed, Feb 7, 2018 at 6:46 PM, <william.c.roberts@intel.com> wrote: > > From: William Roberts <william.c.roberts@intel.com> > > > > 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. 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). > > Signed-off-by: William Roberts <william.c.roberts@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 > >
On Thu, Feb 8, 2018 at 7:20 AM, Paul Moore <paul@paul-moore.com> wrote: > On Wed, Feb 7, 2018 at 6:46 PM, <william.c.roberts@intel.com> wrote: >> From: William Roberts <william.c.roberts@intel.com> >> >> 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@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 >
On Thu, Feb 8, 2018 at 7:47 AM, Stephen Smalley <sds@tycho.nsa.gov> 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@intel.com> wrote: >> > From: William Roberts <william.c.roberts@intel.com> >> > >> > 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@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 >> >>
On Thu, 2018-02-08 at 08:34 -0800, William Roberts wrote: > On Thu, Feb 8, 2018 at 7:47 AM, Stephen Smalley <sds@tycho.nsa.gov> > 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@intel.com> > > > wrote: > > > > From: William Roberts <william.c.roberts@intel.com> > > > > > > > > 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. > > > > > 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@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 > > > > > > > > >
On Thu, Feb 8, 2018 at 8:51 AM, Stephen Smalley <sds@tycho.nsa.gov> 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@tycho.nsa.gov> >> 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@intel.com> >> > > wrote: >> > > > From: William Roberts <william.c.roberts@intel.com> >> > > > >> > > > 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@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 >> > > >> > > >> >> >>
On Thu, Feb 8, 2018 at 11:31 AM, William Roberts <bill.c.roberts@gmail.com> wrote: > On Thu, Feb 8, 2018 at 7:20 AM, Paul Moore <paul@paul-moore.com> wrote: >> On Wed, Feb 7, 2018 at 6:46 PM, <william.c.roberts@intel.com> wrote: ... >>> 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. You did mark it clearly, no worries, I just wanted to make sure everyone reading along at home was clear on things :)
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); }