[PATCH bpf 0/2] Let BPF verifier consider {task,cgroup} is trusted in bpf_iter_reg

[PATCH bpf 0/2] Let BPF verifier consider {task,cgroup} is trusted in bpf_iter_reg

[Chuyi Zhou]

Hi,
The patchset aims to let the BPF verivier consider
bpf_iter__cgroup->cgroup and bpf_iter__task->task is trused suggested by
Alexei[1].

Please see individual patches for more details. And comments are always
welcome.

Link[1]:https://lore.kernel.org/bpf/20231022154527.229117-1-zhouchuyi@bytedance.com/T/#mb57725edc8ccdd50a1b165765c7619b4d65ed1b0

Chuyi Zhou (2):
  bpf: Let verifier consider {task,cgroup} is trusted in bpf_iter_reg
  selftests/bpf: get trusted cgrp from bpf_iter__cgroup directly

 kernel/bpf/cgroup_iter.c                         |  2 +-
 kernel/bpf/task_iter.c                           |  2 +-
 .../testing/selftests/bpf/progs/iters_css_task.c | 16 ++++------------
 3 files changed, 6 insertions(+), 14 deletions(-)

-- 
2.20.1

[PATCH bpf 1/2] bpf: Let verifier consider {task,cgroup} is trusted in bpf_iter_reg

[Chuyi Zhou]

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(-)

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,
-- 
2.20.1

Re: [PATCH bpf 1/2] bpf: Let verifier consider {task,cgroup} is trusted in bpf_iter_reg

[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.

>
> 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,

Re: [PATCH bpf 1/2] bpf: Let verifier consider {task,cgroup} is trusted in bpf_iter_reg

[Chuyi Zhou]

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.

Re: [PATCH bpf 1/2] bpf: Let verifier consider {task,cgroup} is trusted in bpf_iter_reg

[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?

Re: [PATCH bpf 1/2] bpf: Let verifier consider {task,cgroup} is trusted in bpf_iter_reg

[Chuyi Zhou]

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.

Re: [PATCH bpf 1/2] bpf: Let verifier consider {task,cgroup} is trusted in bpf_iter_reg

[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.


>
> Fixes: d4ccaf58a84721 ("bpf: Introduce cgroup iter")
> Fixes: 3c32cc1bceba8a17 ("bpf: Enable bpf_iter targets registering ctx 
> argument types")
>
> Thanks.
>
>
>

Re: [External] Re: [PATCH bpf 1/2] bpf: Let verifier consider {task,cgroup} is trusted in bpf_iter_reg

[Chuyi Zhou]

在 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.

[PATCH bpf 2/2] selftests/bpf: get trusted cgrp from bpf_iter__cgroup directly

[Chuyi Zhou]

Commit f49843afde (selftests/bpf: Add tests for css_task iter combining
with cgroup iter) added a test which demonstrates how css_task iter can be
combined with cgroup iter. That test used bpf_cgroup_from_id() to convert
bpf_iter__cgroup->cgroup to a trusted ptr which is pointless now, since
with the previous fix, we can get a trusted cgroup directly from
bpf_iter__cgroup.

Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com>
---
 .../testing/selftests/bpf/progs/iters_css_task.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/iters_css_task.c b/tools/testing/selftests/bpf/progs/iters_css_task.c
index e180aa1b1..9ac758649 100644
--- a/tools/testing/selftests/bpf/progs/iters_css_task.c
+++ b/tools/testing/selftests/bpf/progs/iters_css_task.c
@@ -56,12 +56,9 @@ SEC("?iter/cgroup")
 int cgroup_id_printer(struct bpf_iter__cgroup *ctx)
 {
 	struct seq_file *seq = ctx->meta->seq;
-	struct cgroup *cgrp, *acquired;
+	struct cgroup *cgrp = ctx->cgroup;
 	struct cgroup_subsys_state *css;
 	struct task_struct *task;
-	u64 cgrp_id;
-
-	cgrp = ctx->cgroup;
 
 	/* epilogue */
 	if (cgrp == NULL) {
@@ -73,20 +70,15 @@ int cgroup_id_printer(struct bpf_iter__cgroup *ctx)
 	if (ctx->meta->seq_num == 0)
 		BPF_SEQ_PRINTF(seq, "prologue\n");
 
-	cgrp_id = cgroup_id(cgrp);
-
-	BPF_SEQ_PRINTF(seq, "%8llu\n", cgrp_id);
+	BPF_SEQ_PRINTF(seq, "%8llu\n", cgroup_id(cgrp));
 
-	acquired = bpf_cgroup_from_id(cgrp_id);
-	if (!acquired)
-		return 0;
-	css = &acquired->self;
+	css = &cgrp->self;
 	css_task_cnt = 0;
 	bpf_for_each(css_task, task, css, CSS_TASK_ITER_PROCS) {
 		if (task->pid == target_pid)
 			css_task_cnt++;
 	}
-	bpf_cgroup_release(acquired);
+
 	return 0;
 }
 
-- 
2.20.1

Re: [PATCH bpf 2/2] selftests/bpf: get trusted cgrp from bpf_iter__cgroup directly

[Yonghong Song]

On 11/5/23 5:34 AM, Chuyi Zhou wrote:
> Commit f49843afde (selftests/bpf: Add tests for css_task iter combining
> with cgroup iter) added a test which demonstrates how css_task iter can be
> combined with cgroup iter. That test used bpf_cgroup_from_id() to convert
> bpf_iter__cgroup->cgroup to a trusted ptr which is pointless now, since
> with the previous fix, we can get a trusted cgroup directly from
> bpf_iter__cgroup.
>
> Signed-off-by: Chuyi Zhou <zhouchuyi@bytedance.com>
Acked-by: Yonghong Song <yonghong.song@linux.dev>