[apparmor] [PATCH] parser: Restore --skip-bad-cache behavior when cache dir DNE

[John Johansen]

On 06/15/2015 10:51 AM, Tyler Hicks wrote:
> On 2015-06-13 11:59:27, John Johansen wrote:
>> On 06/13/2015 10:43 AM, Tyler Hicks wrote:
>>> When the cache directory does not exist, but --write-cache and
>>> --skip-bad-cache are specified, the cache directory and the features
>>> file should be created but writes to the cache directory should not
>>> happen. This patch restores that behavior.
>>>
>>> The errno values libapparmor's aa_policy_cache_new() uses to indicate
>>> when the cache directory does not exist and when an existing, invalid
>>> cache already exists needed to be separated out. They were both ENOENT
>>> but now the latter situation uses EEXIST.
>>>
>>> libapparmor also needed to be updated to not print an error message to
>>> the syslog from aa_policy_cache_new() when the max_caches parameter is
>>> 0, indicating that a new cache should not be created, and the cache
>>> directory does not exist. This is an error situation but a debug message
>>> is more appropriate.
>>>
>>> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
>>> ---
>>>
>>> This patch is intended to be applied as a fixup patch at the end of the greater
>>> patch series.
>>>
>>> The approach taken is to modify the parser to recall into libapparmor, under
>>> certain conditions, to continue supporting the parser's --skip-bad-cache and
>>> --write-cache behavior when the cache directory does not exist.
>>>
>>>  libraries/libapparmor/src/policy_cache.c |  6 ++++--
>>>  parser/parser_main.c                     | 19 ++++++++++++++++++-
>>>  2 files changed, 22 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/libraries/libapparmor/src/policy_cache.c b/libraries/libapparmor/src/policy_cache.c
>>> index f685f0a..72c0176 100644
>>> --- a/libraries/libapparmor/src/policy_cache.c
>>> +++ b/libraries/libapparmor/src/policy_cache.c
>>> @@ -78,7 +78,7 @@ static int init_cache_features(aa_policy_cache *policy_cache,
>>>  	} else if (!aa_features_is_equal(policy_cache->features,
>>>  					 kernel_features)) {
>>>  		if (!create) {
>>> -			errno = ENOENT;
>>> +			errno = EEXIST;
>>>  			return -1;
>>>  		}
>>>  
>>> @@ -166,8 +166,10 @@ open:
>>>  			if (mkdirat(dirfd, path, 0700) == 0)
>>>  				goto open;
>>>  			PERROR("Can't create cache directory '%s': %m\n", path);
>>> -		} else {
>>> +		} else if (create) {
>>>  			PERROR("Can't update cache directory '%s': %m\n", path);
>>> +		} else {
>>> +			PDEBUG("Cache directory '%s' does not exist\n", path);
>>>  		}
>>>  
>> Acked by John Johansen <john.johansen at canonical.com> on the error code
>> portion of the patch
>>
>>>  		save = errno;
>>> diff --git a/parser/parser_main.c b/parser/parser_main.c
>>> index f274d00..3bb115d 100644
>>> --- a/parser/parser_main.c
>>> +++ b/parser/parser_main.c
>>> @@ -918,13 +918,30 @@ int main(int argc, char *argv[])
>>>  		if (create_cache_dir)
>>>  			pwarn(_("The --create-cache-dir option is deprecated. Please use --write-cache.\n"));
>>>  
>>> +retry:
>>>  		retval = aa_policy_cache_new(&policy_cache, features,
>>>  					     AT_FDCWD, cacheloc, max_caches);
>>>  		if (retval) {
>>> -			if (errno != ENOENT) {
>>> +			if (errno != ENOENT && errno != EEXIST) {
>>>  				PERROR(_("Failed setting up policy cache (%s): %s\n"),
>>>  				       cacheloc, strerror(errno));
>>>  				return 1;
>>> +			} else if (errno == ENOENT && max_caches == 0 &&
>>> +				   write_cache && !cond_clear_cache) {
>>> +				/**
>>> +				 * --write-cache and --skip-bad-cache are
>>> +				 * present and errno is ENOENT, meaning that
>>> +				 * the cache dir does not exist. Legacy
>>> +				 * behavior is to create the cache dir in this
>>> +				 * situation so retry with a non-zero
>>> +				 * max_caches. 2.9 went ahead and wrote to the
>>> +				 * newly created cache dir. It has been decided,
>>> +				 * as of r2927, that 2.10 will not write to the
>>> +				 * new cache dir.
>>> +				 */
>>> +				write_cache = 0;
>>> +				max_caches = 1;
>>> +				goto retry;
>>>  			}
>>>  
>>>  			if (show_cache) {
>>>
>> So my take away from the IRC discussion was the r2927 behavior was not
>> correct either
>>
>> I think we need to relook at this and decide exactly what we want going
>> forward. Lets get the API right not worry so much about legacy
>>
>> - Going forward we are going to have multiple caches (just not yet)
>> - Going forward it is possible a cache could be shared between multiple
>>   kernels (again not yet)
> 
> max_caches supports the two above scenarios in a future-proof way
> 
>> - We need to be able to turn creation on and off
> 
> I think max_caches supports this. max_caches > 0 will allow caches to be
> created (and old ones auto-reaped, if needed). max_cache == 0 turns on
> read-only caching so that new caches will not be created but an old,
> valid cache to be used if available.
> 
>> - The skip behavior was added to keep caches intact if they didn't match.
>>   Which isn't so important once we have multip caches. After looking
>>   more at what skip-bad-caches is forcing the code to do, I am now
>>   inclined to just drop it and have the parser issue a warning that
>>   skip-bad-caches is no longer supported.
> 
> I'm fine with this.
> 
>>   Sorry I know I raised the issue around the cache changes, but you
>>   have convinced me its not worth keeping.  If cache skipping is
>>   really needed we can just use skip-cache
> 
> No worries. I agree with you that we need to get the API right.
> 
> If you agree that max_caches does the right thing, then I think the
> original patch in this thread is sufficient. I can send out a follow up
> patch that deprecates --skip-bad-cache. Does that sound like the correct
> plan of action to you?
> 
yes