Re: [PATCH] keys: Fix linking a duplicate key to a keyring's assoc_array

[Petr Pavlu <petr.pavlu@suse.com>]

On 6/8/23 16:12, David Howells wrote:
> Petr Pavlu <petr.pavlu@suse.com> wrote:
> 
>> * Back on the first task, function construct_alloc_key() first runs
>>   __key_link_begin() to determine an assoc_array_edit operation to
>>   insert a new key. Index keys in the array are compared exactly as-is,
>>   using keyring_compare_object(). The operation finds that "abcdef" is
>>   not yet present in the destination keyring.
> 
> Good catch, but I think it's probably the wrong solution.
> 
> keyring_compare_object() needs to use the ->cmp() function from the key type.
> 
> It's not just request_key() that might have a problem, but also key_link().

The way I view the current design is that it kind of consists of two
layers. Lower-level functions key_create(), key_link(), key_move(), etc.
are built directly on top of assoc_array, use the exact comparison and
benefit from the assoc_array speed.

Higher-level function request_key() then provides a callout
functionality and offers an option to do approximate search if a needed
key is already present. This gives a trade-off to potentially reduce
a number of callouts but on the other hand requires a linear search over
the underlying keyrings/assoc_arrays.

The patch tries to only provide a point fix where the request-key logic
in construct_alloc_key() wrongly interacted with the approximate
matching option. If my understanding of the current design is correct
then I think key_link() shouldn't require any change in this regard.

Just wanted to add this point, I can't really comment on whether the
whole thing should be designed differently in the first place.

Thanks,
Petr