On Tue, Jan 28, 2020 at 04:42:51PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx>
> ---
> 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
>
