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

[Luis Henriques <lhenriques@suse.de>]

Luís Henriques <henrix@camandro.org> writes:

> (Sorry for replying from private email address)
>
> On Wed, Nov 22, 2023 at 03:16:48PM -0800, Eric Biggers wrote:
>> On Wed, Nov 22, 2023 at 05:03:24PM +0000, Luis Henriques wrote:
>> > Checking the keys in /proc/key-users is buggy, as there's an extra '\'
>> > character: in '{print \$4}' the '$4' shouldn't be escaped otherwise the
>> > 'awk' command will fail.  This has passed unnoticed because the output
>> > is sent to '_user_do' function and the result assigned to a variable.
>> > 
>> > While there, replace 'awk' by $AWK_PROG.
>> > 
>> > Signed-off-by: Luis Henriques <lhenriques@suse.de>
>> > ---
>> >  tests/generic/581 | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > 
>> > Hi!
>> > 
>> > Please note that I'm not an 'awk' expert and I may be wrong!  But if I do
>> > see an error if I run something like:
>> > 
>> > $ awk '/^[[:space:]]*1000:/{print \$4}' /proc/key-users 
>> > awk: cmd. line:1: /^[[:space:]]*1000:/{print \$4}
>> > awk: cmd. line:1:                            ^ backslash not last character on line
>> > 
>> > But maybe this depends on the awk implementation, although I've tried a few.
>> > 
>> > diff --git a/tests/generic/581 b/tests/generic/581
>> > index cabc7e1c69ab..1a4b571d40ce 100755
>> > --- a/tests/generic/581
>> > +++ b/tests/generic/581
>> > @@ -92,7 +92,7 @@ while grep -E -q '^[0-9a-f]+ [^ ]*i[^ ]*' /proc/keys; do
>> >  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=$(_user_do "$AWK_PROG '/^[[:space:]]*$(id -u fsgqa):/{print $4}' /proc/key-users | cut -d/ -f1")
>> 
>> The backslash is needed to prevent $4 from being expanded by bash, because the
>> whole pipeline with 'awk' and 'cut' is in a double-quoted string:
>> 
>>     "awk '/^[[:space:]]*$(id -u fsgqa):/{print \$4}' /proc/key-users | cut -d/ -f1"
>> 
>> Without escaping the $, bash would replace $4 with the empty string while it's
>> doing expansions on the whole double-quoted string.
>
> /me blushes
>
> 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.

Cheers,
-- 
Luís