Re: [PATCH bpf-next v5 4/8] libbpf: Create a bpf_link in bpf_map__attach_struct_ops().

[Kui-Feng Lee <sinquersw@gmail.com>]

On 3/8/23 13:42, Martin KaFai Lau wrote:
> On 3/7/23 4:50 PM, Kui-Feng Lee wrote:
>> @@ -11566,22 +11591,34 @@ struct bpf_link *bpf_program__attach(const 
>> struct bpf_program *prog)
>>       return link;
>>   }
>> +struct bpf_link_struct_ops {
>> +    struct bpf_link link;
>> +    int map_fd;
>> +};
>> +
>>   static int bpf_link__detach_struct_ops(struct bpf_link *link)
>>   {
>> +    struct bpf_link_struct_ops *st_link;
>>       __u32 zero = 0;
>> -    if (bpf_map_delete_elem(link->fd, &zero))
>> -        return -errno;
>> +    st_link = container_of(link, struct bpf_link_struct_ops, link);
>> -    return 0;
>> +    if (st_link->map_fd < 0) {
> 
> map_fd < 0 should always be true?

If the user pass a wrong link, it can fail.
I check it here explicitly even the kernel returns
an error for deleting an element of a struct_ops w/ link.

> 
>> +        /* Fake bpf_link */
>> +        if (bpf_map_delete_elem(link->fd, &zero))
>> +            return -errno;
>> +        return 0;
>> +    }
>> +
>> +    /* Doesn't support detaching. */
>> +    return -EOPNOTSUPP;
>>   }
>>   struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map)
>>   {
>> -    struct bpf_struct_ops *st_ops;
>> -    struct bpf_link *link;
>> -    __u32 i, zero = 0;
>> -    int err;
>> +    struct bpf_link_struct_ops *link;
>> +    __u32 zero = 0;
>> +    int err, fd;
>>       if (!bpf_map__is_struct_ops(map) || map->fd == -1)
>>           return libbpf_err_ptr(-EINVAL);
>> @@ -11590,31 +11627,34 @@ struct bpf_link 
>> *bpf_map__attach_struct_ops(const struct bpf_map *map)
>>       if (!link)
>>           return libbpf_err_ptr(-EINVAL);
>> -    st_ops = map->st_ops;
>> -    for (i = 0; i < btf_vlen(st_ops->type); i++) {
>> -        struct bpf_program *prog = st_ops->progs[i];
>> -        void *kern_data;
>> -        int prog_fd;
>> +    /* kern_vdata should be prepared during the loading phase. */
>> +    err = bpf_map_update_elem(map->fd, &zero, 
>> map->st_ops->kern_vdata, 0);
>> +    if (err) {
>> +        err = -errno;
>> +        free(link);
>> +        return libbpf_err_ptr(err);
>> +    }
>> -        if (!prog)
>> -            continue;
>> -        prog_fd = bpf_program__fd(prog);
>> -        kern_data = st_ops->kern_vdata + st_ops->kern_func_off[i];
>> -        *(unsigned long *)kern_data = prog_fd;
>> +    if (!(map->def.map_flags & BPF_F_LINK)) {
>> +        /* Fake bpf_link */
>> +        link->link.fd = map->fd;
>> +        link->map_fd = -1;
>> +        link->link.detach = bpf_link__detach_struct_ops;
>> +        return &link->link;
>>       }
>> -    err = bpf_map_update_elem(map->fd, &zero, st_ops->kern_vdata, 0);
>> -    if (err) {
>> +    fd = bpf_link_create(map->fd, -1, BPF_STRUCT_OPS, NULL);
>> +    if (fd < 0) {
>>           err = -errno;
>>           free(link);
>>           return libbpf_err_ptr(err);
>>       }
>> -    link->detach = bpf_link__detach_struct_ops;
>> -    link->fd = map->fd;
>> +    link->link.fd = fd;
>> +    link->map_fd = map->fd;
> 
> Does it need to set link->link.detach?

Yes, I have made some changes to this part. The new code will set
link->link.detach for BPF_F_LINK as well to cleanup fd.

> 
>> -    return link;
>> +    return &link->link;
>>   }
>>   typedef enum bpf_perf_event_ret (*bpf_perf_event_print_t)(struct 
>> perf_event_header *hdr,
>