Re: [PATCH] generic/581: remove extra escape character from awk line

[Luis Henriques <lhenriques@suse.de>]

Eric Biggers <ebiggers@kernel.org> writes:

> On Tue, Nov 28, 2023 at 02:16:44PM +0000, Luis Henriques wrote:
>> >
>> > Yeah, looking closer it makes sense.  Sorry for the noise.  I'm currently
>> > investigating a test failure (which I can't reproduce locally) where
>> > 'orig_key' unexpectedly is set to '1' and makes the test fail because it
>> > was supposed to be '0'.  That's when this caught my attention.  Anyway,
>> > I'll go look somewhere else.
>> 
>> OK, I'm not 100% sure yet, but I've an idea about what's going on with
>> this test failure.
>> 
>> I _think_ that even after the following is done in the test:
>> 
>>     _user_do_rm_enckey $SCRATCH_MNT $keyid
>>     _scratch_cycle_mount
>> 
>> the key garbage collector may not have finish running.  And then, when we
>> read '/proc/key-users', we can race against key_user_put(), which needs
>> key_user_lock, which is also grabbed while the proc file seq_operations
>> are run.
>> 
>> Eric, does this make any sense?  There is a loop in the test to wait for
>> invalidated keys, but I believe it's not relevant anymore since commit
>> d7e7b9af104c ("fscrypt: stop using keyrings subsystem for
>> fscrypt_master_key").  But I might be misunderstanding the code.
>
> Thanks for looking into this!  I had noticed this test is still flaky on arm64
> but haven't had a chance to look into it.  Yes, it's probably related to the key
> garbage collector again.  The test needs to wait for the fscrypt "user" keys
> (key_type_fscrypt_user in the kernel) to be released from the quota.  I think
> that loop in the test does not have the intended effect because it waits for
> "invalidated" keys, but the fscrypt "user" keys (which are charged to the quota)
> are never invalidated; they're just released normally.  There used to be another
> key (in the "keyrings" subsystem sense of the word "key") associated with each
> fscrypt key, and that key was indeed invalidated, but that's no longer the case.
>

Awesome, thanks for confirming this.  That loop probably made sense back
when keys were invalidated -- that behaviour was changed by the commit I
mentioned, I believe.  Anyway, it's probably better to keep it the loop
for testing old kernels, as it doesn't really hurt.

> 
> Maybe there's a better way to wait for the key garbage collector to
> finish.
>
> Or the kernel could be changed to make releasing the key quota be synchronous.
> Unfortunately the keyrings subsystem doesn't seem to work that way, and fscrypt
> is tying into the key quota from the keyrings subsystem, so it is subject to its
> limitations.  But maybe there's a way to do it.

Hmm... yeah, that requires a closer look at that subsystem to see if
something can be done.  I'll try to find something there that would make
that test more reliable.

Cheers,
-- 
Luís