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

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

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?

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;
>