Re: [PATCH] generic/581: try to avoid flakiness in keys quota test

[Murphy Zhou <xzhou@redhat.com>]

On Tue, Jan 28, 2020 at 04:42:51PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> generic/581 passes for me, but Murphy Zhou reported that it started
> failing for him.  The part that failed is the part that sets the key
> quota to the fsgqa user's current number of keys plus 5, then tries to
> add 6 filesystem encryption keys as the fsgqa user.  Adding the 6th key
> unexpectedly succeeded.

Ack. This patch fixed generic/581 failure on my side.

Thanks Eric!

Murphy

> 
> What I think is happening is that because the kernel's keys subsystem
> garbage-collects keys asynchronously, the quota may be freed up later
> than expected after removing fscrypt keys.  Thus the test is flaky.
> 
> It would be nice to fix this in the kernel, but unfortunately there
> doesn't seem to be an easy fix, and the keys subsystem has always worked
> this way.  And it seems unlikely to cause real-world problems, as the
> keys quota really just exists to prevent denial-of-service attacks.
> 
> So, for now just try to make the test more reliable by:
> 
> (1) Reduce the scope of the modified keys quota to just the part of the
>     test that needs it.
> (2) Before getting the current number of keys for the purpose of setting
>     the quota, wait for any invalidated keys to be garbage-collected.
> 
> Tested with a kernel that has a 1 second sleep hacked into the beginning
> of key_garbage_collector().  With that, this test fails before this
> patch and passes afterwards.
> 
> Reported-by: Murphy Zhou <xzhou@redhat.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  tests/generic/581 | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/generic/581 b/tests/generic/581
> index 89aa03c2..bc49eadc 100755
> --- a/tests/generic/581
> +++ b/tests/generic/581
> @@ -45,14 +45,6 @@ _require_scratch_encryption -v 2
>  _scratch_mkfs_encrypted &>> $seqres.full
>  _scratch_mount
>  
> -# Set the fsgqa user's key quota to their current number of keys plus 5.
> -orig_keys=$(_user_do "awk '/^[[:space:]]*$(id -u fsgqa):/{print \$4}' /proc/key-users | cut -d/ -f1")
> -: ${orig_keys:=0}
> -echo "orig_keys=$orig_keys" >> $seqres.full
> -orig_maxkeys=$(</proc/sys/kernel/keys/maxkeys)
> -keys_to_add=5
> -echo $((orig_keys + keys_to_add)) > /proc/sys/kernel/keys/maxkeys
> -
>  dir=$SCRATCH_MNT/dir
>  
>  raw_key=""
> @@ -98,6 +90,24 @@ _user_do_rm_enckey $SCRATCH_MNT $keyid
>  
>  _scratch_cycle_mount	# Clear all keys
>  
> +# Wait for any invalidated keys to be garbage-collected.
> +i=0
> +while grep -E -q '^[0-9a-f]+ [^ ]*i[^ ]*' /proc/keys; do
> +	if ((++i >= 20)); then
> +		echo "Timed out waiting for invalidated keys to be GC'ed" >> $seqres.full
> +		break
> +	fi
> +	sleep 0.5
> +done
> +
> +# Set the user key quota to the fsgqa user's current number of keys plus 5.
> +orig_keys=$(_user_do "awk '/^[[:space:]]*$(id -u fsgqa):/{print \$4}' /proc/key-users | cut -d/ -f1")
> +: ${orig_keys:=0}
> +echo "orig_keys=$orig_keys" >> $seqres.full
> +orig_maxkeys=$(</proc/sys/kernel/keys/maxkeys)
> +keys_to_add=5
> +echo $((orig_keys + keys_to_add)) > /proc/sys/kernel/keys/maxkeys
> +
>  echo
>  echo "# Testing user key quota"
>  for i in `seq $((keys_to_add + 1))`; do
> @@ -106,6 +116,9 @@ for i in `seq $((keys_to_add + 1))`; do
>  	    | sed 's/ with identifier .*$//'
>  done
>  
> +# Restore the original key quota.
> +echo "$orig_maxkeys" > /proc/sys/kernel/keys/maxkeys
> +
>  rm -rf $dir
>  echo
>  _user_do "mkdir $dir"
> -- 
> 2.25.0.341.g760bfbb309-goog
>