[apparmor] [PATCH 0/31] Prepare to move cache loading functionality to libapparmor

[John Johansen]

On 12/05/2014 04:21 PM, Tyler Hicks wrote:
> This patch set is the precursor to moving the cache loading code into
> libapparmor. It sets up a clean, supportable interface for external
> programs to interact with match files, feature files, the policy cache,
> and the kernel interface (apparmorfs). It doesn't provide an API for
> individual binary cache files. That will come in the future but
> shouldn't be needed yet as this patch set provides a way to load all
> binaries in the policy cache.
> 
> Code that is being moved into libapparmor must not print to
> stdout/stderr and it must not make calls such as exit(). A good portion
> of this patch set is shuffling code around so that error codes can be
> returned up the stack instead of printing to stderr and calling exit().
> Unfortunately, some useful user-facing logging and developer-facing
> debug logging must be dropped. errno values are used to convey the error
> back up the stack. The use of errno is probably my least favorite thing
> about the patch set.
> 
> The new APIs mimic those found in GLib with _new(), _ref(), and _unref()
> functions that operate on opaque objects. This style is used so that we
> can, for example, change what our underlying policy cache looks like
> without breaking our ABI. This is important as we're looking to make
> some considerably large changes to the policy cache in the near term.
> 
> There are a few warts in this patch set:
> 
>  1) The most obvious is that the cache loading functions aren't yet in
>     libapparmor. This patch set lays the groundwork and has the
>     potential to be controversial. There's just a little more work left
>     to move the functions over and I've got a decent start on it.
>  2) Objects created from the new APIs aren't properly unref'ed/freed.
>     This is because there are still quite a few exit()'s being called in
>     apparmor_parser. More refactoring needs to be done before we can be
>     sure that cleanup is done before the process dies. Luckily, the new
>     aa_*_unref() functions only free memory (as compared to unlinking a
>     file, for example) and there are many pre-existing memory
>     allocations that aren't currently being freed at exit().
>  3) As mentioned above, individual binary policy files aren't
>     represented in the API. That's not too important until we start
>     looking at moving the policy compilation code over to libapparmor.
>     Right now, saying "load all binaries in the policy cache" should be
>     sufficient.
> 
> This patch set passes the parser's make check tests (including the
> caching tests). I also built a parser against a Utopic libapparmor,
> copied the new parser into /sbin/, and did a successful QRT
> test-apparmor.py run which includes a wide array of tests. I think it
> should be pretty solid.
> 
> Sorry for the patch set growing so large. John initially handed me a set
> of 10 or more patches to begin with and then I tried to follow his lead
> and just kept coming across more and more that needed to be done.
> 
Sorry this took so long. I have gone through all the patches including
the first 10 that I dumped on Tyler, those all looked good to me but I
am not going to stick an acked-by on patches I already have a signed-off-by
on.

Tyler your changes to those patches look fine, and I think our dual signed-off
is enough for those.

So I think my big issue with the series is really just questions around
the api and where we want it to go. Not all of that needs to be determined
now, but I'd like a few other people to kick in their ideas