From: Aditi Ghag <aditi.ghag@isovalent.com>
To: Martin KaFai Lau <martin.lau@linux.dev>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
Andrii Nakryiko <andrii@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
David Vernet <void@manifault.com>,
kernel-team@meta.com
Subject: Re: [RFC PATCH bpf-next] bpf: Add a kfunc filter function to 'struct btf_kfunc_id_set'.
Date: Wed, 12 Apr 2023 08:21:03 -0700 [thread overview]
Message-ID: <898991B6-2611-468B-B2DA-F56D548A3BAF@isovalent.com> (raw)
In-Reply-To: <1ECC8AAA-C2E6-4F8A-B7D3-5E90BDEE7C48@isovalent.com>
> On Apr 10, 2023, at 4:05 PM, Aditi Ghag <aditi.ghag@isovalent.com> wrote:
>
>
>
>> On Apr 3, 2023, at 11:09 PM, Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> From: Martin KaFai Lau <martin.lau@kernel.org>
>>
>> This set (https://lore.kernel.org/bpf/https://lore.kernel.org/bpf/500d452b-f9d5-d01f-d365-2949c4fd37ab@linux.dev/)
>> needs to limit bpf_sock_destroy kfunc to BPF_TRACE_ITER.
>> In the earlier reply, I thought of adding a BTF_KFUNC_HOOK_TRACING_ITER.
>>
>> Instead of adding BTF_KFUNC_HOOK_TRACING_ITER, I quickly hacked something
>> that added a callback filter to 'struct btf_kfunc_id_set'. The filter has
>> access to the prog such that it can filter by other properties of a prog.
>> The prog->expected_attached_type is used in the tracing_iter_filter().
>> It is mostly compiler tested only, so it is still very rough but should
>> be good enough to show the idea.
>>
>> would like to hear how others think. It is pretty much the only
>> piece left for the above mentioned set.
>>
>> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
>> ---
>> include/linux/btf.h | 18 +++---
>> kernel/bpf/btf.c | 59 +++++++++++++++----
>> kernel/bpf/verifier.c | 7 ++-
>> net/core/filter.c | 9 +++
>> .../selftests/bpf/progs/sock_destroy_prog.c | 10 ++++
>> 5 files changed, 83 insertions(+), 20 deletions(-)
>>
>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>> index d53b10cc55f2..84c31b4f5785 100644
>> --- a/include/linux/btf.h
>> +++ b/include/linux/btf.h
>> @@ -99,10 +99,14 @@ struct btf_type;
>> union bpf_attr;
>> struct btf_show;
>> struct btf_id_set;
>> +struct bpf_prog;
>> +
>> +typedef int (*btf_kfunc_filter_t)(const struct bpf_prog *prog, u32 kfunc_id);
>>
>> struct btf_kfunc_id_set {
>> struct module *owner;
>> struct btf_id_set8 *set;
>> + btf_kfunc_filter_t filter;
>> };
>>
>> struct btf_id_dtor_kfunc {
>> @@ -482,7 +486,6 @@ static inline void *btf_id_set8_contains(const struct btf_id_set8 *set, u32 id)
>> return bsearch(&id, set->pairs, set->cnt, sizeof(set->pairs[0]), btf_id_cmp_func);
>> }
>>
>> -struct bpf_prog;
>> struct bpf_verifier_log;
>>
>> #ifdef CONFIG_BPF_SYSCALL
>> @@ -490,10 +493,10 @@ const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id);
>> const char *btf_name_by_offset(const struct btf *btf, u32 offset);
>> struct btf *btf_parse_vmlinux(void);
>> struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
>> -u32 *btf_kfunc_id_set_contains(const struct btf *btf,
>> - enum bpf_prog_type prog_type,
>> - u32 kfunc_btf_id);
>> -u32 *btf_kfunc_is_modify_return(const struct btf *btf, u32 kfunc_btf_id);
>> +u32 *btf_kfunc_id_set_contains(const struct btf *btf, u32 kfunc_btf_id,
>> + const struct bpf_prog *prog);
>> +u32 *btf_kfunc_is_modify_return(const struct btf *btf, u32 kfunc_btf_id,
>> + const struct bpf_prog *prog);
>> int register_btf_kfunc_id_set(enum bpf_prog_type prog_type,
>> const struct btf_kfunc_id_set *s);
>> int register_btf_fmodret_id_set(const struct btf_kfunc_id_set *kset);
>> @@ -520,8 +523,9 @@ static inline const char *btf_name_by_offset(const struct btf *btf,
>> return NULL;
>> }
>> static inline u32 *btf_kfunc_id_set_contains(const struct btf *btf,
>> - enum bpf_prog_type prog_type,
>> - u32 kfunc_btf_id)
>> + u32 kfunc_btf_id,
>> + struct bpf_prog *prog)
>> +
>> {
>> return NULL;
>> }
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index b7e5a5510b91..7685af3ca9c0 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -218,10 +218,17 @@ enum btf_kfunc_hook {
>> enum {
>> BTF_KFUNC_SET_MAX_CNT = 256,
>> BTF_DTOR_KFUNC_MAX_CNT = 256,
>> + BTF_KFUNC_FILTER_MAX_CNT = 16,
>> +};
>> +
>> +struct btf_kfunc_hook_filter {
>> + btf_kfunc_filter_t filters[BTF_KFUNC_FILTER_MAX_CNT];
>> + u32 nr_filters;
>> };
>>
>> struct btf_kfunc_set_tab {
>> struct btf_id_set8 *sets[BTF_KFUNC_HOOK_MAX];
>> + struct btf_kfunc_hook_filter hook_filters[BTF_KFUNC_HOOK_MAX];
>> };
>>
>> struct btf_id_dtor_kfunc_tab {
>> @@ -7712,9 +7719,12 @@ static int btf_check_kfunc_protos(struct btf *btf, u32 func_id, u32 func_flags)
>> /* Kernel Function (kfunc) BTF ID set registration API */
>>
>> static int btf_populate_kfunc_set(struct btf *btf, enum btf_kfunc_hook hook,
>> - struct btf_id_set8 *add_set)
>> + const struct btf_kfunc_id_set *kset)
>> {
>> + struct btf_kfunc_hook_filter *hook_filter;
>> + struct btf_id_set8 *add_set = kset->set;
>> bool vmlinux_set = !btf_is_module(btf);
>> + bool add_filter = !!kset->filter;
>> struct btf_kfunc_set_tab *tab;
>> struct btf_id_set8 *set;
>> u32 set_cnt;
>> @@ -7729,6 +7739,20 @@ static int btf_populate_kfunc_set(struct btf *btf, enum btf_kfunc_hook hook,
>> return 0;
>>
>> tab = btf->kfunc_set_tab;
>> +
>> + if (tab && add_filter) {
>> + int i;
>> +
>> + hook_filter = &tab->hook_filters[hook];
>> + for (i = 0; i < hook_filter->nr_filters; i++) {
>> + if (hook_filter->filters[i] == kset->filter)
>> + add_filter = false;
>> + }
>> +
>> + if (add_filter && hook_filter->nr_filters == BTF_KFUNC_FILTER_MAX_CNT)
>> + return -E2BIG;
>> + }
>> +
>> if (!tab) {
>> tab = kzalloc(sizeof(*tab), GFP_KERNEL | __GFP_NOWARN);
>> if (!tab)
>> @@ -7751,7 +7775,7 @@ static int btf_populate_kfunc_set(struct btf *btf, enum btf_kfunc_hook hook,
>> */
>> if (!vmlinux_set) {
>> tab->sets[hook] = add_set;
>> - return 0;
>> + goto do_add_filter;
>> }
>>
>> /* In case of vmlinux sets, there may be more than one set being
>> @@ -7793,6 +7817,11 @@ static int btf_populate_kfunc_set(struct btf *btf, enum btf_kfunc_hook hook,
>>
>> sort(set->pairs, set->cnt, sizeof(set->pairs[0]), btf_id_cmp_func, NULL);
>>
>> +do_add_filter:
>> + if (add_filter) {
>> + hook_filter = &tab->hook_filters[hook];
>> + hook_filter->filters[hook_filter->nr_filters++] = kset->filter;
>> + }
>> return 0;
>> end:
>> btf_free_kfunc_set_tab(btf);
>> @@ -7801,15 +7830,22 @@ static int btf_populate_kfunc_set(struct btf *btf, enum btf_kfunc_hook hook,
>>
>> static u32 *__btf_kfunc_id_set_contains(const struct btf *btf,
>> enum btf_kfunc_hook hook,
>> + const struct bpf_prog *prog,
>> u32 kfunc_btf_id)
>> {
>> + struct btf_kfunc_hook_filter *hook_filter;
>> struct btf_id_set8 *set;
>> - u32 *id;
>> + u32 *id, i;
>>
>> if (hook >= BTF_KFUNC_HOOK_MAX)
>> return NULL;
>> if (!btf->kfunc_set_tab)
>> return NULL;
>> + hook_filter = &btf->kfunc_set_tab->hook_filters[hook];
>> + for (i = 0; i < hook_filter->nr_filters; i++) {
>> + if (hook_filter->filters[i](prog, kfunc_btf_id))
>> + return NULL;
>> + }
>> set = btf->kfunc_set_tab->sets[hook];
>> if (!set)
>> return NULL;
>> @@ -7862,23 +7898,25 @@ static int bpf_prog_type_to_kfunc_hook(enum bpf_prog_type prog_type)
>> * protection for looking up a well-formed btf->kfunc_set_tab.
>> */
>> u32 *btf_kfunc_id_set_contains(const struct btf *btf,
>> - enum bpf_prog_type prog_type,
>> - u32 kfunc_btf_id)
>> + u32 kfunc_btf_id,
>> + const struct bpf_prog *prog)
>> {
>> + enum bpf_prog_type prog_type = resolve_prog_type(prog);
>> enum btf_kfunc_hook hook;
>> u32 *kfunc_flags;
>>
>> - kfunc_flags = __btf_kfunc_id_set_contains(btf, BTF_KFUNC_HOOK_COMMON, kfunc_btf_id);
>> + kfunc_flags = __btf_kfunc_id_set_contains(btf, BTF_KFUNC_HOOK_COMMON, prog, kfunc_btf_id);
>> if (kfunc_flags)
>> return kfunc_flags;
>>
>> hook = bpf_prog_type_to_kfunc_hook(prog_type);
>> - return __btf_kfunc_id_set_contains(btf, hook, kfunc_btf_id);
>> + return __btf_kfunc_id_set_contains(btf, hook, prog, kfunc_btf_id);
>> }
>>
>> -u32 *btf_kfunc_is_modify_return(const struct btf *btf, u32 kfunc_btf_id)
>> +u32 *btf_kfunc_is_modify_return(const struct btf *btf, u32 kfunc_btf_id,
>> + const struct bpf_prog *prog)
>> {
>> - return __btf_kfunc_id_set_contains(btf, BTF_KFUNC_HOOK_FMODRET, kfunc_btf_id);
>> + return __btf_kfunc_id_set_contains(btf, BTF_KFUNC_HOOK_FMODRET, prog, kfunc_btf_id);
>> }
>>
>> static int __register_btf_kfunc_id_set(enum btf_kfunc_hook hook,
>> @@ -7909,7 +7947,8 @@ static int __register_btf_kfunc_id_set(enum btf_kfunc_hook hook,
>> goto err_out;
>> }
>>
>> - ret = btf_populate_kfunc_set(btf, hook, kset->set);
>> + ret = btf_populate_kfunc_set(btf, hook, kset);
>> +
>> err_out:
>> btf_put(btf);
>> return ret;
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 20eb2015842f..1a854cdb2566 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -10553,7 +10553,7 @@ static int fetch_kfunc_meta(struct bpf_verifier_env *env,
>> *kfunc_name = func_name;
>> func_proto = btf_type_by_id(desc_btf, func->type);
>>
>> - kfunc_flags = btf_kfunc_id_set_contains(desc_btf, resolve_prog_type(env->prog), func_id);
>> + kfunc_flags = btf_kfunc_id_set_contains(desc_btf, func_id, env->prog);
>> if (!kfunc_flags) {
>> return -EACCES;
>> }
>> @@ -18521,7 +18521,8 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>> * in the fmodret id set with the KF_SLEEPABLE flag.
>> */
>> else {
>> - u32 *flags = btf_kfunc_is_modify_return(btf, btf_id);
>> + u32 *flags = btf_kfunc_is_modify_return(btf, btf_id,
>> + prog);
>>
>> if (flags && (*flags & KF_SLEEPABLE))
>> ret = 0;
>> @@ -18549,7 +18550,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>> return -EINVAL;
>> }
>> ret = -EINVAL;
>> - if (btf_kfunc_is_modify_return(btf, btf_id) ||
>> + if (btf_kfunc_is_modify_return(btf, btf_id, prog) ||
>> !check_attach_modify_return(addr, tname))
>> ret = 0;
>> if (ret) {
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index a70c7b9876fa..5e5e6f9baccc 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -11768,9 +11768,18 @@ BTF_SET8_START(sock_destroy_kfunc_set)
>> BTF_ID_FLAGS(func, bpf_sock_destroy)
>> BTF_SET8_END(sock_destroy_kfunc_set)
>>
>> +static int tracing_iter_filter(const struct bpf_prog *prog, u32 kfunc_id)
>> +{
>> + if (btf_id_set8_contains(&sock_destroy_kfunc_set, kfunc_id) &&
>> + prog->expected_attach_type != BPF_TRACE_ITER)
>> + return -EACCES;
>> + return 0;
>> +}
>> +
>> static const struct btf_kfunc_id_set bpf_sock_destroy_kfunc_set = {
>> .owner = THIS_MODULE,
>> .set = &sock_destroy_kfunc_set,
>> + .filter = tracing_iter_filter,
>> };
>>
>> static int init_subsystem(void)
>> diff --git a/tools/testing/selftests/bpf/progs/sock_destroy_prog.c b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
>> index 5c1e65d50598..5204e44f6ae4 100644
>> --- a/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
>> +++ b/tools/testing/selftests/bpf/progs/sock_destroy_prog.c
>> @@ -3,6 +3,7 @@
>> #include "vmlinux.h"
>> #include <bpf/bpf_helpers.h>
>> #include <bpf/bpf_endian.h>
>> +#include <bpf/bpf_tracing.h>
>>
>> #include "bpf_tracing_net.h"
>>
>> @@ -144,4 +145,13 @@ int iter_udp6_server(struct bpf_iter__udp *ctx)
>> return 0;
>> }
>>
>> +SEC("tp_btf/tcp_destroy_sock")
>> +int BPF_PROG(trace_tcp_destroy_sock, struct sock *sk)
>> +{
>> + /* should not load */
>> + bpf_sock_destroy((struct sock_common *)sk);
>> +
>> + return 0;
>> +}
>
> Applied, and tested the patch locally. It works as expected: tp_btf/tcp_destroy_sock is rejected, while the other programs in sock_destroy_prog load successfully.
>
> 1: (85) call bpf_sock_destroy#75448
> calling kernel function bpf_sock_destroy is not allowed
Martin: I'm ready to push the next revision that addresses all the review comments from the current patch. How do you want to coordinate it with this commit?
>
>
>> +
>> char _license[] SEC("license") = "GPL";
>> --
>> 2.34.1
next prev parent reply other threads:[~2023-04-12 15:21 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-30 15:17 [PATCH v5 bpf-next 0/7] bpf: Add socket destroy capability Aditi Ghag
2023-03-30 15:17 ` [PATCH v5 bpf-next 1/7] bpf: tcp: Avoid taking fast sock lock in iterator Aditi Ghag
2023-03-30 15:17 ` [PATCH v5 bpf-next 2/7] udp: seq_file: Remove bpf_seq_afinfo from udp_iter_state Aditi Ghag
2023-03-30 17:35 ` Martin KaFai Lau
2023-03-30 15:17 ` [PATCH v5 bpf-next 3/7] udp: seq_file: Helper function to match socket attributes Aditi Ghag
2023-03-30 18:40 ` kernel test robot
2023-03-30 18:51 ` kernel test robot
2023-03-31 2:52 ` kernel test robot
2023-03-31 20:09 ` Martin KaFai Lau
2023-04-03 15:27 ` Aditi Ghag
2023-04-02 6:18 ` kernel test robot
2023-03-30 15:17 ` [PATCH v5 bpf-next 4/7] bpf: udp: Implement batching for sockets iterator Aditi Ghag
2023-03-31 21:08 ` Martin KaFai Lau
2023-04-03 15:54 ` Aditi Ghag
2023-04-03 19:20 ` Martin KaFai Lau
2023-03-30 15:17 ` [PATCH v5 bpf-next 5/7] bpf: Add bpf_sock_destroy kfunc Aditi Ghag
2023-03-31 22:24 ` Martin KaFai Lau
2023-04-04 6:09 ` [RFC PATCH bpf-next] bpf: Add a kfunc filter function to 'struct btf_kfunc_id_set' Martin KaFai Lau
2023-04-05 15:05 ` Aditi Ghag
2023-04-05 17:26 ` Martin KaFai Lau
2023-04-10 23:05 ` Aditi Ghag
2023-04-12 15:21 ` Aditi Ghag [this message]
2023-03-30 15:17 ` [PATCH v5 bpf-next 6/7] selftests/bpf: Add helper to get port using getsockname Aditi Ghag
2023-03-30 18:41 ` Stanislav Fomichev
2023-03-31 21:37 ` Martin KaFai Lau
2023-03-30 15:17 ` [PATCH v5 bpf-next 7/7] selftests/bpf: Test bpf_sock_destroy Aditi Ghag
2023-03-30 18:46 ` Stanislav Fomichev
2023-03-31 22:32 ` Martin KaFai Lau
2023-04-03 15:55 ` Aditi Ghag
2023-04-03 17:35 ` Martin KaFai Lau
2023-04-04 0:15 ` Aditi Ghag
2023-04-04 1:41 ` Martin KaFai Lau
2023-04-04 14:24 ` Aditi Ghag
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=898991B6-2611-468B-B2DA-F56D548A3BAF@isovalent.com \
--to=aditi.ghag@isovalent.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@meta.com \
--cc=martin.lau@linux.dev \
--cc=void@manifault.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).