All of lore.kernel.org
 help / color / mirror / Atom feed
From: Eric Biggers <ebiggers@kernel.org>
To: fstests@vger.kernel.org
Cc: linux-fscrypt@vger.kernel.org, keyrings@vger.kernel.org,
	Murphy Zhou <xzhou@redhat.com>
Subject: [PATCH] generic/581: try to avoid flakiness in keys quota test
Date: Tue, 28 Jan 2020 16:42:51 -0800	[thread overview]
Message-ID: <20200129004251.133747-1-ebiggers@kernel.org> (raw)

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.

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


WARNING: multiple messages have this Message-ID (diff)
From: Eric Biggers <ebiggers@kernel.org>
To: fstests@vger.kernel.org
Cc: linux-fscrypt@vger.kernel.org, keyrings@vger.kernel.org,
	Murphy Zhou <xzhou@redhat.com>
Subject: [PATCH] generic/581: try to avoid flakiness in keys quota test
Date: Wed, 29 Jan 2020 00:42:51 +0000	[thread overview]
Message-ID: <20200129004251.133747-1-ebiggers@kernel.org> (raw)

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.

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

             reply	other threads:[~2020-01-29  0:43 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-29  0:42 Eric Biggers [this message]
2020-01-29  0:42 ` [PATCH] generic/581: try to avoid flakiness in keys quota test Eric Biggers
2020-02-05  4:39 ` Murphy Zhou
2020-02-05  4:39   ` Murphy Zhou

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200129004251.133747-1-ebiggers@kernel.org \
    --to=ebiggers@kernel.org \
    --cc=fstests@vger.kernel.org \
    --cc=keyrings@vger.kernel.org \
    --cc=linux-fscrypt@vger.kernel.org \
    --cc=xzhou@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.