RE: [RFC] mmap file_contexts and property_contexts:
> > -Original Message- > > From: Roberts, William C > > Sent: Monday, September 19, 2016 2:45 PM > > To: selinux@tycho.nsa.gov; seandroid-l...@tycho.nsa.gov; > > s...@tycho.nsa.gov; jda...@google.com > > Cc: Roberts, William C> > Subject: [RFC] mmap file_contexts and property_contexts: > > > > From: William Roberts > > > > THIS IS WIP... > > > > Rather than using stdio and making copies, just mmap the files and use > > the pointers in place. The affect of this change, is that text file > > load time is now faster than binary load time by 4.7% when testing > > with a file_contexts file from the Android tree. Note that the Android > > doesn't > use monstrous regexs. > > > > Times are the average of 3 runs. > > > > BEFORE: > > Text file allocs: 114803 > > Text file load time: 0.266101 > > Bin file allocs: 93073 > > Bin file load time: 0.248757667 > > > > AFTER: > > Text file allocs: 103933 > > Text file load time: 0.236192667 > > Bin file allocs: 87645 > > Bin file load time: .247607333 Another potential issue, is that mmaping with textfiles also has the sideffect of mapping in all the spaces or tabs, etc used, between fields, so on files with heavy whitespace, it could be really memory intensive. I think Janis's patch that has -r option is a more useful way to go. Without precompiled Regex's its essentially a textfile without the whitespace, and it automatically would get pulled into the mmap interface. I'm abandoning this, since we already get this functionality, without the negatives. Janis, IIRC, you are re-configuring your patch so -r omits pre-compiled regex's (submitted I believe on the list and awaiting other patches) and adding an arch string so we can Detect when we need to recompile the regular expressions if they are supplied but unusable. I think we may want to investigate to have the "omit pre-compiled regex's" work on PCRE library variants as well, but I doubt it would be used since Android is moving to PCRE2. ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
RE: [RFC] mmap file_contexts and property_contexts:
> -Original Message- > From: Stephen Smalley [mailto:s...@tycho.nsa.gov] > Sent: Tuesday, September 20, 2016 5:56 AM > To: Roberts, William C <william.c.robe...@intel.com>; selinux@tycho.nsa.gov; > seandroid-l...@tycho.nsa.gov; jda...@google.com > Subject: Re: [RFC] mmap file_contexts and property_contexts: > > On 09/19/2016 05:45 PM, william.c.robe...@intel.com wrote: > > From: William Roberts <william.c.robe...@intel.com> > > > > THIS IS WIP... > > > > Rather than using stdio and making copies, just mmap the files and use > > the pointers in place. The affect of this change, is that text file s/affect/effect > > load time is now faster than binary load time by 4.7% when testing > > with a file_contexts file from the Android tree. Note that the Android > > doesn't use monstrous regexs. > > > > Times are the average of 3 runs. > > > > BEFORE: > > Text file allocs: 114803 > > Text file load time: 0.266101 > > Bin file allocs: 93073 > > Bin file load time: 0.248757667 > > > > AFTER: > > Text file allocs: 103933 > > Text file load time: 0.236192667 > > Bin file allocs: 87645 > > Bin file load time: .247607333 > > > > THINGS TO DO: > > 1. What's arm performance like? > > 2. What interfaces to backends are busted by this (if any)? > > 3. Test Android Properties > > 4. Im pretty sure this breaks sefcontext_compile, fix. > > 5. Test with PCRE2 enabled. > > 6. Spell check this message! > > 7. Run checkpatch > > > > Signed-off-by: William Roberts <william.c.robe...@intel.com> > > --- > > libselinux/src/label.c | 2 - > > libselinux/src/label_android_property.c | 22 ++--- > > libselinux/src/label_file.c | 140 > > +++- > > libselinux/src/label_file.h | 66 +-- > > libselinux/src/label_internal.h | 3 +- > > libselinux/src/label_support.c | 79 -- > > 6 files changed, 172 insertions(+), 140 deletions(-) > > > > > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c > > index 7156825..4dc440e 100644 > > --- a/libselinux/src/label_file.c > > +++ b/libselinux/src/label_file.c > > @@ -96,43 +96,64 @@ static int nodups_specs(struct saved_data *data, const > char *path) > > return rc; > > } > > > > -static int process_text_file(FILE *fp, const char *prefix, > > -struct selabel_handle *rec, const char *path) > > +static inline struct saved_data *rec_to_data(struct selabel_handle > > +*rec) { > > + return (struct saved_data *)rec->data; } > > + > > +static char *mmap_area_get_line(struct mmap_area *area) { > > + size_t len = area->next_len; > > + if (!len) > > + return NULL; > > + > > + char *start = area->next_addr; > > + char *end = memchr(start, '\n', len); > > + > > + /* the file may not end with a newline */ > > + if (!end) > > + end = (char *)area->next_addr + len - 1; > > + > > + *end = '\0'; > > Couldn't this clobber the last character in the file if the file does not end > with a > newline? Yeah I guess I really can't handle this case without increasing the mapping size or allocating A new buffer to copy into. Considering, that the only time a file not ending with a newline was my own stupid mistake, I am not too worried about the not ending newline, especially considering that the Android build process handles it if it's missing. Perhaps I could always add a terminating \0 to the mapping by growing it by one during mmap And explicitly setting the byte. > > > + /* len includes null byte */ > > + len = end - start; > > + > > + area->next_len -= len + 1; > > + area->next_addr = ++end; > > + return start; > > +} > > + > > +static int process_text_file(const char *path, const char *prefix, > > + struct selabel_handle *rec) > > { > > int rc; > > - size_t line_len; > > unsigned int lineno = 0; > > - char *line_buf = NULL; > > + char *line_buf; > > + struct mmap_area *areas = rec_to_data(rec)->mmap_areas; > > + > > + /* mmap_area_get_line() and process_line() require mutable string > pointers */ > > + rc = mprotect(areas->addr, areas->len, PROT_READ | PROT_WRITE); > > Just map it with PROT_READ|PROT_WRITE in the beginning. Yep. ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
RE: [RFC] mmap file_contexts and property_contexts:
> -Original Message- > From: Stephen Smalley [mailto:s...@tycho.nsa.gov] > Sent: Tuesday, September 20, 2016 6:18 AM > To: Roberts, William C <william.c.robe...@intel.com>; selinux@tycho.nsa.gov; > seandroid-l...@tycho.nsa.gov; jda...@google.com > Subject: Re: [RFC] mmap file_contexts and property_contexts: > > On 09/19/2016 05:51 PM, Roberts, William C wrote: > > FYI I only tested this with checkfc... > > Evidently. matchpathcon and sefcontext_compile both report calls to > free() on invalid pointers and abort. That doesn’t surprise me, I only tested the checkfc usages. Hence #4 in todo's > > > > >> -Original Message- > >> From: Roberts, William C > >> Sent: Monday, September 19, 2016 2:45 PM > >> To: selinux@tycho.nsa.gov; seandroid-l...@tycho.nsa.gov; > >> s...@tycho.nsa.gov; jda...@google.com > >> Cc: Roberts, William C <william.c.robe...@intel.com> > >> Subject: [RFC] mmap file_contexts and property_contexts: > >> > >> From: William Roberts <william.c.robe...@intel.com> > >> > >> THIS IS WIP... > >> > >> Rather than using stdio and making copies, just mmap the files and > >> use the pointers in place. The affect of this change, is that text > >> file load time is now faster than binary load time by 4.7% when > >> testing with a file_contexts file from the Android tree. Note that the > >> Android > doesn't use monstrous regexs. > >> > >> Times are the average of 3 runs. > >> > >> BEFORE: > >> Text file allocs: 114803 > >> Text file load time: 0.266101 > >> Bin file allocs: 93073 > >> Bin file load time: 0.248757667 > >> > >> AFTER: > >> Text file allocs: 103933 > >> Text file load time: 0.236192667 > >> Bin file allocs: 87645 > >> Bin file load time: .247607333 > >> > >> THINGS TO DO: > >> 1. What's arm performance like? > >> 2. What interfaces to backends are busted by this (if any)? > >> 3. Test Android Properties > >> 4. Im pretty sure this breaks sefcontext_compile, fix. > >> 5. Test with PCRE2 enabled. > >> 6. Spell check this message! > >> 7. Run checkpatch > >> > >> Signed-off-by: William Roberts <william.c.robe...@intel.com> > >> --- > >> libselinux/src/label.c | 2 - > >> libselinux/src/label_android_property.c | 22 ++--- > >> libselinux/src/label_file.c | 140 > >> +++- > >> libselinux/src/label_file.h | 66 +-- > >> libselinux/src/label_internal.h | 3 +- > >> libselinux/src/label_support.c | 79 -- > >> 6 files changed, 172 insertions(+), 140 deletions(-) > >> > >> diff --git a/libselinux/src/label.c b/libselinux/src/label.c index > >> 963bfcb..d767b49 > >> 100644 > >> --- a/libselinux/src/label.c > >> +++ b/libselinux/src/label.c > >> @@ -15,8 +15,6 @@ > >> #include "callbacks.h" > >> #include "label_internal.h" > >> > >> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) > >> - > >> typedef int (*selabel_initfunc)(struct selabel_handle *rec, > >>const struct selinux_opt *opts, > >>unsigned nopts); > >> diff --git a/libselinux/src/label_android_property.c > >> b/libselinux/src/label_android_property.c > >> index 290b438..2aac394 100644 > >> --- a/libselinux/src/label_android_property.c > >> +++ b/libselinux/src/label_android_property.c > >> @@ -85,13 +85,19 @@ static int process_line(struct selabel_handle *rec, > >>int pass, unsigned lineno) > >> { > >>int items; > >> - char *prop = NULL, *context = NULL; > >> + union { > >> + struct { > >> + char *prop; > >> + char *context; > >> + }; > >> + char *array[2]; > >> + } found = { .array = { 0 } }; > >>struct saved_data *data = (struct saved_data *)rec->data; > >>spec_t *spec_arr = data->spec_arr; > >>unsigned int nspec = data->nspec; > >>const char *errbuf = NULL; > >> > >> - items = read_spec_entries(line_buf, , 2, , ); > >> + items = read_spec_entries(line_buf, , > >> +ARRAY_SIZE(found.array), found.array); > &
Re: [RFC] mmap file_contexts and property_contexts:
On 09/19/2016 05:51 PM, Roberts, William C wrote: > FYI I only tested this with checkfc... Evidently. matchpathcon and sefcontext_compile both report calls to free() on invalid pointers and abort. > >> -Original Message- >> From: Roberts, William C >> Sent: Monday, September 19, 2016 2:45 PM >> To: selinux@tycho.nsa.gov; seandroid-l...@tycho.nsa.gov; s...@tycho.nsa.gov; >> jda...@google.com >> Cc: Roberts, William C>> Subject: [RFC] mmap file_contexts and property_contexts: >> >> From: William Roberts >> >> THIS IS WIP... >> >> Rather than using stdio and making copies, just mmap the files and use the >> pointers in place. The affect of this change, is that text file load time is >> now faster >> than binary load time by 4.7% when testing with a file_contexts file from the >> Android tree. Note that the Android doesn't use monstrous regexs. >> >> Times are the average of 3 runs. >> >> BEFORE: >> Text file allocs: 114803 >> Text file load time: 0.266101 >> Bin file allocs: 93073 >> Bin file load time: 0.248757667 >> >> AFTER: >> Text file allocs: 103933 >> Text file load time: 0.236192667 >> Bin file allocs: 87645 >> Bin file load time: .247607333 >> >> THINGS TO DO: >> 1. What's arm performance like? >> 2. What interfaces to backends are busted by this (if any)? >> 3. Test Android Properties >> 4. Im pretty sure this breaks sefcontext_compile, fix. >> 5. Test with PCRE2 enabled. >> 6. Spell check this message! >> 7. Run checkpatch >> >> Signed-off-by: William Roberts >> --- >> libselinux/src/label.c | 2 - >> libselinux/src/label_android_property.c | 22 ++--- >> libselinux/src/label_file.c | 140 >> +++- >> libselinux/src/label_file.h | 66 +-- >> libselinux/src/label_internal.h | 3 +- >> libselinux/src/label_support.c | 79 -- >> 6 files changed, 172 insertions(+), 140 deletions(-) >> >> diff --git a/libselinux/src/label.c b/libselinux/src/label.c index >> 963bfcb..d767b49 >> 100644 >> --- a/libselinux/src/label.c >> +++ b/libselinux/src/label.c >> @@ -15,8 +15,6 @@ >> #include "callbacks.h" >> #include "label_internal.h" >> >> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) >> - >> typedef int (*selabel_initfunc)(struct selabel_handle *rec, >> const struct selinux_opt *opts, >> unsigned nopts); >> diff --git a/libselinux/src/label_android_property.c >> b/libselinux/src/label_android_property.c >> index 290b438..2aac394 100644 >> --- a/libselinux/src/label_android_property.c >> +++ b/libselinux/src/label_android_property.c >> @@ -85,13 +85,19 @@ static int process_line(struct selabel_handle *rec, >> int pass, unsigned lineno) >> { >> int items; >> -char *prop = NULL, *context = NULL; >> +union { >> +struct { >> +char *prop; >> +char *context; >> +}; >> +char *array[2]; >> +} found = { .array = { 0 } }; >> struct saved_data *data = (struct saved_data *)rec->data; >> spec_t *spec_arr = data->spec_arr; >> unsigned int nspec = data->nspec; >> const char *errbuf = NULL; >> >> -items = read_spec_entries(line_buf, , 2, , ); >> +items = read_spec_entries(line_buf, , ARRAY_SIZE(found.array), >> +found.array); >> if (items < 0) { >> items = errno; >> selinux_log(SELINUX_ERROR, >> @@ -108,18 +114,14 @@ static int process_line(struct selabel_handle *rec, >> selinux_log(SELINUX_ERROR, >> "%s: line %u is missing fields\n", path, >> lineno); >> -free(prop); >> errno = EINVAL; >> return -1; >> } >> >> -if (pass == 0) { >> -free(prop); >> -free(context); >> -} else if (pass == 1) { >> +if (pass == 1) { >> /* On the second pass, process and store the specification in >> spec. */ >> -spec_arr[nspec].property_key = prop; >> -spec_arr[nspec].lr.ctx_raw = context; >> +spec_arr[nspec].property_key = found.prop; >> +spec_arr[nspec].lr.ctx_raw = found.context; >> >> if (rec->validating) { >> if (selabel_validate(rec, _arr[nspec].lr) < 0) { >> @@ - >> 234,7 +236,7 @@ static void closef(struct selabel_handle *rec) >> for (i = 0; i < data->nspec; i++) { >> spec = >spec_arr[i]; >> free(spec->property_key); >> -free(spec->lr.ctx_raw); >> +//free(spec->lr.ctx_raw); >> free(spec->lr.ctx_trans); >> } >> >> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c index >> 7156825..4dc440e 100644 >> --- a/libselinux/src/label_file.c >> +++
Re: [RFC] mmap file_contexts and property_contexts:
On 09/19/2016 05:45 PM, william.c.robe...@intel.com wrote: > From: William Roberts> > THIS IS WIP... > > Rather than using stdio and making copies, just mmap the files > and use the pointers in place. The affect of this change, is that > text file load time is now faster than binary load time by 4.7% > when testing with a file_contexts file from the Android tree. Note > that the Android doesn't use monstrous regexs. > > Times are the average of 3 runs. > > BEFORE: > Text file allocs: 114803 > Text file load time: 0.266101 > Bin file allocs: 93073 > Bin file load time: 0.248757667 > > AFTER: > Text file allocs: 103933 > Text file load time: 0.236192667 > Bin file allocs: 87645 > Bin file load time: .247607333 > > THINGS TO DO: > 1. What's arm performance like? > 2. What interfaces to backends are busted by this (if any)? > 3. Test Android Properties > 4. Im pretty sure this breaks sefcontext_compile, fix. > 5. Test with PCRE2 enabled. > 6. Spell check this message! > 7. Run checkpatch > > Signed-off-by: William Roberts > --- > libselinux/src/label.c | 2 - > libselinux/src/label_android_property.c | 22 ++--- > libselinux/src/label_file.c | 140 > +++- > libselinux/src/label_file.h | 66 +-- > libselinux/src/label_internal.h | 3 +- > libselinux/src/label_support.c | 79 -- > 6 files changed, 172 insertions(+), 140 deletions(-) > > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c > index 7156825..4dc440e 100644 > --- a/libselinux/src/label_file.c > +++ b/libselinux/src/label_file.c > @@ -96,43 +96,64 @@ static int nodups_specs(struct saved_data *data, const > char *path) > return rc; > } > > -static int process_text_file(FILE *fp, const char *prefix, > - struct selabel_handle *rec, const char *path) > +static inline struct saved_data *rec_to_data(struct selabel_handle *rec) > +{ > + return (struct saved_data *)rec->data; > +} > + > +static char *mmap_area_get_line(struct mmap_area *area) > +{ > + size_t len = area->next_len; > + if (!len) > + return NULL; > + > + char *start = area->next_addr; > + char *end = memchr(start, '\n', len); > + > + /* the file may not end with a newline */ > + if (!end) > + end = (char *)area->next_addr + len - 1; > + > + *end = '\0'; Couldn't this clobber the last character in the file if the file does not end with a newline? > + /* len includes null byte */ > + len = end - start; > + > + area->next_len -= len + 1; > + area->next_addr = ++end; > + return start; > +} > + > +static int process_text_file(const char *path, const char *prefix, > + struct selabel_handle *rec) > { > int rc; > - size_t line_len; > unsigned int lineno = 0; > - char *line_buf = NULL; > + char *line_buf; > + struct mmap_area *areas = rec_to_data(rec)->mmap_areas; > + > + /* mmap_area_get_line() and process_line() require mutable string > pointers */ > + rc = mprotect(areas->addr, areas->len, PROT_READ | PROT_WRITE); Just map it with PROT_READ|PROT_WRITE in the beginning. ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [RFC] mmap file_contexts and property_contexts:
On 09/20/2016 02:27 AM, William Roberts wrote: > On Sep 19, 2016 22:25, "Jason Zaman"wrote: >> >> On 20 Sep 2016 12:50 pm, "William Roberts" > wrote: >>> >>> On Sep 19, 2016 21:16, "Jason Zaman" wrote: On 20 Sep 2016 5:47 am, wrote: > > From: William Roberts > > THIS IS WIP... > > Rather than using stdio and making copies, just mmap the files > and use the pointers in place. The affect of this change, is that > text file load time is now faster than binary load time by 4.7% > when testing with a file_contexts file from the Android tree. Note > that the Android doesn't use monstrous regexs. > > Times are the average of 3 runs. > > BEFORE: > Text file allocs: 114803 > Text file load time: 0.266101 > Bin file allocs: 93073 > Bin file load time: 0.248757667 > > AFTER: > Text file allocs: 103933 > Text file load time: 0.236192667 > Bin file allocs: 87645 > Bin file load time: .247607333 Do you have the scripts that generated these stats so I can play with > it too? These stats are from android right? Do you also have a comparison > for refpolicy too? >>> >>> For generating these I used checkfc.c from the Android tree. I used > valgrind to measure allocations and clock to measure the time in > selabel_open(). >> >> Okay cool I'll fetch that and give it a whirl when I get time. >> I haven't looked that closely yet but just realised, will this need > new perms because of the mmap? If it does, can you send a patch to > refpolicy? >>> >>> I'm confused, mmap is not a permission, even if it was the binary path > already was doing an mmap, so the permission would have been there. We're > just making it so it always mmaps. >> >> Yeah but mmap needs execute perms sometimes (always?). I am out so just > wanted to send an email before I forgot. If it was mmaping already then > there is nothing to worry about :). > > Mmap would only need execute if you attempted to set the prot bits to > execute it use mprotect to change the mapping. Then things like execmod > might come I to play if the mapping was ever writable. The only case where mmap without PROT_EXEC would require execute would be if the process has READ_IMPLIES_EXEC set in its personality. Typically only for programs with the executable stack flag set. Anyway, it is already mmap'ing file_contexts.bin so there shouldn't be an issue here. ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [RFC] mmap file_contexts and property_contexts:
On Sep 19, 2016 22:25, "Jason Zaman"wrote: > > On 20 Sep 2016 12:50 pm, "William Roberts" wrote: > > > > On Sep 19, 2016 21:16, "Jason Zaman" wrote: > > > > > > On 20 Sep 2016 5:47 am, wrote: > > > > > > > > From: William Roberts > > > > > > > > THIS IS WIP... > > > > > > > > Rather than using stdio and making copies, just mmap the files > > > > and use the pointers in place. The affect of this change, is that > > > > text file load time is now faster than binary load time by 4.7% > > > > when testing with a file_contexts file from the Android tree. Note > > > > that the Android doesn't use monstrous regexs. > > > > > > > > Times are the average of 3 runs. > > > > > > > > BEFORE: > > > > Text file allocs: 114803 > > > > Text file load time: 0.266101 > > > > Bin file allocs: 93073 > > > > Bin file load time: 0.248757667 > > > > > > > > AFTER: > > > > Text file allocs: 103933 > > > > Text file load time: 0.236192667 > > > > Bin file allocs: 87645 > > > > Bin file load time: .247607333 > > > > > > Do you have the scripts that generated these stats so I can play with it too? These stats are from android right? Do you also have a comparison for refpolicy too? > > > > For generating these I used checkfc.c from the Android tree. I used valgrind to measure allocations and clock to measure the time in selabel_open(). > > Okay cool I'll fetch that and give it a whirl when I get time. > > > > > > > I haven't looked that closely yet but just realised, will this need new perms because of the mmap? If it does, can you send a patch to refpolicy? > > > > I'm confused, mmap is not a permission, even if it was the binary path already was doing an mmap, so the permission would have been there. We're just making it so it always mmaps. > > Yeah but mmap needs execute perms sometimes (always?). I am out so just wanted to send an email before I forgot. If it was mmaping already then there is nothing to worry about :). Mmap would only need execute if you attempted to set the prot bits to execute it use mprotect to change the mapping. Then things like execmod might come I to play if the mapping was ever writable. > > -- Jason > > > > > > > THINGS TO DO: > > > > 1. What's arm performance like? > > > > 2. What interfaces to backends are busted by this (if any)? > > > > 3. Test Android Properties > > > > 4. Im pretty sure this breaks sefcontext_compile, fix. > > > > 5. Test with PCRE2 enabled. > > > > 6. Spell check this message! > > > > 7. Run checkpatch > > > > > > > > Signed-off-by: William Roberts > > > > --- > > > > libselinux/src/label.c | 2 - > > > > libselinux/src/label_android_property.c | 22 ++--- > > > > libselinux/src/label_file.c | 140 +++- > > > > libselinux/src/label_file.h | 66 +-- > > > > libselinux/src/label_internal.h | 3 +- > > > > libselinux/src/label_support.c | 79 -- > > > > 6 files changed, 172 insertions(+), 140 deletions(-) > > > > > > > > diff --git a/libselinux/src/label.c b/libselinux/src/label.c > > > > index 963bfcb..d767b49 100644 > > > > --- a/libselinux/src/label.c > > > > +++ b/libselinux/src/label.c > > > > @@ -15,8 +15,6 @@ > > > > #include "callbacks.h" > > > > #include "label_internal.h" > > > > > > > > -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) > > > > - > > > > typedef int (*selabel_initfunc)(struct selabel_handle *rec, > > > > const struct selinux_opt *opts, > > > > unsigned nopts); > > > > diff --git a/libselinux/src/label_android_property.c b/libselinux/src/label_android_property.c > > > > index 290b438..2aac394 100644 > > > > --- a/libselinux/src/label_android_property.c > > > > +++ b/libselinux/src/label_android_property.c > > > > @@ -85,13 +85,19 @@ static int process_line(struct selabel_handle *rec, > > > > int pass, unsigned lineno) > > > > { > > > > int items; > > > > - char *prop = NULL, *context = NULL; > > > > + union { > > > > + struct { > > > > + char *prop; > > > > + char *context; > > > > + }; > > > > + char *array[2]; > > > > + } found = { .array = { 0 } }; > > > > struct saved_data *data = (struct saved_data *)rec->data; > > > > spec_t *spec_arr = data->spec_arr; > > > > unsigned int nspec = data->nspec; > > > > const char *errbuf = NULL; > > > > > > > > - items = read_spec_entries(line_buf, , 2, , ); > > > > + items = read_spec_entries(line_buf, , ARRAY_SIZE(found.array), found.array); > > > > if (items < 0) { > > > > items = errno; > > > > selinux_log(SELINUX_ERROR, > > > > @@ -108,18 +114,14 @@
Re: [RFC] mmap file_contexts and property_contexts:
On 20 Sep 2016 12:50 pm, "William Roberts"wrote: > > On Sep 19, 2016 21:16, "Jason Zaman" wrote: > > > > On 20 Sep 2016 5:47 am, wrote: > > > > > > From: William Roberts > > > > > > THIS IS WIP... > > > > > > Rather than using stdio and making copies, just mmap the files > > > and use the pointers in place. The affect of this change, is that > > > text file load time is now faster than binary load time by 4.7% > > > when testing with a file_contexts file from the Android tree. Note > > > that the Android doesn't use monstrous regexs. > > > > > > Times are the average of 3 runs. > > > > > > BEFORE: > > > Text file allocs: 114803 > > > Text file load time: 0.266101 > > > Bin file allocs: 93073 > > > Bin file load time: 0.248757667 > > > > > > AFTER: > > > Text file allocs: 103933 > > > Text file load time: 0.236192667 > > > Bin file allocs: 87645 > > > Bin file load time: .247607333 > > > > Do you have the scripts that generated these stats so I can play with it too? These stats are from android right? Do you also have a comparison for refpolicy too? > > For generating these I used checkfc.c from the Android tree. I used valgrind to measure allocations and clock to measure the time in selabel_open(). Okay cool I'll fetch that and give it a whirl when I get time. > > > > I haven't looked that closely yet but just realised, will this need new perms because of the mmap? If it does, can you send a patch to refpolicy? > > I'm confused, mmap is not a permission, even if it was the binary path already was doing an mmap, so the permission would have been there. We're just making it so it always mmaps. Yeah but mmap needs execute perms sometimes (always?). I am out so just wanted to send an email before I forgot. If it was mmaping already then there is nothing to worry about :). -- Jason > > > > > THINGS TO DO: > > > 1. What's arm performance like? > > > 2. What interfaces to backends are busted by this (if any)? > > > 3. Test Android Properties > > > 4. Im pretty sure this breaks sefcontext_compile, fix. > > > 5. Test with PCRE2 enabled. > > > 6. Spell check this message! > > > 7. Run checkpatch > > > > > > Signed-off-by: William Roberts > > > --- > > > libselinux/src/label.c | 2 - > > > libselinux/src/label_android_property.c | 22 ++--- > > > libselinux/src/label_file.c | 140 +++- > > > libselinux/src/label_file.h | 66 +-- > > > libselinux/src/label_internal.h | 3 +- > > > libselinux/src/label_support.c | 79 -- > > > 6 files changed, 172 insertions(+), 140 deletions(-) > > > > > > diff --git a/libselinux/src/label.c b/libselinux/src/label.c > > > index 963bfcb..d767b49 100644 > > > --- a/libselinux/src/label.c > > > +++ b/libselinux/src/label.c > > > @@ -15,8 +15,6 @@ > > > #include "callbacks.h" > > > #include "label_internal.h" > > > > > > -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) > > > - > > > typedef int (*selabel_initfunc)(struct selabel_handle *rec, > > > const struct selinux_opt *opts, > > > unsigned nopts); > > > diff --git a/libselinux/src/label_android_property.c b/libselinux/src/label_android_property.c > > > index 290b438..2aac394 100644 > > > --- a/libselinux/src/label_android_property.c > > > +++ b/libselinux/src/label_android_property.c > > > @@ -85,13 +85,19 @@ static int process_line(struct selabel_handle *rec, > > > int pass, unsigned lineno) > > > { > > > int items; > > > - char *prop = NULL, *context = NULL; > > > + union { > > > + struct { > > > + char *prop; > > > + char *context; > > > + }; > > > + char *array[2]; > > > + } found = { .array = { 0 } }; > > > struct saved_data *data = (struct saved_data *)rec->data; > > > spec_t *spec_arr = data->spec_arr; > > > unsigned int nspec = data->nspec; > > > const char *errbuf = NULL; > > > > > > - items = read_spec_entries(line_buf, , 2, , ); > > > + items = read_spec_entries(line_buf, , ARRAY_SIZE(found.array), found.array); > > > if (items < 0) { > > > items = errno; > > > selinux_log(SELINUX_ERROR, > > > @@ -108,18 +114,14 @@ static int process_line(struct selabel_handle *rec, > > > selinux_log(SELINUX_ERROR, > > > "%s: line %u is missing fields\n", path, > > > lineno); > > > - free(prop); > > > errno = EINVAL; > > > return -1; > > > } > > > > > > - if (pass == 0) { > > > - free(prop); > > > - free(context); > > > - } else
Re: [RFC] mmap file_contexts and property_contexts:
On Sep 19, 2016 21:16, "Jason Zaman"wrote: > > On 20 Sep 2016 5:47 am, wrote: > > > > From: William Roberts > > > > THIS IS WIP... > > > > Rather than using stdio and making copies, just mmap the files > > and use the pointers in place. The affect of this change, is that > > text file load time is now faster than binary load time by 4.7% > > when testing with a file_contexts file from the Android tree. Note > > that the Android doesn't use monstrous regexs. > > > > Times are the average of 3 runs. > > > > BEFORE: > > Text file allocs: 114803 > > Text file load time: 0.266101 > > Bin file allocs: 93073 > > Bin file load time: 0.248757667 > > > > AFTER: > > Text file allocs: 103933 > > Text file load time: 0.236192667 > > Bin file allocs: 87645 > > Bin file load time: .247607333 > > Do you have the scripts that generated these stats so I can play with it too? These stats are from android right? Do you also have a comparison for refpolicy too? For generating these I used checkfc.c from the Android tree. I used valgrind to measure allocations and clock to measure the time in selabel_open(). > > I haven't looked that closely yet but just realised, will this need new perms because of the mmap? If it does, can you send a patch to refpolicy? I'm confused, mmap is not a permission, even if it was the binary path already was doing an mmap, so the permission would have been there. We're just making it so it always mmaps. > > -- Jason > > > THINGS TO DO: > > 1. What's arm performance like? > > 2. What interfaces to backends are busted by this (if any)? > > 3. Test Android Properties > > 4. Im pretty sure this breaks sefcontext_compile, fix. > > 5. Test with PCRE2 enabled. > > 6. Spell check this message! > > 7. Run checkpatch > > > > Signed-off-by: William Roberts > > --- > > libselinux/src/label.c | 2 - > > libselinux/src/label_android_property.c | 22 ++--- > > libselinux/src/label_file.c | 140 +++- > > libselinux/src/label_file.h | 66 +-- > > libselinux/src/label_internal.h | 3 +- > > libselinux/src/label_support.c | 79 -- > > 6 files changed, 172 insertions(+), 140 deletions(-) > > > > diff --git a/libselinux/src/label.c b/libselinux/src/label.c > > index 963bfcb..d767b49 100644 > > --- a/libselinux/src/label.c > > +++ b/libselinux/src/label.c > > @@ -15,8 +15,6 @@ > > #include "callbacks.h" > > #include "label_internal.h" > > > > -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) > > - > > typedef int (*selabel_initfunc)(struct selabel_handle *rec, > > const struct selinux_opt *opts, > > unsigned nopts); > > diff --git a/libselinux/src/label_android_property.c b/libselinux/src/label_android_property.c > > index 290b438..2aac394 100644 > > --- a/libselinux/src/label_android_property.c > > +++ b/libselinux/src/label_android_property.c > > @@ -85,13 +85,19 @@ static int process_line(struct selabel_handle *rec, > > int pass, unsigned lineno) > > { > > int items; > > - char *prop = NULL, *context = NULL; > > + union { > > + struct { > > + char *prop; > > + char *context; > > + }; > > + char *array[2]; > > + } found = { .array = { 0 } }; > > struct saved_data *data = (struct saved_data *)rec->data; > > spec_t *spec_arr = data->spec_arr; > > unsigned int nspec = data->nspec; > > const char *errbuf = NULL; > > > > - items = read_spec_entries(line_buf, , 2, , ); > > + items = read_spec_entries(line_buf, , ARRAY_SIZE(found.array), found.array); > > if (items < 0) { > > items = errno; > > selinux_log(SELINUX_ERROR, > > @@ -108,18 +114,14 @@ static int process_line(struct selabel_handle *rec, > > selinux_log(SELINUX_ERROR, > > "%s: line %u is missing fields\n", path, > > lineno); > > - free(prop); > > errno = EINVAL; > > return -1; > > } > > > > - if (pass == 0) { > > - free(prop); > > - free(context); > > - } else if (pass == 1) { > > + if (pass == 1) { > > /* On the second pass, process and store the specification in spec. */ > > - spec_arr[nspec].property_key = prop; > > - spec_arr[nspec].lr.ctx_raw = context; > > + spec_arr[nspec].property_key = found.prop; > > + spec_arr[nspec].lr.ctx_raw = found.context; > > > > if (rec->validating) { > > if (selabel_validate(rec, _arr[nspec].lr) < 0) { > > @@ -234,7 +236,7 @@
Re: [RFC] mmap file_contexts and property_contexts:
On 20 Sep 2016 5:47 am,wrote: > > From: William Roberts > > THIS IS WIP... > > Rather than using stdio and making copies, just mmap the files > and use the pointers in place. The affect of this change, is that > text file load time is now faster than binary load time by 4.7% > when testing with a file_contexts file from the Android tree. Note > that the Android doesn't use monstrous regexs. > > Times are the average of 3 runs. > > BEFORE: > Text file allocs: 114803 > Text file load time: 0.266101 > Bin file allocs: 93073 > Bin file load time: 0.248757667 > > AFTER: > Text file allocs: 103933 > Text file load time: 0.236192667 > Bin file allocs: 87645 > Bin file load time: .247607333 Do you have the scripts that generated these stats so I can play with it too? These stats are from android right? Do you also have a comparison for refpolicy too? I haven't looked that closely yet but just realised, will this need new perms because of the mmap? If it does, can you send a patch to refpolicy? -- Jason > THINGS TO DO: > 1. What's arm performance like? > 2. What interfaces to backends are busted by this (if any)? > 3. Test Android Properties > 4. Im pretty sure this breaks sefcontext_compile, fix. > 5. Test with PCRE2 enabled. > 6. Spell check this message! > 7. Run checkpatch > > Signed-off-by: William Roberts > --- > libselinux/src/label.c | 2 - > libselinux/src/label_android_property.c | 22 ++--- > libselinux/src/label_file.c | 140 +++- > libselinux/src/label_file.h | 66 +-- > libselinux/src/label_internal.h | 3 +- > libselinux/src/label_support.c | 79 -- > 6 files changed, 172 insertions(+), 140 deletions(-) > > diff --git a/libselinux/src/label.c b/libselinux/src/label.c > index 963bfcb..d767b49 100644 > --- a/libselinux/src/label.c > +++ b/libselinux/src/label.c > @@ -15,8 +15,6 @@ > #include "callbacks.h" > #include "label_internal.h" > > -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) > - > typedef int (*selabel_initfunc)(struct selabel_handle *rec, > const struct selinux_opt *opts, > unsigned nopts); > diff --git a/libselinux/src/label_android_property.c b/libselinux/src/label_android_property.c > index 290b438..2aac394 100644 > --- a/libselinux/src/label_android_property.c > +++ b/libselinux/src/label_android_property.c > @@ -85,13 +85,19 @@ static int process_line(struct selabel_handle *rec, > int pass, unsigned lineno) > { > int items; > - char *prop = NULL, *context = NULL; > + union { > + struct { > + char *prop; > + char *context; > + }; > + char *array[2]; > + } found = { .array = { 0 } }; > struct saved_data *data = (struct saved_data *)rec->data; > spec_t *spec_arr = data->spec_arr; > unsigned int nspec = data->nspec; > const char *errbuf = NULL; > > - items = read_spec_entries(line_buf, , 2, , ); > + items = read_spec_entries(line_buf, , ARRAY_SIZE(found.array), found.array); > if (items < 0) { > items = errno; > selinux_log(SELINUX_ERROR, > @@ -108,18 +114,14 @@ static int process_line(struct selabel_handle *rec, > selinux_log(SELINUX_ERROR, > "%s: line %u is missing fields\n", path, > lineno); > - free(prop); > errno = EINVAL; > return -1; > } > > - if (pass == 0) { > - free(prop); > - free(context); > - } else if (pass == 1) { > + if (pass == 1) { > /* On the second pass, process and store the specification in spec. */ > - spec_arr[nspec].property_key = prop; > - spec_arr[nspec].lr.ctx_raw = context; > + spec_arr[nspec].property_key = found.prop; > + spec_arr[nspec].lr.ctx_raw = found.context; > > if (rec->validating) { > if (selabel_validate(rec, _arr[nspec].lr) < 0) { > @@ -234,7 +236,7 @@ static void closef(struct selabel_handle *rec) > for (i = 0; i < data->nspec; i++) { > spec = >spec_arr[i]; > free(spec->property_key); > - free(spec->lr.ctx_raw); > + //free(spec->lr.ctx_raw); > free(spec->lr.ctx_trans); > } > > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c > index 7156825..4dc440e 100644 > --- a/libselinux/src/label_file.c > +++ b/libselinux/src/label_file.c > @@ -96,43 +96,64 @@ static int nodups_specs(struct saved_data *data, const char *path) > return rc; > } > > -static int
RE: [RFC] mmap file_contexts and property_contexts:
FYI I only tested this with checkfc... > -Original Message- > From: Roberts, William C > Sent: Monday, September 19, 2016 2:45 PM > To: selinux@tycho.nsa.gov; seandroid-l...@tycho.nsa.gov; s...@tycho.nsa.gov; > jda...@google.com > Cc: Roberts, William C> Subject: [RFC] mmap file_contexts and property_contexts: > > From: William Roberts > > THIS IS WIP... > > Rather than using stdio and making copies, just mmap the files and use the > pointers in place. The affect of this change, is that text file load time is > now faster > than binary load time by 4.7% when testing with a file_contexts file from the > Android tree. Note that the Android doesn't use monstrous regexs. > > Times are the average of 3 runs. > > BEFORE: > Text file allocs: 114803 > Text file load time: 0.266101 > Bin file allocs: 93073 > Bin file load time: 0.248757667 > > AFTER: > Text file allocs: 103933 > Text file load time: 0.236192667 > Bin file allocs: 87645 > Bin file load time: .247607333 > > THINGS TO DO: > 1. What's arm performance like? > 2. What interfaces to backends are busted by this (if any)? > 3. Test Android Properties > 4. Im pretty sure this breaks sefcontext_compile, fix. > 5. Test with PCRE2 enabled. > 6. Spell check this message! > 7. Run checkpatch > > Signed-off-by: William Roberts > --- > libselinux/src/label.c | 2 - > libselinux/src/label_android_property.c | 22 ++--- > libselinux/src/label_file.c | 140 > +++- > libselinux/src/label_file.h | 66 +-- > libselinux/src/label_internal.h | 3 +- > libselinux/src/label_support.c | 79 -- > 6 files changed, 172 insertions(+), 140 deletions(-) > > diff --git a/libselinux/src/label.c b/libselinux/src/label.c index > 963bfcb..d767b49 > 100644 > --- a/libselinux/src/label.c > +++ b/libselinux/src/label.c > @@ -15,8 +15,6 @@ > #include "callbacks.h" > #include "label_internal.h" > > -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) > - > typedef int (*selabel_initfunc)(struct selabel_handle *rec, > const struct selinux_opt *opts, > unsigned nopts); > diff --git a/libselinux/src/label_android_property.c > b/libselinux/src/label_android_property.c > index 290b438..2aac394 100644 > --- a/libselinux/src/label_android_property.c > +++ b/libselinux/src/label_android_property.c > @@ -85,13 +85,19 @@ static int process_line(struct selabel_handle *rec, > int pass, unsigned lineno) > { > int items; > - char *prop = NULL, *context = NULL; > + union { > + struct { > + char *prop; > + char *context; > + }; > + char *array[2]; > + } found = { .array = { 0 } }; > struct saved_data *data = (struct saved_data *)rec->data; > spec_t *spec_arr = data->spec_arr; > unsigned int nspec = data->nspec; > const char *errbuf = NULL; > > - items = read_spec_entries(line_buf, , 2, , ); > + items = read_spec_entries(line_buf, , ARRAY_SIZE(found.array), > +found.array); > if (items < 0) { > items = errno; > selinux_log(SELINUX_ERROR, > @@ -108,18 +114,14 @@ static int process_line(struct selabel_handle *rec, > selinux_log(SELINUX_ERROR, > "%s: line %u is missing fields\n", path, > lineno); > - free(prop); > errno = EINVAL; > return -1; > } > > - if (pass == 0) { > - free(prop); > - free(context); > - } else if (pass == 1) { > + if (pass == 1) { > /* On the second pass, process and store the specification in > spec. */ > - spec_arr[nspec].property_key = prop; > - spec_arr[nspec].lr.ctx_raw = context; > + spec_arr[nspec].property_key = found.prop; > + spec_arr[nspec].lr.ctx_raw = found.context; > > if (rec->validating) { > if (selabel_validate(rec, _arr[nspec].lr) < 0) { > @@ - > 234,7 +236,7 @@ static void closef(struct selabel_handle *rec) > for (i = 0; i < data->nspec; i++) { > spec = >spec_arr[i]; > free(spec->property_key); > - free(spec->lr.ctx_raw); > + //free(spec->lr.ctx_raw); > free(spec->lr.ctx_trans); > } > > diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c index > 7156825..4dc440e 100644 > --- a/libselinux/src/label_file.c > +++ b/libselinux/src/label_file.c > @@ -96,43 +96,64 @@ static int nodups_specs(struct saved_data *data, const > char *path) > return rc; > } > > -static int process_text_file(FILE *fp, const char *prefix, > - struct