On 01/22/2015 07:08 PM, Tyler Hicks wrote:
> 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 <[email protected]>
>>
Acked-by: John Johansen <[email protected]>
>> 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.
>
gah, yep sorry I flipped a condition around aa_features_new
> 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
>
okay, like I said just my griping about equality and wanting to make sure
that is not considered what is valid.
>>> +}
>>> +
>>> +/**
>>> + * 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 */
>>>
>>
>>
--
AppArmor mailing list
[email protected]
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
