From: Stephen Smalley <email@example.com> To: Mike Palmiotto <firstname.lastname@example.org> Cc: SElinux list <email@example.com> Subject: Re: [PATCH v4] libselinux: use kernel status page by default Date: Fri, 24 Jul 2020 12:09:33 -0400 Message-ID: <CAEjxPJ4JF_2jRP5WG6Uz51gEc7H+nRv2YAhcdTUzcwwYmQub_A@mail.gmail.com> (raw) In-Reply-To: <CAEjxPJ4JF2xFMVgRk9wmwg_cAqKuRXc5FKkqHx525mk6JJy0hA@mail.gmail.com> On Fri, Jul 24, 2020 at 11:48 AM Stephen Smalley <firstname.lastname@example.org> wrote: > > On Fri, Jul 24, 2020 at 11:34 AM Stephen Smalley > <email@example.com> wrote: > > > > On Fri, Jul 24, 2020 at 10:14 AM Mike Palmiotto > > <firstname.lastname@example.org> 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 <email@example.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.
next prev parent reply index Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-24 14:13 Mike Palmiotto 2020-07-24 15:34 ` Stephen Smalley 2020-07-24 15:48 ` Stephen Smalley 2020-07-24 16:09 ` Stephen Smalley [this message] 2020-07-24 16:23 ` Mike Palmiotto 2020-07-24 16:29 ` Mike Palmiotto 2020-07-24 18:25 ` Stephen Smalley 2020-07-24 19:34 ` Mike Palmiotto 2020-07-24 19:42 ` Mike Palmiotto 2020-07-24 20:25 ` Stephen Smalley 2020-07-31 14:17 ` Mike Palmiotto
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAEjxPJ4JF_2jRP5WG6Uz51gEc7H+nRv2YAhcdTUzcwwYmQub_A@mail.gmail.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
SELinux Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/selinux/0 selinux/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 selinux selinux/ https://lore.kernel.org/selinux \ email@example.com public-inbox-index selinux Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.kernel.vger.selinux AGPL code for this site: git clone https://public-inbox.org/public-inbox.git