libselinux uses gcc constructors to initialize its selinux_mnt variable. This is really broken and makes it impossible to run systemd+selinux without an initrd. Problem is this: whithout the initrd at the time when systemd is first invoked neither /proc nor /sys will be mounted. Hence libselinux' constructor will also not be able to to see /sys/fs/selinux, nor /proc/filesystems and hence consider SELinux off. That means that if systemd is booted without initrd then is_selinux_enabled() will *always* tell it that selinux is off, and there's nothing systemd can do to fix it. Using gcc constructors is really broken. Your code this way runs even before the first line of systemd itself runs. Given that systemd is the first piece of userspace code usually run your code will hence run even before that -- in a completely undefined minimalistic environment. This is broken in many other ways too: if your library is pulled in via dlopen() [either directly, or indirectly, for example via a PAM module], then this constructor might get executed in a parallel to some other code of the application. Which is necessarily racy, since constructor execution is not serialized. Now you might think that libselinux is never used in threaded apps, but that is simply not true. Newer gdm supports multi-stack PAM. This executes multiple PAM chains in parallel in order to support parallel auth-by-smartcard, auth-by-password and auth-by-fingerprint. Now, gdm does not link against libselinux, but pam_selinux.so does, which is pulled in via PAM/dlopen(). Since gcc constructors are not executed serialized you'll end up running one of those stacks possibly without the constructor having finished. Also, what happens if some other code also uses gcc constructors? There are no real controls to order gcc constructor execution. That means if that other code happens to invoke is_selinux_enabled() this will also show incorrect results. There are very very few cases where gcc constructor usage are acceptable. libselinux is not one of them. Doing things behind the back of the main programmer with gcc constructors is really bad style, especially for a low-level systems library like libselinux. Just don't do it. Maintaining global state is bad enough, due to the multi-threading problems it introduces. But throwing gcc constructors into the mix makes it even worse. The fix I'd recommend is to initialize the variable selinux_mnt lazily, the first time it is accessed, and possibly use __tls for that. That way, systemd has the option to mount /proc and /sys in time.
systemd should be calling selinux_init_load_policy() before ever testing is_selinux_enabled(). If it does that, then selinux_mnt will be initialized by selinux_init_load_policy() via set_selinuxmnt() after it mounts selinuxfs. That's how it worked in SysVinit. Checking is_selinux_enabled() before policy is loaded will always yield no, so init has always been a special case there. We could switch libselinux to use __selinux_once() to initialize selinux_mnt prior to the first reference to it in every libselinux interface that uses it, as we already do elsewhere for other global state. We likely would have done selinux_mnt that way in the first place, but __selinux_once() came along later. I am a little puzzled that this hasn't come up earlier if it is such a big issue, as e.g. Ulrich Drepper reviewed libselinux and submitted a number of changes in the past, but didn't comment on this particular issue to my knowledge. Certainly libselinux has been working this way since 2003 without apparent difficulty. Anyway, a patch to switch libselinux to use __selinux_once() to initialize selinux_mnt seems fine to me. I don't think we need/want tls (__thread) for it, as there is no need for it to be per-thread. We just want to ensure that it gets initialized exactly once before first use. Even with that change, you still need to call selinux_init_load_policy() before testing if you want an answer other than always no.
Nalin, I added you because I thought you stated that pam could not be used within threaded environments?
I would recommend against it (thread-safety isn't guaranteed, and the number of side-effects PAM can have on an unsuspecting calling process is... large). But the main problem is that libselinux's constructors can be run before systemd has a chance to set up the system the way they expect things to be.
Right but as Steven points out, systemd can fix the setup by calling the load_policy functions. Even if we fixed the mnt to not happen or to happen Lazily, systemd calling is_selinux_enabled() will always get true until it loads the policy.
Dan, you're backwards, it always returns false if policy hasn't been loaded, but still. Lennart's idea (before he just switched systemd to use setcon) was to check is_selinux_enabled() to tell if it was before or after the re-exec. It shouldn't be hard for us to get rid of the constructor/destructor, but at the moment it isn't a show stopper.
(In reply to comment #1) > systemd should be calling selinux_init_load_policy() before ever testing > is_selinux_enabled(). We do that. We do this basically: 1. mount /proc, /sys, /dev, if not mounted yet 2. if getcon_raw() works and is != "kernel" DONE 3. selinux_init_load_policy() 4. is_selinux_enabled() 5. getcon() 6. getfilecon("/sbin/init") 7. security_compute_create() 8. setcon() Step 4 always fails when no initrd is used, we don't try 5-8 then anymore. > If it does that, then selinux_mnt will be initialized by > selinux_init_load_policy() via set_selinuxmnt() after it mounts selinuxfs. > That's how it worked in SysVinit. The difference is that on sysvinit you reexec'ed yourself after loading the policy. Which we don't do in systemd anymore. As long as we did that, everything was fine, since the constructor was rerun. > Checking is_selinux_enabled() before policy > is loaded will always yield no, so init has always been a special case there. We don't do that. > We could switch libselinux to use __selinux_once() to initialize selinux_mnt > prior to the first reference to it in every libselinux interface that uses it, > as we already do elsewhere for other global state. We likely would have done > selinux_mnt that way in the first place, but __selinux_once() came along later. > > I am a little puzzled that this hasn't come up earlier if it is such a big > issue, as e.g. Ulrich Drepper reviewed libselinux and submitted a number of > changes in the past, but didn't comment on this particular issue to my > knowledge. Certainly libselinux has been working this way since 2003 without > apparent difficulty. Well, as long as init is reexecuted the constructors are not a problem in init.
I don't quite understand the problem then. selinux_init_load_policy() mounts selinuxfs and then calls set_selinuxmnt() with the mount point directory. This sets selinux_mnt to that directory pathname. So a subsequent call to is_selinux_enabled() should use that pathname and work. Why then would step 4 fail? We aren't relying on the constructor in that situation.
An experiment I tried: I modified load_policy to call and print the result of is_selinux_enabled() after calling selinux_init_load_policy. Then I booted with init=/bin/bash, made sure /selinux was not mounted, and ran load_policy -i (initial policy load). It loaded the policy and then reported that is_selinux_enabled() reported 1. Without performing any re-exec or anything. So I don't know what is happening for systemd.
Hmm, so as it turns out systemd added another caching of the return value of is_selinux_enabled() which didn't get reset. After fixing this things work now. Sorry for the confusion. I'll close the bug now, but I am still of the opinion that you shouldn't use gcc constructors for this, really! Sorry for the noise.