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

[Yonghong Song <yonghong.song@linux.dev>]

On 7/26/24 8:39 AM, Leon Hwang wrote:
> The commit f7866c3587337731 ("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 1c123c567fb138eb ("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 3aac1ead5eb6b76f ("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.

Similar to my previous comment in the cover letter, please use 12 alphanum
for the commit and the commit subject should be in the same line.

>
> 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++;

Empty line is not necessary.

> 	bpf_tail_call_static(skb, &jmp_table, 0);

Empty line is not necessary.

> 	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>
>
> __noinline
> int subprog(struct __sk_buff *skb)
> {
> 	volatile int ret = 1;
>
> 	asm volatile (""::"r+"(ret));

Let us replace the above asm volatile to __sink(ret). Also, I think 'volatile' is not needed.

Also remove the empty line.

> 	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@redhat.com>
> Cc: Martin KaFai Lau <martin.lau@kernel.org>
> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>

Other than the above, LGTM.

Acked-by: Yonghong Song <yonghong.song@linux.dev>