* [PATCH bpf-next 0/3] allow bpf_map_sum_elem_count for all program types
[not found] <20230714141747.41560-1-aspsk@isovalent.com>
@ 2023-07-14 14:20 ` Anton Protopopov
2023-07-14 14:20 ` [PATCH bpf-next 1/3] bpf: consider CONST_PTR_TO_MAP as trusted pointer to struct bpf_map Anton Protopopov
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Anton Protopopov @ 2023-07-14 14:20 UTC (permalink / raw)
To: Martin KaFai Lau, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, Hou Tao, Joe Stringer, Anton Protopopov, bpf,
linux-kernel, linux-kselftest
This short series is a follow up to the recent series [1] which added
per-cpu insert/delete statistics for maps. The bpf_map_sum_elem_count
kfunc presented in the original series was only available to tracing
programs, so let's make it available to all.
The first patch allows to treat CONST_PTR_TO_MAP as trusted pointers
from kfunc's point of view.
The second patch just adds const to the map argument of the
bpf_map_sum_elem_count kfunc.
The third patch registers the bpf_map_sum_elem_count for all programs,
and patches selftests correspondingly.
Anton Protopopov (3):
bpf: consider CONST_PTR_TO_MAP as trusted pointer to struct bpf_map
bpf: make an argument const in the bpf_map_sum_elem_count kfunc
bpf: allow any program to use the bpf_map_sum_elem_count kfunc
include/linux/btf_ids.h | 1 +
kernel/bpf/map_iter.c | 7 +++----
kernel/bpf/verifier.c | 5 ++++-
tools/testing/selftests/bpf/progs/map_ptr_kern.c | 5 +++++
4 files changed, 13 insertions(+), 5 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH bpf-next 1/3] bpf: consider CONST_PTR_TO_MAP as trusted pointer to struct bpf_map
2023-07-14 14:20 ` [PATCH bpf-next 0/3] allow bpf_map_sum_elem_count for all program types Anton Protopopov
@ 2023-07-14 14:20 ` Anton Protopopov
2023-07-14 17:56 ` Alexei Starovoitov
2023-07-14 14:20 ` [PATCH bpf-next 2/3] bpf: make an argument const in the bpf_map_sum_elem_count kfunc Anton Protopopov
2023-07-14 14:21 ` [PATCH bpf-next 3/3] bpf: allow any program to use " Anton Protopopov
2 siblings, 1 reply; 8+ messages in thread
From: Anton Protopopov @ 2023-07-14 14:20 UTC (permalink / raw)
To: Martin KaFai Lau, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, Hou Tao, Joe Stringer, Anton Protopopov, bpf,
linux-kernel, linux-kselftest
Patch verifier to regard values of type CONST_PTR_TO_MAP as trusted
pointers to struct bpf_map. This allows kfuncs to work with `struct
bpf_map *` arguments.
Save some bytes by defining btf_bpf_map_id as BTF_ID_LIST_GLOBAL_SINGLE
(which is u32[1]), not as BTF_ID_LIST (which is u32[64]).
Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
---
include/linux/btf_ids.h | 1 +
kernel/bpf/map_iter.c | 3 +--
kernel/bpf/verifier.c | 5 ++++-
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
index 00950cc03bff..a3462a9b8e18 100644
--- a/include/linux/btf_ids.h
+++ b/include/linux/btf_ids.h
@@ -267,5 +267,6 @@ MAX_BTF_TRACING_TYPE,
extern u32 btf_tracing_ids[];
extern u32 bpf_cgroup_btf_id[];
extern u32 bpf_local_storage_map_btf_id[];
+extern u32 btf_bpf_map_id[];
#endif
diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
index d06d3b7150e5..b67996147895 100644
--- a/kernel/bpf/map_iter.c
+++ b/kernel/bpf/map_iter.c
@@ -78,8 +78,7 @@ static const struct seq_operations bpf_map_seq_ops = {
.show = bpf_map_seq_show,
};
-BTF_ID_LIST(btf_bpf_map_id)
-BTF_ID(struct, bpf_map)
+BTF_ID_LIST_GLOBAL_SINGLE(btf_bpf_map_id, struct, bpf_map)
static const struct bpf_iter_seq_info bpf_map_seq_info = {
.seq_ops = &bpf_map_seq_ops,
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0b9da95331d7..5663f97ef292 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5419,6 +5419,9 @@ static bool is_trusted_reg(const struct bpf_reg_state *reg)
if (reg->ref_obj_id)
return true;
+ if (reg->type == CONST_PTR_TO_MAP)
+ return true;
+
/* If a register is not referenced, it is trusted if it has the
* MEM_ALLOC or PTR_TRUSTED type modifiers, and no others. Some of the
* other type modifiers may be safe, but we elect to take an opt-in
@@ -10052,13 +10055,13 @@ static bool __btf_type_is_scalar_struct(struct bpf_verifier_env *env,
return true;
}
-
static u32 *reg2btf_ids[__BPF_REG_TYPE_MAX] = {
#ifdef CONFIG_NET
[PTR_TO_SOCKET] = &btf_sock_ids[BTF_SOCK_TYPE_SOCK],
[PTR_TO_SOCK_COMMON] = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
[PTR_TO_TCP_SOCK] = &btf_sock_ids[BTF_SOCK_TYPE_TCP],
#endif
+ [CONST_PTR_TO_MAP] = btf_bpf_map_id,
};
enum kfunc_ptr_arg_type {
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH bpf-next 2/3] bpf: make an argument const in the bpf_map_sum_elem_count kfunc
2023-07-14 14:20 ` [PATCH bpf-next 0/3] allow bpf_map_sum_elem_count for all program types Anton Protopopov
2023-07-14 14:20 ` [PATCH bpf-next 1/3] bpf: consider CONST_PTR_TO_MAP as trusted pointer to struct bpf_map Anton Protopopov
@ 2023-07-14 14:20 ` Anton Protopopov
2023-07-14 14:21 ` [PATCH bpf-next 3/3] bpf: allow any program to use " Anton Protopopov
2 siblings, 0 replies; 8+ messages in thread
From: Anton Protopopov @ 2023-07-14 14:20 UTC (permalink / raw)
To: Martin KaFai Lau, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, Hou Tao, Joe Stringer, Anton Protopopov, bpf,
linux-kernel, linux-kselftest
We use the map pointer only to read the counter values, no locking
involved, so mark the argument as const.
Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
---
kernel/bpf/map_iter.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
index b67996147895..011adb41858e 100644
--- a/kernel/bpf/map_iter.c
+++ b/kernel/bpf/map_iter.c
@@ -197,7 +197,7 @@ __diag_push();
__diag_ignore_all("-Wmissing-prototypes",
"Global functions as their definitions will be in vmlinux BTF");
-__bpf_kfunc s64 bpf_map_sum_elem_count(struct bpf_map *map)
+__bpf_kfunc s64 bpf_map_sum_elem_count(const struct bpf_map *map)
{
s64 *pcount;
s64 ret = 0;
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH bpf-next 3/3] bpf: allow any program to use the bpf_map_sum_elem_count kfunc
2023-07-14 14:20 ` [PATCH bpf-next 0/3] allow bpf_map_sum_elem_count for all program types Anton Protopopov
2023-07-14 14:20 ` [PATCH bpf-next 1/3] bpf: consider CONST_PTR_TO_MAP as trusted pointer to struct bpf_map Anton Protopopov
2023-07-14 14:20 ` [PATCH bpf-next 2/3] bpf: make an argument const in the bpf_map_sum_elem_count kfunc Anton Protopopov
@ 2023-07-14 14:21 ` Anton Protopopov
2 siblings, 0 replies; 8+ messages in thread
From: Anton Protopopov @ 2023-07-14 14:21 UTC (permalink / raw)
To: Martin KaFai Lau, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, Hou Tao, Joe Stringer, Anton Protopopov, bpf,
linux-kernel, linux-kselftest
Register the bpf_map_sum_elem_count func for all programs, and update the
map_ptr subtest of the test_progs test to test the new functionality.
The usage is allowed as long as the pointer to the map is trusted (when
using tracing programs) or is a const pointer to map, as in the following
example:
struct {
__uint(type, BPF_MAP_TYPE_HASH);
...
} hash SEC(".maps");
...
static inline int some_bpf_prog(void)
{
struct bpf_map *map = (struct bpf_map *)&hash;
__s64 count;
count = bpf_map_sum_elem_count(map);
...
}
Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
---
kernel/bpf/map_iter.c | 2 +-
tools/testing/selftests/bpf/progs/map_ptr_kern.c | 5 +++++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
index 011adb41858e..6fc9dae9edc8 100644
--- a/kernel/bpf/map_iter.c
+++ b/kernel/bpf/map_iter.c
@@ -226,6 +226,6 @@ static const struct btf_kfunc_id_set bpf_map_iter_kfunc_set = {
static int init_subsystem(void)
{
- return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_map_iter_kfunc_set);
+ return register_btf_kfunc_id_set(BPF_PROG_TYPE_UNSPEC, &bpf_map_iter_kfunc_set);
}
late_initcall(init_subsystem);
diff --git a/tools/testing/selftests/bpf/progs/map_ptr_kern.c b/tools/testing/selftests/bpf/progs/map_ptr_kern.c
index db388f593d0a..3325da17ec81 100644
--- a/tools/testing/selftests/bpf/progs/map_ptr_kern.c
+++ b/tools/testing/selftests/bpf/progs/map_ptr_kern.c
@@ -103,6 +103,8 @@ struct {
__type(value, __u32);
} m_hash SEC(".maps");
+__s64 bpf_map_sum_elem_count(struct bpf_map *map) __ksym;
+
static inline int check_hash(void)
{
struct bpf_htab *hash = (struct bpf_htab *)&m_hash;
@@ -115,6 +117,8 @@ static inline int check_hash(void)
VERIFY(hash->elem_size == 64);
VERIFY(hash->count.counter == 0);
+ VERIFY(bpf_map_sum_elem_count(map) == 0);
+
for (i = 0; i < HALF_ENTRIES; ++i) {
const __u32 key = i;
const __u32 val = 1;
@@ -123,6 +127,7 @@ static inline int check_hash(void)
return 0;
}
VERIFY(hash->count.counter == HALF_ENTRIES);
+ VERIFY(bpf_map_sum_elem_count(map) == HALF_ENTRIES);
return 1;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 1/3] bpf: consider CONST_PTR_TO_MAP as trusted pointer to struct bpf_map
2023-07-14 14:20 ` [PATCH bpf-next 1/3] bpf: consider CONST_PTR_TO_MAP as trusted pointer to struct bpf_map Anton Protopopov
@ 2023-07-14 17:56 ` Alexei Starovoitov
2023-07-16 7:50 ` Anton Protopopov
0 siblings, 1 reply; 8+ messages in thread
From: Alexei Starovoitov @ 2023-07-14 17:56 UTC (permalink / raw)
To: Anton Protopopov
Cc: Martin KaFai Lau, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, Hou Tao, Joe Stringer, bpf, LKML,
open list:KERNEL SELFTEST FRAMEWORK
On Fri, Jul 14, 2023 at 7:20 AM Anton Protopopov <aspsk@isovalent.com> wrote:
>
> Patch verifier to regard values of type CONST_PTR_TO_MAP as trusted
> pointers to struct bpf_map. This allows kfuncs to work with `struct
> bpf_map *` arguments.
>
> Save some bytes by defining btf_bpf_map_id as BTF_ID_LIST_GLOBAL_SINGLE
> (which is u32[1]), not as BTF_ID_LIST (which is u32[64]).
>
> Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> ---
> include/linux/btf_ids.h | 1 +
> kernel/bpf/map_iter.c | 3 +--
> kernel/bpf/verifier.c | 5 ++++-
> 3 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> index 00950cc03bff..a3462a9b8e18 100644
> --- a/include/linux/btf_ids.h
> +++ b/include/linux/btf_ids.h
> @@ -267,5 +267,6 @@ MAX_BTF_TRACING_TYPE,
> extern u32 btf_tracing_ids[];
> extern u32 bpf_cgroup_btf_id[];
> extern u32 bpf_local_storage_map_btf_id[];
> +extern u32 btf_bpf_map_id[];
>
> #endif
> diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
> index d06d3b7150e5..b67996147895 100644
> --- a/kernel/bpf/map_iter.c
> +++ b/kernel/bpf/map_iter.c
> @@ -78,8 +78,7 @@ static const struct seq_operations bpf_map_seq_ops = {
> .show = bpf_map_seq_show,
> };
>
> -BTF_ID_LIST(btf_bpf_map_id)
> -BTF_ID(struct, bpf_map)
> +BTF_ID_LIST_GLOBAL_SINGLE(btf_bpf_map_id, struct, bpf_map)
>
> static const struct bpf_iter_seq_info bpf_map_seq_info = {
> .seq_ops = &bpf_map_seq_ops,
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0b9da95331d7..5663f97ef292 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5419,6 +5419,9 @@ static bool is_trusted_reg(const struct bpf_reg_state *reg)
> if (reg->ref_obj_id)
> return true;
>
> + if (reg->type == CONST_PTR_TO_MAP)
> + return true;
> +
Overall it looks great.
Instead of above, how about the following instead:
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0b9da95331d7..cd08167dc347 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10775,7 +10775,7 @@ static int check_kfunc_args(struct
bpf_verifier_env *env, struct bpf_kfunc_call_
if (!is_kfunc_trusted_args(meta) && !is_kfunc_rcu(meta))
break;
- if (!is_trusted_reg(reg)) {
+ if (!is_trusted_reg(reg) &&
!reg2btf_ids[base_type(reg->type)]) {
This way we won't need to list every convertible type in is_trusted_reg.
I'm a bit hesitant to put reg2btf_ids[] check directly into is_trusted_reg().
Maybe it's ok, but it needs more analysis.
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 1/3] bpf: consider CONST_PTR_TO_MAP as trusted pointer to struct bpf_map
2023-07-14 17:56 ` Alexei Starovoitov
@ 2023-07-16 7:50 ` Anton Protopopov
2023-07-19 0:54 ` Alexei Starovoitov
0 siblings, 1 reply; 8+ messages in thread
From: Anton Protopopov @ 2023-07-16 7:50 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Martin KaFai Lau, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, Hou Tao, Joe Stringer, bpf, LKML,
open list:KERNEL SELFTEST FRAMEWORK
On Fri, Jul 14, 2023 at 10:56:00AM -0700, Alexei Starovoitov wrote:
> On Fri, Jul 14, 2023 at 7:20 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> >
> > Patch verifier to regard values of type CONST_PTR_TO_MAP as trusted
> > pointers to struct bpf_map. This allows kfuncs to work with `struct
> > bpf_map *` arguments.
> >
> > Save some bytes by defining btf_bpf_map_id as BTF_ID_LIST_GLOBAL_SINGLE
> > (which is u32[1]), not as BTF_ID_LIST (which is u32[64]).
> >
> > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > ---
> > include/linux/btf_ids.h | 1 +
> > kernel/bpf/map_iter.c | 3 +--
> > kernel/bpf/verifier.c | 5 ++++-
> > 3 files changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> > index 00950cc03bff..a3462a9b8e18 100644
> > --- a/include/linux/btf_ids.h
> > +++ b/include/linux/btf_ids.h
> > @@ -267,5 +267,6 @@ MAX_BTF_TRACING_TYPE,
> > extern u32 btf_tracing_ids[];
> > extern u32 bpf_cgroup_btf_id[];
> > extern u32 bpf_local_storage_map_btf_id[];
> > +extern u32 btf_bpf_map_id[];
> >
> > #endif
> > diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
> > index d06d3b7150e5..b67996147895 100644
> > --- a/kernel/bpf/map_iter.c
> > +++ b/kernel/bpf/map_iter.c
> > @@ -78,8 +78,7 @@ static const struct seq_operations bpf_map_seq_ops = {
> > .show = bpf_map_seq_show,
> > };
> >
> > -BTF_ID_LIST(btf_bpf_map_id)
> > -BTF_ID(struct, bpf_map)
> > +BTF_ID_LIST_GLOBAL_SINGLE(btf_bpf_map_id, struct, bpf_map)
> >
> > static const struct bpf_iter_seq_info bpf_map_seq_info = {
> > .seq_ops = &bpf_map_seq_ops,
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 0b9da95331d7..5663f97ef292 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -5419,6 +5419,9 @@ static bool is_trusted_reg(const struct bpf_reg_state *reg)
> > if (reg->ref_obj_id)
> > return true;
> >
> > + if (reg->type == CONST_PTR_TO_MAP)
> > + return true;
> > +
>
> Overall it looks great.
> Instead of above, how about the following instead:
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0b9da95331d7..cd08167dc347 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -10775,7 +10775,7 @@ static int check_kfunc_args(struct
> bpf_verifier_env *env, struct bpf_kfunc_call_
> if (!is_kfunc_trusted_args(meta) && !is_kfunc_rcu(meta))
> break;
>
> - if (!is_trusted_reg(reg)) {
> + if (!is_trusted_reg(reg) &&
> !reg2btf_ids[base_type(reg->type)]) {
>
>
> This way we won't need to list every convertible type in is_trusted_reg.
>
> I'm a bit hesitant to put reg2btf_ids[] check directly into is_trusted_reg().
> Maybe it's ok, but it needs more analysis.
I am not sure I see a difference in adding a check you proposed above and
adding the reg2btf_ids[] check directly into the is_trusted_reg() function.
Basically, we say "if type is in reg2btf_ids[], then consider it trusted" in
both cases. AFAIS, currently the reg2btf_ids[] contains only trusted types,
however, could it happen that we add a non-trusted type there?
So, I would leave the patch as is (which also makes sense because the
const-ptr-to-map is a special case), or add the "reg2btf_ids[] check"
directly into the is_trusted_reg() function.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 1/3] bpf: consider CONST_PTR_TO_MAP as trusted pointer to struct bpf_map
2023-07-16 7:50 ` Anton Protopopov
@ 2023-07-19 0:54 ` Alexei Starovoitov
2023-07-19 7:10 ` Anton Protopopov
0 siblings, 1 reply; 8+ messages in thread
From: Alexei Starovoitov @ 2023-07-19 0:54 UTC (permalink / raw)
To: Anton Protopopov
Cc: Martin KaFai Lau, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, Hou Tao, Joe Stringer, bpf, LKML,
open list:KERNEL SELFTEST FRAMEWORK
On Sun, Jul 16, 2023 at 12:49 AM Anton Protopopov <aspsk@isovalent.com> wrote:
>
> On Fri, Jul 14, 2023 at 10:56:00AM -0700, Alexei Starovoitov wrote:
> > On Fri, Jul 14, 2023 at 7:20 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> > >
> > > Patch verifier to regard values of type CONST_PTR_TO_MAP as trusted
> > > pointers to struct bpf_map. This allows kfuncs to work with `struct
> > > bpf_map *` arguments.
> > >
> > > Save some bytes by defining btf_bpf_map_id as BTF_ID_LIST_GLOBAL_SINGLE
> > > (which is u32[1]), not as BTF_ID_LIST (which is u32[64]).
> > >
> > > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > > ---
> > > include/linux/btf_ids.h | 1 +
> > > kernel/bpf/map_iter.c | 3 +--
> > > kernel/bpf/verifier.c | 5 ++++-
> > > 3 files changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> > > index 00950cc03bff..a3462a9b8e18 100644
> > > --- a/include/linux/btf_ids.h
> > > +++ b/include/linux/btf_ids.h
> > > @@ -267,5 +267,6 @@ MAX_BTF_TRACING_TYPE,
> > > extern u32 btf_tracing_ids[];
> > > extern u32 bpf_cgroup_btf_id[];
> > > extern u32 bpf_local_storage_map_btf_id[];
> > > +extern u32 btf_bpf_map_id[];
> > >
> > > #endif
> > > diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
> > > index d06d3b7150e5..b67996147895 100644
> > > --- a/kernel/bpf/map_iter.c
> > > +++ b/kernel/bpf/map_iter.c
> > > @@ -78,8 +78,7 @@ static const struct seq_operations bpf_map_seq_ops = {
> > > .show = bpf_map_seq_show,
> > > };
> > >
> > > -BTF_ID_LIST(btf_bpf_map_id)
> > > -BTF_ID(struct, bpf_map)
> > > +BTF_ID_LIST_GLOBAL_SINGLE(btf_bpf_map_id, struct, bpf_map)
> > >
> > > static const struct bpf_iter_seq_info bpf_map_seq_info = {
> > > .seq_ops = &bpf_map_seq_ops,
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 0b9da95331d7..5663f97ef292 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -5419,6 +5419,9 @@ static bool is_trusted_reg(const struct bpf_reg_state *reg)
> > > if (reg->ref_obj_id)
> > > return true;
> > >
> > > + if (reg->type == CONST_PTR_TO_MAP)
> > > + return true;
> > > +
> >
> > Overall it looks great.
> > Instead of above, how about the following instead:
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 0b9da95331d7..cd08167dc347 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -10775,7 +10775,7 @@ static int check_kfunc_args(struct
> > bpf_verifier_env *env, struct bpf_kfunc_call_
> > if (!is_kfunc_trusted_args(meta) && !is_kfunc_rcu(meta))
> > break;
> >
> > - if (!is_trusted_reg(reg)) {
> > + if (!is_trusted_reg(reg) &&
> > !reg2btf_ids[base_type(reg->type)]) {
> >
> >
> > This way we won't need to list every convertible type in is_trusted_reg.
> >
> > I'm a bit hesitant to put reg2btf_ids[] check directly into is_trusted_reg().
> > Maybe it's ok, but it needs more analysis.
>
> I am not sure I see a difference in adding a check you proposed above and
> adding the reg2btf_ids[] check directly into the is_trusted_reg() function.
> Basically, we say "if type is in reg2btf_ids[], then consider it trusted" in
> both cases. AFAIS, currently the reg2btf_ids[] contains only trusted types,
> however, could it happen that we add a non-trusted type there?
>
> So, I would leave the patch as is (which also makes sense because the
> const-ptr-to-map is a special case), or add the "reg2btf_ids[] check"
> directly into the is_trusted_reg() function.
Fair enough. Let's add reg2btf_ids[] to is_trusted_reg() directly.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 1/3] bpf: consider CONST_PTR_TO_MAP as trusted pointer to struct bpf_map
2023-07-19 0:54 ` Alexei Starovoitov
@ 2023-07-19 7:10 ` Anton Protopopov
0 siblings, 0 replies; 8+ messages in thread
From: Anton Protopopov @ 2023-07-19 7:10 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Martin KaFai Lau, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, Hou Tao, Joe Stringer, bpf, LKML,
open list:KERNEL SELFTEST FRAMEWORK
On Tue, Jul 18, 2023 at 05:54:27PM -0700, Alexei Starovoitov wrote:
> On Sun, Jul 16, 2023 at 12:49 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> >
> > On Fri, Jul 14, 2023 at 10:56:00AM -0700, Alexei Starovoitov wrote:
> > > On Fri, Jul 14, 2023 at 7:20 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> > > >
> > > > Patch verifier to regard values of type CONST_PTR_TO_MAP as trusted
> > > > pointers to struct bpf_map. This allows kfuncs to work with `struct
> > > > bpf_map *` arguments.
> > > >
> > > > Save some bytes by defining btf_bpf_map_id as BTF_ID_LIST_GLOBAL_SINGLE
> > > > (which is u32[1]), not as BTF_ID_LIST (which is u32[64]).
> > > >
> > > > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > > > ---
> > > > include/linux/btf_ids.h | 1 +
> > > > kernel/bpf/map_iter.c | 3 +--
> > > > kernel/bpf/verifier.c | 5 ++++-
> > > > 3 files changed, 6 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> > > > index 00950cc03bff..a3462a9b8e18 100644
> > > > --- a/include/linux/btf_ids.h
> > > > +++ b/include/linux/btf_ids.h
> > > > @@ -267,5 +267,6 @@ MAX_BTF_TRACING_TYPE,
> > > > extern u32 btf_tracing_ids[];
> > > > extern u32 bpf_cgroup_btf_id[];
> > > > extern u32 bpf_local_storage_map_btf_id[];
> > > > +extern u32 btf_bpf_map_id[];
> > > >
> > > > #endif
> > > > diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
> > > > index d06d3b7150e5..b67996147895 100644
> > > > --- a/kernel/bpf/map_iter.c
> > > > +++ b/kernel/bpf/map_iter.c
> > > > @@ -78,8 +78,7 @@ static const struct seq_operations bpf_map_seq_ops = {
> > > > .show = bpf_map_seq_show,
> > > > };
> > > >
> > > > -BTF_ID_LIST(btf_bpf_map_id)
> > > > -BTF_ID(struct, bpf_map)
> > > > +BTF_ID_LIST_GLOBAL_SINGLE(btf_bpf_map_id, struct, bpf_map)
> > > >
> > > > static const struct bpf_iter_seq_info bpf_map_seq_info = {
> > > > .seq_ops = &bpf_map_seq_ops,
> > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > > index 0b9da95331d7..5663f97ef292 100644
> > > > --- a/kernel/bpf/verifier.c
> > > > +++ b/kernel/bpf/verifier.c
> > > > @@ -5419,6 +5419,9 @@ static bool is_trusted_reg(const struct bpf_reg_state *reg)
> > > > if (reg->ref_obj_id)
> > > > return true;
> > > >
> > > > + if (reg->type == CONST_PTR_TO_MAP)
> > > > + return true;
> > > > +
> > >
> > > Overall it looks great.
> > > Instead of above, how about the following instead:
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 0b9da95331d7..cd08167dc347 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -10775,7 +10775,7 @@ static int check_kfunc_args(struct
> > > bpf_verifier_env *env, struct bpf_kfunc_call_
> > > if (!is_kfunc_trusted_args(meta) && !is_kfunc_rcu(meta))
> > > break;
> > >
> > > - if (!is_trusted_reg(reg)) {
> > > + if (!is_trusted_reg(reg) &&
> > > !reg2btf_ids[base_type(reg->type)]) {
> > >
> > >
> > > This way we won't need to list every convertible type in is_trusted_reg.
> > >
> > > I'm a bit hesitant to put reg2btf_ids[] check directly into is_trusted_reg().
> > > Maybe it's ok, but it needs more analysis.
> >
> > I am not sure I see a difference in adding a check you proposed above and
> > adding the reg2btf_ids[] check directly into the is_trusted_reg() function.
> > Basically, we say "if type is in reg2btf_ids[], then consider it trusted" in
> > both cases. AFAIS, currently the reg2btf_ids[] contains only trusted types,
> > however, could it happen that we add a non-trusted type there?
> >
> > So, I would leave the patch as is (which also makes sense because the
> > const-ptr-to-map is a special case), or add the "reg2btf_ids[] check"
> > directly into the is_trusted_reg() function.
>
> Fair enough. Let's add reg2btf_ids[] to is_trusted_reg() directly.
Thanks, I will send v2.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-07-19 7:09 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20230714141747.41560-1-aspsk@isovalent.com>
2023-07-14 14:20 ` [PATCH bpf-next 0/3] allow bpf_map_sum_elem_count for all program types Anton Protopopov
2023-07-14 14:20 ` [PATCH bpf-next 1/3] bpf: consider CONST_PTR_TO_MAP as trusted pointer to struct bpf_map Anton Protopopov
2023-07-14 17:56 ` Alexei Starovoitov
2023-07-16 7:50 ` Anton Protopopov
2023-07-19 0:54 ` Alexei Starovoitov
2023-07-19 7:10 ` Anton Protopopov
2023-07-14 14:20 ` [PATCH bpf-next 2/3] bpf: make an argument const in the bpf_map_sum_elem_count kfunc Anton Protopopov
2023-07-14 14:21 ` [PATCH bpf-next 3/3] bpf: allow any program to use " Anton Protopopov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).