[apparmor] [PATCH 25/31] parser: Create initial interface for policy cache
John Johansen
john.johansen at canonical.com
Thu Jan 22 18:15:48 UTC 2015
On 12/05/2014 04:22 PM, Tyler Hicks wrote:
> This API has the same look-and-feel of the previous aa_match and
> aa_features APIs.
>
> The cache setup code was heavily dependent on globals set by CLI
> options. Options such as "skip the read cache", or "skip the write
> cache", or "don't clear the cache if it isn't valid", won't be useful
> for all aa_policy_cache API users so some of that logic was lifted out
> of the API. The constructor function still provides a bool parameter
> that specifies if the cache should be created or not.
>
> If the policy cache is invalid (currently meaning that the cache
> features file doesn't match the kernel features file), then a new
> aa_policy_cache object is still created but a call to
> aa_policy_cache_is_valid() will return false. The caller can then decide
> what to do (create a new valid cache, stop, etc.)
>
> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
comments inline
> ---
> parser/parser_main.c | 56 +++++++++++------
> parser/policy_cache.c | 171 +++++++++++++++++++++++++++++++++++++++-----------
> parser/policy_cache.h | 12 +++-
> 3 files changed, 182 insertions(+), 57 deletions(-)
>
> diff --git a/parser/parser_main.c b/parser/parser_main.c
> index 5b0e215..2de8edd 100644
> --- a/parser/parser_main.c
> +++ b/parser/parser_main.c
> @@ -860,6 +860,7 @@ static void setup_flags(void)
>
> int main(int argc, char *argv[])
> {
> + aa_policy_cache *policy_cache;
> int retval, last_error;
> int i;
> int optind;
> @@ -892,29 +893,48 @@ int main(int argc, char *argv[])
>
> setup_flags();
>
> - if (!cacheloc && asprintf(&cacheloc, "%s/cache", basedir) == -1) {
> - PERROR(_("Memory allocation error."));
> - return 1;
> - }
> -
> - if (force_clear_cache) {
> - if (clear_cache_files(cacheloc)) {
> - PERROR(_("Failed to clear cache files (%s): %s\n"),
> - cacheloc, strerror(errno));
> + if ((!skip_cache && (write_cache || !skip_read_cache)) ||
> + force_clear_cache) {
> + if (!cacheloc && asprintf(&cacheloc, "%s/cache", basedir) == -1) {
> + PERROR(_("Memory allocation error."));
> return 1;
> }
>
> - return 0;
> - }
> + if (force_clear_cache) {
> + if (clear_cache_files(cacheloc)) {
> + PERROR(_("Failed to clear cache files (%s): %s\n"),
> + cacheloc, strerror(errno));
> + return 1;
> + }
> +
> + return 0;
> + }
Not a criticism of the patch but a question of is this the behavior we want for --force_clear_cache?
That is if we succeed to abort with exit code 0 (ie. if force-clear-cache is specified we will never
process any profiles are anything
>
> - if (create_cache_dir)
> - pwarn(_("The --create-cache-dir option is deprecated. Please use --write-cache.\n"));
> + if (create_cache_dir)
> + pwarn(_("The --create-cache-dir option is deprecated. Please use --write-cache.\n"));
>
> - retval = setup_cache(features, cacheloc);
> - if (retval) {
> - PERROR(_("Failed setting up policy cache (%s): %s\n"),
> - cacheloc, strerror(errno));
> - return 1;
> + retval = aa_policy_cache_new(&policy_cache, features, cacheloc,
> + write_cache);
> + if (retval) {
> + if (errno != ENOENT) {
> + PERROR(_("Failed setting up policy cache (%s): %s\n"),
> + cacheloc, strerror(errno));
> + 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)) {
> + skip_read_cache = 1;
> + } else if (!write_cache || !cond_clear_cache) {
> + if (show_cache)
> + PERROR("Cache read/write disabled: Policy cache is invalid\n");
> + write_cache = 0;
> + skip_read_cache = 1;
> + }
> + }
The way this is setup we change cache behavior
If write_cache is specified the cache is always cleared at aa_policy_cache_new
and the following cond_clear_cache test is useless
now whether we want to always clear the cache on write cache is the features
differ is another question
> }
>
> retval = last_error = 0;
> diff --git a/parser/policy_cache.c b/parser/policy_cache.c
> index 6c8d5ca..f5061b1 100644
> --- a/parser/policy_cache.c
> +++ b/parser/policy_cache.c
> @@ -35,6 +35,14 @@
>
> #define le16_to_cpu(x) ((uint16_t)(le16toh (*(uint16_t *) x)))
>
> +struct aa_policy_cache {
> + unsigned int ref_count;
> + aa_features *features;
> + aa_features *kernel_features;
> + char *path;
> + char *features_path;
> +};
> +
> const char header_string[] = "\004\010\000version\000\002";
> #define HEADER_STRING_SIZE 12
> bool valid_cached_file_version(const char *cachename)
> @@ -107,37 +115,44 @@ int clear_cache_files(const char *path)
> return dirat_for_each(NULL, path, NULL, clear_cache_cb);
> }
>
> -int create_cache(const char *cachedir, const char *path, aa_features *features)
> +static int create_cache(aa_policy_cache *policy_cache, aa_features *features)
> {
> struct stat stat_file;
> autofclose FILE * f = NULL;
>
> - if (clear_cache_files(cachedir) != 0)
> + if (clear_cache_files(policy_cache->path) != 0)
> goto error;
>
> create_file:
> - if (aa_features_write_to_file(features, path) == -1)
> + if (aa_features_write_to_file(features,
> + policy_cache->features_path) == -1)
> goto error;
>
> + aa_features_unref(policy_cache->features);
> + policy_cache->features = aa_features_ref(features);
> return 0;
>
> error:
> /* does the dir exist? */
> - if (stat(cachedir, &stat_file) == -1) {
> - if (mkdir(cachedir, 0700) == 0)
> + if (stat(policy_cache->path, &stat_file) == -1) {
> + if (mkdir(policy_cache->path, 0700) == 0)
> goto create_file;
> if (show_cache)
> - PERROR(_("Can't create cache directory: %s\n"), cachedir);
> + PERROR(_("Can't create cache directory: %s\n"),
> + policy_cache->path);
> } else if (!S_ISDIR(stat_file.st_mode)) {
> if (show_cache)
> - PERROR(_("File in cache directory location: %s\n"), cachedir);
> + PERROR(_("File in cache directory location: %s\n"),
> + policy_cache->path);
> } else {
> if (show_cache)
> - PERROR(_("Can't update cache directory: %s\n"), cachedir);
> + PERROR(_("Can't update cache directory: %s\n"),
> + policy_cache->path);
> }
>
> if (show_cache)
> - PERROR("Cache write disabled: cannot create %s\n", path);
> + PERROR("Cache write disabled: cannot create %s\n",
> + policy_cache->features_path);
> write_cache = 0;
>
> return -1;
> @@ -224,45 +239,127 @@ void install_cache(const char *cachetmpname, const char *cachename)
> }
> }
>
> -int setup_cache(aa_features *kernel_features, const char *cacheloc)
> +static int init_cache_features(aa_policy_cache *policy_cache,
> + aa_features *kernel_features, bool create)
> {
> - autofree char *cache_features_path = NULL;
> - aa_features *cache_features;
> + if (aa_features_new(&policy_cache->features,
> + policy_cache->features_path)) {
> + policy_cache->features = NULL;
> + if (!create || errno != ENOENT)
> + return -1;
> +
> + if (create_cache(policy_cache, kernel_features))
> + return -1;
this could be just
return create_cache(...
> + }
> +
> + return 0;
> +}
>
> - if (!cacheloc) {
> +/**
> + * aa_policy_cache_new - create a new policy_cache from a path
> + * @policy_cache: will point to the address of an allocated and initialized
> + * 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
> + *
> + * 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)
> +{
> + aa_policy_cache *pc;
> +
> + *policy_cache = NULL;
> +
> + if (!path) {
> errno = EINVAL;
> return -1;
> }
>
> - /*
> - * Deal with cache directory versioning:
> - * - If cache/.features is missing, create it if --write-cache.
> - * - If cache/.features exists, and does not match features_string,
> - * force cache reading/writing off.
> - */
> - if (asprintf(&cache_features_path, "%s/.features", cacheloc) == -1) {
> - PERROR(_("Memory allocation error."));
> + pc = (aa_policy_cache *) calloc(1, sizeof(*pc));
> + if (!pc) {
> errno = ENOMEM;
> return -1;
> }
> + aa_policy_cache_ref(pc);
>
> - if (!aa_features_new(&cache_features, cache_features_path)) {
> - if (!aa_features_is_equal(kernel_features, cache_features)) {
> - if (write_cache && cond_clear_cache) {
> - if (create_cache(cacheloc, cache_features_path,
> - kernel_features))
> - skip_read_cache = 1;
> - } else {
> - if (show_cache)
> - PERROR("Cache read/write disabled: Police cache is invalid\n");
> - write_cache = 0;
> - skip_read_cache = 1;
> - }
> - }
> - aa_features_unref(cache_features);
> - } else if (write_cache) {
> - create_cache(cacheloc, cache_features_path, kernel_features);
> + pc->path = strdup(path);
> + if (!pc->path) {
> + aa_policy_cache_unref(pc);
> + errno = ENOMEM;
> + return -1;
> + }
> +
> + if (asprintf(&pc->features_path, "%s/.features", pc->path) == -1) {
> + pc->features_path = NULL;
> + aa_policy_cache_unref(pc);
> + errno = ENOMEM;
> + return -1;
> + }
> +
> + if (init_cache_features(pc, kernel_features, create)) {
> + int save = errno;
> +
> + aa_policy_cache_unref(pc);
> + errno = save;
> + return -1;
> }
>
> + pc->kernel_features = aa_features_ref(kernel_features);
> + *policy_cache = pc;
> +
> return 0;
> }
> +
> +/**
> + * aa_policy_cache_ref - increments the ref count of a policy_cache
> + * @policy_cache: the policy_cache
> + *
> + * Returns: the policy_cache
> + */
> +aa_policy_cache *aa_policy_cache_ref(aa_policy_cache *policy_cache)
> +{
> + atomic_inc(&policy_cache->ref_count);
> + return policy_cache;
> +}
> +
> +/**
> + * aa_policy_cache_unref - decrements the ref count and frees the policy_cache when 0
> + * @policy_cache: the policy_cache (can be NULL)
> + */
> +void aa_policy_cache_unref(aa_policy_cache *policy_cache)
> +{
> + if (policy_cache && atomic_dec_and_test(&policy_cache->ref_count)) {
> + free(policy_cache->features_path);
> + free(policy_cache->path);
> + free(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);
So this is just me griping about wanting to get away from the equality test
this is currently equiv to what we have but not really a true validity test
It is possible to have cache that has more features compiled into it than the
current kernel supports and the kernel will still enforce it correctly. It is
also possible the kernel won't and the cache must be cleared (it depends on the
feature).
The reverse situation where the cache supports fewer features than the current
kernel (kernel backport situation). Is interesting in that we currently handle
it poorly. The compiler, though it doesn't support the full feature set, will
store the kernel feature set in .features file but set a different abi in each
of the compiled cache files. The abi lets us detect the cache file is obsolete
but only on a per cache file basis, Not a global cache valid basis. So if we
are in an upgrade situation it is possible that if the userspace only used
aa_policy_cache_is_valid() it would start loading old files that should be
removed.
And we haven't even gotten into some of the other feature issues that would
let us eliminate duplication and compile times between different kernels,
but then storage is cheap now so maybe those aren't worth solving.
> +}
> +
> +/**
> + * 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);
> +}
> diff --git a/parser/policy_cache.h b/parser/policy_cache.h
> index 728de57..7192939 100644
> --- a/parser/policy_cache.h
> +++ b/parser/policy_cache.h
> @@ -42,12 +42,20 @@ void set_mru_tstamp(struct timespec t);
> void update_mru_tstamp(FILE *file, const char *path);
> bool valid_cached_file_version(const char *cachename);
> int clear_cache_files(const char *path);
> -int create_cache(const char *cachedir, const char *path, const char *features);
> char *cache_filename(const char *cachedir, const char *basename);
> void valid_read_cache(const char *cachename);
> int cache_hit(const char *cachename);
> int setup_cache_tmp(const char **cachetmpname, const char *cachename);
> void install_cache(const char *cachetmpname, const char *cachename);
> -int setup_cache(aa_features *kernel_features, const char *cacheloc);
> +
> +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);
> +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);
>
> #endif /* __AA_POLICY_CACHE_H */
>
More information about the AppArmor
mailing list