interfaces/apparmor: load unchanged but unloaded profiles

[zyga]

This patch changes Setup() so that after loading changed profiles
there's a second pass that looks at apparmor profiles actually loaded
into the kernel and loads any profiles missing.

This scenario might happen if the install operation is interrupted after
writing profiles to disk but before loading them into the kernel.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

[zyga]

Pinging @jdstrand for a review

[jdstrand]

0644?

[zyga]

This is just a test but fair enough :-)

[jdstrand]

I think you may have typoed 0655 and meant 0644.

This looks fine codewise and what it is doing with the profiles. I wonder though, what if Setup() gets interrupted in this part of the code. Will it be run again at a later time?

LGTM so long as the typo is fixed and if Setup() is working as designed.

[zyga]

@jdstrand yes, it will be re-tried the next time snappy boots

[chipaca]

This LGTM, but seems to suffer from a bad case of moving code around without running the tests after.

[niemeyer]

What happens if the profile is already loaded, is written down to disk, changed, but then not reloaded? It will not be reloaded above, nor below. Sounds like we're going over a lot of trouble with reading files back, re-verifying that things are indeed loaded, and still have the potential for easy bugs.

Can we instead change the approach so it is simpler and thus harder to fail?

[zyga]

Without patching apparmor to give us cryptographic hashes of the loaded profile we are always going to be in a hard place. We could unload all profiles but that will quickly bite us as those things are slow to compute.

@jdstrand What do you think?

[jdstrand]

I previously mentioned that I had a hard time understanding the purpose of this addition. We are invoking the parser in such a way that you point it at a policy file, it compiles it into a cache file and loads it into the kernel. If the cache file doesn't exist on reboot, the parser will create it; if it does exist and is newer than the profile, the parser will use the cache file; if it does exist but is older than the profile, the parser will recompile and create a new cache file. Snappy should continue with this approach.

Therefore, all you need to do is make sure all the profiles you want are created and then run the parser on them (obviously checking for errors; but also don't update the mtime unless they actually changed). If things get interrupted on what would otherwise be a successful parser run, don't invalidate the cache files you already computed, just run the parser on them in the way you always would (reloading an already loaded profile from cache is cheap, fast and robust).

[zyga]

@jdstrand this is exactly what this patch is doing. The tricky part is part is that apparmor backend code only loads / unloads profiles in reaction changes to profiles on disk. If you interrupt the current code at the right time then there will be no disk changes when the task is re-tried.

Thinking about it now I think I will unconditionally load all profiles. This will take care of apparmor include file changes (apparmor_parser -p discussion from the email thread) and will be a simple thing to do before we invest in a more efficient version.

[niemeyer]

No, this is not actually what the patched code is doing. The code does a complex dance trying to observe what actually changed, and then makes it even more complex by observing if any of the files on disk are not loaded.

What we should do instead, is to load everything the logic might possibly have changed. I suspect that's going to be much simpler, more robust, and perhaps even faster, ironically, since we won't be re-reading stuff from disk.

[zyga]

@chipaca the elusive simplicity of github's update button

[zyga]

I'm closing this for now as I plan to use a more reliable method that solves other problems while being simpler overall.