[PATCH v4] libselinux: use kernel status page by default

[PATCH v4] libselinux: use kernel status page by default

[Mike Palmiotto]

Commit bc2a8f418e3b ("libselinux: add selinux_status_* interfaces for
/selinux/status") introduced the sestatus mechanism, which allows for
mmap()'ing of the kernel status page as a replacement for avc_netlink.

The mechanism was initially intended for use by userspace object
managers which were calculating access decisions within their
application and did not rely on the libselinux AVC implementation. In
order to properly make use of sestatus within avc_has_perm(), the status
mechanism needs to properly set avc internals during status events;
else, avc_enforcing is never updated upon sestatus changes.

This commit introduces a new selinux_status_loop() function, which
replaces the default netlink-equivalent, avc_netlink_loop(). The
function watches the kernel status page until an error occurs, at which
point it will exit the thread. In the event that the status page cannot
be opened (on avc_open), the thread will continue to function as before
by using a fallback netlink socket.

This allows us to replace the call to avc_netlink_open() in
avc_init_internal() with a call to selinux_status_open() and remove the
avc_netlink_check_nb() call from the critical code path in
avc_has_perm_noaudit(), as well as selinux_check_access().

Userspace object managers that still need a netlink socket can call
avc_netlink_acquire_fd() to open open and/or obtain one.

Update the manpage to reflect the new avc_netlink_acquire_fd()
functionality.

Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com>
---
Testing:
  - dbus-daemon v1.12.8 on RHEL8.2
  - dbus-broker v22 on F32

Changelog:
  V2:
  - Added selinux_status_loop function for watcher threads.
  - Replaced avc_netlink_open with selinux_status_open.
  - Moved avc_netlink_open into avc_netlink_acquire_fd.
  - Replaced avc_netlink_check_nb() call in selinux_check_access with sestatus
  equivalent.
  - Updated manpage and mapfile.

  V3:
  - Made selinux_status_loop an internal function and got rid of manpage.
  - Got rid of superfluous selinux_status_close() in selinux_status_loop().
  - Got rid of avc_enforcing modification in selinux_status_getenforce().
  - Some style fixes.
  - Updated commit message subject line/details.

  V4:
  - Dropped mapfile changes.
  - Fixed indentation.

 libselinux/man/man3/avc_netlink_loop.3 |  8 +++
 libselinux/src/avc.c                   | 14 ++---
 libselinux/src/avc_internal.c          | 82 ++++++++++++++++++--------
 libselinux/src/avc_internal.h          |  7 +++
 libselinux/src/checkAccess.c           |  2 +-
 libselinux/src/sestatus.c              | 27 +++++++++
 6 files changed, 106 insertions(+), 34 deletions(-)

diff --git a/libselinux/man/man3/avc_netlink_loop.3 b/libselinux/man/man3/avc_netlink_loop.3
index c8268a12..f03d7813 100644
--- a/libselinux/man/man3/avc_netlink_loop.3
+++ b/libselinux/man/man3/avc_netlink_loop.3
@@ -54,6 +54,11 @@ closes the netlink socket.  This function is called automatically by
 returns the netlink socket descriptor number and informs the userspace AVC
 not to check the socket descriptor automatically on calls to
 .BR avc_has_perm (3).
+If no such socket descriptor exists,
+.BR avc_netlink_acquire_fd (3)
+will first call
+.BR avc_netlink_open (3)
+and then return the resulting fd.
 
 .BR avc_netlink_release_fd ()
 returns control of the netlink socket to the userspace AVC, re-enabling
@@ -78,6 +83,9 @@ with a return value return zero on success.  On error, \-1 is returned and
 .I errno
 is set appropriately.
 .
+.SH "AUTHOR"
+Originally KaiGai Kohei. Updated by Mike Palmiotto <mike.palmiotto@crunchydata.com>
+.
 .SH "SEE ALSO"
 .BR avc_open (3),
 .BR selinux_set_callback (3),
diff --git a/libselinux/src/avc.c b/libselinux/src/avc.c
index b4648b2d..5f3b00cf 100644
--- a/libselinux/src/avc.c
+++ b/libselinux/src/avc.c
@@ -50,7 +50,7 @@ struct avc_callback_node {
 	struct avc_callback_node *next;
 };
 
-static void *avc_netlink_thread = NULL;
+static void *avc_status_thread = NULL;
 static void *avc_lock = NULL;
 static void *avc_log_lock = NULL;
 static struct avc_node *avc_node_freelist = NULL;
@@ -215,15 +215,15 @@ static int avc_init_internal(const char *prefix,
 		avc_enforcing = rc;
 	}
 
-	rc = avc_netlink_open(0);
+	rc = selinux_status_open(1);
 	if (rc < 0) {
 		avc_log(SELINUX_ERROR,
-			"%s:  can't open netlink socket: %d (%s)\n",
+			"%s: could not open selinux status page: %d (%s)\n",
 			avc_prefix, errno, strerror(errno));
 		goto out;
 	}
 	if (avc_using_threads) {
-		avc_netlink_thread = avc_create_thread(&avc_netlink_loop);
+		avc_status_thread = avc_create_thread(&selinux_status_loop);
 		avc_netlink_trouble = 0;
 	}
 	avc_running = 1;
@@ -558,8 +558,8 @@ void avc_destroy(void)
 	avc_get_lock(avc_lock);
 
 	if (avc_using_threads)
-		avc_stop_thread(avc_netlink_thread);
-	avc_netlink_close();
+		avc_stop_thread(avc_status_thread);
+	selinux_status_close();
 
 	for (i = 0; i < AVC_CACHE_SLOTS; i++) {
 		node = avc_cache.slots[i];
@@ -766,7 +766,7 @@ int avc_has_perm_noaudit(security_id_t ssid,
 		avd_init(avd);
 
 	if (!avc_using_threads && !avc_app_main_loop) {
-		(void)avc_netlink_check_nb();
+		(void) selinux_status_updated();
 	}
 
 	if (!aeref) {
diff --git a/libselinux/src/avc_internal.c b/libselinux/src/avc_internal.c
index 568a3d92..4ef92452 100644
--- a/libselinux/src/avc_internal.c
+++ b/libselinux/src/avc_internal.c
@@ -53,6 +53,49 @@ int avc_enforcing = 1;
 int avc_setenforce = 0;
 int avc_netlink_trouble = 0;
 
+/* process setenforce events for netlink and sestatus */
+int avc_process_setenforce(int enforcing)
+{
+	int rc = 0;
+
+	avc_log(SELINUX_INFO,
+		"%s:  received setenforce notice (enforcing=%d)\n",
+		avc_prefix, enforcing);
+	if (avc_setenforce)
+		goto out;
+	avc_enforcing = enforcing;
+	if (avc_enforcing && (rc = avc_ss_reset(0)) < 0) {
+		avc_log(SELINUX_ERROR,
+			"%s:  cache reset returned %d (errno %d)\n",
+			avc_prefix, rc, errno);
+		return rc;
+	}
+
+out:
+	return selinux_netlink_setenforce(enforcing);
+}
+
+/* process policyload events for netlink and sestatus */
+int avc_process_policyload(uint32_t seqno)
+{
+	int rc = 0;
+
+	avc_log(SELINUX_INFO,
+		"%s:  received policyload notice (seqno=%u)\n",
+		avc_prefix, seqno);
+	rc = avc_ss_reset(seqno);
+	if (rc < 0) {
+		avc_log(SELINUX_ERROR,
+			"%s:  cache reset returned %d (errno %d)\n",
+			avc_prefix, rc, errno);
+		return rc;
+	}
+
+	selinux_flush_class_cache();
+
+	return selinux_netlink_policyload(seqno);
+}
+
 /* netlink socket code */
 static int fd = -1;
 
@@ -177,20 +220,7 @@ static int avc_netlink_process(void *buf)
 
 	case SELNL_MSG_SETENFORCE:{
 		struct selnl_msg_setenforce *msg = NLMSG_DATA(nlh);
-		msg->val = !!msg->val;
-		avc_log(SELINUX_INFO,
-			"%s:  received setenforce notice (enforcing=%d)\n",
-			avc_prefix, msg->val);
-		if (avc_setenforce)
-			break;
-		avc_enforcing = msg->val;
-		if (avc_enforcing && (rc = avc_ss_reset(0)) < 0) {
-			avc_log(SELINUX_ERROR,
-				"%s:  cache reset returned %d (errno %d)\n",
-				avc_prefix, rc, errno);
-			return rc;
-		}
-		rc = selinux_netlink_setenforce(msg->val);
+		rc = avc_process_setenforce(!!msg->val);
 		if (rc < 0)
 			return rc;
 		break;
@@ -198,18 +228,7 @@ static int avc_netlink_process(void *buf)
 
 	case SELNL_MSG_POLICYLOAD:{
 		struct selnl_msg_policyload *msg = NLMSG_DATA(nlh);
-		avc_log(SELINUX_INFO,
-			"%s:  received policyload notice (seqno=%u)\n",
-			avc_prefix, msg->seqno);
-		rc = avc_ss_reset(msg->seqno);
-		if (rc < 0) {
-			avc_log(SELINUX_ERROR,
-				"%s:  cache reset returned %d (errno %d)\n",
-				avc_prefix, rc, errno);
-			return rc;
-		}
-		selinux_flush_class_cache();
-		rc = selinux_netlink_policyload(msg->seqno);
+		rc = avc_process_policyload(msg->seqno);
 		if (rc < 0)
 			return rc;
 		break;
@@ -284,6 +303,17 @@ void avc_netlink_loop(void)
 
 int avc_netlink_acquire_fd(void)
 {
+	if (fd < 0) {
+		int rc = 0;
+		rc = avc_netlink_open(0);
+		if (rc < 0) {
+			avc_log(SELINUX_ERROR,
+				"%s: could not open netlink socket: %d (%s)\n",
+				avc_prefix, errno, strerror(errno));
+			return rc;
+		}
+	}
+
     avc_app_main_loop = 1;
 
     return fd;
diff --git a/libselinux/src/avc_internal.h b/libselinux/src/avc_internal.h
index 3f8a6bb1..2dda6490 100644
--- a/libselinux/src/avc_internal.h
+++ b/libselinux/src/avc_internal.h
@@ -32,6 +32,13 @@ extern void (*avc_func_get_lock) (void *);
 extern void (*avc_func_release_lock) (void *);
 extern void (*avc_func_free_lock) (void *);
 
+/* selinux status processing for netlink and sestatus */
+extern int avc_process_setenforce(int enforcing);
+extern int avc_process_policyload(uint32_t seqno);
+
+/* watch selinux status page */
+extern void selinux_status_loop(void);
+
 static inline void set_callbacks(const struct avc_memory_callback *mem_cb,
 				 const struct avc_log_callback *log_cb,
 				 const struct avc_thread_callback *thread_cb,
diff --git a/libselinux/src/checkAccess.c b/libselinux/src/checkAccess.c
index 3491fded..b337ea64 100644
--- a/libselinux/src/checkAccess.c
+++ b/libselinux/src/checkAccess.c
@@ -39,7 +39,7 @@ int selinux_check_access(const char *scon, const char *tcon, const char *class,
 	if (rc < 0)
 		return rc;
 
-	(void) avc_netlink_check_nb();
+	(void) selinux_status_updated();
 
        sclass = string_to_security_class(class);
        if (sclass == 0) {
diff --git a/libselinux/src/sestatus.c b/libselinux/src/sestatus.c
index 86267ff8..72733c2d 100644
--- a/libselinux/src/sestatus.c
+++ b/libselinux/src/sestatus.c
@@ -39,6 +39,7 @@ struct selinux_status_t
 static struct selinux_status_t *selinux_status = NULL;
 static int			selinux_status_fd;
 static uint32_t			last_seqno;
+static uint32_t			last_policyload;
 
 static uint32_t			fallback_sequence;
 static int			fallback_enforcing;
@@ -116,6 +117,15 @@ int selinux_status_updated(void)
 
 	if (last_seqno != curr_seqno)
 	{
+		if (avc_enforcing != (int) selinux_status->enforcing) {
+			if (avc_process_setenforce(selinux_status->enforcing) < 0)
+				return -1;
+		}
+		if (last_policyload != selinux_status->policyload) {
+			if (avc_process_policyload(selinux_status->policyload) < 0)
+				return -1;
+			last_policyload = selinux_status->policyload;
+		}
 		last_seqno = curr_seqno;
 		result = 1;
 	}
@@ -316,6 +326,23 @@ error:
 	return -1;
 }
 
+/*
+ * selinux_status_loop
+ *
+ * Run routine for checking kernel status page in a listening thread.
+ * Falls back on netlink socket in the event of failure to open status page.
+ */
+void selinux_status_loop(void)
+{
+	/* Check kernel status page until error occurs */
+	while (selinux_status_updated() >= 0)
+		;
+
+	avc_log(SELINUX_ERROR,
+		"%s: status thread terminating due to error: %d (%s)\n",
+		avc_prefix, errno, strerror(errno));
+}
+
 /*
  * selinux_status_close
  *
-- 
2.27.0

Re: [PATCH v4] libselinux: use kernel status page by default

[Stephen Smalley]

On Fri, Jul 24, 2020 at 10:14 AM Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:
>
> Commit bc2a8f418e3b ("libselinux: add selinux_status_* interfaces for
> /selinux/status") introduced the sestatus mechanism, which allows for
> mmap()'ing of the kernel status page as a replacement for avc_netlink.
>
> The mechanism was initially intended for use by userspace object
> managers which were calculating access decisions within their
> application and did not rely on the libselinux AVC implementation. In
> order to properly make use of sestatus within avc_has_perm(), the status
> mechanism needs to properly set avc internals during status events;
> else, avc_enforcing is never updated upon sestatus changes.
>
> This commit introduces a new selinux_status_loop() function, which
> replaces the default netlink-equivalent, avc_netlink_loop(). The
> function watches the kernel status page until an error occurs, at which
> point it will exit the thread. In the event that the status page cannot
> be opened (on avc_open), the thread will continue to function as before
> by using a fallback netlink socket.
>
> This allows us to replace the call to avc_netlink_open() in
> avc_init_internal() with a call to selinux_status_open() and remove the
> avc_netlink_check_nb() call from the critical code path in
> avc_has_perm_noaudit(), as well as selinux_check_access().
>
> Userspace object managers that still need a netlink socket can call
> avc_netlink_acquire_fd() to open open and/or obtain one.
>
> Update the manpage to reflect the new avc_netlink_acquire_fd()
> functionality.
>
> Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com>
> ---
> Testing:
>   - dbus-daemon v1.12.8 on RHEL8.2
>   - dbus-broker v22 on F32

This looks good to me as far as the code is concerned.  However,
installing the patched libselinux and rebooting, I notice that
afterward I have dbus-daemon running on a Fedora rawhide instance and
consuming nearly 100% CPU constantly.  I'm guessing it is sitting in
the status loop. Not sure why there is a dbus-daemon instance running
at all since dbus-broker seems to be the default in Fedora and
systemctl shows dbus-daemon as disabled.  But if I revert to the stock
libselinux, it stops hogging CPU.  Thoughts?

Re: [PATCH v4] libselinux: use kernel status page by default

[Stephen Smalley]

On Fri, Jul 24, 2020 at 11:34 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Fri, Jul 24, 2020 at 10:14 AM Mike Palmiotto
> <mike.palmiotto@crunchydata.com> wrote:
> >
> > Commit bc2a8f418e3b ("libselinux: add selinux_status_* interfaces for
> > /selinux/status") introduced the sestatus mechanism, which allows for
> > mmap()'ing of the kernel status page as a replacement for avc_netlink.
> >
> > The mechanism was initially intended for use by userspace object
> > managers which were calculating access decisions within their
> > application and did not rely on the libselinux AVC implementation. In
> > order to properly make use of sestatus within avc_has_perm(), the status
> > mechanism needs to properly set avc internals during status events;
> > else, avc_enforcing is never updated upon sestatus changes.
> >
> > This commit introduces a new selinux_status_loop() function, which
> > replaces the default netlink-equivalent, avc_netlink_loop(). The
> > function watches the kernel status page until an error occurs, at which
> > point it will exit the thread. In the event that the status page cannot
> > be opened (on avc_open), the thread will continue to function as before
> > by using a fallback netlink socket.
> >
> > This allows us to replace the call to avc_netlink_open() in
> > avc_init_internal() with a call to selinux_status_open() and remove the
> > avc_netlink_check_nb() call from the critical code path in
> > avc_has_perm_noaudit(), as well as selinux_check_access().
> >
> > Userspace object managers that still need a netlink socket can call
> > avc_netlink_acquire_fd() to open open and/or obtain one.
> >
> > Update the manpage to reflect the new avc_netlink_acquire_fd()
> > functionality.
> >
> > Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com>
> > ---
> > Testing:
> >   - dbus-daemon v1.12.8 on RHEL8.2
> >   - dbus-broker v22 on F32
>
> This looks good to me as far as the code is concerned.  However,
> installing the patched libselinux and rebooting, I notice that
> afterward I have dbus-daemon running on a Fedora rawhide instance and
> consuming nearly 100% CPU constantly.  I'm guessing it is sitting in
> the status loop. Not sure why there is a dbus-daemon instance running
> at all since dbus-broker seems to be the default in Fedora and
> systemctl shows dbus-daemon as disabled.  But if I revert to the stock
> libselinux, it stops hogging CPU.  Thoughts?

Used gdb to attach to the separate thread and got a traceback before
and after the libselinux patch.
Sure enough, before it is performing a blocking poll() operation and
hence sleeping.  After it is spinning in
the status loop.

Re: [PATCH v4] libselinux: use kernel status page by default

[Stephen Smalley]

On Fri, Jul 24, 2020 at 11:48 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Fri, Jul 24, 2020 at 11:34 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Fri, Jul 24, 2020 at 10:14 AM Mike Palmiotto
> > <mike.palmiotto@crunchydata.com> wrote:
> > >
> > > Commit bc2a8f418e3b ("libselinux: add selinux_status_* interfaces for
> > > /selinux/status") introduced the sestatus mechanism, which allows for
> > > mmap()'ing of the kernel status page as a replacement for avc_netlink.
> > >
> > > The mechanism was initially intended for use by userspace object
> > > managers which were calculating access decisions within their
> > > application and did not rely on the libselinux AVC implementation. In
> > > order to properly make use of sestatus within avc_has_perm(), the status
> > > mechanism needs to properly set avc internals during status events;
> > > else, avc_enforcing is never updated upon sestatus changes.
> > >
> > > This commit introduces a new selinux_status_loop() function, which
> > > replaces the default netlink-equivalent, avc_netlink_loop(). The
> > > function watches the kernel status page until an error occurs, at which
> > > point it will exit the thread. In the event that the status page cannot
> > > be opened (on avc_open), the thread will continue to function as before
> > > by using a fallback netlink socket.
> > >
> > > This allows us to replace the call to avc_netlink_open() in
> > > avc_init_internal() with a call to selinux_status_open() and remove the
> > > avc_netlink_check_nb() call from the critical code path in
> > > avc_has_perm_noaudit(), as well as selinux_check_access().
> > >
> > > Userspace object managers that still need a netlink socket can call
> > > avc_netlink_acquire_fd() to open open and/or obtain one.
> > >
> > > Update the manpage to reflect the new avc_netlink_acquire_fd()
> > > functionality.
> > >
> > > Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com>
> > > ---
> > > Testing:
> > >   - dbus-daemon v1.12.8 on RHEL8.2
> > >   - dbus-broker v22 on F32
> >
> > This looks good to me as far as the code is concerned.  However,
> > installing the patched libselinux and rebooting, I notice that
> > afterward I have dbus-daemon running on a Fedora rawhide instance and
> > consuming nearly 100% CPU constantly.  I'm guessing it is sitting in
> > the status loop. Not sure why there is a dbus-daemon instance running
> > at all since dbus-broker seems to be the default in Fedora and
> > systemctl shows dbus-daemon as disabled.  But if I revert to the stock
> > libselinux, it stops hogging CPU.  Thoughts?
>
> Used gdb to attach to the separate thread and got a traceback before
> and after the libselinux patch.
> Sure enough, before it is performing a blocking poll() operation and
> hence sleeping.  After it is spinning in
> the status loop.

So, options would appear to be:
1) Drop the usage of avc_using_threads altogether, i.e. even if the
caller provided a thread callback, don't create another thread and
just call selinux_status_updated() on every avc_has_perm_noaudit()
unless avc_app_main_loop is set.  Rationale: dbus-daemon was only
using threads to avoid the overhead of avc_netlink_check_nb() on every
avc_has_perm_noaudit() call, and we've eliminated that via use of
sestatus, hence we don't need to create a separate thread at all.
-or-
2) If using threads, then create the netlink socket during avc_init
and keep using the netlink loop for the thread.  This preserves the
blocking behavior.

#1 seems more optimal to me and gets rid of threading for dbus-daemon,
which was something they didn't like anyway.

Re: [PATCH v4] libselinux: use kernel status page by default

[Mike Palmiotto]

On Fri, Jul 24, 2020 at 12:09 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Fri, Jul 24, 2020 at 11:48 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Fri, Jul 24, 2020 at 11:34 AM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > >
> > > On Fri, Jul 24, 2020 at 10:14 AM Mike Palmiotto
> > > <mike.palmiotto@crunchydata.com> wrote:
> > > >
> > > > Commit bc2a8f418e3b ("libselinux: add selinux_status_* interfaces for
> > > > /selinux/status") introduced the sestatus mechanism, which allows for
> > > > mmap()'ing of the kernel status page as a replacement for avc_netlink.
> > > >
> > > > The mechanism was initially intended for use by userspace object
> > > > managers which were calculating access decisions within their
> > > > application and did not rely on the libselinux AVC implementation. In
> > > > order to properly make use of sestatus within avc_has_perm(), the status
> > > > mechanism needs to properly set avc internals during status events;
> > > > else, avc_enforcing is never updated upon sestatus changes.
> > > >
> > > > This commit introduces a new selinux_status_loop() function, which
> > > > replaces the default netlink-equivalent, avc_netlink_loop(). The
> > > > function watches the kernel status page until an error occurs, at which
> > > > point it will exit the thread. In the event that the status page cannot
> > > > be opened (on avc_open), the thread will continue to function as before
> > > > by using a fallback netlink socket.
> > > >
> > > > This allows us to replace the call to avc_netlink_open() in
> > > > avc_init_internal() with a call to selinux_status_open() and remove the
> > > > avc_netlink_check_nb() call from the critical code path in
> > > > avc_has_perm_noaudit(), as well as selinux_check_access().
> > > >
> > > > Userspace object managers that still need a netlink socket can call
> > > > avc_netlink_acquire_fd() to open open and/or obtain one.
> > > >
> > > > Update the manpage to reflect the new avc_netlink_acquire_fd()
> > > > functionality.
> > > >
> > > > Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com>
> > > > ---
> > > > Testing:
> > > >   - dbus-daemon v1.12.8 on RHEL8.2
> > > >   - dbus-broker v22 on F32
> > >
> > > This looks good to me as far as the code is concerned.  However,
> > > installing the patched libselinux and rebooting, I notice that
> > > afterward I have dbus-daemon running on a Fedora rawhide instance and
> > > consuming nearly 100% CPU constantly.  I'm guessing it is sitting in
> > > the status loop. Not sure why there is a dbus-daemon instance running
> > > at all since dbus-broker seems to be the default in Fedora and
> > > systemctl shows dbus-daemon as disabled.  But if I revert to the stock
> > > libselinux, it stops hogging CPU.  Thoughts?
> >
> > Used gdb to attach to the separate thread and got a traceback before
> > and after the libselinux patch.
> > Sure enough, before it is performing a blocking poll() operation and
> > hence sleeping.  After it is spinning in
> > the status loop.
>
> So, options would appear to be:
> 1) Drop the usage of avc_using_threads altogether, i.e. even if the
> caller provided a thread callback, don't create another thread and
> just call selinux_status_updated() on every avc_has_perm_noaudit()
> unless avc_app_main_loop is set.  Rationale: dbus-daemon was only
> using threads to avoid the overhead of avc_netlink_check_nb() on every
> avc_has_perm_noaudit() call, and we've eliminated that via use of
> sestatus, hence we don't need to create a separate thread at all.
> -or-
> 2) If using threads, then create the netlink socket during avc_init
> and keep using the netlink loop for the thread.  This preserves the
> blocking behavior.
>
> #1 seems more optimal to me and gets rid of threading for dbus-daemon,
> which was something they didn't like anyway.

Perhaps this is misguided, but it seems like avc_init is deprecated
and along with it the ability to even set a custom thread callback.
IOTW there does not appear to be a mechanism to set a thread callback
while using avc_open (only avc_init). Perhaps we can just get rid of
the default callback for avc_open and allow the (deprecated) avc_init
to continue using it as it does?

Is this basically what you were proposing for #2? I think I'd be more
inclined to go with that approach, in case userspace object managers
are doing other things in their thread callback.

-- 
Mike Palmiotto
https://crunchydata.com

Re: [PATCH v4] libselinux: use kernel status page by default

[Mike Palmiotto]

On Fri, Jul 24, 2020 at 12:23 PM Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:
>
> On Fri, Jul 24, 2020 at 12:09 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Fri, Jul 24, 2020 at 11:48 AM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > >
> > > On Fri, Jul 24, 2020 at 11:34 AM Stephen Smalley
> > > <stephen.smalley.work@gmail.com> wrote:
> > > >
> > > > On Fri, Jul 24, 2020 at 10:14 AM Mike Palmiotto
> > > > <mike.palmiotto@crunchydata.com> wrote:
> > > > >
> > > > > Commit bc2a8f418e3b ("libselinux: add selinux_status_* interfaces for
> > > > > /selinux/status") introduced the sestatus mechanism, which allows for
> > > > > mmap()'ing of the kernel status page as a replacement for avc_netlink.
> > > > >
> > > > > The mechanism was initially intended for use by userspace object
> > > > > managers which were calculating access decisions within their
> > > > > application and did not rely on the libselinux AVC implementation. In
> > > > > order to properly make use of sestatus within avc_has_perm(), the status
> > > > > mechanism needs to properly set avc internals during status events;
> > > > > else, avc_enforcing is never updated upon sestatus changes.
> > > > >
> > > > > This commit introduces a new selinux_status_loop() function, which
> > > > > replaces the default netlink-equivalent, avc_netlink_loop(). The
> > > > > function watches the kernel status page until an error occurs, at which
> > > > > point it will exit the thread. In the event that the status page cannot
> > > > > be opened (on avc_open), the thread will continue to function as before
> > > > > by using a fallback netlink socket.
> > > > >
> > > > > This allows us to replace the call to avc_netlink_open() in
> > > > > avc_init_internal() with a call to selinux_status_open() and remove the
> > > > > avc_netlink_check_nb() call from the critical code path in
> > > > > avc_has_perm_noaudit(), as well as selinux_check_access().
> > > > >
> > > > > Userspace object managers that still need a netlink socket can call
> > > > > avc_netlink_acquire_fd() to open open and/or obtain one.
> > > > >
> > > > > Update the manpage to reflect the new avc_netlink_acquire_fd()
> > > > > functionality.
> > > > >
> > > > > Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com>
> > > > > ---
> > > > > Testing:
> > > > >   - dbus-daemon v1.12.8 on RHEL8.2
> > > > >   - dbus-broker v22 on F32
> > > >
> > > > This looks good to me as far as the code is concerned.  However,
> > > > installing the patched libselinux and rebooting, I notice that
> > > > afterward I have dbus-daemon running on a Fedora rawhide instance and
> > > > consuming nearly 100% CPU constantly.  I'm guessing it is sitting in
> > > > the status loop. Not sure why there is a dbus-daemon instance running
> > > > at all since dbus-broker seems to be the default in Fedora and
> > > > systemctl shows dbus-daemon as disabled.  But if I revert to the stock
> > > > libselinux, it stops hogging CPU.  Thoughts?
> > >
> > > Used gdb to attach to the separate thread and got a traceback before
> > > and after the libselinux patch.
> > > Sure enough, before it is performing a blocking poll() operation and
> > > hence sleeping.  After it is spinning in
> > > the status loop.
> >
> > So, options would appear to be:
> > 1) Drop the usage of avc_using_threads altogether, i.e. even if the
> > caller provided a thread callback, don't create another thread and
> > just call selinux_status_updated() on every avc_has_perm_noaudit()
> > unless avc_app_main_loop is set.  Rationale: dbus-daemon was only
> > using threads to avoid the overhead of avc_netlink_check_nb() on every
> > avc_has_perm_noaudit() call, and we've eliminated that via use of
> > sestatus, hence we don't need to create a separate thread at all.
> > -or-
> > 2) If using threads, then create the netlink socket during avc_init
> > and keep using the netlink loop for the thread.  This preserves the
> > blocking behavior.
> >
> > #1 seems more optimal to me and gets rid of threading for dbus-daemon,
> > which was something they didn't like anyway.
>
> Perhaps this is misguided, but it seems like avc_init is deprecated
> and along with it the ability to even set a custom thread callback.
> IOTW there does not appear to be a mechanism to set a thread callback

s/IOTW/IOW/


> while using avc_open (only avc_init). Perhaps we can just get rid of
> the default callback for avc_open and allow the (deprecated) avc_init
> to continue using it as it does?
>
> Is this basically what you were proposing for #2? I think I'd be more
> inclined to go with that approach, in case userspace object managers
> are doing other things in their thread callback.

--
Mike Palmiotto
https://crunchydata.com

Re: [PATCH v4] libselinux: use kernel status page by default

[Stephen Smalley]

On Fri, Jul 24, 2020 at 12:23 PM Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:
>
> On Fri, Jul 24, 2020 at 12:09 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Fri, Jul 24, 2020 at 11:48 AM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > >
> > > On Fri, Jul 24, 2020 at 11:34 AM Stephen Smalley
> > > <stephen.smalley.work@gmail.com> wrote:
> > > >
> > > > On Fri, Jul 24, 2020 at 10:14 AM Mike Palmiotto
> > > > <mike.palmiotto@crunchydata.com> wrote:
> > > > >
> > > > > Commit bc2a8f418e3b ("libselinux: add selinux_status_* interfaces for
> > > > > /selinux/status") introduced the sestatus mechanism, which allows for
> > > > > mmap()'ing of the kernel status page as a replacement for avc_netlink.
> > > > >
> > > > > The mechanism was initially intended for use by userspace object
> > > > > managers which were calculating access decisions within their
> > > > > application and did not rely on the libselinux AVC implementation. In
> > > > > order to properly make use of sestatus within avc_has_perm(), the status
> > > > > mechanism needs to properly set avc internals during status events;
> > > > > else, avc_enforcing is never updated upon sestatus changes.
> > > > >
> > > > > This commit introduces a new selinux_status_loop() function, which
> > > > > replaces the default netlink-equivalent, avc_netlink_loop(). The
> > > > > function watches the kernel status page until an error occurs, at which
> > > > > point it will exit the thread. In the event that the status page cannot
> > > > > be opened (on avc_open), the thread will continue to function as before
> > > > > by using a fallback netlink socket.
> > > > >
> > > > > This allows us to replace the call to avc_netlink_open() in
> > > > > avc_init_internal() with a call to selinux_status_open() and remove the
> > > > > avc_netlink_check_nb() call from the critical code path in
> > > > > avc_has_perm_noaudit(), as well as selinux_check_access().
> > > > >
> > > > > Userspace object managers that still need a netlink socket can call
> > > > > avc_netlink_acquire_fd() to open open and/or obtain one.
> > > > >
> > > > > Update the manpage to reflect the new avc_netlink_acquire_fd()
> > > > > functionality.
> > > > >
> > > > > Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com>
> > > > > ---
> > > > > Testing:
> > > > >   - dbus-daemon v1.12.8 on RHEL8.2
> > > > >   - dbus-broker v22 on F32
> > > >
> > > > This looks good to me as far as the code is concerned.  However,
> > > > installing the patched libselinux and rebooting, I notice that
> > > > afterward I have dbus-daemon running on a Fedora rawhide instance and
> > > > consuming nearly 100% CPU constantly.  I'm guessing it is sitting in
> > > > the status loop. Not sure why there is a dbus-daemon instance running
> > > > at all since dbus-broker seems to be the default in Fedora and
> > > > systemctl shows dbus-daemon as disabled.  But if I revert to the stock
> > > > libselinux, it stops hogging CPU.  Thoughts?
> > >
> > > Used gdb to attach to the separate thread and got a traceback before
> > > and after the libselinux patch.
> > > Sure enough, before it is performing a blocking poll() operation and
> > > hence sleeping.  After it is spinning in
> > > the status loop.
> >
> > So, options would appear to be:
> > 1) Drop the usage of avc_using_threads altogether, i.e. even if the
> > caller provided a thread callback, don't create another thread and
> > just call selinux_status_updated() on every avc_has_perm_noaudit()
> > unless avc_app_main_loop is set.  Rationale: dbus-daemon was only
> > using threads to avoid the overhead of avc_netlink_check_nb() on every
> > avc_has_perm_noaudit() call, and we've eliminated that via use of
> > sestatus, hence we don't need to create a separate thread at all.
> > -or-
> > 2) If using threads, then create the netlink socket during avc_init
> > and keep using the netlink loop for the thread.  This preserves the
> > blocking behavior.
> >
> > #1 seems more optimal to me and gets rid of threading for dbus-daemon,
> > which was something they didn't like anyway.
>
> Perhaps this is misguided, but it seems like avc_init is deprecated
> and along with it the ability to even set a custom thread callback.
> IOTW there does not appear to be a mechanism to set a thread callback
> while using avc_open (only avc_init). Perhaps we can just get rid of
> the default callback for avc_open and allow the (deprecated) avc_init
> to continue using it as it does?
>
> Is this basically what you were proposing for #2? I think I'd be more
> inclined to go with that approach, in case userspace object managers
> are doing other things in their thread callback.

I think that's the same as #2 if I understood you currently.  That's
fine if you prefer it.
So then programs using avc_init() with non-NULL thread callbacks
(hence avc_using_threads == 1) will still create the netlink socket
and start a thread running avc_netlink_loop().  And programs using
avc_netlink_acquire_fd() will create the netlink socket if not already
created and can use it however they want.  Everything else will move
to using the status page.

Re: [PATCH v4] libselinux: use kernel status page by default

[Mike Palmiotto]

On Fri, Jul 24, 2020 at 2:26 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Fri, Jul 24, 2020 at 12:23 PM Mike Palmiotto
> <mike.palmiotto@crunchydata.com> wrote:
> >
> > On Fri, Jul 24, 2020 at 12:09 PM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > >
> > > On Fri, Jul 24, 2020 at 11:48 AM Stephen Smalley
> > > <stephen.smalley.work@gmail.com> wrote:
> > > >
> > > > On Fri, Jul 24, 2020 at 11:34 AM Stephen Smalley
> > > > <stephen.smalley.work@gmail.com> wrote:
> > > > >
> > > > > On Fri, Jul 24, 2020 at 10:14 AM Mike Palmiotto
> > > > > <mike.palmiotto@crunchydata.com> wrote:
> > > > > >
> > > > > > Commit bc2a8f418e3b ("libselinux: add selinux_status_* interfaces for
> > > > > > /selinux/status") introduced the sestatus mechanism, which allows for
> > > > > > mmap()'ing of the kernel status page as a replacement for avc_netlink.
> > > > > >
> > > > > > The mechanism was initially intended for use by userspace object
> > > > > > managers which were calculating access decisions within their
> > > > > > application and did not rely on the libselinux AVC implementation. In
> > > > > > order to properly make use of sestatus within avc_has_perm(), the status
> > > > > > mechanism needs to properly set avc internals during status events;
> > > > > > else, avc_enforcing is never updated upon sestatus changes.
> > > > > >
> > > > > > This commit introduces a new selinux_status_loop() function, which
> > > > > > replaces the default netlink-equivalent, avc_netlink_loop(). The
> > > > > > function watches the kernel status page until an error occurs, at which
> > > > > > point it will exit the thread. In the event that the status page cannot
> > > > > > be opened (on avc_open), the thread will continue to function as before
> > > > > > by using a fallback netlink socket.
> > > > > >
> > > > > > This allows us to replace the call to avc_netlink_open() in
> > > > > > avc_init_internal() with a call to selinux_status_open() and remove the
> > > > > > avc_netlink_check_nb() call from the critical code path in
> > > > > > avc_has_perm_noaudit(), as well as selinux_check_access().
> > > > > >
> > > > > > Userspace object managers that still need a netlink socket can call
> > > > > > avc_netlink_acquire_fd() to open open and/or obtain one.
> > > > > >
> > > > > > Update the manpage to reflect the new avc_netlink_acquire_fd()
> > > > > > functionality.
> > > > > >
> > > > > > Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com>
> > > > > > ---
> > > > > > Testing:
> > > > > >   - dbus-daemon v1.12.8 on RHEL8.2
> > > > > >   - dbus-broker v22 on F32
> > > > >
> > > > > This looks good to me as far as the code is concerned.  However,
> > > > > installing the patched libselinux and rebooting, I notice that
> > > > > afterward I have dbus-daemon running on a Fedora rawhide instance and
> > > > > consuming nearly 100% CPU constantly.  I'm guessing it is sitting in
> > > > > the status loop. Not sure why there is a dbus-daemon instance running
> > > > > at all since dbus-broker seems to be the default in Fedora and
> > > > > systemctl shows dbus-daemon as disabled.  But if I revert to the stock
> > > > > libselinux, it stops hogging CPU.  Thoughts?
> > > >
> > > > Used gdb to attach to the separate thread and got a traceback before
> > > > and after the libselinux patch.
> > > > Sure enough, before it is performing a blocking poll() operation and
> > > > hence sleeping.  After it is spinning in
> > > > the status loop.
> > >
> > > So, options would appear to be:
> > > 1) Drop the usage of avc_using_threads altogether, i.e. even if the
> > > caller provided a thread callback, don't create another thread and
> > > just call selinux_status_updated() on every avc_has_perm_noaudit()
> > > unless avc_app_main_loop is set.  Rationale: dbus-daemon was only
> > > using threads to avoid the overhead of avc_netlink_check_nb() on every
> > > avc_has_perm_noaudit() call, and we've eliminated that via use of
> > > sestatus, hence we don't need to create a separate thread at all.
> > > -or-
> > > 2) If using threads, then create the netlink socket during avc_init
> > > and keep using the netlink loop for the thread.  This preserves the
> > > blocking behavior.
> > >
> > > #1 seems more optimal to me and gets rid of threading for dbus-daemon,
> > > which was something they didn't like anyway.
> >
> > Perhaps this is misguided, but it seems like avc_init is deprecated
> > and along with it the ability to even set a custom thread callback.
> > IOTW there does not appear to be a mechanism to set a thread callback
> > while using avc_open (only avc_init). Perhaps we can just get rid of
> > the default callback for avc_open and allow the (deprecated) avc_init
> > to continue using it as it does?
> >
> > Is this basically what you were proposing for #2? I think I'd be more
> > inclined to go with that approach, in case userspace object managers
> > are doing other things in their thread callback.
>
> I think that's the same as #2 if I understood you currently.  That's
> fine if you prefer it.
> So then programs using avc_init() with non-NULL thread callbacks
> (hence avc_using_threads == 1) will still create the netlink socket
> and start a thread running avc_netlink_loop().  And programs using
> avc_netlink_acquire_fd() will create the netlink socket if not already
> created and can use it however they want.  Everything else will move
> to using the status page.

What do you think about moving avc_create_thread call (if
avc_using_threads is set) into avc_netlink_acquire_fd().

That way, if the caller is using avc_init with a create_thread
callback, they can get their netlink socket and create the netlink
thread and everything will function as before. In theory, this would
also work for the sestatus netlink fallback.

-- 
Mike Palmiotto
https://crunchydata.com

Re: [PATCH v4] libselinux: use kernel status page by default

[Mike Palmiotto]

On Fri, Jul 24, 2020 at 3:34 PM Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:
>
> On Fri, Jul 24, 2020 at 2:26 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Fri, Jul 24, 2020 at 12:23 PM Mike Palmiotto
> > <mike.palmiotto@crunchydata.com> wrote:
> > >
> > > On Fri, Jul 24, 2020 at 12:09 PM Stephen Smalley
> > > <stephen.smalley.work@gmail.com> wrote:
> > > >
> > > > On Fri, Jul 24, 2020 at 11:48 AM Stephen Smalley
> > > > <stephen.smalley.work@gmail.com> wrote:
> > > > >
> > > > > On Fri, Jul 24, 2020 at 11:34 AM Stephen Smalley
> > > > > <stephen.smalley.work@gmail.com> wrote:
> > > > > >
> > > > > > On Fri, Jul 24, 2020 at 10:14 AM Mike Palmiotto
> > > > > > <mike.palmiotto@crunchydata.com> wrote:
> > > > > > >
> > > > > > > Commit bc2a8f418e3b ("libselinux: add selinux_status_* interfaces for
> > > > > > > /selinux/status") introduced the sestatus mechanism, which allows for
> > > > > > > mmap()'ing of the kernel status page as a replacement for avc_netlink.
> > > > > > >
> > > > > > > The mechanism was initially intended for use by userspace object
> > > > > > > managers which were calculating access decisions within their
> > > > > > > application and did not rely on the libselinux AVC implementation. In
> > > > > > > order to properly make use of sestatus within avc_has_perm(), the status
> > > > > > > mechanism needs to properly set avc internals during status events;
> > > > > > > else, avc_enforcing is never updated upon sestatus changes.
> > > > > > >
> > > > > > > This commit introduces a new selinux_status_loop() function, which
> > > > > > > replaces the default netlink-equivalent, avc_netlink_loop(). The
> > > > > > > function watches the kernel status page until an error occurs, at which
> > > > > > > point it will exit the thread. In the event that the status page cannot
> > > > > > > be opened (on avc_open), the thread will continue to function as before
> > > > > > > by using a fallback netlink socket.
> > > > > > >
> > > > > > > This allows us to replace the call to avc_netlink_open() in
> > > > > > > avc_init_internal() with a call to selinux_status_open() and remove the
> > > > > > > avc_netlink_check_nb() call from the critical code path in
> > > > > > > avc_has_perm_noaudit(), as well as selinux_check_access().
> > > > > > >
> > > > > > > Userspace object managers that still need a netlink socket can call
> > > > > > > avc_netlink_acquire_fd() to open open and/or obtain one.
> > > > > > >
> > > > > > > Update the manpage to reflect the new avc_netlink_acquire_fd()
> > > > > > > functionality.
> > > > > > >
> > > > > > > Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com>
> > > > > > > ---
> > > > > > > Testing:
> > > > > > >   - dbus-daemon v1.12.8 on RHEL8.2
> > > > > > >   - dbus-broker v22 on F32
> > > > > >
> > > > > > This looks good to me as far as the code is concerned.  However,
> > > > > > installing the patched libselinux and rebooting, I notice that
> > > > > > afterward I have dbus-daemon running on a Fedora rawhide instance and
> > > > > > consuming nearly 100% CPU constantly.  I'm guessing it is sitting in
> > > > > > the status loop. Not sure why there is a dbus-daemon instance running
> > > > > > at all since dbus-broker seems to be the default in Fedora and
> > > > > > systemctl shows dbus-daemon as disabled.  But if I revert to the stock
> > > > > > libselinux, it stops hogging CPU.  Thoughts?
> > > > >
> > > > > Used gdb to attach to the separate thread and got a traceback before
> > > > > and after the libselinux patch.
> > > > > Sure enough, before it is performing a blocking poll() operation and
> > > > > hence sleeping.  After it is spinning in
> > > > > the status loop.
> > > >
> > > > So, options would appear to be:
> > > > 1) Drop the usage of avc_using_threads altogether, i.e. even if the
> > > > caller provided a thread callback, don't create another thread and
> > > > just call selinux_status_updated() on every avc_has_perm_noaudit()
> > > > unless avc_app_main_loop is set.  Rationale: dbus-daemon was only
> > > > using threads to avoid the overhead of avc_netlink_check_nb() on every
> > > > avc_has_perm_noaudit() call, and we've eliminated that via use of
> > > > sestatus, hence we don't need to create a separate thread at all.
> > > > -or-
> > > > 2) If using threads, then create the netlink socket during avc_init
> > > > and keep using the netlink loop for the thread.  This preserves the
> > > > blocking behavior.
> > > >
> > > > #1 seems more optimal to me and gets rid of threading for dbus-daemon,
> > > > which was something they didn't like anyway.
> > >
> > > Perhaps this is misguided, but it seems like avc_init is deprecated
> > > and along with it the ability to even set a custom thread callback.
> > > IOTW there does not appear to be a mechanism to set a thread callback
> > > while using avc_open (only avc_init). Perhaps we can just get rid of
> > > the default callback for avc_open and allow the (deprecated) avc_init
> > > to continue using it as it does?
> > >
> > > Is this basically what you were proposing for #2? I think I'd be more
> > > inclined to go with that approach, in case userspace object managers
> > > are doing other things in their thread callback.
> >
> > I think that's the same as #2 if I understood you currently.  That's
> > fine if you prefer it.
> > So then programs using avc_init() with non-NULL thread callbacks
> > (hence avc_using_threads == 1) will still create the netlink socket
> > and start a thread running avc_netlink_loop().  And programs using
> > avc_netlink_acquire_fd() will create the netlink socket if not already
> > created and can use it however they want.  Everything else will move
> > to using the status page.
>
> What do you think about moving avc_create_thread call (if
> avc_using_threads is set) into avc_netlink_acquire_fd().
>
> That way, if the caller is using avc_init with a create_thread
> callback, they can get their netlink socket and create the netlink
> thread and everything will function as before. In theory, this would
> also work for the sestatus netlink fallback.

Alternatively, we could just move the thread creation into the
sestatus fallback, since, as you pointed out, the only reason for
creating a thread would be to avoid the avc_netlink_check_nb()
overhead.


-- 
Mike Palmiotto
https://crunchydata.com

Re: [PATCH v4] libselinux: use kernel status page by default

[Stephen Smalley]

On Fri, Jul 24, 2020 at 3:43 PM Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:
>
> On Fri, Jul 24, 2020 at 3:34 PM Mike Palmiotto
> <mike.palmiotto@crunchydata.com> wrote:
> >
> > On Fri, Jul 24, 2020 at 2:26 PM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > >
> > > On Fri, Jul 24, 2020 at 12:23 PM Mike Palmiotto
> > > <mike.palmiotto@crunchydata.com> wrote:
> > > >
> > > > On Fri, Jul 24, 2020 at 12:09 PM Stephen Smalley
> > > > <stephen.smalley.work@gmail.com> wrote:
> > > > >
> > > > > On Fri, Jul 24, 2020 at 11:48 AM Stephen Smalley
> > > > > <stephen.smalley.work@gmail.com> wrote:
> > > > > >
> > > > > > On Fri, Jul 24, 2020 at 11:34 AM Stephen Smalley
> > > > > > <stephen.smalley.work@gmail.com> wrote:
> > > > > > >
> > > > > > > On Fri, Jul 24, 2020 at 10:14 AM Mike Palmiotto
> > > > > > > <mike.palmiotto@crunchydata.com> wrote:
> > > > > > > >
> > > > > > > > Commit bc2a8f418e3b ("libselinux: add selinux_status_* interfaces for
> > > > > > > > /selinux/status") introduced the sestatus mechanism, which allows for
> > > > > > > > mmap()'ing of the kernel status page as a replacement for avc_netlink.
> > > > > > > >
> > > > > > > > The mechanism was initially intended for use by userspace object
> > > > > > > > managers which were calculating access decisions within their
> > > > > > > > application and did not rely on the libselinux AVC implementation. In
> > > > > > > > order to properly make use of sestatus within avc_has_perm(), the status
> > > > > > > > mechanism needs to properly set avc internals during status events;
> > > > > > > > else, avc_enforcing is never updated upon sestatus changes.
> > > > > > > >
> > > > > > > > This commit introduces a new selinux_status_loop() function, which
> > > > > > > > replaces the default netlink-equivalent, avc_netlink_loop(). The
> > > > > > > > function watches the kernel status page until an error occurs, at which
> > > > > > > > point it will exit the thread. In the event that the status page cannot
> > > > > > > > be opened (on avc_open), the thread will continue to function as before
> > > > > > > > by using a fallback netlink socket.
> > > > > > > >
> > > > > > > > This allows us to replace the call to avc_netlink_open() in
> > > > > > > > avc_init_internal() with a call to selinux_status_open() and remove the
> > > > > > > > avc_netlink_check_nb() call from the critical code path in
> > > > > > > > avc_has_perm_noaudit(), as well as selinux_check_access().
> > > > > > > >
> > > > > > > > Userspace object managers that still need a netlink socket can call
> > > > > > > > avc_netlink_acquire_fd() to open open and/or obtain one.
> > > > > > > >
> > > > > > > > Update the manpage to reflect the new avc_netlink_acquire_fd()
> > > > > > > > functionality.
> > > > > > > >
> > > > > > > > Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com>
> > > > > > > > ---
> > > > > > > > Testing:
> > > > > > > >   - dbus-daemon v1.12.8 on RHEL8.2
> > > > > > > >   - dbus-broker v22 on F32
> > > > > > >
> > > > > > > This looks good to me as far as the code is concerned.  However,
> > > > > > > installing the patched libselinux and rebooting, I notice that
> > > > > > > afterward I have dbus-daemon running on a Fedora rawhide instance and
> > > > > > > consuming nearly 100% CPU constantly.  I'm guessing it is sitting in
> > > > > > > the status loop. Not sure why there is a dbus-daemon instance running
> > > > > > > at all since dbus-broker seems to be the default in Fedora and
> > > > > > > systemctl shows dbus-daemon as disabled.  But if I revert to the stock
> > > > > > > libselinux, it stops hogging CPU.  Thoughts?
> > > > > >
> > > > > > Used gdb to attach to the separate thread and got a traceback before
> > > > > > and after the libselinux patch.
> > > > > > Sure enough, before it is performing a blocking poll() operation and
> > > > > > hence sleeping.  After it is spinning in
> > > > > > the status loop.
> > > > >
> > > > > So, options would appear to be:
> > > > > 1) Drop the usage of avc_using_threads altogether, i.e. even if the
> > > > > caller provided a thread callback, don't create another thread and
> > > > > just call selinux_status_updated() on every avc_has_perm_noaudit()
> > > > > unless avc_app_main_loop is set.  Rationale: dbus-daemon was only
> > > > > using threads to avoid the overhead of avc_netlink_check_nb() on every
> > > > > avc_has_perm_noaudit() call, and we've eliminated that via use of
> > > > > sestatus, hence we don't need to create a separate thread at all.
> > > > > -or-
> > > > > 2) If using threads, then create the netlink socket during avc_init
> > > > > and keep using the netlink loop for the thread.  This preserves the
> > > > > blocking behavior.
> > > > >
> > > > > #1 seems more optimal to me and gets rid of threading for dbus-daemon,
> > > > > which was something they didn't like anyway.
> > > >
> > > > Perhaps this is misguided, but it seems like avc_init is deprecated
> > > > and along with it the ability to even set a custom thread callback.
> > > > IOTW there does not appear to be a mechanism to set a thread callback
> > > > while using avc_open (only avc_init). Perhaps we can just get rid of
> > > > the default callback for avc_open and allow the (deprecated) avc_init
> > > > to continue using it as it does?
> > > >
> > > > Is this basically what you were proposing for #2? I think I'd be more
> > > > inclined to go with that approach, in case userspace object managers
> > > > are doing other things in their thread callback.
> > >
> > > I think that's the same as #2 if I understood you currently.  That's
> > > fine if you prefer it.
> > > So then programs using avc_init() with non-NULL thread callbacks
> > > (hence avc_using_threads == 1) will still create the netlink socket
> > > and start a thread running avc_netlink_loop().  And programs using
> > > avc_netlink_acquire_fd() will create the netlink socket if not already
> > > created and can use it however they want.  Everything else will move
> > > to using the status page.
> >
> > What do you think about moving avc_create_thread call (if
> > avc_using_threads is set) into avc_netlink_acquire_fd().
> >
> > That way, if the caller is using avc_init with a create_thread
> > callback, they can get their netlink socket and create the netlink
> > thread and everything will function as before. In theory, this would
> > also work for the sestatus netlink fallback.
>
> Alternatively, we could just move the thread creation into the
> sestatus fallback, since, as you pointed out, the only reason for
> creating a thread would be to avoid the avc_netlink_check_nb()
> overhead.

avc_netlink_acquire_fd() isn't called by dbus-daemon in its current
release used in Fedora/RHEL.
So adding it there won't help.  We could add it to
selinux_status_open().  Just need to make sure we don't call
avc_netlink_open() twice there (it is already called in the fallback
case) or make it safe to do so.

Re: [PATCH v4] libselinux: use kernel status page by default

[Mike Palmiotto]

On Fri, Jul 24, 2020 at 4:25 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Fri, Jul 24, 2020 at 3:43 PM Mike Palmiotto
> <mike.palmiotto@crunchydata.com> wrote:
> >
> > On Fri, Jul 24, 2020 at 3:34 PM Mike Palmiotto
> > <mike.palmiotto@crunchydata.com> wrote:
> > >
> > > On Fri, Jul 24, 2020 at 2:26 PM Stephen Smalley
> > > <stephen.smalley.work@gmail.com> wrote:
> > > >
> > > > On Fri, Jul 24, 2020 at 12:23 PM Mike Palmiotto
> > > > <mike.palmiotto@crunchydata.com> wrote:
> > > > >
> > > > > On Fri, Jul 24, 2020 at 12:09 PM Stephen Smalley
> > > > > <stephen.smalley.work@gmail.com> wrote:
> > > > > >
> > > > > > On Fri, Jul 24, 2020 at 11:48 AM Stephen Smalley
> > > > > > <stephen.smalley.work@gmail.com> wrote:
> > > > > > >
> > > > > > > On Fri, Jul 24, 2020 at 11:34 AM Stephen Smalley
> > > > > > > <stephen.smalley.work@gmail.com> wrote:
> > > > > > > >
<snip>
> > > > > > So, options would appear to be:
> > > > > > 1) Drop the usage of avc_using_threads altogether, i.e. even if the
> > > > > > caller provided a thread callback, don't create another thread and
> > > > > > just call selinux_status_updated() on every avc_has_perm_noaudit()
> > > > > > unless avc_app_main_loop is set.  Rationale: dbus-daemon was only
> > > > > > using threads to avoid the overhead of avc_netlink_check_nb() on every
> > > > > > avc_has_perm_noaudit() call, and we've eliminated that via use of
> > > > > > sestatus, hence we don't need to create a separate thread at all.
> > > > > > -or-
> > > > > > 2) If using threads, then create the netlink socket during avc_init
> > > > > > and keep using the netlink loop for the thread.  This preserves the
> > > > > > blocking behavior.
> > > > > >
> > > > > > #1 seems more optimal to me and gets rid of threading for dbus-daemon,
> > > > > > which was something they didn't like anyway.
> > > > >
> > > > > Perhaps this is misguided, but it seems like avc_init is deprecated
> > > > > and along with it the ability to even set a custom thread callback.
> > > > > IOTW there does not appear to be a mechanism to set a thread callback
> > > > > while using avc_open (only avc_init). Perhaps we can just get rid of
> > > > > the default callback for avc_open and allow the (deprecated) avc_init
> > > > > to continue using it as it does?
> > > > >
> > > > > Is this basically what you were proposing for #2? I think I'd be more
> > > > > inclined to go with that approach, in case userspace object managers
> > > > > are doing other things in their thread callback.
> > > >
> > > > I think that's the same as #2 if I understood you currently.  That's
> > > > fine if you prefer it.
> > > > So then programs using avc_init() with non-NULL thread callbacks
> > > > (hence avc_using_threads == 1) will still create the netlink socket
> > > > and start a thread running avc_netlink_loop().  And programs using
> > > > avc_netlink_acquire_fd() will create the netlink socket if not already
> > > > created and can use it however they want.  Everything else will move
> > > > to using the status page.
> > >
> > > What do you think about moving avc_create_thread call (if
> > > avc_using_threads is set) into avc_netlink_acquire_fd().
> > >
> > > That way, if the caller is using avc_init with a create_thread
> > > callback, they can get their netlink socket and create the netlink
> > > thread and everything will function as before. In theory, this would
> > > also work for the sestatus netlink fallback.
> >
> > Alternatively, we could just move the thread creation into the
> > sestatus fallback, since, as you pointed out, the only reason for
> > creating a thread would be to avoid the avc_netlink_check_nb()
> > overhead.
>
> avc_netlink_acquire_fd() isn't called by dbus-daemon in its current
> release used in Fedora/RHEL.
> So adding it there won't help.  We could add it to
> selinux_status_open().  Just need to make sure we don't call
> avc_netlink_open() twice there (it is already called in the fallback
> case) or make it safe to do so.

I've given this a bit more thought, and I'm actually leaning toward
your option #1, Stephen.

Doing away with netlink threads (for non-fallback) should be fine. The
only real change in functionality would be handling of status events
on the next access check, rather than immediately in the thread.

I have a patch repaired to use this approach (and properly handle avc
threads for the sestatus fallback case). I just need to
rebase/review/re-test before submitting.

Thanks for all of the feedback and sorry for the delay.



-- 
Mike Palmiotto
https://crunchydata.com