Re: [PATCH bpf-next v1 2/3] bpf: add bpf_relay_output kfunc

[Hou Tao <houtao@huaweicloud.com>]

Hi,

On 12/27/2023 6:01 PM, Philo Lu wrote:
> A kfunc is needed to write into the relay channel, named
> bpf_relay_output. The usage is same as bpf_ringbuf_output helper. It
> only works after relay files are set, i.e., after calling
> map_update_elem for the created relay map.
>
> Signed-off-by: Philo Lu <lulie@linux.alibaba.com>
> ---
>  kernel/bpf/helpers.c  |  3 +++
>  kernel/bpf/relaymap.c | 22 ++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index be72824f32b2..22480b69ff27 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2617,6 +2617,9 @@ BTF_ID_FLAGS(func, bpf_dynptr_is_null)
>  BTF_ID_FLAGS(func, bpf_dynptr_is_rdonly)
>  BTF_ID_FLAGS(func, bpf_dynptr_size)
>  BTF_ID_FLAGS(func, bpf_dynptr_clone)
> +#ifdef CONFIG_RELAY
> +BTF_ID_FLAGS(func, bpf_relay_output)
> +#endif
>  BTF_SET8_END(common_btf_ids)

Could you explain the reason why bpf_relay_out is placed in
common_btf_ids instead of generic_kfunc_set ?
>  
>  static const struct btf_kfunc_id_set common_kfunc_set = {
> diff --git a/kernel/bpf/relaymap.c b/kernel/bpf/relaymap.c
> index 02b33a8e6b6c..37280d60133c 100644
> --- a/kernel/bpf/relaymap.c
> +++ b/kernel/bpf/relaymap.c
> @@ -6,6 +6,7 @@
>  #include <linux/slab.h>
>  #include <linux/bpf.h>
>  #include <linux/err.h>
> +#include <linux/btf.h>
>  
>  #define RELAY_CREATE_FLAG_MASK (BPF_F_OVERWRITE)
>  
> @@ -197,3 +198,24 @@ const struct bpf_map_ops relay_map_ops = {
>  	.map_mem_usage = relay_map_mem_usage,
>  	.map_btf_id = &relay_map_btf_ids[0],
>  };
> +
> +__bpf_kfunc_start_defs();
> +
> +__bpf_kfunc int bpf_relay_output(struct bpf_map *map,
> +				   void *data, u64 data__sz, u32 flags)
> +{
> +	struct bpf_relay_map *rmap;
> +
> +	/* not support any flag now */
> +	if (unlikely(!map || flags))
> +		return -EINVAL;
> +
> +	rmap = container_of(map, struct bpf_relay_map, map);

How does bpf_relay_out() guarantee the passed map is a relay map ? And
just like bpf_map_sum_elem_count(), I think KF_TRUSTED_ARGS is also
necessary for bpf_relay_output().
> +	if (!rmap->relay_chan->has_base_filename)
> +		return -ENOENT;
> +

I think a comment is needed here. It needs to explain why checking
->has_base_filename is enough to guarantee the concurrently running of
bpf_relay_output() and .map_update_elem() is safe.

> +	relay_write(rmap->relay_chan, data, data__sz);
> +	return 0;
> +}
> +
> +__bpf_kfunc_end_defs();