| Message ID | 20231105133458.1315620-2-zhouchuyi@bytedance.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Let BPF verifier consider {task,cgroup} is trusted in bpf_iter_reg | expand |
On 11/5/23 5:34 AM, Chuyi Zhou wrote: > BTF_TYPE_SAFE_TRUSTED(struct bpf_iter__task) in verifier.c wanted to > teach BPF verifier that bpf_iter__task -> task is a trusted ptr. But it > doesn't work well. > > The reason is, bpf_iter__task -> task would go through btf_ctx_access() > which enforces the reg_type of 'task' is ctx_arg_info->reg_type, and in > task_iter.c, we actually explicitly declare that the > ctx_arg_info->reg_type is PTR_TO_BTF_ID_OR_NULL. > > This patch sets ctx_arg_info->reg_type is PTR_TO_BTF_ID_OR_NULL | > PTR_TRUSTED in task_reg_info. Actually we have a previous case like this. See https://lore.kernel.org/all/20230706133932.45883-3-aspsk@isovalent.com/ where PTR_TRUSTED is added to the arg flag for map_iter. You could mention this case in your commit message. > > Similarly, bpf_cgroup_reg_info -> cgroup is also PTR_TRUSTED since we are > under the protection of cgroup_mutex and we would check cgroup_is_dead() > in __cgroup_iter_seq_show(). > > Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com> Acked-by: Yonghong Song <yonghong.song@linux.dev> > --- > kernel/bpf/cgroup_iter.c | 2 +- > kernel/bpf/task_iter.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/bpf/cgroup_iter.c b/kernel/bpf/cgroup_iter.c > index d1b5c5618..f04a468cf 100644 > --- a/kernel/bpf/cgroup_iter.c > +++ b/kernel/bpf/cgroup_iter.c > @@ -282,7 +282,7 @@ static struct bpf_iter_reg bpf_cgroup_reg_info = { > .ctx_arg_info_size = 1, > .ctx_arg_info = { > { offsetof(struct bpf_iter__cgroup, cgroup), > - PTR_TO_BTF_ID_OR_NULL }, > + PTR_TO_BTF_ID_OR_NULL | PTR_TRUSTED }, > }, > .seq_info = &cgroup_iter_seq_info, > }; > diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c > index 4e156dca4..26082b978 100644 > --- a/kernel/bpf/task_iter.c > +++ b/kernel/bpf/task_iter.c > @@ -704,7 +704,7 @@ static struct bpf_iter_reg task_reg_info = { > .ctx_arg_info_size = 1, > .ctx_arg_info = { > { offsetof(struct bpf_iter__task, task), > - PTR_TO_BTF_ID_OR_NULL }, > + PTR_TO_BTF_ID_OR_NULL | PTR_TRUSTED }, > }, > .seq_info = &task_seq_info, > .fill_link_info = bpf_iter_fill_link_info,
On 11/5/23 5:34 AM, Chuyi Zhou wrote: > BTF_TYPE_SAFE_TRUSTED(struct bpf_iter__task) in verifier.c wanted to > teach BPF verifier that bpf_iter__task -> task is a trusted ptr. But it > doesn't work well. > > The reason is, bpf_iter__task -> task would go through btf_ctx_access() > which enforces the reg_type of 'task' is ctx_arg_info->reg_type, and in > task_iter.c, we actually explicitly declare that the > ctx_arg_info->reg_type is PTR_TO_BTF_ID_OR_NULL. > > This patch sets ctx_arg_info->reg_type is PTR_TO_BTF_ID_OR_NULL | > PTR_TRUSTED in task_reg_info. > > Similarly, bpf_cgroup_reg_info -> cgroup is also PTR_TRUSTED since we are > under the protection of cgroup_mutex and we would check cgroup_is_dead() > in __cgroup_iter_seq_show(). > Make sense. I think the bpf_tcp_iter made similar change in tcp_seq_info also. What may be the Fixes tag? Is it fixing the recent kfunc of the css_task iter?
Hello, 在 2023/11/7 02:26, Yonghong Song 写道: > > On 11/5/23 5:34 AM, Chuyi Zhou wrote: >> BTF_TYPE_SAFE_TRUSTED(struct bpf_iter__task) in verifier.c wanted to >> teach BPF verifier that bpf_iter__task -> task is a trusted ptr. But it >> doesn't work well. >> >> The reason is, bpf_iter__task -> task would go through btf_ctx_access() >> which enforces the reg_type of 'task' is ctx_arg_info->reg_type, and in >> task_iter.c, we actually explicitly declare that the >> ctx_arg_info->reg_type is PTR_TO_BTF_ID_OR_NULL. >> >> This patch sets ctx_arg_info->reg_type is PTR_TO_BTF_ID_OR_NULL | >> PTR_TRUSTED in task_reg_info. > > Actually we have a previous case like this. See > > https://lore.kernel.org/all/20230706133932.45883-3-aspsk@isovalent.com/ > > where PTR_TRUSTED is added to the arg flag for map_iter. > > You could mention this case in your commit message. > Thanks, will do it in next version.
Hello, 在 2023/11/7 02:29, Martin KaFai Lau 写道: > On 11/5/23 5:34 AM, Chuyi Zhou wrote: >> BTF_TYPE_SAFE_TRUSTED(struct bpf_iter__task) in verifier.c wanted to >> teach BPF verifier that bpf_iter__task -> task is a trusted ptr. But it >> doesn't work well. >> >> The reason is, bpf_iter__task -> task would go through btf_ctx_access() >> which enforces the reg_type of 'task' is ctx_arg_info->reg_type, and in >> task_iter.c, we actually explicitly declare that the >> ctx_arg_info->reg_type is PTR_TO_BTF_ID_OR_NULL. >> >> This patch sets ctx_arg_info->reg_type is PTR_TO_BTF_ID_OR_NULL | >> PTR_TRUSTED in task_reg_info. >> >> Similarly, bpf_cgroup_reg_info -> cgroup is also PTR_TRUSTED since we are >> under the protection of cgroup_mutex and we would check cgroup_is_dead() >> in __cgroup_iter_seq_show(). >> > > Make sense. I think the bpf_tcp_iter made similar change in tcp_seq_info > also. What may be the Fixes tag? Is it fixing the recent kfunc of the > css_task iter? > Thanks for the review. I think it's not a fix for recent kfunc of css_task iter. We are working at SEC("iter/task") and SEC("iter/cgroup"). I'm not sure whether it's a 'fix' for cgroup_iter/task_iter. If we need fix tags, do we need to split this patch into two separate patches? Or add two fix tags on commit log: Fixes: d4ccaf58a84721 ("bpf: Introduce cgroup iter") Fixes: 3c32cc1bceba8a17 ("bpf: Enable bpf_iter targets registering ctx argument types") Thanks.
On 11/6/23 6:44 PM, Chuyi Zhou wrote: > Hello, > > 在 2023/11/7 02:29, Martin KaFai Lau 写道: >> On 11/5/23 5:34 AM, Chuyi Zhou wrote: >>> BTF_TYPE_SAFE_TRUSTED(struct bpf_iter__task) in verifier.c wanted to >>> teach BPF verifier that bpf_iter__task -> task is a trusted ptr. But it >>> doesn't work well. >>> >>> The reason is, bpf_iter__task -> task would go through btf_ctx_access() >>> which enforces the reg_type of 'task' is ctx_arg_info->reg_type, and in >>> task_iter.c, we actually explicitly declare that the >>> ctx_arg_info->reg_type is PTR_TO_BTF_ID_OR_NULL. >>> >>> This patch sets ctx_arg_info->reg_type is PTR_TO_BTF_ID_OR_NULL | >>> PTR_TRUSTED in task_reg_info. >>> >>> Similarly, bpf_cgroup_reg_info -> cgroup is also PTR_TRUSTED since >>> we are >>> under the protection of cgroup_mutex and we would check >>> cgroup_is_dead() >>> in __cgroup_iter_seq_show(). >>> >> >> Make sense. I think the bpf_tcp_iter made similar change in >> tcp_seq_info also. What may be the Fixes tag? Is it fixing the recent >> kfunc of the css_task iter? >> > > Thanks for the review. > > I think it's not a fix for recent kfunc of css_task iter. We are > working at SEC("iter/task") and SEC("iter/cgroup"). > > I'm not sure whether it's a 'fix' for cgroup_iter/task_iter. If we > need fix tags, do we need to split this patch into two separate > patches? Or add two fix tags on commit log: I think the patch itself is not a fix, rather an improvement. The bpf_iter predates kfunc/PTR_TRUSTED stuff. The argument 'task' or 'cgroup' are already trusted so the bpf_iter program can print out useful data. But recent kfunc things requires some parameters to be marked as PTR_TRUSTED so that they can be passed to kfunc, so this patch enables this usage for kfunc in bpf_iter programs. > > Fixes: d4ccaf58a84721 ("bpf: Introduce cgroup iter") > Fixes: 3c32cc1bceba8a17 ("bpf: Enable bpf_iter targets registering ctx > argument types") > > Thanks. > > >
在 2023/11/7 14:52, Yonghong Song 写道: > > On 11/6/23 6:44 PM, Chuyi Zhou wrote: >> Hello, >> >> 在 2023/11/7 02:29, Martin KaFai Lau 写道: >>> On 11/5/23 5:34 AM, Chuyi Zhou wrote: >>>> BTF_TYPE_SAFE_TRUSTED(struct bpf_iter__task) in verifier.c wanted to >>>> teach BPF verifier that bpf_iter__task -> task is a trusted ptr. But it >>>> doesn't work well. >>>> >>>> The reason is, bpf_iter__task -> task would go through btf_ctx_access() >>>> which enforces the reg_type of 'task' is ctx_arg_info->reg_type, and in >>>> task_iter.c, we actually explicitly declare that the >>>> ctx_arg_info->reg_type is PTR_TO_BTF_ID_OR_NULL. >>>> >>>> This patch sets ctx_arg_info->reg_type is PTR_TO_BTF_ID_OR_NULL | >>>> PTR_TRUSTED in task_reg_info. >>>> >>>> Similarly, bpf_cgroup_reg_info -> cgroup is also PTR_TRUSTED since >>>> we are >>>> under the protection of cgroup_mutex and we would check >>>> cgroup_is_dead() >>>> in __cgroup_iter_seq_show(). >>>> >>> >>> Make sense. I think the bpf_tcp_iter made similar change in >>> tcp_seq_info also. What may be the Fixes tag? Is it fixing the recent >>> kfunc of the css_task iter? >>> >> >> Thanks for the review. >> >> I think it's not a fix for recent kfunc of css_task iter. We are >> working at SEC("iter/task") and SEC("iter/cgroup"). >> >> I'm not sure whether it's a 'fix' for cgroup_iter/task_iter. If we >> need fix tags, do we need to split this patch into two separate >> patches? Or add two fix tags on commit log: > > I think the patch itself is not a fix, rather an improvement. The > bpf_iter predates kfunc/PTR_TRUSTED stuff. The argument 'task' > or 'cgroup' are already trusted so the bpf_iter program can print out > useful data. > But recent kfunc things requires some parameters to be marked as > PTR_TRUSTED so that they can be passed to kfunc, > so this patch enables this usage for kfunc in bpf_iter programs. > > Thanks. I will send v2.
diff --git a/kernel/bpf/cgroup_iter.c b/kernel/bpf/cgroup_iter.c index d1b5c5618..f04a468cf 100644 --- a/kernel/bpf/cgroup_iter.c +++ b/kernel/bpf/cgroup_iter.c @@ -282,7 +282,7 @@ static struct bpf_iter_reg bpf_cgroup_reg_info = { .ctx_arg_info_size = 1, .ctx_arg_info = { { offsetof(struct bpf_iter__cgroup, cgroup), - PTR_TO_BTF_ID_OR_NULL }, + PTR_TO_BTF_ID_OR_NULL | PTR_TRUSTED }, }, .seq_info = &cgroup_iter_seq_info, }; diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c index 4e156dca4..26082b978 100644 --- a/kernel/bpf/task_iter.c +++ b/kernel/bpf/task_iter.c @@ -704,7 +704,7 @@ static struct bpf_iter_reg task_reg_info = { .ctx_arg_info_size = 1, .ctx_arg_info = { { offsetof(struct bpf_iter__task, task), - PTR_TO_BTF_ID_OR_NULL }, + PTR_TO_BTF_ID_OR_NULL | PTR_TRUSTED }, }, .seq_info = &task_seq_info, .fill_link_info = bpf_iter_fill_link_info,
BTF_TYPE_SAFE_TRUSTED(struct bpf_iter__task) in verifier.c wanted to teach BPF verifier that bpf_iter__task -> task is a trusted ptr. But it doesn't work well. The reason is, bpf_iter__task -> task would go through btf_ctx_access() which enforces the reg_type of 'task' is ctx_arg_info->reg_type, and in task_iter.c, we actually explicitly declare that the ctx_arg_info->reg_type is PTR_TO_BTF_ID_OR_NULL. This patch sets ctx_arg_info->reg_type is PTR_TO_BTF_ID_OR_NULL | PTR_TRUSTED in task_reg_info. Similarly, bpf_cgroup_reg_info -> cgroup is also PTR_TRUSTED since we are under the protection of cgroup_mutex and we would check cgroup_is_dead() in __cgroup_iter_seq_show(). Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com> --- kernel/bpf/cgroup_iter.c | 2 +- kernel/bpf/task_iter.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)