Bug 726544

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.