[LTP] [PATCH 1/2] add_key05: Avoid race with key garbage collection

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Jan Stancek <jstancek@redhat.com> writes:

> ----- Original Message -----
>> Hello,
>> 
>> Jan Stancek <jstancek@redhat.com> writes:
>> 
>> > ----- Original Message -----
>> >> Hi Richard
>> >>
>> >> > The key subsystem independently tracks user info against UID. If a user
>> >> > is
>> >> > deleted and the UID reused for a new user then the key subsystem will
>> >> > mistake
>> >> > the new user for the old one.
>> >
>> > Thanks for raising this problem Richard. This matches CKI failure
>> > we seen recently. (CC Li and Rachel)
>> >
>> >> Does any documentation or kernel comment mentioned this? I didn't notice
>> >> this before.
>> >> >
>> >> > The keys/keyrings may not be accessible to the new user, but if they are
>> >> > not
>> >> > yet garbage collected (which happens asynchronously) then the new user
>> >> > may
>> >> > be
>> >> > exceeding its quota limits.
>> >> >
>> >> > This results in a race condition where this test can fail because the
>> >> > old
>> >> > thread keyring is taking up the full quota. We should be able to avoid
>> >> > this
>> >> > by
>> >> > creating two users in parallel instead of sequentially so that they have
>> >> > different UIDs.
>> >> I guess you may want to creat two user, so next, the key subsystem
>> >> think the new user is different from  the last deleting user. It can
>> >> avoid race.
>> >>
>> >> But you patch overrides ltpuser, in actually, we still use
>> >> ltp_add_key05_1 in SAFE_SETUID.
>> >>
>> >> Also, this patch doesn't handle delete user when we using -i parameters.
>> >
>> > -i might be problem, but other than that I think it works, at least for
>> > default run.
>> >
>> > Though I'm wondering, shouldn't the test delete keys it creates,
>> > rather than relying on garbage collection?
>> 
>> I'm assuming the keys are 'deleted' when the thread keyring is destroyed
>> when the child process exits. However they are not freed until later by
>> garbage collection (maybe I am confusing deferred freeing with 'garbage
>> collection'?).
>
> Do you know how large is the race window?
>
> Default /proc/sys/kernel/keys/gc_delay is 300, so if it's tied to this
> garbage collect, I'd expect it to fail almost all the time.

It doesn't appear to be tied to that.

>
>> 
>> We could explicitly delete/revoke the individual keys, but AFAICT there
>> would still be a race because freeing is still asynchronous. Ofcourse
>> there might be a reliable way to force freeing?
>
> gc_delay is only one I recall.
>
> If it's tied to process being around, I can try similar approach from 
> e747d0456adc ("syscalls/tgkill03: wait for defunct tid to get detached")
> where we wait for /proc/<pid>/task/<tid> to disappear.


This might work as the work is scheduled to be done in process context,
so the task may remain until the keys have been freed.

-- 
Thank you,
Richard.