Re: [PATCH bpf-next 2/5] bpf: Fix concurrent regression in map_create()

[Leon Hwang]

On 18/5/26 23:40, bot+bpf-ci@xxxxxxxxxx wrote:
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 83de8fb9b9aa..322865a88b3a 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -1647,11 +1647,18 @@ static int map_create(union bpf_attr *attr, bpfptr_t uattr, struct bpf_common_at
>>
>>  	/* preserve original error even if log finalization is successful */
>>  	ret = bpf_log_attr_finalize(&attr_log, log);
>> -	if (ret) {
>> -		if (err >= 0)
>> -			close_fd(err);
>> +	if (ret && err < 0)
>> +		/*
>> +		 * Failed to finalize the log.
>> +		 * Should not close_fd(err) here. Since the bpf_map_new_fd()
>> +		 * has published the map fd, if a concurrent thread closes the
>> +		 * fd, then opens new, unrelated file that receives the exact
>> +		 * same fd number, close_fd(err) might inadvertently close the
>> +		 * unrelated file.
>> +		 * As a trade-off, override the err only when failed to finalize
>> +		 * the log and failed to create map.
>> +		 */
>>  		err = ret;
> 
> In the v13 review thread, Alexei Starovoitov suggested moving
> bpf_log_attr_finalize() earlier in the flow to eliminate the race
> window entirely:
> 
>   "bot is correct. Let's avoid these races. Pls move
>    bpf_log_attr_finalize() into map_create and do it before
>    security_bpf_map_create."
> 
>   https://lore.kernel.org/bpf/CAADnVQ+XR3kyqizGgGhtG5xiBu3oK3O+COUMfPDQDBSJbm=56A@xxxxxxxxxxxxxx/
> 
> Does the current approach fully address the race concern? While removing
> close_fd() prevents the specific file descriptor hijacking issue, the
> suggestion was to finalize the log before security_bpf_map_create(),
> which would avoid creating the race window in the first place.
> 
> Is there a reason why the log finalization cannot be moved earlier as
> Alexei suggested?
> 
I've tried his suggestion, attached below.

However, I think there should be a simpler approach to fix it.

I preferred this simple one, so I posted it.

Should I follow the suggestion instead of this trade-off approach?

Thanks,
Leon

---