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.johan...@canonical.com
---
=== modified file 'parser/parser_main.c'
--- parser/parser_main.c2012-07-17 23:00:53 +
+++ parser/parser_main.c2012-08-07 15:55:43 +
@@ -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-cacheDo 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
Re: [apparmor] Cache update broken
On 08/07/2012 09:58 AM, Seth Arnold wrote:
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 am okay with --clear-cache-if-needed but its going to be used as more than a
debugging tool
suse will be using it in conjunction with --write-cache to clear the cache out
when the
feature set changes.
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; }
sure
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] Cache update broken
Hello, John, thanks for honoring the golden rules of bad programming in your patch! I'm especially talking about rule 18 - take great care in setting bad defaults ;-) Am Dienstag, 7. August 2012 schrieb Seth Arnold: I expect --clear-cache-if-needed to be the default set in the config file Let me ask a simple question: Can you give me a good reason _not_ to automatically clear the cache if .features differs, and to keep an outdated cache? [1] IMHO most people want their cache updated automatically, and it doesn't make much sense to force everybody to add --clear-cache-if-needed to the initscript or the config file. Can we please make it the default _without_ the need for an additional parameter or config option? If you really want, feel free to introduce a --never-clear-cache-automatically parrameter / config file option - but I doubt many people will use it ;-) -- redundant for ubuntu but also a chance to bring both initscripts together again -- at least for this feature. I don't know the history and background why there are separate initscripts (pointers welcome), but I'm a big fan of avoiding duplicate work (especially if I have to maintain the duplicate ;-)) A direct --clear-cache would just be a debugging tool for admins, and rarely used (hopefully) at that. Indeed. It might be a nice feature, but I'd give it a low priority [2]. The avarage admin most probably knows how to delete all files in a directory ;-) Regards, Christian Boltz [1] oh, now I remember: rule 22 - invent new ways to make your program slow ;-) [2] aa-enable is more important IMHO because it needs to a) delete a symlink b) load the profile -- Ich selbst benutze kweather nicht (ich guck einfach aus dem Fenster). [Hartmut Meyer in suse-linux] -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] Cache update broken
On 08/07/2012 01:34 PM, Christian Boltz wrote: Hello, John, thanks for honoring the golden rules of bad programming in your patch! I'm especially talking about rule 18 - take great care in setting bad defaults ;-) Hehe I did it on purpose to get a discussion of what it should be on list :) Basically I was hoping for more input than just our little discussion, and the change is really easy to make so a 2nd iteration won't take very long. Am Dienstag, 7. August 2012 schrieb Seth Arnold: I expect --clear-cache-if-needed to be the default set in the config file Let me ask a simple question: Can you give me a good reason _not_ to automatically clear the cache if .features differs, and to keep an outdated cache? [1] well I can think of a corner case where you are booting between different kernels, and want to keep one. But I don't really think that is a good reason, and I think the proper solution for that is having separate caches for each kernel. IMHO most people want their cache updated automatically, and it doesn't make much sense to force everybody to add --clear-cache-if-needed to the initscript or the config file. Can we please make it the default _without_ the need for an additional parameter or config option? Well I suspect that is what we will do If you really want, feel free to introduce a --never-clear-cache-automatically parrameter / config file option - but I doubt many people will use it ;-) -- redundant for ubuntu but also a chance to bring both initscripts together again -- at least for this feature. I don't know the history and background why there are separate initscripts (pointers welcome), but I'm a big fan of avoiding duplicate work (especially if I have to maintain the duplicate ;-)) Well the history is fairly simple, Ubuntu added caching support when it was in a very early iteration in the parser. I can't remember whether there was even time stamp checks in the first iteration that shipped (you could do that by dumping the profile out using -S and using a separate utility to do the actual load). But I do know that timestamp changes within include files where not taken into account and cache clearing was being done by a script. The goal has always been to merge back changes where appropriate or replace the code as thing in the upstream project progressed. So keeping the old script behavior isn't necessarily a goal. Using the --write-cache with cache-clearing will have to be brought up and discussed. I specifically mentioned that suse was going to use this for its cache in my reply to Seth because I misunderstood what he was saying about a debug option. Of course he is right, a clear cache option that isn't default does feel like an admin debug option. A direct --clear-cache would just be a debugging tool for admins, and rarely used (hopefully) at that. Indeed. It might be a nice feature, but I'd give it a low priority [2]. The avarage admin most probably knows how to delete all files in a directory ;-) yeah well thankfully that debug option is really easy to do now. Regards, Christian Boltz [1] oh, now I remember: rule 22 - invent new ways to make your program slow ;-) Having broken cache management is a new way to make your program slow? :) [2] aa-enable is more important IMHO because it needs to a) delete a symlink b) load the profile sure that make sense, but I'll way it against I am in the patch right now and can add it with just a few lines of code while its fresh in my mind. -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] Cache update broken
On 08/04/2012 06:53 AM, Christian Boltz wrote:
Hello,
okay attached is v2 of the patch, its changes are:
* makes cache clearing the default behavior if --write-cache is enabled
* changes --no-clear-cache to --skip-bad-cache (really the same thing but
hopefully a better name?)
* adds a --purge-cache debug option to force the cache to be cleared
regardless of state
* adds documentation to the apparmor_parser man page
---
apparmor: add clearing the profile cache when inconsistent
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.johan...@canonical.com
---
=== modified file 'parser/apparmor_parser.pod'
--- parser/apparmor_parser.pod 2012-02-24 12:21:59 +
+++ parser/apparmor_parser.pod 2012-08-07 22:06:34 +
@@ -138,6 +138,15 @@
is running with --replace, it may make sense to also use
--skip-read-cache with the --write-cache option.
+=item -W, --purge-cache
+
+Unconditionally clear out cached profiles.
+
+=item -W, --skip-bad-cache
+
+Skip updating the cache if it contains cached profiles in a bad or
+inconsistant state
+
=item -L, --cache-loc
Set the location of the cache directory. If not specified the cache location
=== modified file 'parser/parser_main.c'
--- parser/parser_main.c2012-07-17 23:00:53 +
+++ parser/parser_main.c2012-08-07 22:00:54 +
@@ -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,8 @@
int skip_cache = 0;
int skip_read_cache = 0;
int write_cache = 0;
+int cond_clear_cache = 1; /* only applies if write is set */
+int force_clear_cache = 0; /* force clearing regargless of state */
int preprocess_only = 0;
int skip_mode_force = 0;
struct timespec mru_tstamp;
@@ -109,6 +112,8 @@
{skip-read-cache, 0, 0, 'T'},
{write-cache, 0, 0, 'W'},
{show-cache, 0, 0, 'k'},
+ {skip-bad-cache, 0, 0, 129}, /* no short option */
+ {purge-cache, 0, 0, 130}, /* no short option */
{cache-loc, 1, 0, 'L'},
{debug, 0, 0, 'd'},
{dump,1, 0, 'D'},
@@ -151,6 +156,8 @@
-K, --skip-cacheDo 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
+ --skip-bad-cacheDon't clear cache if out of sync\n
+ --purge-cache Clear cache regardless of its state\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 +534,12 @@
case 'T':
skip_read_cache = 1;
break;
+ case 129:
+ cond_clear_cache = 0;
+ break;
+ case 130:
+ force_clear_cache = 1;
+ break;
case 'L':
cacheloc = strdup(optarg);
break;
@@ -1165,11 +1178,128 @@
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;
+
Re: [apparmor] Cache update broken
Excellent, it feels much improved despite breaking Christian's Rules. :)
Could you add another monkey test for --purge-cache?
The short option -W leaked into the manpage update.
Thanks John.
-Original Message-
From: John Johansen john.johan...@canonical.com
Sender: apparmor-boun...@lists.ubuntu.com
Date: Tue, 07 Aug 2012 15:14:21
To: Christian Boltzappar...@cboltz.de
Cc: apparmor@lists.ubuntu.com
Subject: Re: [apparmor] Cache update broken
On 08/04/2012 06:53 AM, Christian Boltz wrote:
Hello,
okay attached is v2 of the patch, its changes are:
* makes cache clearing the default behavior if --write-cache is enabled
* changes --no-clear-cache to --skip-bad-cache (really the same thing but
hopefully a better name?)
* adds a --purge-cache debug option to force the cache to be cleared
regardless of state
* adds documentation to the apparmor_parser man page
---
apparmor: add clearing the profile cache when inconsistent
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.johan...@canonical.com
---
=== modified file 'parser/apparmor_parser.pod'
--- parser/apparmor_parser.pod 2012-02-24 12:21:59 +
+++ parser/apparmor_parser.pod 2012-08-07 22:06:34 +
@@ -138,6 +138,15 @@
is running with --replace, it may make sense to also use
--skip-read-cache with the --write-cache option.
+=item -W, --purge-cache
+
+Unconditionally clear out cached profiles.
+
+=item -W, --skip-bad-cache
+
+Skip updating the cache if it contains cached profiles in a bad or
+inconsistant state
+
=item -L, --cache-loc
Set the location of the cache directory. If not specified the cache location
=== modified file 'parser/parser_main.c'
--- parser/parser_main.c2012-07-17 23:00:53 +
+++ parser/parser_main.c2012-08-07 22:00:54 +
@@ -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,8 @@
int skip_cache = 0;
int skip_read_cache = 0;
int write_cache = 0;
+int cond_clear_cache = 1; /* only applies if write is set */
+int force_clear_cache = 0; /* force clearing regargless of state */
int preprocess_only = 0;
int skip_mode_force = 0;
struct timespec mru_tstamp;
@@ -109,6 +112,8 @@
{skip-read-cache, 0, 0, 'T'},
{write-cache, 0, 0, 'W'},
{show-cache, 0, 0, 'k'},
+ {skip-bad-cache, 0, 0, 129}, /* no short option */
+ {purge-cache, 0, 0, 130}, /* no short option */
{cache-loc, 1, 0, 'L'},
{debug, 0, 0, 'd'},
{dump,1, 0, 'D'},
@@ -151,6 +156,8 @@
-K, --skip-cacheDo 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
+ --skip-bad-cacheDon't clear cache if out of sync\n
+ --purge-cache Clear cache regardless of its state\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 +534,12 @@
case 'T':
skip_read_cache = 1;
break;
+ case 129:
+ cond_clear_cache = 0;
+ break;
+ case 130:
+ force_clear_cache = 1;
+ break;
case 'L':
cacheloc = strdup(optarg);
break;
@@ -1165,11 +1178,128 @@
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, .)
Re: [apparmor] Cache update broken
On 08/07/2012 03:26 PM, Seth Arnold wrote: Excellent, it feels much improved despite breaking Christian's Rules. :) heh, well hopefully this version fixes that Could you add another monkey test for --purge-cache? sure The short option -W leaked into the manpage update. fixed, thanks -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] Cache update broken
Hello, (I've seen the updated patch and already submitted it to a test package in the OBS in home:cboltz - but I'll comment on this mail nevertheless. Most of the comments below should have a SCNR marker - don't take them too serious ;-) Am Dienstag, 7. August 2012 schrieb John Johansen: On 08/07/2012 01:34 PM, Christian Boltz wrote: John, thanks for honoring the golden rules of bad programming in your patch! I'm especially talking about rule 18 - take great care in setting bad defaults ;-) Hehe I did it on purpose to get a discussion of what it should be on list :) Looks like it worked ;-) Am Dienstag, 7. August 2012 schrieb Seth Arnold: I expect --clear-cache-if-needed to be the default set in the config file Let me ask a simple question: Can you give me a good reason _not_ to automatically clear the cache if .features differs, and to keep an outdated cache? [1] well I can think of a corner case where you are booting between different kernels, and want to keep one. But I don't really think that is a good reason, That's indeed a corner case - but OTOH people doing this will waste lots of time with booting - updating the cache every time they choose another kernel is only a small part of this and won't hurt much. and I think the proper solution for that is having separate caches for each kernel. ... and run a cleanup script whenever an old kernel is removed? That's of course possible, but I'd say it's not worth the effort. (Hey, it's just a cache, and worst thing that can happen is that we waste some seconds.) -- redundant for ubuntu but also a chance to bring both initscripts together again -- at least for this feature. I don't know the history and background why there are separate initscripts (pointers welcome), but I'm a big fan of avoiding duplicate work (especially if I have to maintain the duplicate ;-)) Well the history is fairly simple, Ubuntu added caching support when it was in a very early iteration in the parser. [...] Sounds like someone didn't like if blocks ;-) Hmmm... Wasn't there something? Wait... Rule 14: Don't use small if blocks. Use cp instead. *SCNR* [1] oh, now I remember: rule 22 - invent new ways to make your program slow ;-) Having broken cache management is a new way to make your program slow? :) Yes - it makes booting slow because it keeps the useless cache. I'm not sure if someone used this method before to slow down booting ;-) so it might really be really new. (I'm talking about the load the AppArmor profiles part of booting here, of course.) [2] aa-enable is more important IMHO because it needs to a) delete a symlink b) load the profile sure that make sense, but I'll way it against I am in the patch right now and can add it with just a few lines of code while its fresh in my mind. Of course. My footnote was more about at which position of your wishlist would you place these features? That's all ;-) If we get a lower-priority feature nearly for free, I won't object ;-) Regards, Christian Boltz -- [1] Schmerzen wg. einer Zerrung -- Nicht alles, was hinkt, ist ein Vergleich. In manchen Fällen ist es auch ein David Haller... *SCNR* [ David Haller und Mario van der Linde in suse-linux] -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
