On 06/15/2015 10:51 AM, Tyler Hicks wrote: > On 2015-06-13 11:59:27, John Johansen wrote: >> On 06/13/2015 10:43 AM, Tyler Hicks wrote: >>> When the cache directory does not exist, but --write-cache and >>> --skip-bad-cache are specified, the cache directory and the features >>> file should be created but writes to the cache directory should not >>> happen. This patch restores that behavior. >>> >>> The errno values libapparmor's aa_policy_cache_new() uses to indicate >>> when the cache directory does not exist and when an existing, invalid >>> cache already exists needed to be separated out. They were both ENOENT >>> but now the latter situation uses EEXIST. >>> >>> libapparmor also needed to be updated to not print an error message to >>> the syslog from aa_policy_cache_new() when the max_caches parameter is >>> 0, indicating that a new cache should not be created, and the cache >>> directory does not exist. This is an error situation but a debug message >>> is more appropriate. >>> >>> Signed-off-by: Tyler Hicks <[email protected]> >>> --- >>> >>> This patch is intended to be applied as a fixup patch at the end of the >>> greater >>> patch series. >>> >>> The approach taken is to modify the parser to recall into libapparmor, under >>> certain conditions, to continue supporting the parser's --skip-bad-cache and >>> --write-cache behavior when the cache directory does not exist. >>> >>> libraries/libapparmor/src/policy_cache.c | 6 ++++-- >>> parser/parser_main.c | 19 ++++++++++++++++++- >>> 2 files changed, 22 insertions(+), 3 deletions(-) >>> >>> diff --git a/libraries/libapparmor/src/policy_cache.c >>> b/libraries/libapparmor/src/policy_cache.c >>> index f685f0a..72c0176 100644 >>> --- a/libraries/libapparmor/src/policy_cache.c >>> +++ b/libraries/libapparmor/src/policy_cache.c >>> @@ -78,7 +78,7 @@ static int init_cache_features(aa_policy_cache >>> *policy_cache, >>> } else if (!aa_features_is_equal(policy_cache->features, >>> kernel_features)) { >>> if (!create) { >>> - errno = ENOENT; >>> + errno = EEXIST; >>> return -1; >>> } >>> >>> @@ -166,8 +166,10 @@ open: >>> if (mkdirat(dirfd, path, 0700) == 0) >>> goto open; >>> PERROR("Can't create cache directory '%s': %m\n", path); >>> - } else { >>> + } else if (create) { >>> PERROR("Can't update cache directory '%s': %m\n", path); >>> + } else { >>> + PDEBUG("Cache directory '%s' does not exist\n", path); >>> } >>> >> Acked by John Johansen <[email protected]> on the error code >> portion of the patch >> >>> save = errno; >>> diff --git a/parser/parser_main.c b/parser/parser_main.c >>> index f274d00..3bb115d 100644 >>> --- a/parser/parser_main.c >>> +++ b/parser/parser_main.c >>> @@ -918,13 +918,30 @@ int main(int argc, char *argv[]) >>> if (create_cache_dir) >>> pwarn(_("The --create-cache-dir option is deprecated. >>> Please use --write-cache.\n")); >>> >>> +retry: >>> retval = aa_policy_cache_new(&policy_cache, features, >>> AT_FDCWD, cacheloc, max_caches); >>> if (retval) { >>> - if (errno != ENOENT) { >>> + if (errno != ENOENT && errno != EEXIST) { >>> PERROR(_("Failed setting up policy cache (%s): >>> %s\n"), >>> cacheloc, strerror(errno)); >>> return 1; >>> + } else if (errno == ENOENT && max_caches == 0 && >>> + write_cache && !cond_clear_cache) { >>> + /** >>> + * --write-cache and --skip-bad-cache are >>> + * present and errno is ENOENT, meaning that >>> + * the cache dir does not exist. Legacy >>> + * behavior is to create the cache dir in this >>> + * situation so retry with a non-zero >>> + * max_caches. 2.9 went ahead and wrote to the >>> + * newly created cache dir. It has been decided, >>> + * as of r2927, that 2.10 will not write to the >>> + * new cache dir. >>> + */ >>> + write_cache = 0; >>> + max_caches = 1; >>> + goto retry; >>> } >>> >>> if (show_cache) { >>> >> So my take away from the IRC discussion was the r2927 behavior was not >> correct either >> >> I think we need to relook at this and decide exactly what we want going >> forward. Lets get the API right not worry so much about legacy >> >> - Going forward we are going to have multiple caches (just not yet) >> - Going forward it is possible a cache could be shared between multiple >> kernels (again not yet) > > max_caches supports the two above scenarios in a future-proof way > >> - We need to be able to turn creation on and off > > I think max_caches supports this. max_caches > 0 will allow caches to be > created (and old ones auto-reaped, if needed). max_cache == 0 turns on > read-only caching so that new caches will not be created but an old, > valid cache to be used if available. > >> - The skip behavior was added to keep caches intact if they didn't match. >> Which isn't so important once we have multip caches. After looking >> more at what skip-bad-caches is forcing the code to do, I am now >> inclined to just drop it and have the parser issue a warning that >> skip-bad-caches is no longer supported. > > I'm fine with this. > >> Sorry I know I raised the issue around the cache changes, but you >> have convinced me its not worth keeping. If cache skipping is >> really needed we can just use skip-cache > > No worries. I agree with you that we need to get the API right. > > If you agree that max_caches does the right thing, then I think the > original patch in this thread is sufficient. I can send out a follow up > patch that deprecates --skip-bad-cache. Does that sound like the correct > plan of action to you? > yes
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
