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

[Tyler Hicks]

On 2015-01-22 10:12:25, John Johansen wrote:
> 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.

There is a lot to digest and the time that I sent them out was less than
ideal since the holidays were approaching.

I agree that acked-by does not make sense for where we already have our
signed-off-by.

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

I thought that dual sign-off was sufficient, too. If anyone has a
problem with that, please speak up and start reviewing. ;)

> 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

Yep - the actual code changes are intended to be minimal. The API
changes should be the focus of the review, IMO.

Tyler
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150122/2d53db19/attachment.pgp>