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

[John Johansen]

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)
- We need to be able to turn creation on and off
- 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.

  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