[apparmor] [PATCH 25/31] parser: Create initial interface for policy cache
Tyler Hicks
tyhicks at canonical.com
Fri Jan 23 03:08:24 UTC 2015
On 2015-01-22 10:15:48, John Johansen wrote:
> 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
I'm not sure that I'm following what you're saying. This is the same
behavior that we have in trunk today. If you specify
--force-clear-cache, the only thing that happens is the cache files are
cleared and the parser exits.
From main() of parser_main.c:
if (force_clear_cache)
exit(clear_cache_files(cacheloc));
> >
> > - 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
I don't think this is correct.
The cache is only cleared, before consulting cond_clear_cache, if
aa_features_new() fails when called by init_cache_features(). It only
fails if there's not a valid features file due to the dir not existing,
the .features file not existing, etc.
If there is a valid .features file, aa_policy_cache_new() will return
success. Then there's the check to aa_policy_cache_is_valid(). If that
fails and write_cache and cond_clear_cache are true, then we clear the
cache.
>
> 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(...
I've made this change in my local tree. Additionally, I carried the
change into the "libapparmor: Move the aa_policy_cache API" patch in the
second set of patches.
>
> > + }
> > +
> > + 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.
This is exactly why I named the function aa_policy_is_valid() instead of
something like aa_policy_cache_features_equals_kernel_features(). We can
change the check to whatever we want in the future. We can change the
implementation from doing a dumb equality check and do the intelligent
checks that you mentioned. Callers of this function won't know the
difference.
I see the systemd support and intelligent cache handling as two
different chunks of work. Hopefully we can come up with an API that
allows for systemd support now and intelligent cache handling in the
future. Otherwise, we need to make these intelligent caching changes
first and then turn it all into a library.
Tyler
> > +}
> > +
> > +/**
> > + * 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 */
> >
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150122/b626d2be/attachment.pgp>
More information about the AppArmor
mailing list