Skip to contentSkip to conversation

Change semantic of -r in sefcontext_compile

PATCH In review Janis Danisevskis 3 participants 7 messages Sep 16, 2016 SELinux security module

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Change semantic of -r in sefcontext_compile

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(-)
libselinux/utils/sefcontext_compile.c +3 -5
index 770ec4c..c1284d5 100644
@@ -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]);
Signature · 1 line
2.8.0.rc3.226.g39d4020
#1 lore raw
Quoted 4 lines · 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.

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.

Quoted 41 lines
---
 libselinux/utils/sefcontext_compile.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
libselinux/utils/sefcontext_compile.c +3 -5
index 770ec4c..c1284d5 100644
@@ -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 lore raw
Quoted 9 lines · Stephen Smalley
Quoted 4 lines · 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.

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.

Quoted 48 lines
Quoted 34 lines
---
 libselinux/utils/sefcontext_compile.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
libselinux/utils/sefcontext_compile.c +2 -4
index 770ec4c..c1284d5 100644
@@ -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;

libselinux/utils/sefcontext_compile.c int main(int argc, char *argv[]) · line 303

Quoted 8 lines
libselinux/utils/sefcontext_compile.c +1 -1
@@ -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
[email protected]
To unsubscribe, send email to [email protected].
To get help, send an email containing "help" to [email protected].

Signature · 3 lines
Respectfully,

William C Roberts
#3 lore raw
Quoted 16 lines · William Roberts
Quoted 9 lines · Stephen Smalley
Quoted 4 lines · 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.

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.

Quoted 5 lines

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.

#4 lore raw
Quoted 3 lines · Stephen Smalley
Quoted 1 line · William Roberts

On Fri, Sep 16, 2016 at 7:41 AM, Stephen Smalley <[email protected]>

Quoted 11 lines · wrote:
Quoted 11 lines
Quoted 9 lines · Janis Danisevskis
Quoted 4 lines

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.

Quoted 5 lines
Quoted 5 lines

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",

Quoted 3 lines
Quoted 3 lines

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

Quoted 5 lines
Quoted 1 line

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.

#5 lore raw

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.

Quoted 40 lines · William Roberts
Quoted 3 lines · Stephen Smalley
Quoted 1 line · William Roberts

On Fri, Sep 16, 2016 at 7:41 AM, Stephen Smalley <[email protected]>

Quoted 11 lines · wrote:
Quoted 11 lines
Quoted 9 lines · Janis Danisevskis
Quoted 4 lines

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.

Quoted 5 lines
Quoted 5 lines

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",

Quoted 3 lines
Quoted 3 lines

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

Quoted 5 lines
Quoted 1 line

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.

#6 lore raw
Quoted 8 lines · Janis Danisevskis

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>