setfscreatecon optimizations

setfscreatecon optimizations

[Ondrej Oprala]

Hi, there have been some attempts on the coreutils mailing list at 
optimizing cp copying by caching the selinux security context ( please 
see: http://lists.gnu.org/archive/html/coreutils/2013-01/msg00012.html ).
Would it be possible to perform some kind of caching inside 
setfscreatecon? For example not going through the whole process of 
setfscreatecon if the context to be set equals the current one?
Thanks,
Ondrej

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: setfscreatecon optimizations

[Stephen Smalley]

On 01/08/2013 08:20 AM, Ondrej Oprala wrote:
> Hi, there have been some attempts on the coreutils mailing list at
> optimizing cp copying by caching the selinux security context ( please
> see: http://lists.gnu.org/archive/html/coreutils/2013-01/msg00012.html ).
> Would it be possible to perform some kind of caching inside
> setfscreatecon? For example not going through the whole process of
> setfscreatecon if the context to be set equals the current one?

I think that should be possible.  The relevant code that would need to 
be modified is in libselinux/src/procattr.c.  You would likely want to 
modify the common helper for all of the set*con functions, 
setprocattrcon_raw().  A conceptually similar cache exists in 
libselinux/src/setrans_client.c; note the methods used to make it 
thread-safe and to ensure that it is properly freed.  You would need a 
separate cache for each kind of attribute that can be set, i.e. current, 
exec, fscreate, keycreate, sockcreate.  You could likely also cache the tid.

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: setfscreatecon optimizations

[Ondrej Oprala]

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

Thank you for the pointers, it helped me out a lot. I tested the patched 
libselinux with
$cp -a dir1 dir2
and got an approx. 20% save in instruction count. The patch is attached.
Thanks,
Ondrej

On 01/08/2013 03:48 PM, Stephen Smalley wrote:
> On 01/08/2013 08:20 AM, Ondrej Oprala wrote:
>> Hi, there have been some attempts on the coreutils mailing list at
>> optimizing cp copying by caching the selinux security context ( please
>> see: 
>> http://lists.gnu.org/archive/html/coreutils/2013-01/msg00012.html ).
>> Would it be possible to perform some kind of caching inside
>> setfscreatecon? For example not going through the whole process of
>> setfscreatecon if the context to be set equals the current one?
>
> I think that should be possible.  The relevant code that would need to 
> be modified is in libselinux/src/procattr.c.  You would likely want to 
> modify the common helper for all of the set*con functions, 
> setprocattrcon_raw().  A conceptually similar cache exists in 
> libselinux/src/setrans_client.c; note the methods used to make it 
> thread-safe and to ensure that it is properly freed. You would need a 
> separate cache for each kind of attribute that can be set, i.e. 
> current, exec, fscreate, keycreate, sockcreate. You could likely also 
> cache the tid.


[-- Attachment #2: DIFF --]
[-- Type: text/plain, Size: 0 bytes --]

Re: setfscreatecon optimizations

[Eric Paris]

On Wed, Jan 9, 2013 at 7:03 AM, Ondrej Oprala <ooprala@redhat.com> wrote:
> Thank you for the pointers, it helped me out a lot. I tested the patched
> libselinux with
> $cp -a dir1 dir2
> and got an approx. 20% save in instruction count. The patch is attached.
> Thanks,

Thanks!  I'm working on it right now.  You did the:
        __selinux_once(once, init_procattr);
        init_thread_destructor();
Before variable declaration and you do not handle context==NULL, which
is how you clear the value.  I'll fix both of those up and apply to
the devel tree.  Thanks so much!

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: setfscreatecon optimizations

[Eric Paris]

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

Something like the attached?

On Wed, Jan 9, 2013 at 9:30 AM, Eric Paris <eparis@parisplace.org> wrote:
> On Wed, Jan 9, 2013 at 7:03 AM, Ondrej Oprala <ooprala@redhat.com> wrote:
>> Thank you for the pointers, it helped me out a lot. I tested the patched
>> libselinux with
>> $cp -a dir1 dir2
>> and got an approx. 20% save in instruction count. The patch is attached.
>> Thanks,
>
> Thanks!  I'm working on it right now.  You did the:
>         __selinux_once(once, init_procattr);
>         init_thread_destructor();
> Before variable declaration and you do not handle context==NULL, which
> is how you clear the value.  I'll fix both of those up and apply to
> the devel tree.  Thanks so much!

[-- Attachment #2: tmp.patch --]
[-- Type: application/octet-stream, Size: 3356 bytes --]

commit 24ffca8f6b93aae5eb881b01abd9738df59c83e6
Author: Ondrej Oprala <ooprala@redhat.com>
Date:   Wed Jan 9 11:37:43 2013 +0100

    libselinux: optimize set*con functions
    
    Set*con now caches the security context and only re-sets it if it changes.
    
    Signed-off-by: Eric Paris <eparis@redhat.com>

diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c
index 83381e4..40345e9 100644
--- a/libselinux/src/procattr.c
+++ b/libselinux/src/procattr.c
@@ -8,11 +8,54 @@
 #include "selinux_internal.h"
 #include "policy.h"
 
+static __thread pid_t tid = -1;
+static __thread security_context_t prev_current = NULL;
+static __thread security_context_t prev_exec = NULL;
+static __thread security_context_t prev_fscreate = NULL;
+static __thread security_context_t prev_keycreate = NULL;
+static __thread security_context_t prev_sockcreate = NULL;
+
+static pthread_once_t once = PTHREAD_ONCE_INIT;
+static pthread_key_t destructor_key;
+static int destructor_key_initialized = 0;
+static __thread char destructor_initialized;
+
 static pid_t gettid(void)
 {
 	return syscall(__NR_gettid);
 }
 
+static void procattr_thread_destructor(void __attribute__((unused)) *unused)
+{
+	free(prev_current);
+	free(prev_exec);
+	free(prev_fscreate);
+	free(prev_keycreate);
+	free(prev_sockcreate);
+}
+
+void __attribute__((destructor)) procattr_destructor(void);
+
+void hidden __attribute__((destructor)) procattr_destructor(void)
+{
+	if (destructor_key_initialized)
+		__selinux_key_delete(destructor_key);
+}
+
+static inline void init_thread_destructor(void)
+{
+	if (destructor_initialized == 0) {
+		__selinux_setspecific(destructor_key, (void *)1);
+		destructor_initialized = 1;
+	}
+}
+
+static void init_procattr(void)
+{
+	if (__selinux_key_create(&destructor_key, procattr_thread_destructor) == 0)
+		destructor_key_initialized = 1;
+}
+
 static int getprocattrcon_raw(security_context_t * context,
 			      pid_t pid, const char *attr)
 {
@@ -20,7 +63,6 @@ static int getprocattrcon_raw(security_context_t * context,
 	size_t size;
 	int fd, rc;
 	ssize_t ret;
-	pid_t tid;
 	int errno_hold;
 
 	if (pid > 0)
@@ -87,19 +129,56 @@ static int getprocattrcon(security_context_t * context,
 	return ret;
 }
 
+#define UPDATE_CACHE(context, cache)	\
+	if (!context && !cache)		\
+		return 0;		\
+	if (!context) {			\
+		free(cache);		\
+		cache = NULL;		\
+	} else {			\
+		if (cache && !strcmp(context,cache)) \
+			return 0;	\
+		free(cache);		\
+		cache = strdup(context);\
+	}
+
 static int setprocattrcon_raw(security_context_t context,
 			      pid_t pid, const char *attr)
 {
 	char *path;
 	int fd, rc;
-	pid_t tid;
 	ssize_t ret;
 	int errno_hold;
 
+	__selinux_once(once, init_procattr);
+	init_thread_destructor();
+
+	switch (attr[0]) {
+		case 'c':
+			UPDATE_CACHE(context, prev_current);
+			break;
+		case 'e':
+			UPDATE_CACHE(context, prev_exec);
+			break;
+		case 'f':
+			UPDATE_CACHE(context, prev_fscreate);
+			break;
+		case 'k':
+			UPDATE_CACHE(context, prev_keycreate);
+			break;
+		case 's':
+			UPDATE_CACHE(context, prev_sockcreate);
+			break;
+		default:
+			return -1;
+	};
+
 	if (pid > 0)
 		rc = asprintf(&path, "/proc/%d/attr/%s", pid, attr);
 	else {
-		tid = gettid();
+		if (tid == -1)
+			tid = gettid();
+
 		rc = asprintf(&path, "/proc/self/task/%d/attr/%s", tid, attr);
 	}
 	if (rc < 0)

Re: setfscreatecon optimizations

[Ondrej Oprala]

Fantastic :) thank you very much.

On 01/09/2013 03:51 PM, Eric Paris wrote:
> Something like the attached?
>
> On Wed, Jan 9, 2013 at 9:30 AM, Eric Paris <eparis@parisplace.org> wrote:
>> On Wed, Jan 9, 2013 at 7:03 AM, Ondrej Oprala <ooprala@redhat.com> wrote:
>>> Thank you for the pointers, it helped me out a lot. I tested the patched
>>> libselinux with
>>> $cp -a dir1 dir2
>>> and got an approx. 20% save in instruction count. The patch is attached.
>>> Thanks,
>> Thanks!  I'm working on it right now.  You did the:
>>          __selinux_once(once, init_procattr);
>>          init_thread_destructor();
>> Before variable declaration and you do not handle context==NULL, which
>> is how you clear the value.  I'll fix both of those up and apply to
>> the devel tree.  Thanks so much!


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: setfscreatecon optimizations

[Daniel J Walsh]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/09/2013 03:17 PM, Ondrej Oprala wrote:
> Fantastic :) thank you very much.
> 
> On 01/09/2013 03:51 PM, Eric Paris wrote:
>> Something like the attached?
>> 
>> On Wed, Jan 9, 2013 at 9:30 AM, Eric Paris <eparis@parisplace.org>
>> wrote:
>>> On Wed, Jan 9, 2013 at 7:03 AM, Ondrej Oprala <ooprala@redhat.com>
>>> wrote:
>>>> Thank you for the pointers, it helped me out a lot. I tested the
>>>> patched libselinux with $cp -a dir1 dir2 and got an approx. 20% save
>>>> in instruction count. The patch is attached. Thanks,
>>> Thanks!  I'm working on it right now.  You did the: 
>>> __selinux_once(once, init_procattr); init_thread_destructor(); Before
>>> variable declaration and you do not handle context==NULL, which is how
>>> you clear the value.  I'll fix both of those up and apply to the devel
>>> tree.  Thanks so much!
> 
> 
> -- This message was distributed to subscribers of the selinux mailing
> list. If you no longer wish to subscribe, send mail to
> majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes
> as the message.
rpm -q libselinux
libselinux-2.1.12-12.fc19.x86_64

Has the optimized code for anyone brave enough to play with rawhide.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)
Comment: Using GnuPG with undefined - http://www.enigmail.net/

iEYEARECAAYFAlDt4ZEACgkQrlYvE4MpobPr1ACbBlS2t4VWOHIyn31RDx2YeRG+
NLkAn0tdu6VDDrfrnh4sXi+CttD9e4+d
=znNd
-----END PGP SIGNATURE-----

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.