On Tue, Feb 11, 2020 at 9:51 PM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote: > > On 2/11/20 11:39 AM, Ondrej Mosnacek wrote: > > In these rules, each rule with the same (target type, target class, > > filename) values is (in practice) always mapped to the same result type. > > Therefore, it is much more efficient to group the rules by (ttype, > > tclass, filename). > > > > Thus, this patch drops the stype field from the key and changes the > > datum to be a linked list of one or more structures that contain a > > result type and an ebitmap of source types that map the given target to > > the given result type under the given filename. The size of the hash > > table is also incremented to 2048 to be more optimal for Fedora policy > > (which currently has ~2500 unique (ttype, tclass, filename) tuples, > > regardless of whether the 'unconfined' module is enabled). > > > > Not only does this dramtically reduce memory usage when the policy > > contains a lot of unconfined domains (ergo a lot of filename based > > transitions), but it also slightly reduces memory usage of strongly > > confined policies (modeled on Fedora policy with 'unconfined' module > > disabled) and significantly reduces lookup times of these rules on > > Fedora (roughly matches the performance of the rhashtable conversion > > patch [1] posted recently to selinux@xxxxxxxxxxxxxxx). > > > > An obvious next step is to change binary policy format to match this > > layout, so that disk space is also saved. However, since that requires > > more work (including matching userspace changes) and this patch is > > already beneficial on its own, I'm posting it separately. > > > > Performance/memory usage comparison: > > > > Kernel | Policy load | Policy load | Mem usage | Mem usage | openbench > > | | (-unconfined) | | (-unconfined) | (createfiles) > > -----------------|-------------|---------------|-----------|---------------|-------------- > > reference | 1,30s | 0,91s | 90MB | 77MB | 55 us/file > > rhashtable patch | 0.98s | 0,85s | 85MB | 75MB | 38 us/file > > this patch | 0,95s | 0,87s | 75MB | 75MB | 40 us/file > > > > (Memory usage is measured after boot. With SELinux disabled the memory > > usage was ~60MB on the same system.) > > > > [1] https://lore.kernel.org/selinux/20200116213937.77795-1-dev@xxxxxxxxxx/T/ > > > > Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx> > > --- > > security/selinux/ss/policydb.c | 175 ++++++++++++++++++++------------- > > security/selinux/ss/policydb.h | 8 +- > > security/selinux/ss/services.c | 16 +-- > > 3 files changed, 120 insertions(+), 79 deletions(-) > > > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > > index 981797bfc547..62283033bb7d 100644 > > --- a/security/selinux/ss/policydb.c > > +++ b/security/selinux/ss/policydb.c > > @@ -1882,64 +1884,93 @@ out: > > > > static int filename_trans_read_one(struct policydb *p, void *fp) > > { > <snip> > > + exists = false; > > + last = NULL; > > + datum = hashtab_search(p->filename_trans, &key); > > + while (datum) { > > + if (ebitmap_get_bit(&datum->stypes, stype - 1)) { > > + exists = true; > > + break; > > + } > > + if (datum->otype == otype) { > > + last = NULL; > > Why set last to NULL here? Seemingly unused afterward if datum is non-NULL? Good point. I don't like unnecessary code, so I'll wait a while for possible other comments and then do a v2. -- Ondrej Mosnacek <omosnace at redhat dot com> Software Engineer, Security Technologies Red Hat, Inc.
