[apparmor] [patch] fixes for LP: #775785, full /tmp reload issue

[Steve Beattie]

On Sat, Aug 13, 2011 at 12:08:28AM +0200, Christian Boltz wrote:
> Am Donnerstag, 4. August 2011 schrieb Steve Beattie:
> > Bug: https://bugs.launchpad.net/bugs/775785
> > 
> > Attached is a patch to make the initscript not fail if /tmp is full
> > by converting the comm(1) usage on temporary files to an embedded
> > awk script. On both Ubuntu and OpenSUSE, a version of awk (mawk in
> > Ubuntu, gawk in OpenSUSE) is either a direct or indirect dependency
> > on the minimal or base package set, and the original reporter also
> > mentioned that an awk-based solution would be palatable in a way that
> > converting to bash, or using perl or python here would not be.
> 
> The easier solution would be to replace (pseudocode)
> 
> list_profiles > $tmpfile
> cat $tmpfile | do_it
> 
> with
> 
> list_profiles | do_it
> 
> But it's probably a matter of taste if the "old" code with lots of pipes
> or the awk script are more readable ;-)

See below why the awk bits are necessary.

> Besides that, I somehow wonder about this part of your patch:
> 
> > Index: b/parser/rc.apparmor.functions
> > ===================================================================
> > --- a/parser/rc.apparmor.functions
> > +++ b/parser/rc.apparmor.functions
> 
> > +       profiles_names_list | awk '
> > +BEGIN {
> > +  while (getline < "'${SFS_MOUNTPOINT}'/profiles" ) {
> 
> I'm not an awk expert, but to me it looks like awk has two possible
> data sources:
> a) result of profiles_names_list via STDIN
> b) reading ${SFS_MOUNTPOINT}'/profiles
> 
> If I'm right, can you decide on one and remove the other, please? ;-)

You're correct that awk is using two sources of input. It needs
to, because what this portion of the initscript helper is doing is
comparing the list of all the profiles configured in /etc/apparmor.d/
against the list of profiles the kernel has loaded and unloading
profiles that are in the latter set but not the former. The point
is to make consistent between the set of configured profiles and the
set of loaded profiles.

An example where this would occur is if a profile or hat were deleted
or disabled by an admin; the reload action action should take this
into account.

It by definition requires two sources of input (the set of configured
profiles and the set of loaded profiles) which is why the simple
pipeline is insufficient, and why awk takes an input on stdin and
opens a separate input location.

To restate, the problem with the existing code is that it creates
temporary files in $TMPDIR (by default /tmp) and if that partition
is full, problems with the reload action ensue. Alternate solutions
include switching the initscript to use bash and its <$() extension
or setting TMPDIR to /dev/shm/. The former is unpalatable to some
(particularly for an initscript), and for the latter, /dev/shm is
only guaranteed to exist on GNU libc based systems (glibc apparently
expects /dev/shm to exist for its POSIX shared memory implementation;
see shm_overview(7)).  So to me, awk (sans GNU extensions) looks to
be the least bad option here.

> To avoid regressions, can you please test if the following profile is 
> handled correctly?
> 
> profile "/tmp/BOfH " {
>   /bin/bash r,
> }
> 
> BTW: It works with the current initscript on openSUSE 11.4.
> 
> However, I noted a small issue: /sys/kernel/security/apparmor/profiles 
> seems to hide the space:
> 
> # cat /sys/kernel/security/apparmor/profiles  |sed 's/ /+/g' # shortened
> /tmp/BOfH+(enforce)
> /sbin/syslogd+(enforce)
> 
> I'd expect another space:
> /tmp/BOfH++(enforce)
>          ^^

Yes, this is a separate issue. Are you sure they work in that instance?
Either on reload, "/tmp/BOfH " will always get unloaded or it will
never get unloaded even if the profile is deleted. I'm seeing the
former in my test factory instance (which admittedly is the worse
behavior of the two to have). Somewhere in the kernel (I believe),
the trailing space is getting dropped.

> > I've also attached a patch for the Ubuntu version of the initscript,
> > which is distinct from the upstream versions.
> 
> Sounds like maintenance "fun" - what's the reason for the different 
> initscript?

Simplification, I believe. I keep meaning to remove a lot of the
historical cruft in the upstream initscripts, but it hasn't percolated
to the top of the work queue.

I would like it to happen so that Ubuntu can use the upstream scripts,
because I'd like the functions file to be shared across distros so
that we only need to fix the majority of initscript bugs in one place.

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20110815/d57a5b75/attachment.pgp>