[bpf-next,01/16] bpf: Introduce map flag BPF_F_DYNPTR_IN_KEY

Commit Message

Hou Tao Oct. 8, 2024, 9:14 a.m. UTC

From: Hou Tao <[e-mail]>

Introduce map flag BPF_F_DYNPTR_IN_KEY to support dynptr in map key. Add
the corresponding helper bpf_map_has_dynptr_key() to check whether or
not the dynptr-key is supported.

For map with dynptr key support, it needs to use map_extra to specify
the maximum length of these dynptrs. The implementation of the map will
check whether map_extra is smaller than the limitation imposed by memory
allocation during map creation. It may also use map_extra to optimizate
the memory allocation for dynptr.

Signed-off-by: Hou Tao <[e-mail]>
---
 include/linux/bpf.h            | 5 +++++
 include/uapi/linux/bpf.h       | 3 +++
 kernel/bpf/syscall.c           | 1 +
 tools/include/uapi/linux/bpf.h | 3 +++
 4 files changed, 12 insertions(+)

Comments

Alexei Starovoitov Oct. 10, 2024, 2:21 a.m. UTC | #1

On Tue, Oct 8, 2024 at 2:02 AM Hou Tao <[e-mail]> wrote:
>
> index c6cd7c7aeeee..07f7df308a01 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1409,6 +1409,9 @@ enum {
>
>  /* Do not translate kernel bpf_arena pointers to user pointers */
>         BPF_F_NO_USER_CONV      = (1U << 18),
> +
> +/* Create a map with bpf_dynptr in key */
> +       BPF_F_DYNPTR_IN_KEY     = (1U << 19),
>  };

If I'm reading the other patches correctly this uapi flag
is unnecessary.
BTF describes the fields and dynptr is either there or not.
Why require users to add an extra flag ?

Hou Tao Oct. 21, 2024, 1:45 p.m. UTC | #2

Hi,

On 10/10/2024 10:21 AM, Alexei Starovoitov wrote:
> On Tue, Oct 8, 2024 at 2:02 AM Hou Tao <[e-mail]> wrote:
>> index c6cd7c7aeeee..07f7df308a01 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -1409,6 +1409,9 @@ enum {
>>
>>  /* Do not translate kernel bpf_arena pointers to user pointers */
>>         BPF_F_NO_USER_CONV      = (1U << 18),
>> +
>> +/* Create a map with bpf_dynptr in key */
>> +       BPF_F_DYNPTR_IN_KEY     = (1U << 19),
>>  };
> If I'm reading the other patches correctly this uapi flag
> is unnecessary.
> BTF describes the fields and dynptr is either there or not.
> Why require users to add an extra flag ?

Sorry for the late reply. The reason for an extra flag is to make a bpf
map which had already used bpf_dynptr in its key to work as before. I
was not sure whether or not there is such case, so I added an extra
flag. If the case is basically impossible, I can remove it in the next
revision.

Alexei Starovoitov Oct. 22, 2024, 3:53 a.m. UTC | #3

On Mon, Oct 21, 2024 at 6:46 AM Hou Tao <[e-mail]> wrote:
>
> Hi,
>
> On 10/10/2024 10:21 AM, Alexei Starovoitov wrote:
> > On Tue, Oct 8, 2024 at 2:02 AM Hou Tao <[e-mail]> wrote:
> >> index c6cd7c7aeeee..07f7df308a01 100644
> >> --- a/include/uapi/linux/bpf.h
> >> +++ b/include/uapi/linux/bpf.h
> >> @@ -1409,6 +1409,9 @@ enum {
> >>
> >>  /* Do not translate kernel bpf_arena pointers to user pointers */
> >>         BPF_F_NO_USER_CONV      = (1U << 18),
> >> +
> >> +/* Create a map with bpf_dynptr in key */
> >> +       BPF_F_DYNPTR_IN_KEY     = (1U << 19),
> >>  };
> > If I'm reading the other patches correctly this uapi flag
> > is unnecessary.
> > BTF describes the fields and dynptr is either there or not.
> > Why require users to add an extra flag ?
>
> Sorry for the late reply. The reason for an extra flag is to make a bpf
> map which had already used bpf_dynptr in its key to work as before. I
> was not sure whether or not there is such case, so I added an extra
> flag. If the case is basically impossible, I can remove it in the next
> revision.

Hmm. bpf_dynptr is a kernel type and iirc (after paging in
the context after 12 days of silence) you were proposing to add
a new bpf_dynptr_user type which theoretically can be present
in the key, but it's fine to break such progs.

Hou Tao Oct. 22, 2024, 4:22 a.m. UTC | #4

Hi,

On 10/22/2024 11:53 AM, Alexei Starovoitov wrote:
> On Mon, Oct 21, 2024 at 6:46 AM Hou Tao <[e-mail]> wrote:
>> Hi,
>>
>> On 10/10/2024 10:21 AM, Alexei Starovoitov wrote:
>>> On Tue, Oct 8, 2024 at 2:02 AM Hou Tao <[e-mail]> wrote:
>>>> index c6cd7c7aeeee..07f7df308a01 100644
>>>> --- a/include/uapi/linux/bpf.h
>>>> +++ b/include/uapi/linux/bpf.h
>>>> @@ -1409,6 +1409,9 @@ enum {
>>>>
>>>>  /* Do not translate kernel bpf_arena pointers to user pointers */
>>>>         BPF_F_NO_USER_CONV      = (1U << 18),
>>>> +
>>>> +/* Create a map with bpf_dynptr in key */
>>>> +       BPF_F_DYNPTR_IN_KEY     = (1U << 19),
>>>>  };
>>> If I'm reading the other patches correctly this uapi flag
>>> is unnecessary.
>>> BTF describes the fields and dynptr is either there or not.
>>> Why require users to add an extra flag ?
>> Sorry for the late reply. The reason for an extra flag is to make a bpf
>> map which had already used bpf_dynptr in its key to work as before. I
>> was not sure whether or not there is such case, so I added an extra
>> flag. If the case is basically impossible, I can remove it in the next
>> revision.
> Hmm. bpf_dynptr is a kernel type and iirc (after paging in
> the context after 12 days of silence) you were proposing to add
> a new bpf_dynptr_user type which theoretically can be present
> in the key, but it's fine to break such progs.

Got it. Will remove the extra flag in the next revision. Sorry again for
the long delay.  Will try to reply timely next time. bpf_dynptr_user is
only for syscall, bpf_dynptr will be used in map key.

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 19d8ca8ac960..f61bf427e14e 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -308,6 +308,11 @@  struct bpf_map {
 	s64 __percpu *elem_count;
 };
 
+static inline bool bpf_map_has_dynptr_key(const struct bpf_map *map)
+{
+	return map->map_flags & BPF_F_DYNPTR_IN_KEY;
+}
+
 static inline const char *btf_field_type_name(enum btf_field_type type)
 {
 	switch (type) {
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index c6cd7c7aeeee..07f7df308a01 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1409,6 +1409,9 @@  enum {
 
 /* Do not translate kernel bpf_arena pointers to user pointers */
 	BPF_F_NO_USER_CONV	= (1U << 18),
+
+/* Create a map with bpf_dynptr in key */
+	BPF_F_DYNPTR_IN_KEY     = (1U << 19),
 };
 
 /* Flags for BPF_PROG_QUERY. */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index a8f1808a1ca5..bffd803c5977 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1232,6 +1232,7 @@  static int map_create(union bpf_attr *attr)
 
 	if (attr->map_type != BPF_MAP_TYPE_BLOOM_FILTER &&
 	    attr->map_type != BPF_MAP_TYPE_ARENA &&
+	    !(attr->map_flags & BPF_F_DYNPTR_IN_KEY) &&
 	    attr->map_extra != 0)
 		return -EINVAL;
 
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 1fb3cb2636e6..14f223282bfa 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1409,6 +1409,9 @@  enum {
 
 /* Do not translate kernel bpf_arena pointers to user pointers */
 	BPF_F_NO_USER_CONV	= (1U << 18),
+
+/* Create a map with bpf_dynptr in key */
+	BPF_F_DYNPTR_IN_KEY     = (1U << 19),
 };
 
 /* Flags for BPF_PROG_QUERY. */