[apparmor] [PATCH v2 01/14] libapparmor: Simplify aa_policy_cache API
John Johansen
john.johansen at canonical.com
Fri May 29 11:31:10 UTC 2015
On 04/02/2015 08:17 AM, Tyler Hicks wrote:
> This patch changes the aa_policy_cache_new() prototype and gets rid of
> aa_policy_cache_is_valid() and aa_policy_cache_create().
>
> The create bool of aa_policy_cache_new() is replaced with a 16 bit
> unsigned int used to specify the maximum number of caches that should be
> present in the specified cache directory. If the number is exceeded, the
> old cache directories are reaped. The definition of "old" is private to
> libapparmor and only 1 cache directory is currently supported. However,
> that will change in the near future and multiple cache directories will
> be supported.
>
> If 0 is specified for the max_caches parameter, no new caches can be
> created and only an existing, valid cache can be used. An error is
> returned if no valid caches exist in that case.
>
> If UINT16_MAX is specified, an unlimited amount of caches can be created
> and reaping is disabled.
>
> This means that 0 to (2^16)-2, or infinite, caches will be supported in
> the future.
>
> This change allows for the parser to continue to support the
> --skip-bad-cache (by passing 0 for max_caches) and the --write-cache
> option (by passing 1 or more for max_caches) without confusing
> libapparmor users with the aa_policy_cache_{is_valid,create}()
> functions.
this is less confusing than is_valid,create() ?
the patch changes the semantics around skip-bad-cache/cond_clear_cache
>
> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
> ---
> libraries/libapparmor/include/sys/apparmor.h | 4 +-
> libraries/libapparmor/src/libapparmor.map | 2 +-
> libraries/libapparmor/src/policy_cache.c | 56 ++++++------
> parser/parser_main.c | 22 ++---
> tests/regression/apparmor/aa_policy_cache.c | 130 +++++++++------------------
> tests/regression/apparmor/aa_policy_cache.sh | 22 ++---
> 6 files changed, 88 insertions(+), 148 deletions(-)
>
> diff --git a/libraries/libapparmor/include/sys/apparmor.h b/libraries/libapparmor/include/sys/apparmor.h
> index 99ce36b..743d967 100644
> --- a/libraries/libapparmor/include/sys/apparmor.h
> +++ b/libraries/libapparmor/include/sys/apparmor.h
> @@ -142,12 +142,10 @@ int aa_kernel_interface_write_policy(int fd, const char *buffer, size_t size);
> typedef struct aa_policy_cache aa_policy_cache;
> int aa_policy_cache_new(aa_policy_cache **policy_cache,
> aa_features *kernel_features, const char *path,
> - bool create);
> + uint16_t max_caches);
> aa_policy_cache *aa_policy_cache_ref(aa_policy_cache *policy_cache);
> void aa_policy_cache_unref(aa_policy_cache *policy_cache);
>
> -bool aa_policy_cache_is_valid(aa_policy_cache *policy_cache);
> -int aa_policy_cache_create(aa_policy_cache *policy_cache);
> int aa_policy_cache_remove(const char *path);
> int aa_policy_cache_replace_all(aa_policy_cache *policy_cache,
> aa_kernel_interface *kernel_interface);
> diff --git a/libraries/libapparmor/src/libapparmor.map b/libraries/libapparmor/src/libapparmor.map
> index 3f43494..2f440f0 100644
> --- a/libraries/libapparmor/src/libapparmor.map
> +++ b/libraries/libapparmor/src/libapparmor.map
> @@ -77,7 +77,7 @@ APPARMOR_2.10 {
> aa_policy_cache_ref;
> aa_policy_cache_unref;
> aa_policy_cache_is_valid;
aa_policy_cache_is_valid has been left in the map despite being removed
> - aa_policy_cache_create;
> + aa_policy_cache_make_valid;
> aa_policy_cache_remove;
> aa_policy_cache_replace_all;
> local:
> diff --git a/libraries/libapparmor/src/policy_cache.c b/libraries/libapparmor/src/policy_cache.c
> index a9e43bb..b4391b1 100644
> --- a/libraries/libapparmor/src/policy_cache.c
> +++ b/libraries/libapparmor/src/policy_cache.c
> @@ -85,16 +85,29 @@ error:
> static int init_cache_features(aa_policy_cache *policy_cache,
> aa_features *kernel_features, bool create)
+ bool cond_clear_cache
> {
> + bool call_create_cache = false;
> +
> if (aa_features_new(&policy_cache->features,
> policy_cache->features_path)) {
> policy_cache->features = NULL;
> if (!create || errno != ENOENT)
> return -1;
>
> - return create_cache(policy_cache, kernel_features);
> + /* The cache directory needs to be created */
> + call_create_cache = true;
> + } else if (!aa_features_is_equal(policy_cache->features,
> + kernel_features)) {
> + if (!create) {
if (!create || !cond_clear) {
> + errno = ENOENT;
> + return -1;
> + }
> +
> + /* The cache directory needs to be refreshed */
> + call_create_cache = true;
> }
>
> - return 0;
> + return call_create_cache ?
> + create_cache(policy_cache, kernel_features) : 0;
> }
>
> struct replace_all_cb_data {
> @@ -131,16 +144,21 @@ static int replace_all_cb(DIR *dir unused, const char *name, struct stat *st,
> * aa_policy_cache_new object upon success
> * @kernel_features: features representing the currently running kernel
> * @path: path to the policy cache
> - * @create: true if the cache should be created if it doesn't already exist
> + * @max_caches: The maximum number of policy caches, one for each unique set of
> + * kernel features, before older caches are auto-reaped. 0 means
> + * that no new caches should be created (existing, valid caches
> + * will be used) and auto-reaping is disabled. UINT16_MAX means
> + * that a cache can be created and auto-reaping is disabled.
> *
> * Returns: 0 on success, -1 on error with errno set and *@policy_cache
> * pointing to NULL
> */
> int aa_policy_cache_new(aa_policy_cache **policy_cache,
> aa_features *kernel_features, const char *path,
> - bool create)
> + uint16_t max_caches)
> {
> aa_policy_cache *pc;
> + bool create = max_caches > 0;
>
> *policy_cache = NULL;
>
> @@ -149,6 +167,11 @@ int aa_policy_cache_new(aa_policy_cache **policy_cache,
> return -1;
> }
>
> + if (max_caches > 1) {
> + errno = ENOTSUP;
> + return -1;
> + }
> +
> pc = calloc(1, sizeof(*pc));
> if (!pc) {
> errno = ENOMEM;
> @@ -212,31 +235,6 @@ void aa_policy_cache_unref(aa_policy_cache *policy_cache)
> }
>
> /**
> - * aa_policy_cache_is_valid - checks if the policy_cache is valid for the currently running kernel
> - * @policy_cache: the policy_cache
> - *
> - * Returns: true if the policy_cache is valid for the currently running kernel,
> - * false if not
> - */
> -bool aa_policy_cache_is_valid(aa_policy_cache *policy_cache)
> -{
> - return aa_features_is_equal(policy_cache->features,
> - policy_cache->kernel_features);
> -}
> -
> -/**
> - * aa_policy_cache_create - creates a valid policy_cache for the currently running kernel
> - * @policy_cache: the policy_cache
> - *
> - * Returns: 0 on success, -1 on error with errno set and features pointing to
> - * NULL
> - */
> -int aa_policy_cache_create(aa_policy_cache *policy_cache)
> -{
> - return create_cache(policy_cache, policy_cache->kernel_features);
> -}
> -
> -/**
> * aa_policy_cache_remove - removes all policy cache files under a path
> * @path: the path to a policy cache directory
> *
> diff --git a/parser/parser_main.c b/parser/parser_main.c
> index 8aee148..3ba2d1a 100644
> --- a/parser/parser_main.c
> +++ b/parser/parser_main.c
> @@ -898,6 +898,8 @@ int main(int argc, char *argv[])
>
> if ((!skip_cache && (write_cache || !skip_read_cache)) ||
> force_clear_cache) {
> + uint16_t max_caches = write_cache && cond_clear_cache ? 1 : 0;
> +
> if (!cacheloc && asprintf(&cacheloc, "%s/cache", basedir) == -1) {
> PERROR(_("Memory allocation error."));
> return 1;
> @@ -917,7 +919,7 @@ int main(int argc, char *argv[])
> pwarn(_("The --create-cache-dir option is deprecated. Please use --write-cache.\n"));
>
> retval = aa_policy_cache_new(&policy_cache, features, cacheloc,
> - write_cache);
> + max_caches);
This is not correct, previously aa_policy_cache_new was called with its create parameter set only
by write_cache. Now it depends on write_cache && cond_clear_cache
aa_policy_cache_new will need another parameter to get the semanitcs of cond_clear_cache right.
> if (retval) {
> if (errno != ENOENT) {
> PERROR(_("Failed setting up policy cache (%s): %s\n"),
> @@ -925,22 +927,16 @@ int main(int argc, char *argv[])
> return 1;
> }
>
> - write_cache = 0;
> - skip_read_cache = 0;
> - } else if (!aa_policy_cache_is_valid(policy_cache)) {
> - if (write_cache && cond_clear_cache &&
> - aa_policy_cache_create(policy_cache)) {
> - if (show_cache)
> + if (show_cache) {
> + if (max_caches > 0)
> PERROR("Cache write disabled: Cannot create cache '%s': %m\n",
> cacheloc);
> - write_cache = 0;
> - skip_read_cache = 1;
> - } else if (!write_cache || !cond_clear_cache) {
> - if (show_cache)
> + else
> PERROR("Cache read/write disabled: Policy cache is invalid\n");
> - write_cache = 0;
> - skip_read_cache = 1;
> }
> +
> + write_cache = 0;
> + skip_read_cache = 1;
> }
> }
>
> diff --git a/tests/regression/apparmor/aa_policy_cache.c b/tests/regression/apparmor/aa_policy_cache.c
> index b08fd1f..48b3a71 100644
> --- a/tests/regression/apparmor/aa_policy_cache.c
> +++ b/tests/regression/apparmor/aa_policy_cache.c
> @@ -16,36 +16,33 @@
>
> #include <errno.h>
> #include <fcntl.h>
> +#include <limits.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> #include <sys/apparmor.h>
>
> -#define OPT_CREATE "create"
> -#define OPT_IS_VALID "is-valid"
> #define OPT_NEW "new"
> -#define OPT_NEW_CREATE "new-create"
> #define OPT_REMOVE "remove"
> #define OPT_REMOVE_POLICY "remove-policy"
> #define OPT_REPLACE_ALL "replace-all"
> +#define OPT_FLAG_MAX_CACHES "--max-caches"
>
> static void usage(const char *prog)
> {
> fprintf(stderr,
> - "FAIL - usage: %s %s <PATH>\n"
> - " %s %s <PATH>\n"
> - " %s %s <PATH>\n"
> - " %s %s <PATH>\n"
> + "FAIL - usage: %s %s [%s N] <PATH>\n"
> " %s %s <PATH>\n"
> " %s %s <PROFILE_NAME>\n"
> - " %s %s <PATH>\n",
> - prog, OPT_CREATE, prog, OPT_IS_VALID, prog, OPT_NEW,
> - prog, OPT_NEW_CREATE, prog, OPT_REMOVE, prog, OPT_REMOVE_POLICY,
> - prog, OPT_REPLACE_ALL);
> + " %s %s [%s N] <PATH>\n",
> + prog, OPT_NEW, OPT_FLAG_MAX_CACHES,
> + prog, OPT_REMOVE,
> + prog, OPT_REMOVE_POLICY,
> + prog, OPT_REPLACE_ALL, OPT_FLAG_MAX_CACHES);
> }
>
> -static int test_create(const char *path)
> +static int test_new(const char *path, uint16_t max_caches)
> {
> aa_features *features = NULL;
> aa_policy_cache *policy_cache = NULL;
> @@ -56,64 +53,7 @@ static int test_create(const char *path)
> goto out;
> }
>
> - if (aa_policy_cache_new(&policy_cache, features, path, false)) {
> - perror("FAIL - aa_policy_cache_new");
> - goto out;
> - }
> -
> - if (aa_policy_cache_create(policy_cache)) {
> - perror("FAIL - aa_policy_cache_create");
> - goto out;
> - }
> -
> - rc = 0;
> -out:
> - aa_features_unref(features);
> - aa_policy_cache_unref(policy_cache);
> - return rc;
> -}
> -
> -static int test_is_valid(const char *path)
> -{
> - aa_features *features = NULL;
> - aa_policy_cache *policy_cache = NULL;
> - int rc = 1;
> -
> - if (aa_features_new_from_kernel(&features)) {
> - perror("FAIL - aa_features_new_from_kernel");
> - goto out;
> - }
> -
> - if (aa_policy_cache_new(&policy_cache, features, path, false)) {
> - perror("FAIL - aa_policy_cache_new");
> - goto out;
> - }
> -
> - if (!aa_policy_cache_is_valid(policy_cache)) {
> - errno = EINVAL;
> - perror("FAIL - aa_policy_cache_is_valid");
> - goto out;
> - }
> -
> - rc = 0;
> -out:
> - aa_features_unref(features);
> - aa_policy_cache_unref(policy_cache);
> - return rc;
> -}
> -
> -static int test_new(const char *path, bool create)
> -{
> - aa_features *features = NULL;
> - aa_policy_cache *policy_cache = NULL;
> - int rc = 1;
> -
> - if (aa_features_new_from_kernel(&features)) {
> - perror("FAIL - aa_features_new_from_kernel");
> - goto out;
> - }
> -
> - if (aa_policy_cache_new(&policy_cache, features, path, create)) {
> + if (aa_policy_cache_new(&policy_cache, features, path, max_caches)) {
> perror("FAIL - aa_policy_cache_new");
> goto out;
> }
> @@ -167,7 +107,7 @@ out:
> return rc;
> }
>
> -static int test_replace_all(const char *path)
> +static int test_replace_all(const char *path, uint16_t max_caches)
> {
> aa_features *features = NULL;
> aa_policy_cache *policy_cache = NULL;
> @@ -178,7 +118,7 @@ static int test_replace_all(const char *path)
> goto out;
> }
>
> - if (aa_policy_cache_new(&policy_cache, features, path, false)) {
> + if (aa_policy_cache_new(&policy_cache, features, path, max_caches)) {
> perror("FAIL - aa_policy_cache_new");
> goto out;
> }
> @@ -197,27 +137,45 @@ out:
>
> int main(int argc, char **argv)
> {
> + uint16_t max_caches = 0;
> + const char *str = NULL;
> int rc = 1;
>
> - if (argc != 3) {
> + if (!(argc == 3 || argc == 5)) {
> usage(argv[0]);
> exit(1);
> }
>
> - if (strcmp(argv[1], OPT_CREATE) == 0) {
> - rc = test_create(argv[2]);
> - } else if (strcmp(argv[1], OPT_IS_VALID) == 0) {
> - rc = test_is_valid(argv[2]);
> - } else if (strcmp(argv[1], OPT_NEW) == 0) {
> - rc = test_new(argv[2], false);
> - } else if (strcmp(argv[1], OPT_NEW_CREATE) == 0) {
> - rc = test_new(argv[2], true);
> - } else if (strcmp(argv[1], OPT_REMOVE) == 0) {
> - rc = test_remove(argv[2]);
> - } else if (strcmp(argv[1], OPT_REMOVE_POLICY) == 0) {
> - rc = test_remove_policy(argv[2]);
> + str = argv[argc - 1];
> +
> + if (argc == 5) {
> + unsigned long tmp;
> +
> + errno = 0;
> + tmp = strtoul(argv[3], NULL, 10);
> + if ((errno == ERANGE && tmp == ULONG_MAX) ||
> + (errno && tmp == 0)) {
> + perror("FAIL - strtoul");
> + exit(1);
> + }
> +
> + if (tmp > UINT16_MAX) {
> + fprintf(stderr, "FAIL - %lu larger than UINT16_MAX\n",
> + tmp);
> + exit(1);
> + }
> +
> + max_caches = tmp;
> + }
> +
> + if (strcmp(argv[1], OPT_NEW) == 0) {
> + rc = test_new(str, max_caches);
> + } else if (strcmp(argv[1], OPT_REMOVE) == 0 && argc == 3) {
> + rc = test_remove(str);
> + } else if (strcmp(argv[1], OPT_REMOVE_POLICY) == 0 && argc == 3) {
> + rc = test_remove_policy(str);
> } else if (strcmp(argv[1], OPT_REPLACE_ALL) == 0) {
> - rc = test_replace_all(argv[2]);
> + rc = test_replace_all(str, max_caches);
> } else {
> usage(argv[0]);
> }
> diff --git a/tests/regression/apparmor/aa_policy_cache.sh b/tests/regression/apparmor/aa_policy_cache.sh
> index fb9a830..dfb75a8 100755
> --- a/tests/regression/apparmor/aa_policy_cache.sh
> +++ b/tests/regression/apparmor/aa_policy_cache.sh
> @@ -36,7 +36,7 @@ remove_cachedir()
>
> create_empty_cache()
> {
> - $test new-create "$cachedir" > /dev/null
> + $test new --max-caches 1 "$cachedir" > /dev/null
> }
>
> create_cache_files()
> @@ -105,28 +105,18 @@ runchecktest "AA_POLICY_CACHE new (no cachedir)" fail new "$cachedir"
> create_cachedir
> runchecktest "AA_POLICY_CACHE new (no .features)" fail new "$cachedir"
> remove_cachedir
> -runchecktest "AA_POLICY_CACHE new-create (no cachedir)" pass new-create "$cachedir"
> -runchecktest "AA_POLICY_CACHE new-create (existing cache)" pass new-create "$cachedir"
> +runchecktest "AA_POLICY_CACHE new create (no cachedir)" pass new --max-caches 1 "$cachedir"
> +runchecktest "AA_POLICY_CACHE new create (existing cache)" pass new --max-caches 1 "$cachedir"
> runchecktest "AA_POLICY_CACHE new (existing cache)" pass new "$cachedir"
>
> -runchecktest "AA_POLICY_CACHE is-valid (good .features)" pass is-valid "$cachedir"
> install_bad_features_file
> -runchecktest "AA_POLICY_CACHE is-valid (bad .features)" fail is-valid "$cachedir"
> -remove_cachedir
> -runchecktest "AA_POLICY_CACHE is-valid (no cachedir)" fail is-valid "$cachedir"
> -
> -create_cachedir
> -install_bad_features_file
> -runchecktest "AA_POLICY_CACHE create (bad .features)" pass create "$cachedir"
> -runchecktest "AA_POLICY_CACHE create (good .features)" pass create "$cachedir"
> -remove_features_file
> -runchecktest "AA_POLICY_CACHE create (no .features)" fail create "$cachedir"
> -remove_cachedir
> -runchecktest "AA_POLICY_CACHE create (no cachedir)" fail create "$cachedir"
> +runchecktest "AA_POLICY_CACHE new (bad .features)" fail new "$cachedir"
> +runchecktest "AA_POLICY_CACHE new create (bad .features)" pass new --max-caches 1 "$cachedir"
>
> # Make sure that no test policies are already loaded
> verify_policies_are_not_loaded
>
> +remove_cachedir
> runchecktest "AA_POLICY_CACHE replace-all (no cachedir)" fail replace-all "$cachedir"
> create_cachedir
> runchecktest "AA_POLICY_CACHE replace-all (no .features)" fail replace-all "$cachedir"
>
More information about the AppArmor
mailing list