[PATCH] Change semantic of -r in sefcontext_compile

[PATCH] Change semantic of -r in sefcontext_compile

[Janis Danisevskis]

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.
---
 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]);
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH] Change semantic of -r in sefcontext_compile

[Stephen Smalley]

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]);
> 

Re: [PATCH] Change semantic of -r in sefcontext_compile

[William Roberts]

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.



-- 
Respectfully,

William C Roberts

Re: [PATCH] Change semantic of -r in sefcontext_compile

[Stephen Smalley]

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.

Re: [PATCH] Change semantic of -r in sefcontext_compile

[William Roberts]

[-- Attachment #1: Type: text/plain, Size: 1730 bytes --]

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.

[-- Attachment #2: Type: text/html, Size: 2257 bytes --]

Re: [PATCH] Change semantic of -r in sefcontext_compile

[Janis Danisevskis]

[-- Attachment #1: Type: text/plain, Size: 2518 bytes --]

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.
>

[-- Attachment #2: Type: text/html, Size: 3377 bytes --]

Re: [PATCH] Change semantic of -r in sefcontext_compile

[William Roberts]

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>