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

[Martin KaFai Lau <martin.lau@xxxxxxxxx>]

On 3/20/23 12:24 PM, Kui-Feng Lee wrote:
>   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);
> @@ -11596,31 +11637,32 @@ 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 != -EBUSY) {
> +		free(link);
> +		return libbpf_err_ptr(err);
> +	}
>   
> -		if (!prog)
> -			continue;
> +	link->link.detach = bpf_link__detach_struct_ops;
>   
> -		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)) {

hmm... This still does not look right. 'err' could be -EBUSY here and should not 
be treated as success for non BPF_F_LINK case. The above 'err && err != -EBUSY' 
check should also consider the BPF_F_LINK map_flags.

> +		/* w/o a real link */
> +		link->link.fd = map->fd;
> +		link->map_fd = -1;
> +		return &link->link;
>   	}
>   
> -	err = bpf_map_update_elem(map->fd, &zero, st_ops->kern_vdata, 0);
> -	if (err) {
> -		err = -errno;
> +	fd = bpf_link_create(map->fd, 0, BPF_STRUCT_OPS, NULL);
> +	if (fd < 0) {
>   		free(link);
> -		return libbpf_err_ptr(err);
> +		return libbpf_err_ptr(fd);
>   	}
>   
> -	link->detach = bpf_link__detach_struct_ops;
> -	link->fd = map->fd;
> +	link->link.fd = fd;
> +	link->map_fd = map->fd;
>   
> -	return link;
> +	return &link->link;
>   }