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

Commit Message

Leon Hwang July 26, 2024, 3:39 p.m. UTC

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.

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>

__noinline
int subprog(struct __sk_buff *skb)
{
	volatile int ret = 1;

	asm volatile (""::"r+"(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 <[e-mail]>
Cc: Martin KaFai Lau <[e-mail]>
Signed-off-by: Leon Hwang <[e-mail]>
---
 include/linux/bpf_verifier.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Yonghong Song July 26, 2024, 7:34 p.m. UTC | #1

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 <[e-mail]>
> Cc: Martin KaFai Lau <[e-mail]>
> Signed-off-by: Leon Hwang <[e-mail]>

Other than the above, LGTM.

Acked-by: Yonghong Song <[e-mail]>

Patch

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)