Re: [PATCH bpf-next 1/2] bpf: Fix updating attached freplace prog to PROG_ARRAY map

26 July 2024 at 14:15, "Yonghong Song" [off-list ref] wrote:

On 7/25/24 8:27 PM, leon.hwang@linux.dev wrote:

quoted

26 July 2024 at 04:58, "Yonghong Song" [off-list ref] wrote:

quoted

On 7/24/24 5:32 PM, Leon Hwang wrote:

 The commit f7866c3587337731 ("bpf: Fix null pointer dereference in

 resolve_prog_type() for BPF_PROG_TYPE_EXT") fixed the following panic,

 which was caused by updating attached freplace prog to PROG_ARRAY map.

quoted

I am confused here. You mentioned that commit f7866c3587337731

 fixed the panic below. But looking at commit message:

 https://lore.kernel.org/bpf/20240711145819.254178-2-wutengda@huaweicloud.com (local)

 it does not seem the case.

 The commit fixed this panic meanwhile.

 This panic seems confusing. I'll remove it in patch v2.

[...]

quoted

---

 include/linux/bpf_verifier.h | 4 ++--

 1 file changed, 2 insertions(+), 2 deletions(-)

 diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h

 index 5cea15c81b8a8..387e034e73d0e 100644

 --- a/include/linux/bpf_verifier.h

 +++ b/include/linux/bpf_verifier.h

 @@ -874,8 +874,8 @@ static inline u32 type_flag(u32 type)

 /* only use after check_attach_btf_id() */

 static inline enum bpf_prog_type resolve_prog_type(const struct bpf_prog *prog)

 {

 - return (prog->type == BPF_PROG_TYPE_EXT && prog->aux->dst_prog) ?

 - prog->aux->dst_prog->type : prog->type;

 + return prog->type == BPF_PROG_TYPE_EXT ?

 + prog->aux->saved_dst_prog_type : prog->type;

quoted

If prog->aux->dst_prog is NULL, is it possible that prog->aux->saved_dst_prog_type

 (0, corresponding to BPF_PROG_TYPE_UNSPEC) could be returned? Do we need to do

 return (prog->type == BPF_PROG_TYPE_EXT && prog->aux->saved_dst_prog_type) ?

 prog->aux->saved_dst_prog_type : prog->type;

 Maybe I missed something here?

 It seems better to check prog->aux->saved_dst_prog_type. But I don't think so.

 prog->aux->saved_dst_prog_type is set in check_attach_btf_id(). And there is no

 resolve_prog_type() before check_attach_btf_id() in bpf_check().

 Therefore, resolve_prog_type() must be called after check_attach_btf_id().

In check_attach_btf_id(), I see

 if (tgt_prog) {

 prog->aux->saved_dst_prog_type = tgt_prog->type;

 prog->aux->saved_dst_attach_type = tgt_prog->expected_attach_type;

 }

So it is possible prog->aux->saved_dst_prog_type is 0 (default value).

I don't know that if tgt_prog is NULL, whether later resolve_prog_type()

will be called or not. Need more checking here.

This is the case that commit f7866c3587337731 ("bpf: Fix null pointer dereference
in resolve_prog_type() for BPF_PROG_TYPE_EXT") fixed, which is loading freplace
prog without tgt_prog.

With this patch, while loading freplace prog without tgt_prog, resolve_prog_type()
returns 0 instead of BPF_PROG_TYPE_EXT.

It's better to return a meaningful prog type in resolve_prog_type() anyway.

I accept your suggestion.

Thanks,
Leon