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

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

On 5/19/26 3:47 AM, Leon Hwang wrote:
> On 19/5/26 00:43, Mykyta Yatsenko wrote:
>>
>>
>> On 5/18/26 3:54 PM, Leon Hwang wrote:
> [...]
>>>  	/* 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()
>>
>> nit: you can't close_fd(err) here, because err < 0? The comment overall appears to
>> explain why we are making this change right now, rather than why this works this way.
>>
> 
> Will update the comment to
> 
> 		/*
> 		 * Failed to finalize the log.
> 		 *
> 		 * Should not close_fd(err) here by
> 		 *
> 		 *  if (ret) {
> 		 *      if (err >= 0)
> 		 *              close_fd(err);
> 		 *      err = ret;
> 		 *  }
> 		 *
> 		 * 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.
> 		 */
> 
> The comment is to explain the reason for overriding err.
> 
>> If map_crate() failed with error code and then log finalization failed, do we really
>> want to override error code? It sounds like map_create error is more important.
>>
> 
> In that case, there are two error codes that should be returned to user
> space. How to achieve it?
> 
> Since we should not ignore any error code of them, can we report the
> error code of __map_create() failure by adding an error attr to struct
> bpf_common_attr? Hmm, seems not a good idea.
> 
> Any idea?

I think we should do what Alexei suggests.

For example: we return ENOSPC when log is too short, regardless
of the status of the operation for BPF_PROG_LOAD (see bpf_object_load_prog()
in libbpf.c), it makes sense to do it consistently for map_create as well.

log_finalize() error always overrides map_create() error.

A precedent: We are safe from this race in bpf_prog_load(): bpf_prog_alloc_id() makes
prog exposed, only after bpf_check() succeeds, which finalizes log.
We should do similar for map_create().

> 
> Thanks,
> Leon
> 
>>> +		 * 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;
>>> -	}
>>>  
>>>  	kfree(log);
>>>  	return err;
>>
>