[apparmor] Cache update broken
Seth Arnold
seth.arnold at gmail.com
Tue Aug 7 16:58:42 UTC 2012
The patch reads well, though the --help needs to indicate that the flag only works if --write-cache is given _and_the features don't match.
I would much rather --clear-cache be a debugging tool for sysadmin use that immediately and directly clears the cache, and introduce a different name for this option. I propose --clear-cache-if-needed.
I think the test (yay tests) would read better with this changed to use cmp -s:
+echo -n "monkey" | diff -q $basedir/cache/$profile - | grep -q 'differ' || { echo "FAIL"; exit 1; }
+echo -n "monkey" | cmp -s $basedir/cache/$profile || { echo "FAIL"; exit 1; }
Thanks John
-----Original Message-----
From: John Johansen <john.johansen at canonical.com>
Sender: apparmor-bounces at lists.ubuntu.com
Date: Tue, 07 Aug 2012 09:17:46
To: Christian Boltz<apparmor at cboltz.de>
Cc: <apparmor at lists.ubuntu.com>
Subject: Re: [apparmor] Cache update broken
On 08/04/2012 06:53 AM, Christian Boltz wrote:
> Hello,
>
> I received a bugreport that loading AppArmor needs 25 seconds at boot:
> https://bugzilla.novell.com/show_bug.cgi?id=774529
> I can reproduce the problem on my system (AppArmor 2.8.0)
>
> It looks like the cache is not updated, and (for obvious reasons) the
> outdated cache isn't used.
>
> # grep '^[^#]' /etc/apparmor/parser.conf
> write-cache
> show-cache
>
> # apparmor_parser -r /etc/apparmor.d/usr.lib.dovecot.deliver
> Cache read/write disabled: /sys/kernel/security/apparmor/features does
> not match /etc/apparmor.d/cache/.features
> Cache miss: /etc/apparmor.d/usr.lib.dovecot.deliver
>
> Expected behaviour IMHO: update the cache and the .features file.
>
> Any idea what is wrong? (A patch would be even better ;-)
>
>
Alright attached is a first pass at a patch.
The bug basically comes down to the apparmor cache will not be updated
by --write-cache when .features does not match because the parser processes
policy one profile at a time is trying to avoid having profiles of different
binary versions in the cache.
The solution is to clear the cache. Ubuntu is doing this in the init
scripts but suse went with using the defaults file and always setting the
--write-cache option.
The patch adds a --clear-cache option to the parser
There are is still documentation to be done and decisions about defaults,
currently --clear-cache has the following behavior
it clears the cache if --write-cache is enabled and the cache features
do not match.
We could
1. change current behavior to be on by default, and change to a no-clear-cache
flag.
2. make --clear-cache always clear the cache
2.1 regardless of whether --write-cache is enabled
2.2 regardless of whether features do not match
2.3 regardless of both --write-cache and features not matching
Also note currently it is only clearing out regular files, in a non-recursive
manner.
---
apparmor: add the ability to clear the profile cache
Add the ability to clear out the binary profile cache. This removes the
need to have a separate script to handle the logic of checking and
removing the cache if it is out of date.
The parser already does all the checking to determine cache validity
so it makes sense to allow the parser to clear out inconsistent cache
when it has been instructed to update the cache.
Signed-off-by: John Johnansen <john.johansen at canonical.com>
---
=== modified file 'parser/parser_main.c'
--- parser/parser_main.c 2012-07-17 23:00:53 +0000
+++ parser/parser_main.c 2012-08-07 15:55:43 +0000
@@ -24,6 +24,7 @@
#include <string.h>
#include <stdlib.h>
#include <stdarg.h>
+#include <stddef.h>
#include <getopt.h>
#include <errno.h>
#include <fcntl.h>
@@ -71,6 +72,7 @@
int skip_cache = 0;
int skip_read_cache = 0;
int write_cache = 0;
+int clear_cache = 0; /* only applies if write is set */
int preprocess_only = 0;
int skip_mode_force = 0;
struct timespec mru_tstamp;
@@ -109,6 +111,8 @@
{"skip-read-cache", 0, 0, 'T'},
{"write-cache", 0, 0, 'W'},
{"show-cache", 0, 0, 'k'},
+ {"clear-cache", 0, 0, 128}, /* no short option */
+ {"no-clear-cache", 0, 0, 129}, /* no short option */
{"cache-loc", 1, 0, 'L'},
{"debug", 0, 0, 'd'},
{"dump", 1, 0, 'D'},
@@ -151,6 +155,7 @@
"-K, --skip-cache Do not attempt to load or save cached profiles\n"
"-T, --skip-read-cache Do not attempt to load cached profiles\n"
"-W, --write-cache Save cached profile (force with -T)\n"
+ " --clear-cache Clear cache\n"
"-L, --cache-loc n Set the location of the profile cache\n"
"-q, --quiet Don't emit warnings\n"
"-v, --verbose Show profile names as they load\n"
@@ -527,6 +532,12 @@
case 'T':
skip_read_cache = 1;
break;
+ case 128:
+ clear_cache = 1;
+ break;
+ case 129:
+ clear_cache = 0;
+ break;
case 'L':
cacheloc = strdup(optarg);
break;
@@ -1165,6 +1176,120 @@
return retval;
}
+static int dir_for_each(const char *dname,
+ int (* callback)(const char *, struct dirent *,
+ struct stat *)) {
+ struct dirent *dirent, *ent;
+ char *path = NULL;
+ DIR *dir = NULL;
+ int error;
+
+ dirent = malloc(offsetof(struct dirent, d_name) +
+ pathconf(dname, _PC_NAME_MAX) + 1);
+ if (!dirent) {
+ PDEBUG(_("could not alloc dirent"));
+ return -1;
+ }
+
+ PDEBUG("Opened cache directory \"%s\"\n", dname);
+ if (!(dir = opendir(dname))) {
+ free(dirent);
+ PDEBUG(_("opendir failed '%s'"), dname);
+ return -1;
+ }
+
+ for (error = readdir_r(dir, dirent, &ent);
+ error == 0 && ent != NULL;
+ error = readdir_r(dir, dirent, &ent)) {
+ struct stat my_stat;
+
+ if (strcmp(dirent->d_name, ".") == 0 ||
+ strcmp(dirent->d_name, "..") == 0)
+ continue;
+
+ if (asprintf(&path, "%s/%s", dname, dirent->d_name) < 0)
+ {
+ PDEBUG(_("Memory allocation error."));
+ goto fail;
+ }
+
+ if (stat(path, &my_stat)) {
+ PDEBUG(_("stat failed for '%s'"), path);
+ goto fail;
+ }
+
+ if (callback(path, dirent, &my_stat)) {
+ PDEBUG(_("dir_for_each callback failed\n"));
+ goto fail;
+ }
+
+ free(path);
+ path = NULL;
+ }
+
+ free(dirent);
+ closedir(dir);
+ return error;
+
+fail:
+ error = errno;
+ free(dirent);
+ free(path);
+ closedir(dir);
+ errno = error;
+
+ return -1;
+}
+
+static int clear_cache_cb(const char *path, __unused struct dirent *dirent,
+ struct stat *ent_stat)
+{
+ /* remove regular files */
+ if (S_ISREG(ent_stat->st_mode))
+ return unlink(path);
+
+ /* do nothing with other file types */
+ return 0;
+}
+
+static int clear_cache_files(const char *path)
+{
+ char *cache;
+ int error;
+
+ if (asprintf(&cache, "%s/cache", path) == -1) {
+ perror("asprintf");
+ exit(1);
+ }
+
+ error = dir_for_each(cache, clear_cache_cb);
+
+ free(cache);
+
+ return error;
+}
+
+static int create_cache(const char *path, const char *features)
+{
+ FILE * f = NULL;
+
+ f = fopen(path, "w");
+ if (f) {
+ if (fwrite(features, strlen(features), 1, f) != 1 )
+ goto fail;
+
+ fclose(f);
+ }
+
+ return 0;
+fail:
+ if (show_cache)
+ PERROR("Cache write disabled: cannot create %s\n", path);
+ write_cache = 0;
+
+ return -1;
+}
+
static void setup_flags(void)
{
char *cache_features_path = NULL;
@@ -1203,30 +1328,23 @@
get_flags_string(&cache_flags, cache_features_path);
if (cache_flags) {
if (strcmp(flags_string, cache_flags) != 0) {
- if (show_cache) PERROR("Cache read/write disabled: %s does not match %s\n", FLAGS_FILE, cache_features_path);
- write_cache = 0;
- skip_read_cache = 1;
+ if (write_cache && clear_cache) {
+ if (clear_cache_files(basedir) ||
+ create_cache(cache_features_path,
+ flags_string)) {
+ skip_read_cache = 1;
+ }
+ } else {
+ if (show_cache)
+ PERROR("Cache read/write disabled: %s does not match %s\n", FLAGS_FILE, cache_features_path);
+ write_cache = 0;
+ skip_read_cache = 1;
+ }
}
free(cache_flags);
cache_flags = NULL;
- }
- else if (write_cache) {
- FILE * f = NULL;
- int failure = 0;
-
- f = fopen(cache_features_path, "w");
- if (!f) failure = 1;
- else {
- if (fwrite(flags_string, strlen(flags_string), 1, f) != 1 ) {
- failure = 1;
- }
- if (fclose(f) != 0) failure = 1;
- }
-
- if (failure) {
- if (show_cache) PERROR("Cache write disabled: cannot write to %s\n", cache_features_path);
- write_cache = 0;
- }
+ } else if (write_cache) {
+ create_cache(cache_features_path, flags_string);
}
free(cache_features_path);
=== modified file 'parser/tst/caching.sh'
--- parser/tst/caching.sh 2012-03-09 12:25:03 +0000
+++ parser/tst/caching.sh 2012-08-07 15:57:56 +0000
@@ -99,6 +99,24 @@
[ -f $basedir/cache/$profile ] && echo "FAIL ($basedir/cache/$profile exists)" && exit 1
echo "ok"
+rm -f $basedir/cache/.features || true
+rm -f $basedir/cache/$profile || true
+echo -n "monkey" > $basedir/cache/.features
+echo -n "monkey" > $basedir/cache/$profile
+echo -n "monkey" > $basedir/cache/monkey
+#../apparmor_parser $ARGS -v --write-cache --clear-cache -r $basedir/$profile | grep -q 'Replacement succeeded for' || { echo "Cache clear setup FAIL"; exit 1; }
+../apparmor_parser $ARGS -v --write-cache --clear-cache -r $basedir/$profile || { echo "Cache clear setup FAIL"; exit 1; }
+echo -n "Cache clear updates features: "
+echo -n "monkey" | diff -q $basedir/cache/.features - | grep -q 'differ' || { echo "FAIL"; exit 1; }
+echo "ok"
+echo -n "Cache clear writes updated profile: "
+echo -n "monkey" | diff -q $basedir/cache/$profile - | grep -q 'differ' || { echo "FAIL"; exit 1; }
+echo "ok"
+echo -n "Cache clear cleans out all files: "
+[ -f $basedir/cache/monkey ] && { echo "FAIL"; exit 1; }
+echo "ok"
+rm -f $basedir/cache/monkey
+
echo -n "Profiles are cached when requested (again): "
rm -f $basedir/cache/.features || true
rm -f $basedir/cache/$profile || true
--
AppArmor mailing list
AppArmor at lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
More information about the AppArmor
mailing list