corrected sidtab locking patch

corrected sidtab locking patch

[Jason Tang]

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

The following patch allows a program to enable locking in
libsepol/sidtab.c.  By default sidtab.c does not lock the sidtab when
adding and removing entries; this causes problems for multi-threaded
programs using it.  Programs may set up function pointers to perform
locking as they see fit.


-- 
Jason Tang / jtang@tresys.com

[-- Attachment #2: patch-locking --]
[-- Type: text/x-patch, Size: 1812 bytes --]

diff -purN nsa/selinux-usr/libsepol/include/sepol/sidtab.h nsa-selinux-usr/libsepol/include/sepol/sidtab.h
--- nsa/selinux-usr/libsepol/include/sepol/sidtab.h	2005-07-13 15:42:37.000000000 -0400
+++ nsa-selinux-usr/libsepol/include/sepol/sidtab.h	2005-08-24 17:40:19.000000000 -0400
@@ -72,6 +72,11 @@ extern void sepol_sidtab_set(sidtab_t *d
 
 extern void sepol_sidtab_shutdown(sidtab_t *s);
 
+typedef void (* sepol_sidtab_lock_func)(sidtab_t *);
+extern void sepol_sidtab_set_init_func(sepol_sidtab_lock_func func);
+extern void sepol_sidtab_set_lock_func(sepol_sidtab_lock_func func);
+extern void sepol_sidtab_set_unlock_func(sepol_sidtab_lock_func func);
+
 #endif	/* _SIDTAB_H_ */
 
 /* FLASK */
diff -purN nsa/selinux-usr/libsepol/src/sidtab.c nsa-selinux-usr/libsepol/src/sidtab.c
--- nsa/selinux-usr/libsepol/src/sidtab.c	2005-05-13 15:53:31.000000000 -0400
+++ nsa-selinux-usr/libsepol/src/sidtab.c	2005-08-24 17:28:06.000000000 -0400
@@ -23,6 +23,14 @@
 #define SIDTAB_LOCK(s) 
 #define SIDTAB_UNLOCK(s)
 
+static void do_no_lock_func(sidtab_t *s __attribute__((unused))) {
+        /* as the name suggests, this function does not do anything */
+}
+
+static sepol_sidtab_lock_func INIT_SIDTAB_LOCK = do_no_lock_func;
+static sepol_sidtab_lock_func SIDTAB_LOCK = do_no_lock_func;
+static sepol_sidtab_lock_func SIDTAB_UNLOCK = do_no_lock_func;
+
 int sepol_sidtab_init(sidtab_t *s) 
 {
 	int i;
@@ -334,5 +342,17 @@ void sepol_sidtab_shutdown(sidtab_t *s)
 	SIDTAB_UNLOCK(s);
 }
 
+void sepol_sidtab_set_init_func(sepol_sidtab_lock_func func) {
+        INIT_SIDTAB_LOCK = func;
+}
+
+void sepol_sidtab_set_lock_func(sepol_sidtab_lock_func func) {
+        SIDTAB_LOCK = func;
+}
+
+void sepol_sidtab_set_unlock_func(sepol_sidtab_lock_func func) {
+        SIDTAB_UNLOCK = func;
+}
+
 /* FLASK */
 

Re: corrected sidtab locking patch

[Stephen Smalley]

On Fri, 2005-08-26 at 13:14 -0400, Jason Tang wrote:
> The following patch allows a program to enable locking in
> libsepol/sidtab.c.  By default sidtab.c does not lock the sidtab when
> adding and removing entries; this causes problems for multi-threaded
> programs using it.  Programs may set up function pointers to perform
> locking as they see fit.
  
- Patch doesn't remove the legacy #define's for the old locking macros.
In fact, it would be better to name the static functions differently and
change the #define's to expand to the new functions instead of just
directly defining the functions with those names.

- Are you sure that this is now thread-safe?  Kernel-side code was also
relying on other aspects, e.g. use of wmb() during sidtab_insert to
ensure that we can safely walk the lists lock-free in sidtab_search, and
also used atomic (non-blocking) allocation when holding the lock.

- There are certainly other aspects of libsepol that are not presently
thread-safe.  One obvious example is its copy of services.c, which lacks
the locking present in the kernel version.  Do you allow policy reloads?

-- 
Stephen Smalley
National Security Agency


--
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: corrected sidtab locking patch

[Stephen Smalley]

On Fri, 2005-08-26 at 13:39 -0400, Stephen Smalley wrote:
> On Fri, 2005-08-26 at 13:14 -0400, Jason Tang wrote:
> > The following patch allows a program to enable locking in
> > libsepol/sidtab.c.  By default sidtab.c does not lock the sidtab when
> > adding and removing entries; this causes problems for multi-threaded
> > programs using it.  Programs may set up function pointers to perform
> > locking as they see fit.
>   
> - Patch doesn't remove the legacy #define's for the old locking macros.
> In fact, it would be better to name the static functions differently and
> change the #define's to expand to the new functions instead of just
> directly defining the functions with those names.
> 
> - Are you sure that this is now thread-safe?  Kernel-side code was also
> relying on other aspects, e.g. use of wmb() during sidtab_insert to
> ensure that we can safely walk the lists lock-free in sidtab_search, and
> also used atomic (non-blocking) allocation when holding the lock.
> 
> - There are certainly other aspects of libsepol that are not presently
> thread-safe.  One obvious example is its copy of services.c, which lacks
> the locking present in the kernel version.  Do you allow policy reloads?

BTW, it might be simpler to just do coarse-grained locking in the
calling application around all calls to libsepol.
 
-- 
Stephen Smalley
National Security Agency


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