Hi,
On 2/14/2025 2:49 PM, Hou Tao wrote:
> Hi,
>
> On 2/14/2025 12:17 PM, Alexei Starovoitov wrote:
>> On Thu, Feb 13, 2025 at 8:12 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>>> Hi,
>>>
>>> On 2/14/2025 7:56 AM, Alexei Starovoitov wrote:
>>>> On Sat, Jan 25, 2025 at 2:59 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>>>>> From: Hou Tao <houtao1@xxxxxxxxxx>
>>>>>
>>>>> When there is bpf_dynptr field in the map key btf type or the map key
>>>>> btf type is bpf_dyntr, set BPF_INT_F_DYNPTR_IN_KEY in map_flags.
>>>>>
>>>>> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
>>>>> ---
>>>>> kernel/bpf/syscall.c | 36 ++++++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 36 insertions(+)
>>>>>
>>>>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>>>>> index 07c67ad1a6a07..46b96d062d2db 100644
>>>>> --- a/kernel/bpf/syscall.c
>>>>> +++ b/kernel/bpf/syscall.c
>>>>> @@ -1360,6 +1360,34 @@ static struct btf *get_map_btf(int btf_fd)
>>>>> return btf;
>>>>> }
>>>>>
> SNIP
>>>>> #define BPF_MAP_CREATE_LAST_FIELD map_token_fd
>>>>> /* called via syscall */
>>>>> static int map_create(union bpf_attr *attr)
>>>>> @@ -1398,6 +1426,14 @@ static int map_create(union bpf_attr *attr)
>>>>> btf = get_map_btf(attr->btf_fd);
>>>>> if (IS_ERR(btf))
>>>>> return PTR_ERR(btf);
>>>>> +
>>>>> + err = map_has_dynptr_in_key_type(btf, attr->btf_key_type_id, attr->key_size);
>>>>> + if (err < 0)
>>>>> + goto put_btf;
>>>>> + if (err > 0) {
>>>>> + attr->map_flags |= BPF_INT_F_DYNPTR_IN_KEY;
>>>> I don't like this inband signaling in the uapi field.
>>>> The whole refactoring in patch 4 to do patch 6 and
>>>> subsequent bpf_map_has_dynptr_key() in various places
>>>> feels like reinventing the wheel.
>>>>
>>>> We already have map_check_btf() mechanism that works for
>>>> existing special fields inside BTF.
>>>> Please use it.
>>> Yes. However map->key_record is only available after the map is created,
>>> but the creation of hash map needs to check it before the map is
>>> created. Instead of using an internal flag, how about adding extra
>>> argument for both ->map_alloc_check() and ->map_alloc() as proposed in
>>> the commit message of the previous patch ?
>>>> map_has_dynptr_in_key_type() can be done in map_check_btf()
>>>> after map is created, no ?
>>> No. both ->map_alloc_check() and ->map_alloc() need to know whether
>>> dynptr is enabled (as explained in the previous commit message). Both of
>>> these functions are called before the map is created.
>> Is that the explanation?
>> "
>> The reason for an internal map flag is twofolds:
>> 1) user doesn't need to set the map flag explicitly
>> map_create() will use the presence of bpf_dynptr in map key as an
>> indicator of enabling dynptr key.
>> 2) avoid adding new arguments for ->map_alloc_check() and ->map_alloc()
>> map_create() needs to pass the supported status of dynptr key to
>> ->map_alloc_check (e.g., check the maximum length of dynptr data size)
>> and ->map_alloc (e.g., check whether dynptr key fits current map type).
>> Adding new arguments for these callbacks to achieve that will introduce
>> too much churns.
>>
>> Therefore, the patch uses the topmost bit of map_flags as the internal
>> map flag. map_create() checks whether the internal flag is set in the
>> beginning and bpf_map_get_info_by_fd() clears the internal flag before
>> returns the map flags to userspace.
>> "
>>
>> As commented in the other patch map_extra can be dropped (I hope).
>> When it's gone, the map can be destroyed after creation in map_check_btf().
>> What am I missing?
> If I understanding correctly, you are suggesting to replace
> (map->map_flags & BPF_INT_F_DYNPTR_IN_KEY) with !!map->key_record, right
> ? And you also don't want to move map_check_btf() before the invocation
> of ->map_alloc_check() and ->map_alloc(), right ? However, beside the
> checking of map_extra, ->map_alloc_check() also needs to know whether
> the dynptr-typed key is suitable for current hash map type or map flags.
> ->map_alloc() also needs to allocate a bpf mem allocator for the dynptr
> key. So are you proposing the following steps for creating a dynkey hash
> map:
>
> 1) ->map_alloc_check()
> no change
>
> 2) ->map_alloc()
> allocate bpf mem allocator for dynptr unconditionally
>
> 3) map_check_btf()
> invokes an new map callback (e.g., ->map_alloc_post_check()) to check
> whether the created map is mismatched with the dynptr key and destroy it
> if it is.
Sorry, I misread the code, so the third steps is:
3) ->map_check_btf()
In ->map_check_btf() callback, check whether the created map is
mismatched with the dynptr key. If it is, let map_create() destroys the map.
>
>
>
