[apparmor] [PATCH v2 01/14] libapparmor: Simplify aa_policy_cache API
John Johansen
john.johansen at canonical.com
Fri Jun 5 09:14:54 UTC 2015
On 05/29/2015 04:04 PM, Tyler Hicks wrote:
> On 2015-05-29 04:31:10, John Johansen wrote:
>> 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() ?
>
> It removes two calls that libapparmor consumers would need to make and
> replaces them with one argument. Seems easier to me but maybe that's not
> the case.
>
>>
>> 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
>
> Thanks - I'll remove that.
>
>>
>>> - 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.
>
> I think --skip-bad-cache/cond_clear_cache semantics are still preserved.
> (Note that the policy cache tests still pass)
>
Not quite
> By default, write_cache is 0 and cond_clear_cache is 1. That means that
> max_caches will be 0 and no new cache files will be created.
>
> If the --write-cache option is specified, write_cache is set to 1 and,
> by default, cond_clear_cache is 1 so max_caches will be 1. That means
> that invalid cache files will be cleared and new cache files will be
> written.
>
> If the --skip-bad-cache option is specified, cond_clear_cache is 0 and,
> by default, write_cache is 0 so max_caches will be 0. That means that
> only existing cache files will be preserved and no new cache files will
> be written.
>
> If the --write-cache and --skip-bad-cache options are specified,
> write_cache is set to 1 and cond_clear_cache is set to 0 so max_caches
> will be 0. At first glance, it feels like this is where the semantics
> break down. However, the writing of new cache files isn't yet handled by
> libapparmor. It is still controlled by the write_cache global in the
> parser and is performed by the process_profile() function in
> parser_main.c.
>
> Is there a scenario that I'm missing? I'll be happy to add tests to
> parser/tst/caching.py for that scenario and then make the necessary
> adjustments.
>
This is where it deviates. Under the old system, if the cache doesn't
exist it will be setup (dir and features file) and written, under the
new semantic a missing cache won't get created as max_caches == 0, which
sets create to 0.
More information about the AppArmor
mailing list