Re: [PATCH bpf-next v13 6/8] bpf: Add syscall common attributes support for map_create

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

On 13/5/26 07:36, sashiko-bot@kernel.org wrote:
[...]
>> +
>> +	err = __map_create(attr, uattr, log);
>> +
>> +	ret = bpf_log_attr_finalize(&attr_log, log);
>> +	if (ret) {
>> +		if (err >= 0)
>> +			close_fd(err);
> 
> Is it safe to call close_fd() here? Since __map_create() uses
> bpf_map_new_fd() to allocate and publish the file descriptor via
> fd_install(), the file descriptor is already visible to userspace.
> 

I think it is acceptable to close_fd() here, that the duration between
fd_install() and close_fd() is short.

> Could this introduce a regression where a concurrent thread closes this
> file descriptor and opens a new, unrelated file that receives the exact
> same fd number? If that happens, it seems this close_fd() might
> inadvertently close the unrelated file.
> 

This regression is valid.

However, since the new fd hasn't been returned to user space, user space
thread seems unable to get the exact fd number to close it.

> Additionally, since the map is published globally via its ID before fd
> installation in bpf_map_alloc_id(), could another thread acquire a
> reference to the map before this error path? It looks like the map might
> remain alive and accessible even though the syscall returns a failure.
> 
I think this is not an issue, as the map is valid for other threads. The
map's lifecycle is maintained by its own refcnt. close_fd() will dec the
refcnt instead of destroying the map.

Thanks,
Leon

>> +		err = ret;
>> +	}
>> +
>> +free:
>> +	kfree(log);
>> +	return err;
>> +}
>