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;
> >> }
> >>
> >> +static int map_has_dynptr_in_key_type(struct btf *btf, u32 btf_key_id, u32 key_size)
> >> +{
> >> + const struct btf_type *type;
> >> + struct btf_record *record;
> >> + u32 btf_key_size;
> >> +
> >> + if (!btf_key_id)
> >> + return 0;
> >> +
> >> + type = btf_type_id_size(btf, &btf_key_id, &btf_key_size);
> >> + if (!type || btf_key_size != key_size)
> >> + return -EINVAL;
> >> +
> >> + /* For dynptr key, key BTF type must be struct */
> >> + if (!__btf_type_is_struct(type))
> >> + return 0;
> >> +
> >> + if (btf_type_is_dynptr(btf, type))
> >> + return 1;
> >> +
> >> + record = btf_parse_fields(btf, type, BPF_DYNPTR, key_size);
> >> + if (IS_ERR(record))
> >> + return PTR_ERR(record);
> >> +
> >> + btf_record_free(record);
> >> + return !!record;
> >> +}
> >> +
> >> #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?
