Re: [PATCH bpf-next v4 1/4] bpf: Prevent updating extended prog to prog_array map

[Leon Hwang <leon.hwang@linux.dev>]

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);
>>  }
> 
> [...]
>