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

[Tyler Hicks]

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?

Tyler
-------------- 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/20150615/80a8105b/attachment-0001.pgp>