Re: [PATCH bpf-next v3 1/6] bpf: Resolve and cache fd_array objects at load time

On 7/2/26 9:06 PM, Anton Protopopov wrote:
> On 26/07/02 04:36PM, Daniel Borkmann wrote:
>> The fd_array passed to BPF_PROG_LOAD carries the map and module BTF file
>> descriptors a program binds. The verifier reads it more than once during
>> a load: process_fd_array() walks it to bind the maps and BTFs, and
>> check_and_resolve_insns() and the kfunc BTF resolver later read it again
>> to resolve the program's BPF_PSEUDO_MAP_IDX and module kfunc refs.
>>
>> For signed BPF, we need these upfront in memory, thus resolve each fd to
>> its object once and cache it by fd_array index, then bind that cached
>> object for the rest of the load. env->fd_array becomes a small per-slot
>> {map, btf} cache rather than a bpfptr_t; every later reference is then
>> an in-bounds lookup of an already-resolved object, and an index outside
>> the cache is rejected instead of read from user memory:
>>
>>    - continuous (fd_array_cnt given): the caller declares the length and
>>      every entry is resolved and bound up front (used also by signed loader)
>>
>>    - sparse (no fd_array_cnt): left as the legacy path with no fd_array
>>      cache; each reference reads its fd from the caller's fd_array and
>>      resolves it on the spot. Deduplication in used_maps and the kfunc BTF
>>      table keeps this correct, and only unsigned programs use this shape.
>>
>> UAPI-wise nothing changes, its just the internals on how BPF processes
>> and caches the fd_array favors. Split these into separate helpers for
>> continuous versus sparse to make it easier to follow.
> 
> Overall looks ok. A few comments below.
> 
[...]
>> -	/* Check for integer overflow */
>> -	if (attr->fd_array_cnt >= (U32_MAX / size)) {
>> -		verbose(env, "fd_array_cnt is too big (%u)\n", attr->fd_array_cnt);
>> -		return -EINVAL;
>> +	if (cnt > MAX_FD_ARRAY_CNT) {
> 
> So, I _think_ I've done the "unlimited" thing because there can be duplicates
> in fd_array. The limit is actually tracked by __add_{map_btf} So here we hard
> limit on the size of the fd_array itself. (Even without duplicates, this
> fd_array can contain, say, MAX_KFUNC_BTFS different maps, which will be in any
> case rejected by __add_used_map.)
> 
>> +		verbose(env, "fd_array has too many entries (%u, max %u)\n",
>> +			cnt, MAX_FD_ARRAY_CNT);
>> +		return -E2BIG;
>>   	}
>>   
[...]
>>   
>> +static int process_fd_array(struct bpf_verifier_env *env,
>> +			    union bpf_attr *attr, bpfptr_t uattr)
>> +{
>> +	bpfptr_t fd_array = make_bpfptr(attr->fd_array, uattr.is_kernel);
>> +
>> +	if (bpfptr_is_null(fd_array))
>> +		return 0;
>> +	/*
>> +	 * New API: the caller passes fd_array_cnt and a continuous array that
>> +	 * is resolved and bound up front. Legacy API (no fd_array_cnt): keep
>> +	 * the caller's array and resolve entries lazily, on first reference.
>> +	 */
>> +	if (attr->fd_array_cnt)
>> +		return process_fd_array_continuous(env, fd_array,
>> +						   attr->fd_array_cnt);
> 
> Looks like this returns success in case (!fd_array && attr->fd_array_cnt),
> which is a misconfiguration and should be rejected.
Thanks for the review, addressing both in v4!