On Thu, 30 Oct 2014 13:46:47 -0400 Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
> After about 10 minutes, my NFSv4 functional tests fail because the
> ownership of the test files goes to "-2". Looking at /proc/keys
> shows that the id_resolv keys that map to my test user ID have
> expired. The ownership problem persists until the expired keys are
> purged from the keyring, and fresh keys are obtained.
>
> I bisected the problem to 3.13 commit b2a4df200d57 ("KEYS: Expand
> the capacity of a keyring"). This commit inadvertantly changes the
> API contract of the internal function keyring_search_aux().
>
> The root cause appears to be that b2a4df200d57 made "no state check"
> the default behavior. "No state check" means the keyring search
> iterator function skips checking the key's expiry timeout, and
> returns expired keys. request_key_and_link() depends on getting
> an -EAGAIN result code to know when to perform an upcall to refresh
> an expired key.
>
> Restore the previous default behavior of keyring_search_aux() and
> the meaning of the KEYRING_SEARCH_{DO,NO}_STATE_CHECK flags.
>
> Fixes: b2a4df200d57 ("KEYS: Expand the capacity of a keyring")
> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> ---
> Resend with correct linux-nfs address.
>
> security/keys/keyring.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index 8314a7d..1294582 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -519,9 +519,14 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
> struct keyring_search_context *ctx = iterator_data;
> const struct key *key = keyring_ptr_to_key(object);
> unsigned long kflags = key->flags;
> + bool state_check_needed;
>
> kenter("{%d}", key->serial);
>
> + state_check_needed = !(ctx->flags & KEYRING_SEARCH_NO_STATE_CHECK);
> + if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK)
> + state_check_needed = true;
> +
> /* ignore keys not of this type */
> if (key->type != ctx->index_key.type) {
> kleave(" = 0 [!type]");
> @@ -529,7 +534,7 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
> }
>
> /* skip invalidated, revoked and expired keys */
> - if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
> + if (state_check_needed) {
> if (kflags & ((1 << KEY_FLAG_INVALIDATED) |
> (1 << KEY_FLAG_REVOKED))) {
> ctx->result = ERR_PTR(-EKEYREVOKED);
> @@ -559,7 +564,7 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
> goto skipped;
> }
>
> - if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
> + if (state_check_needed) {
> /* we set a different error code if we pass a negative key */
> if (kflags & (1 << KEY_FLAG_NEGATIVE)) {
> smp_rmb();
> @@ -642,8 +647,6 @@ static bool search_nested_keyrings(struct key *keyring,
> }
>
> ctx->skipped_ret = 0;
> - if (ctx->flags & KEYRING_SEARCH_NO_STATE_CHECK)
> - ctx->flags &= ~KEYRING_SEARCH_DO_STATE_CHECK;
>
> /* Start processing a new keyring */
> descend_to_keyring:
>
Reviewed-by: NeilBrown <neilb@xxxxxxx>
security/keys/internal.h says:
#define KEYRING_SEARCH_NO_STATE_CHECK 0x0001 /* Skip state checks */
#define KEYRING_SEARCH_DO_STATE_CHECK 0x0002 /* Override NO_STATE_CHECK */
Your match makes it obvious that DO overrides NO. The current code
doesn't get that right.
Is this on its way upstream yet (not in -next that I could see).
Thanks,
NeilBrown
Attachment:
pgplADRDrx7hN.pgp
Description: OpenPGP digital signature
