On 2024/7/28 19:46, Leon Hwang wrote:
> The commit f7866c358733 ("bpf: Fix null pointer dereference in resolve_prog_type() for BPF_PROG_TYPE_EXT")
> fixed a NULL pointer dereference panic, but didn't fix the issue that
> fails to update attached freplace prog to prog_array map.
>
> Since commit 1c123c567fb1 ("bpf: Resolve fext program type when checking map compatibility"),
> freplace prog and its target prog are able to tail call each other.
>
> And the commit 3aac1ead5eb6 ("bpf: Move prog->aux->linked_prog and trampoline into bpf_link on attach")
> sets prog->aux->dst_prog as NULL after attaching freplace prog to its
> target prog.
>
> Then, as for following example:
>
> tailcall_freplace.c:
>
> // SPDX-License-Identifier: GPL-2.0
>
> \#include <linux/bpf.h>
> \#include <bpf/bpf_helpers.h>
>
> struct {
> __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
> __uint(max_entries, 1);
> __uint(key_size, sizeof(__u32));
> __uint(value_size, sizeof(__u32));
> } jmp_table SEC(".maps");
>
> int count = 0;
>
> SEC("freplace")
> int entry_freplace(struct __sk_buff *skb)
> {
> count++;
> bpf_tail_call_static(skb, &jmp_table, 0);
> return count;
> }
>
> char __license[] SEC("license") = "GPL";
>
> tc_bpf2bpf.c:
>
> // SPDX-License-Identifier: GPL-2.0
>
> \#include <linux/bpf.h>
> \#include <bpf/bpf_helpers.h>
> \#include "bpf_misc.h"
>
> __noinline
> int subprog(struct __sk_buff *skb)
> {
> int ret = 1;
>
> __sink(ret);
> return ret;
> }
>
> SEC("tc")
> int entry_tc(struct __sk_buff *skb)
> {
> return subprog(skb);
> }
>
> char __license[] SEC("license") = "GPL";
>
> And entry_freplace's target is the entry_tc's subprog.
>
> After loading entry_freplace, the jmp_table's owner type is
> BPF_PROG_TYPE_SCHED_CLS.
>
> Next, after attaching entry_freplace to entry_tc's subprog, its prog->aux->
> dst_prog is NULL.
>
> Next, while updating entry_freplace to jmp_table, bpf_prog_map_compatible()
> returns false because resolve_prog_type() returns BPF_PROG_TYPE_EXT instead
> of BPF_PROG_TYPE_SCHED_CLS.
>
> With this patch, resolve_prog_type() returns BPF_PROG_TYPE_SCHED_CLS to
> support updating the attached entry_freplace to jmp_table.
>
> Fixes: f7866c358733 ("bpf: Fix null pointer dereference in resolve_prog_type() for BPF_PROG_TYPE_EXT")
> Cc: Toke Høiland-Jørgensen <toke@xxxxxxxxxx>
> Cc: Martin KaFai Lau <martin.lau@xxxxxxxxxx>
> Acked-by: Yonghong Song <yonghong.song@xxxxxxxxx>
> Signed-off-by: Leon Hwang <leon.hwang@xxxxxxxxx>
> ---
> 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..bfd093ac333f2 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->aux->saved_dst_prog_type : prog->type;
> }
>
> static inline bool bpf_prog_check_recur(const struct bpf_prog *prog)
This is indeed a better way to fix both two issues we encountered. LGTM.
