H,
On 11/27/2024 2:44 PM, Anton Protopopov wrote:
> On 24/11/26 10:11AM, Hou Tao wrote:
>> Hi,
>>
>> On 11/19/2024 6:15 PM, Anton Protopopov wrote:
>>> The fd_array attribute of the BPF_PROG_LOAD syscall may contain a set
>>> of file descriptors: maps or btfs. This field was introduced as a
>>> sparse array. Introduce a new attribute, fd_array_cnt, which, if
>>> present, indicates that the fd_array is a continuous array of the
>>> corresponding length.
>>>
>>> If fd_array_cnt is non-zero, then every map in the fd_array will be
>>> bound to the program, as if it was used by the program. This
>>> functionality is similar to the BPF_PROG_BIND_MAP syscall, but such
>>> maps can be used by the verifier during the program load.
>>>
>>> Signed-off-by: Anton Protopopov <aspsk@xxxxxxxxxxxxx>
>>> ---
SNIP
>>> +static int add_fd_from_fd_array(struct bpf_verifier_env *env, int fd)
>>> +{
>>> + struct bpf_map *map;
>>> + CLASS(fd, f)(fd);
>>> + int ret;
>>> +
>>> + map = __bpf_map_get(f);
>>> + if (!IS_ERR(map)) {
>>> + ret = add_used_map(env, map);
>>> + if (ret < 0)
>>> + return ret;
>>> + return 0;
>>> + }
>>> +
>>> + if (!IS_ERR(__btf_get_by_fd(f)))
>>> + return 0;
>> For fd_array_cnt > 0 case, does it need to handle BTF fd case ? If it
>> does, these returned BTFs should be saved in somewhere, otherewise,
>> these BTFs will be leaked.
> ATM we don't actually store BTFs here. The __btf_get_by_fd doesn't
> increase the refcnt, so no leaks.
Yes. You are right, I just mis-read the implementation of
__btf_get_by_fd().
>
>>> + if (!fd)
>>> + return 0;
>>> +
>>> + verbose(env, "fd %d is not pointing to valid bpf_map or btf\n", fd);
>>> + return PTR_ERR(map);
>>> +}
>>> +
>>> +static int env_init_fd_array(struct bpf_verifier_env *env, union bpf_attr *attr, bpfptr_t uattr)
>>> +{
>>> + int size = sizeof(int) * attr->fd_array_cnt;
>>> + int *copy;
>>> + int ret;
>>> + int i;
>>> +
>>> + if (attr->fd_array_cnt >= MAX_USED_MAPS)
>>> + return -E2BIG;
>>> +
>>> + env->fd_array = make_bpfptr(attr->fd_array, uattr.is_kernel);
>>> +
>>> + /*
>>> + * The only difference between old (no fd_array_cnt is given) and new
>>> + * APIs is that in the latter case the fd_array is expected to be
>>> + * continuous and is scanned for map fds right away
>>> + */
>>> + if (!size)
>>> + return 0;
>>> +
>>> + copy = kzalloc(size, GFP_KERNEL);
>>> + if (!copy)
>>> + return -ENOMEM;
>>> +
>>> + if (copy_from_bpfptr_offset(copy, env->fd_array, 0, size)) {
>>> + ret = -EFAULT;
>>> + goto free_copy;
>>> + }
>> It is better to use kvmemdup_bpfptr() instead.
> Thanks for the hint. As suggested by Alexei, I will remove the memory
> allocation here altogether.
I see.
>
>>> +
>>> + for (i = 0; i < attr->fd_array_cnt; i++) {
>>> + ret = add_fd_from_fd_array(env, copy[i]);
>>> + if (ret)
>>> + goto free_copy;
>>> + }
>>> +
>>> +free_copy:
>>> + kfree(copy);
>>> + return ret;
>>> +}
>>> +
>>> int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u32 uattr_size)
>>> {
>>> u64 start_time = ktime_get_ns();
>>> @@ -22557,7 +22632,9 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
>>> env->insn_aux_data[i].orig_idx = i;
>>> env->prog = *prog;
>>> env->ops = bpf_verifier_ops[env->prog->type];
>>> - env->fd_array = make_bpfptr(attr->fd_array, uattr.is_kernel);
>>> + ret = env_init_fd_array(env, attr, uattr);
>>> + if (ret)
>>> + goto err_free_aux_data;
>> These maps saved in env->used_map will also be leaked.
> Yeah, thanks, actually, env->used_map contents will be leaked (if
> error occurs here or until we get to after `goto err_unlock`), so
> I will rewrite the init/error path.
Glad to hear that。
