netdev - Re: [RFC bpf-next v2 08/14] bpf: Helper to simplify calling kernel routines from unrolled kfuncs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 4 Nov 2022 17:40:05 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Stanislav Fomichev <sdf@...gle.com>
Cc:     bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
        andrii@...nel.org, martin.lau@...ux.dev, song@...nel.org,
        yhs@...com, john.fastabend@...il.com, kpsingh@...nel.org,
        haoluo@...gle.com, jolsa@...nel.org,
        David Ahern <dsahern@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Willem de Bruijn <willemb@...gle.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Anatoly Burakov <anatoly.burakov@...el.com>,
        Alexander Lobakin <alexandr.lobakin@...el.com>,
        Magnus Karlsson <magnus.karlsson@...il.com>,
        Maryam Tahhan <mtahhan@...hat.com>, xdp-hints@...-project.net,
        netdev@...r.kernel.org
Subject: Re: [RFC bpf-next v2 08/14] bpf: Helper to simplify calling kernel
 routines from unrolled kfuncs

On Thu, Nov 03, 2022 at 08:25:26PM -0700, Stanislav Fomichev wrote:
> When we need to call the kernel function from the unrolled
> kfunc, we have to take extra care: r6-r9 belong to the callee,
> not us, so we can't use these registers to stash our r1.
> 
> We use the same trick we use elsewhere: ask the user
> to provide extra on-stack storage.
> 
> Also, note, the program being called has to receive and
> return the context.
> 
> Cc: John Fastabend <john.fastabend@...il.com>
> Cc: David Ahern <dsahern@...il.com>
> Cc: Martin KaFai Lau <martin.lau@...ux.dev>
> Cc: Jakub Kicinski <kuba@...nel.org>
> Cc: Willem de Bruijn <willemb@...gle.com>
> Cc: Jesper Dangaard Brouer <brouer@...hat.com>
> Cc: Anatoly Burakov <anatoly.burakov@...el.com>
> Cc: Alexander Lobakin <alexandr.lobakin@...el.com>
> Cc: Magnus Karlsson <magnus.karlsson@...il.com>
> Cc: Maryam Tahhan <mtahhan@...hat.com>
> Cc: xdp-hints@...-project.net
> Cc: netdev@...r.kernel.org
> Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> ---
>  include/net/xdp.h |  4 ++++
>  net/core/xdp.c    | 24 +++++++++++++++++++++++-
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index 8c97c6996172..09c05d1da69c 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -440,10 +440,14 @@ static inline u32 xdp_metadata_kfunc_id(int id)
>  	return xdp_metadata_kfunc_ids.pairs[id].id;
>  }
>  void xdp_metadata_export_to_skb(const struct bpf_prog *prog, struct bpf_patch *patch);
> +void xdp_kfunc_call_preserving_r1(struct bpf_patch *patch, size_t r0_offset,
> +				  void *kfunc);
>  #else
>  #define xdp_metadata_magic 0
>  static inline u32 xdp_metadata_kfunc_id(int id) { return 0; }
>  static void xdp_metadata_export_to_skb(const struct bpf_prog *prog, struct bpf_patch *patch) { return 0; }
> +static void xdp_kfunc_call_preserving_r1(struct bpf_patch *patch, size_t r0_offset,
> +					 void *kfunc) {}
>  #endif
>  
>  #endif /* __LINUX_NET_XDP_H__ */
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 8204fa05c5e9..16dd7850b9b0 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -737,6 +737,7 @@ BTF_SET8_START_GLOBAL(xdp_metadata_kfunc_ids)
>  XDP_METADATA_KFUNC_xxx
>  #undef XDP_METADATA_KFUNC
>  BTF_SET8_END(xdp_metadata_kfunc_ids)
> +EXPORT_SYMBOL(xdp_metadata_kfunc_ids);
>  
>  /* Make sure userspace doesn't depend on our layout by using
>   * different pseudo-generated magic value.
> @@ -756,7 +757,8 @@ static const struct btf_kfunc_id_set xdp_metadata_kfunc_set = {
>   *
>   * The above also means we _cannot_ easily call any other helper/kfunc
>   * because there is no place for us to preserve our R1 argument;
> - * existing R6-R9 belong to the callee.
> + * existing R6-R9 belong to the callee. For the cases where calling into
> + * the kernel is the only option, see xdp_kfunc_call_preserving_r1.
>   */
>  void xdp_metadata_export_to_skb(const struct bpf_prog *prog, struct bpf_patch *patch)
>  {
> @@ -832,6 +834,26 @@ void xdp_metadata_export_to_skb(const struct bpf_prog *prog, struct bpf_patch *p
>  
>  	bpf_patch_resolve_jmp(patch);
>  }
> +EXPORT_SYMBOL(xdp_metadata_export_to_skb);
> +
> +/* Helper to generate the bytecode that calls the supplied kfunc.
> + * The kfunc has to accept a pointer to the context and return the
> + * same pointer back. The user also has to supply an offset within
> + * the context to store r0.
> + */
> +void xdp_kfunc_call_preserving_r1(struct bpf_patch *patch, size_t r0_offset,
> +				  void *kfunc)
> +{
> +	bpf_patch_append(patch,
> +		/* r0 = kfunc(r1); */
> +		BPF_EMIT_CALL(kfunc),
> +		/* r1 = r0; */
> +		BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
> +		/* r0 = *(r1 + r0_offset); */
> +		BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_1, r0_offset),
> +	);
> +}
> +EXPORT_SYMBOL(xdp_kfunc_call_preserving_r1);

That's one twisted logic :)
I guess it works for preserving r1, but r2-r5 are gone and r6-r9 cannot be touched.
So it works for the most basic case of returning single value from that additional
kfunc while preserving single r1.
I'm afraid that will be very limiting moving forward.
imo we need push/pop insns. It shouldn't difficult to add to the interpreter and JITs.
Since this patching is done after verificaiton they will be kernel internal insns.
Like we have BPF_REG_AX internal reg.