Re: [PATCH bpf-next 1/6] bpf: Support ->fill_link_info for kprobe_multi

[Stanislav Fomichev <sdf@google.com>]

On 06/02, Yafang Shao wrote:
> By adding support for ->fill_link_info to the kprobe_multi link, users will
> be able to inspect it using `bpftool link show`. This enhancement will
> expose both the count of probed functions and their respective addresses to
> the user.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  include/uapi/linux/bpf.h       |  4 ++++
>  kernel/trace/bpf_trace.c       | 26 ++++++++++++++++++++++++++
>  tools/include/uapi/linux/bpf.h |  4 ++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index a7b5e91..22c8168 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -6438,6 +6438,10 @@ struct bpf_link_info {
>  			__s32 priority;
>  			__u32 flags;
>  		} netfilter;
> +		struct {
> +			__u64 addrs;
> +			__u32 count;
> +		} kprobe_multi;
>  	};
>  } __attribute__((aligned(8)));
>  
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 2bc41e6..ec53bc9 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2548,9 +2548,35 @@ static void bpf_kprobe_multi_link_dealloc(struct bpf_link *link)
>  	kfree(kmulti_link);
>  }
>  
> +static int bpf_kprobe_multi_link_fill_link_info(const struct bpf_link *link,
> +						struct bpf_link_info *info)
> +{
> +	u64 *uaddrs = (u64 *)u64_to_user_ptr(info->kprobe_multi.addrs);

Maybe tag this as __user as well?

	u64 __user *uaddrs = u64_to_user_ptr(info->kprobe_multi.addrs);

copy_to_user expects __user tagged memory, so most likely sparse tool
will complain on your current approach.

> +	struct bpf_kprobe_multi_link *kmulti_link;
> +	u32 ucount = info->kprobe_multi.count;
> +
> +	if (!uaddrs ^ !ucount)
> +		return -EINVAL;
> +
> +	kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
> +	if (!uaddrs) {
> +		info->kprobe_multi.count = kmulti_link->cnt;
> +		return 0;
> +	}
> +
> +	if (!ucount)
> +		return 0;
> +	if (ucount != kmulti_link->cnt)
> +		return -EINVAL;

[..]

> +	if (copy_to_user(uaddrs, kmulti_link->addrs, ucount * sizeof(u64)))
> +		return -EFAULT;

I'm not super familiar with this part, so maybe stupid question:
do we need to hold any locks during the copy? IOW, can kmulti_link->addrs
be updated concurrently?