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

Commit Message

Philo Lu Dec. 27, 2023, 10:01 a.m. UTC

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 <[e-mail]>
---
 kernel/bpf/helpers.c  |  3 +++
 kernel/bpf/relaymap.c | 22 ++++++++++++++++++++++
 2 files changed, 25 insertions(+)

Comments

Hou Tao Dec. 27, 2023, 2:23 p.m. UTC | #1

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 <[e-mail]>
> ---
>  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();

Alexei Starovoitov Dec. 27, 2023, 6:07 p.m. UTC | #2

On Wed, Dec 27, 2023 at 2:01 AM Philo Lu <[e-mail]> wrote:
>
> +__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);
> +       if (!rmap->relay_chan->has_base_filename)
> +               return -ENOENT;
> +
> +       relay_write(rmap->relay_chan, data, data__sz);
> +       return 0;

This just opens a can of worms.
Above is not nmi safe. relay_write() can be used only out of
known context which effectively makes it unusable out of bpf tracing
progs that can kprobe attach anywhere in the kernel.
perf_event buffer is the only sure way to deliver events to user
space with overwrite.
bpf ringbuf is a best effort due to
if (in_nmi()) if (!spin_trylock_irqsave

Sorry, but it's a nack to allow bpf progs interface with relay.

Patch

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)
 
 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);
+	if (!rmap->relay_chan->has_base_filename)
+		return -ENOENT;
+
+	relay_write(rmap->relay_chan, data, data__sz);
+	return 0;
+}
+
+__bpf_kfunc_end_defs();