On 2/18/20 11:18 AM, Stephen Smalley wrote:
On 2/18/20 10:21 AM, Ondrej Mosnacek wrote:
On Tue, Feb 18, 2020 at 3:59 PM Stephen Smalley <sds@xxxxxxxxxxxxx>
wrote:
On 2/17/20 6:49 AM, Ondrej Mosnacek wrote:
Instead allocate hash tables with just the right size based on the
actual number of elements (which is almost always known beforehand, we
just need to defer the hashtab allocation to the right time). The only
case when we don't know the size (with the current policy format) is
the
new filename transitions hashtable. Here I just left the existing
value.
After this patch, the time to load Fedora policy on x86_64 decreases
from 950 ms to 220 ms. If the unconfined module is removed, it
decreases
from 870 ms to 170 ms. It is also likely that other operations are
going
to be faster, mainly string_to_context_struct() or mls_compute_sid(),
but I didn't try to quantify that.
The memory usage increases a bit after this patch, but only by ~1-2 MB
(it is hard to measure precisely). I believe it is a small price to pay
for the increased performance.
Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
---
security/selinux/ss/hashtab.c | 21 ++++++++++++--
security/selinux/ss/hashtab.h | 2 +-
security/selinux/ss/policydb.c | 53
+++++++++++++---------------------
security/selinux/ss/policydb.h | 2 --
4 files changed, 40 insertions(+), 38 deletions(-)
diff --git a/security/selinux/ss/hashtab.c
b/security/selinux/ss/hashtab.c
index ebfdaa31ee32..554a91ef3f06 100644
--- a/security/selinux/ss/hashtab.c
+++ b/security/selinux/ss/hashtab.c
@@ -27,6 +41,9 @@ struct hashtab *hashtab_create(u32
(*hash_value)(struct hashtab *h, const void *
p->nel = 0;
p->hash_value = hash_value;
p->keycmp = keycmp;
+ if (!size)
+ return p;
+
p->htable = kmalloc_array(size, sizeof(*p->htable), GFP_KERNEL);
if (!p->htable) {
kfree(p);
Thanks, this looks promising. However, I was wondering: if we end up
with size == 0 (e.g. policy happens to have an empty table), does the
rest of the hashtab_* code always correctly handle the fact that
->htable could be NULL? Doesn't look obviously safe to me on a quick
look.
Hm... it seems I didn't think this through when I was trying to handle
this case. I was rebasing this patch all over the place as I was
working on other changes in parallel, so maybe I just lost the safety
somewhere along the way... I think I will just clamp the minimum size
to 1, as that seems both safer and simpler. The extra 8-byte
allocation shouldn't cost much (there are only O(number of classes +
commons) hash tables and these make no sense to have 0 entries).
Hmm...on booting this kernel, I am seeing a number of calls to
hashtab_create() with nel_hint==0 leading to size == 0 and nothing is
obviously breaking, so maybe this is safe?
I guess this happens for any class that doesn't define any of its own
permissions (beyond the inherited common ones). Probably could just add
some simple checks to hashtab_* where absent to ensure we don't ever
dereference ->htable if NULL.
