On 7/25/24 8:27 PM, leon.hwang@xxxxxxxxx wrote:
26 July 2024 at 04:58, "Yonghong Song" <yonghong.song@xxxxxxxxx> wrote:
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.
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@xxxxxxxxxxxxxxx
it does not seem the case.
The commit fixed this panic meanwhile.
This panic seems confusing. I'll remove it in patch v2.
[...]
---
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;
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.
