On Sun, Sep 29, 2024 at 6:54 PM Leon Hwang <hffilwlqm@xxxxxxxxx> 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 <leon.hwang@xxxxxxxxx>
> > ---
> > 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.
