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.
?
