| Message ID | 1474031328-29743-1-git-send-email-jdanis@android.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 09/16/2016 09:08 AM, Janis Danisevskis wrote: > This patch reestablishes the default behavior of sefcontext_compile > to include precompiled regular expressions in the output. If linked > against PCRE2 the flag "-r" now causes the precompiled regular > expressions to be omitted from the output. I thought your original rationale was more compelling. If we add detection of the relevant arch properties, then we can do this. Otherwise, I don't think we should. > --- > libselinux/utils/sefcontext_compile.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/libselinux/utils/sefcontext_compile.c b/libselinux/utils/sefcontext_compile.c > index 770ec4c..c1284d5 100644 > --- a/libselinux/utils/sefcontext_compile.c > +++ b/libselinux/utils/sefcontext_compile.c > @@ -263,12 +263,10 @@ static void usage(const char *progname) > " will be fc_file with the .bin suffix appended.\n\t" > "-p Optional binary policy file that will be used to\n\t" > " validate contexts defined in the fc_file.\n\t" > - "-r Include precompiled regular expressions in the output.\n\t" > + "-r Omit precompiled regular expressions in the output.\n\t" > " (PCRE2 only. Compiled PCRE2 regular expressions are\n\t" > " not portable across architectures. When linked against\n\t" > " PCRE this flag is ignored)\n\t" > - " Omit precompiled regular expressions (only meaningful\n\t" > - " when using PCRE2 regular expression back-end).\n\t" > "fc_file The text based file contexts file to be processed.\n", > progname); > exit(EXIT_FAILURE); > @@ -278,7 +276,7 @@ int main(int argc, char *argv[]) > { > const char *path = NULL; > const char *out_file = NULL; > - int do_write_precompregex = 0; > + int do_write_precompregex = 1; > char stack_path[PATH_MAX + 1]; > char *tmp = NULL; > int fd, rc, opt; > @@ -299,7 +297,7 @@ int main(int argc, char *argv[]) > policy_file = optarg; > break; > case 'r': > - do_write_precompregex = 1; > + do_write_precompregex = 0; > break; > default: > usage(argv[0]); >
On Fri, Sep 16, 2016 at 7:41 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 09/16/2016 09:08 AM, Janis Danisevskis wrote: >> This patch reestablishes the default behavior of sefcontext_compile >> to include precompiled regular expressions in the output. If linked >> against PCRE2 the flag "-r" now causes the precompiled regular >> expressions to be omitted from the output. > > I thought your original rationale was more compelling. If we add > detection of the relevant arch properties, then we can do this. > Otherwise, I don't think we should. I was assuming based on the thread earlier that those patches would be coming. If we cant detect and compile on the current "undefined behavior" case, then this needs to stay as is. But I thought someone had a list of PCRE things that can be checked for "arch", so its just a matter of encoding those, assuming that list is correct. Binary file_contexts only make sense if you compile in the regex info, else just use the textual representation. > >> --- >> libselinux/utils/sefcontext_compile.c | 8 +++----- >> 1 file changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/libselinux/utils/sefcontext_compile.c b/libselinux/utils/sefcontext_compile.c >> index 770ec4c..c1284d5 100644 >> --- a/libselinux/utils/sefcontext_compile.c >> +++ b/libselinux/utils/sefcontext_compile.c >> @@ -263,12 +263,10 @@ static void usage(const char *progname) >> " will be fc_file with the .bin suffix appended.\n\t" >> "-p Optional binary policy file that will be used to\n\t" >> " validate contexts defined in the fc_file.\n\t" >> - "-r Include precompiled regular expressions in the output.\n\t" >> + "-r Omit precompiled regular expressions in the output.\n\t" >> " (PCRE2 only. Compiled PCRE2 regular expressions are\n\t" >> " not portable across architectures. When linked against\n\t" >> " PCRE this flag is ignored)\n\t" >> - " Omit precompiled regular expressions (only meaningful\n\t" >> - " when using PCRE2 regular expression back-end).\n\t" >> "fc_file The text based file contexts file to be processed.\n", >> progname); >> exit(EXIT_FAILURE); >> @@ -278,7 +276,7 @@ int main(int argc, char *argv[]) >> { >> const char *path = NULL; >> const char *out_file = NULL; >> - int do_write_precompregex = 0; >> + int do_write_precompregex = 1; >> char stack_path[PATH_MAX + 1]; >> char *tmp = NULL; >> int fd, rc, opt; >> @@ -299,7 +297,7 @@ int main(int argc, char *argv[]) >> policy_file = optarg; >> break; >> case 'r': >> - do_write_precompregex = 1; >> + do_write_precompregex = 0; >> break; >> default: >> usage(argv[0]); >> > > _______________________________________________ > Seandroid-list mailing list > Seandroid-list@tycho.nsa.gov > To unsubscribe, send email to Seandroid-list-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Seandroid-list-request@tycho.nsa.gov.
On 09/16/2016 11:08 AM, William Roberts wrote: > On Fri, Sep 16, 2016 at 7:41 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >> On 09/16/2016 09:08 AM, Janis Danisevskis wrote: >>> This patch reestablishes the default behavior of sefcontext_compile >>> to include precompiled regular expressions in the output. If linked >>> against PCRE2 the flag "-r" now causes the precompiled regular >>> expressions to be omitted from the output. >> >> I thought your original rationale was more compelling. If we add >> detection of the relevant arch properties, then we can do this. >> Otherwise, I don't think we should. > > I was assuming based on the thread earlier that those patches would be coming. > If we cant detect and compile on the current "undefined behavior" > case, then this > needs to stay as is. > > But I thought someone had a list of PCRE things that can be checked for "arch", > so its just a matter of encoding those, assuming that list is correct. > > Binary file_contexts only make sense if you compile in the regex info, else > just use the textual representation. That was my thought originally, but Janis did say that it was still faster, and Android presently only ships file_contexts.bin, so we can't just break that.
On Sep 16, 2016 08:12, "Stephen Smalley" <sds@tycho.nsa.gov> wrote: > > On 09/16/2016 11:08 AM, William Roberts wrote: > > On Fri, Sep 16, 2016 at 7:41 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > >> On 09/16/2016 09:08 AM, Janis Danisevskis wrote: > >>> This patch reestablishes the default behavior of sefcontext_compile > >>> to include precompiled regular expressions in the output. If linked > >>> against PCRE2 the flag "-r" now causes the precompiled regular > >>> expressions to be omitted from the output. > >> > >> I thought your original rationale was more compelling. If we add > >> detection of the relevant arch properties, then we can do this. > >> Otherwise, I don't think we should. > > > > I was assuming based on the thread earlier that those patches would be coming. > > If we cant detect and compile on the current "undefined behavior" > > case, then this > > needs to stay as is. > > > > But I thought someone had a list of PCRE things that can be checked for "arch", > > so its just a matter of encoding those, assuming that list is correct. > > > > Binary file_contexts only make sense if you compile in the regex info, else > > just use the textual representation. > > That was my thought originally, but Janis did say that it was still > faster, and Android presently only ships file_contexts.bin, so we can't > just break that. I'm not saying that we break anything, but I think we should really scrutinize the decision to keep binary fc's on Android. The only way it could be faster at the moment is mmap and pcre2. We need to get some raw numbers of pcre2 textual vs binary load times. If it's within 30% I'll have that gap closed soon. It also takes up about 3 times the disk space for binary.
I don't really care much about the behavior of sefcontext_compile. I just thought making the default behavior the safest would be the best option. Before android is using it, I will have to sync the (now modified and improved - thank you) patches back into AOSP, which I intend to do. I have some benchmarks measuring load and lookup time for file contexts. I am eager to review and benchmark William's patches and explore a bit myself. And once all options are on the table I can make a case for the fastest solution to make it into Android. Concerning the arch string I respond in the other add support for pcre2 thread. On Fri, Sep 16, 2016 at 4:20 PM, William Roberts <bill.c.roberts@gmail.com> wrote: > On Sep 16, 2016 08:12, "Stephen Smalley" <sds@tycho.nsa.gov> wrote: > > > > On 09/16/2016 11:08 AM, William Roberts wrote: > > > On Fri, Sep 16, 2016 at 7:41 AM, Stephen Smalley <sds@tycho.nsa.gov> > wrote: > > >> On 09/16/2016 09:08 AM, Janis Danisevskis wrote: > > >>> This patch reestablishes the default behavior of sefcontext_compile > > >>> to include precompiled regular expressions in the output. If linked > > >>> against PCRE2 the flag "-r" now causes the precompiled regular > > >>> expressions to be omitted from the output. > > >> > > >> I thought your original rationale was more compelling. If we add > > >> detection of the relevant arch properties, then we can do this. > > >> Otherwise, I don't think we should. > > > > > > I was assuming based on the thread earlier that those patches would be > coming. > > > If we cant detect and compile on the current "undefined behavior" > > > case, then this > > > needs to stay as is. > > > > > > But I thought someone had a list of PCRE things that can be checked > for "arch", > > > so its just a matter of encoding those, assuming that list is correct. > > > > > > Binary file_contexts only make sense if you compile in the regex info, > else > > > just use the textual representation. > > > > That was my thought originally, but Janis did say that it was still > > faster, and Android presently only ships file_contexts.bin, so we can't > > just break that. > > I'm not saying that we break anything, but I think we should really > scrutinize the decision to keep binary fc's on Android. The only way it > could be faster at the moment is mmap and pcre2. We need to get some raw > numbers of pcre2 textual vs binary load times. If it's within 30% I'll have > that gap closed soon. It also takes up about 3 times the disk space for > binary. >
On Fri, Sep 16, 2016 at 11:44 AM, Janis Danisevskis <jdanis@android.com> wrote: > I don't really care much about the behavior of sefcontext_compile. I just > thought making the default behavior the safest would be the best option. > Before android is using it, I will have to sync the (now modified and > improved - thank you) patches back into AOSP, which I intend to do. I have > some benchmarks measuring load and lookup time for file contexts. I am eager > to review and benchmark William's patches and explore a bit myself. And once > all options are on the table I can make a case for the fastest solution to > make it into Android. + More Google dudes so they see this. This is where Android's fork is a PITA. My current TODO list looks like this: 1. Get process_file cleanups up streamed (in progress) 2. Get performance improvements up streamed (I have things local, they need more work) 3. Rectify the Android/Upstream fork. For two, I have a UDOO ARM board I was going to add into my performance testing, since it will run a debian distro I can just use upstream libselinux and compile checkfc or something for it. I was going to use that to see if theirs an architectura delta between arm and x86 with the performance improvements. All my numbers are only off of an x86 platform. For three, ideall to me, for phase I this looks like having usptream + android.c and android.h files. This way for updating, we can merge, with no conflicts as all android changes will be confined to those two files. Ill update upstream to have a method to kill unused interfaces in a graceful way as well as fixup android.c/android.h for anything that has been moved into upstream already, like the work Richard did with digest files. After 3, then we can trivially test item 2 on an Android platform, giving us solid, real world android numbers and getting us closer to unification. Does anyone have any objections, concerns on this list, let me know so I can take them into consideration. <snip>
diff --git a/libselinux/utils/sefcontext_compile.c b/libselinux/utils/sefcontext_compile.c index 770ec4c..c1284d5 100644 --- a/libselinux/utils/sefcontext_compile.c +++ b/libselinux/utils/sefcontext_compile.c @@ -263,12 +263,10 @@ static void usage(const char *progname) " will be fc_file with the .bin suffix appended.\n\t" "-p Optional binary policy file that will be used to\n\t" " validate contexts defined in the fc_file.\n\t" - "-r Include precompiled regular expressions in the output.\n\t" + "-r Omit precompiled regular expressions in the output.\n\t" " (PCRE2 only. Compiled PCRE2 regular expressions are\n\t" " not portable across architectures. When linked against\n\t" " PCRE this flag is ignored)\n\t" - " Omit precompiled regular expressions (only meaningful\n\t" - " when using PCRE2 regular expression back-end).\n\t" "fc_file The text based file contexts file to be processed.\n", progname); exit(EXIT_FAILURE); @@ -278,7 +276,7 @@ int main(int argc, char *argv[]) { const char *path = NULL; const char *out_file = NULL; - int do_write_precompregex = 0; + int do_write_precompregex = 1; char stack_path[PATH_MAX + 1]; char *tmp = NULL; int fd, rc, opt; @@ -299,7 +297,7 @@ int main(int argc, char *argv[]) policy_file = optarg; break; case 'r': - do_write_precompregex = 1; + do_write_precompregex = 0; break; default: usage(argv[0]);