On 2024/10/1 19:13, Eduard Zingerman wrote:
> On Sun, 2024-09-29 at 21:27 +0800, Leon Hwang wrote:
>
> [...]
>
>> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
>> index 79660e3fca4c1..4a4de4f014be9 100644
>> --- a/kernel/bpf/arraymap.c
>> +++ b/kernel/bpf/arraymap.c
>> @@ -947,16 +947,29 @@ static void *prog_fd_array_get_ptr(struct bpf_map *map,
>> struct file *map_file, int fd)
>> {
>> struct bpf_prog *prog = bpf_prog_get(fd);
>> + bool is_extended;
>>
>> if (IS_ERR(prog))
>> return prog;
>>
>> - if (!bpf_prog_map_compatible(map, prog)) {
>> - bpf_prog_put(prog);
>> - return ERR_PTR(-EINVAL);
>> - }
>> + if (!bpf_prog_map_compatible(map, prog))
>> + goto out_put_prog;
>> +
>> + mutex_lock(&prog->aux->ext_mutex);
>> + is_extended = prog->aux->is_extended;
>> + mutex_unlock(&prog->aux->ext_mutex);
>> + if (is_extended)
>> + /* Extended prog can not be tail callee. It's to prevent a
>> + * potential infinite loop like:
>> + * tail callee prog entry -> tail callee prog subprog ->
>> + * freplace prog entry --tailcall-> tail callee prog entry.
>> + */
>> + goto out_put_prog;
>
> Nit: I think return value should be -EBUSY in this case.
Ack.
>
>>
>> return prog;
>> +out_put_prog:
>> + bpf_prog_put(prog);
>> + return ERR_PTR(-EINVAL);
>> }
>>
>
> [...]
>
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index a8f1808a1ca54..db17c52fa35db 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -3212,14 +3212,23 @@ static void bpf_tracing_link_release(struct bpf_link *link)
>> {
>> struct bpf_tracing_link *tr_link =
>> container_of(link, struct bpf_tracing_link, link.link);
>> -
>> - WARN_ON_ONCE(bpf_trampoline_unlink_prog(&tr_link->link,
>> - tr_link->trampoline));
>> + struct bpf_prog *tgt_prog = tr_link->tgt_prog;
>> +
>> + if (link->prog->type == BPF_PROG_TYPE_EXT) {
>> + mutex_lock(&tgt_prog->aux->ext_mutex);
>> + WARN_ON_ONCE(bpf_trampoline_unlink_prog(&tr_link->link,
>> + tr_link->trampoline));
>> + tgt_prog->aux->is_extended = false;
>
> In case if unlink fails is_extended should not be reset.
>
Nope.
In bpf_trampoline_unlink_prog(), 'tr->extension_prog = NULL;' always no
matter whether fail to unlink.
So, it should reset is_extended always too.
Thanks,
Leon
>> + mutex_unlock(&tgt_prog->aux->ext_mutex);
>> + } else {
>> + WARN_ON_ONCE(bpf_trampoline_unlink_prog(&tr_link->link,
>> + tr_link->trampoline));
>> + }
>>
>> bpf_trampoline_put(tr_link->trampoline);
>>
>> /* tgt_prog is NULL if target is a kernel function */
>> - if (tr_link->tgt_prog)
>> + if (tgt_prog)
>> bpf_prog_put(tr_link->tgt_prog);
>> }
>
> [...]
>
