[bpf-next,v4,2/4] bpf: Prevent extending tail callee prog with freplace prog

Commit Message

Leon Hwang Sept. 29, 2024, 1:27 p.m. UTC

Alongside previous patch, the infinite loop issue caused by combination of
tailcal and freplace can be prevented completely.

The previous patch can not prevent the use case that updates a prog to
prog_array map and then extends subprog of the prog with freplace prog.

This patch fixes the case by preventing extending a prog, which has been
updated to prog_array map, with freplace prog.

If a prog has been updated to prog_array map, it or its subprog can not
be extended by freplace prog.

Signed-off-by: Leon Hwang <[e-mail]>
---
 include/linux/bpf.h   |  3 ++-
 kernel/bpf/arraymap.c |  9 ++++++++-
 kernel/bpf/syscall.c  | 11 +++++++++++
 3 files changed, 21 insertions(+), 2 deletions(-)

Comments

Leon Hwang Sept. 30, 2024, 1:53 a.m. UTC | #1

On 29/9/24 21:27, Leon Hwang wrote:
> Alongside previous patch, the infinite loop issue caused by combination of
> tailcal and freplace can be prevented completely.
> 
> The previous patch can not prevent the use case that updates a prog to
> prog_array map and then extends subprog of the prog with freplace prog.
> 
> This patch fixes the case by preventing extending a prog, which has been
> updated to prog_array map, with freplace prog.
> 
> If a prog has been updated to prog_array map, it or its subprog can not
> be extended by freplace prog.
> 
> Signed-off-by: Leon Hwang <[e-mail]>
> ---
>  include/linux/bpf.h   |  3 ++-
>  kernel/bpf/arraymap.c |  9 ++++++++-
>  kernel/bpf/syscall.c  | 11 +++++++++++
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index aac6d2f42830c..dc19ad99e2857 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1484,7 +1484,8 @@ struct bpf_prog_aux {
>  	bool exception_cb;
>  	bool exception_boundary;
>  	bool is_extended; /* true if extended by freplace program */
> -	struct mutex ext_mutex; /* mutex for is_extended */
> +	u32 prog_array_member_cnt; /* counts how many times as member of prog_array */
> +	struct mutex ext_mutex; /* mutex for is_extended and prog_array_member_cnt */
>  	struct bpf_arena *arena;
>  	/* BTF_KIND_FUNC_PROTO for valid attach_btf_id */
>  	const struct btf_type *attach_func_proto;
> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index 4a4de4f014be9..91b5bdf4dc72d 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -957,6 +957,8 @@ static void *prog_fd_array_get_ptr(struct bpf_map *map,
>  
>  	mutex_lock(&prog->aux->ext_mutex);
>  	is_extended = prog->aux->is_extended;
> +	if (!is_extended)
> +		prog->aux->prog_array_member_cnt++;

prog_array_member_cnt must check U32_MAX before incrementing. Or it will
overflow u32. So it will be better like:

	mutex_lock(&prog->aux->ext_mutex);
	is_invalid = prog->aux->is_extended || prog->aux->prog_array_member_cnt
== U32_MAX;
	if (!is_invalid)
		prog->aux->prog_array_member_cnt++;
	mutex_unlock(&prog->aux->ext_mutex);
	if (is_invalid)
		goto out_put_prog;

Thanks,
Leon

Alexei Starovoitov Oct. 4, 2024, 7:33 p.m. UTC | #2

On Sun, Sep 29, 2024 at 6:54 PM Leon Hwang <[e-mail]> wrote:
>
>
>
> On 29/9/24 21:27, Leon Hwang wrote:
> > Alongside previous patch, the infinite loop issue caused by combination of
> > tailcal and freplace can be prevented completely.
> >
> > The previous patch can not prevent the use case that updates a prog to
> > prog_array map and then extends subprog of the prog with freplace prog.
> >
> > This patch fixes the case by preventing extending a prog, which has been
> > updated to prog_array map, with freplace prog.
> >
> > If a prog has been updated to prog_array map, it or its subprog can not
> > be extended by freplace prog.
> >
> > Signed-off-by: Leon Hwang <[e-mail]>
> > ---
> >  include/linux/bpf.h   |  3 ++-
> >  kernel/bpf/arraymap.c |  9 ++++++++-
> >  kernel/bpf/syscall.c  | 11 +++++++++++
> >  3 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index aac6d2f42830c..dc19ad99e2857 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -1484,7 +1484,8 @@ struct bpf_prog_aux {
> >       bool exception_cb;
> >       bool exception_boundary;
> >       bool is_extended; /* true if extended by freplace program */
> > -     struct mutex ext_mutex; /* mutex for is_extended */
> > +     u32 prog_array_member_cnt; /* counts how many times as member of prog_array */
> > +     struct mutex ext_mutex; /* mutex for is_extended and prog_array_member_cnt */
> >       struct bpf_arena *arena;
> >       /* BTF_KIND_FUNC_PROTO for valid attach_btf_id */
> >       const struct btf_type *attach_func_proto;
> > diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> > index 4a4de4f014be9..91b5bdf4dc72d 100644
> > --- a/kernel/bpf/arraymap.c
> > +++ b/kernel/bpf/arraymap.c
> > @@ -957,6 +957,8 @@ static void *prog_fd_array_get_ptr(struct bpf_map *map,
> >
> >       mutex_lock(&prog->aux->ext_mutex);
> >       is_extended = prog->aux->is_extended;
> > +     if (!is_extended)
> > +             prog->aux->prog_array_member_cnt++;
>
> prog_array_member_cnt must check U32_MAX before incrementing. Or it will
> overflow u32. So it will be better like:
>
>         mutex_lock(&prog->aux->ext_mutex);
>         is_invalid = prog->aux->is_extended || prog->aux->prog_array_member_cnt
> == U32_MAX;

No. Just make it u64 instead.

btw the whole thing can be done with a single atomic64_t:
- set it to 1 at the start then

- prog_fd_array_get_ptr() will do
atomic64_inc_not_zero

- prog_fd_array_put_ptr() will do
atomic64_add_unless(,-1, 1)

- freplace attach will do
cmpxchg(,1,0)

so 1 - initial state
2,3,.. - prog in prog_array
0 - prog was extended.

If == 0 -> cannot add to prog_array
if > 1 -> cannot freplace.

but it's too clever.
It's better to use mutex and keep bool + count,
but extra mutex is unnecessary.
Reuse prog->aux->dst_mutex.
Grab it prog_fd_array_get_ptr() and do the check and cnt++

Also pls combine patch 1 and 2.
They do one logical step.

Eduard Zingerman Oct. 4, 2024, 8:37 p.m. UTC | #3

On Fri, 2024-10-04 at 12:33 -0700, Alexei Starovoitov wrote:

[...]

> so 1 - initial state
> 2,3,.. - prog in prog_array
> 0 - prog was extended.

This sounds interesting, but need to think a bit.

> If == 0 -> cannot add to prog_array
> if > 1 -> cannot freplace.
> 
> but it's too clever.
> It's better to use mutex and keep bool + count,
> but extra mutex is unnecessary.
> Reuse prog->aux->dst_mutex.
> Grab it prog_fd_array_get_ptr() and do the check and cnt++

I think it is not possible to grab the correct mutex in
prog_fd_array_get_ptr().

bpf_tracing_prog_attach() operates on two programs:
- one named 'prog' is the freplace program;
- another named 'tgt_prog' is the program to attach 'prog' to.

bpf_tracing_prog_attach() grabs prog->aux->dst_mutex.
Inside prog_fd_array_get_ptr() there is only a pointer to program
being put into array, potential target of the freplace.
From bpf_tracing_prog_attach() it is referred as 'tgt_prog'.
As far as I understand, there is no way to get a pointer to an active
freplace program from prog_fd_array_get_ptr().

[...]

Eduard Zingerman Oct. 4, 2024, 8:52 p.m. UTC | #4

On Fri, 2024-10-04 at 12:33 -0700, Alexei Starovoitov wrote:

[...]

> btw the whole thing can be done with a single atomic64_t:
> - set it to 1 at the start then
> 
> - prog_fd_array_get_ptr() will do
> atomic64_inc_not_zero
> 
> - prog_fd_array_put_ptr() will do
> atomic64_add_unless(,-1, 1)
> 
> - freplace attach will do
> cmpxchg(,1,0)
> 
> so 1 - initial state
> 2,3,.. - prog in prog_array
> 0 - prog was extended.
> 
> If == 0 -> cannot add to prog_array
> if > 1 -> cannot freplace.

I think this should work, because we no longer need to jungle two values.
I kinda like it.

[...]

Alexei Starovoitov Oct. 4, 2024, 11:30 p.m. UTC | #5

On Fri, Oct 4, 2024 at 1:52 PM Eduard Zingerman <[e-mail]> wrote:
>
> On Fri, 2024-10-04 at 12:33 -0700, Alexei Starovoitov wrote:
>
> [...]
>
> > btw the whole thing can be done with a single atomic64_t:
> > - set it to 1 at the start then
> >
> > - prog_fd_array_get_ptr() will do
> > atomic64_inc_not_zero
> >
> > - prog_fd_array_put_ptr() will do
> > atomic64_add_unless(,-1, 1)
> >
> > - freplace attach will do
> > cmpxchg(,1,0)
> >
> > so 1 - initial state
> > 2,3,.. - prog in prog_array
> > 0 - prog was extended.
> >
> > If == 0 -> cannot add to prog_array
> > if > 1 -> cannot freplace.
>
> I think this should work, because we no longer need to jungle two values.
> I kinda like it.

It's a bit too clever.

With mutex it's much easier to reason about:

struct bpf_prog_aux {
   mutex ext_mutex;
   bool is_extended;
   u64 prog_array_member_cnt;
};

freplace link on tgt_prog:
guard(mutex)(&aux->ext_mutex);
if (aux->prog_array_member_cnt) {
  // reject freplace
} else {
  aux->is_extended = true;
}

freplace unlink:
guard(mutex)(&aux->ext_mutex);
aux->is_extended = false;

and similar in prog_fd_array_get_ptr():
guard(mutex)(&aux->ext_mutex);
if (aux->is_extended) {
   // reject adding to prog_array
} else {
  aux->prog_array_member_cnt++;
}

in prog_fd_array_put_ptr():
guard(mutex)(&aux->ext_mutex);
aux->prog_array_member_cnt--;

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index aac6d2f42830c..dc19ad99e2857 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1484,7 +1484,8 @@  struct bpf_prog_aux {
 	bool exception_cb;
 	bool exception_boundary;
 	bool is_extended; /* true if extended by freplace program */
-	struct mutex ext_mutex; /* mutex for is_extended */
+	u32 prog_array_member_cnt; /* counts how many times as member of prog_array */
+	struct mutex ext_mutex; /* mutex for is_extended and prog_array_member_cnt */
 	struct bpf_arena *arena;
 	/* BTF_KIND_FUNC_PROTO for valid attach_btf_id */
 	const struct btf_type *attach_func_proto;
diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 4a4de4f014be9..91b5bdf4dc72d 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -957,6 +957,8 @@  static void *prog_fd_array_get_ptr(struct bpf_map *map,
 
 	mutex_lock(&prog->aux->ext_mutex);
 	is_extended = prog->aux->is_extended;
+	if (!is_extended)
+		prog->aux->prog_array_member_cnt++;
 	mutex_unlock(&prog->aux->ext_mutex);
 	if (is_extended)
 		/* Extended prog can not be tail callee. It's to prevent a
@@ -974,8 +976,13 @@  static void *prog_fd_array_get_ptr(struct bpf_map *map,
 
 static void prog_fd_array_put_ptr(struct bpf_map *map, void *ptr, bool need_defer)
 {
+	struct bpf_prog *prog = ptr;
+
+	mutex_lock(&prog->aux->ext_mutex);
+	prog->aux->prog_array_member_cnt--;
+	mutex_unlock(&prog->aux->ext_mutex);
 	/* bpf_prog is freed after one RCU or tasks trace grace period */
-	bpf_prog_put(ptr);
+	bpf_prog_put(prog);
 }
 
 static u32 prog_fd_array_sys_lookup_elem(void *ptr)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index db17c52fa35db..4beec9729f742 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3287,6 +3287,17 @@  static int bpf_extend_prog(struct bpf_tracing_link *link,
 	int err = 0;
 
 	mutex_lock(&aux->ext_mutex);
+	if (aux->prog_array_member_cnt) {
+		/* Program extensions can not extend target prog when the target
+		 * prog has been updated to any prog_array map as tail callee.
+		 * It's to prevent a potential infinite loop like:
+		 * tgt prog entry -> tgt prog subprog -> freplace prog entry
+		 * --tailcall-> tgt prog entry.
+		 */
+		err = -EINVAL;
+		goto out_unlock;
+	}
+
 	err = bpf_trampoline_link_prog(&link->link, tr);
 	if (err)
 		goto out_unlock;