On Wed, Jan 29, 2025 at 1:00 PM Song Liu <song@xxxxxxxxxx> wrote:
>
> btf_kfunc_id_set.remap can pick proper version of a kfunc for the calling
> context. Use this logic to select bpf_dynptr_from_skb or
> bpf_dynptr_from_skb_rdonly. This will make the verifier simpler.
>
> Unfortunately, btf_kfunc_id_set.remap cannot cover the DYNPTR_TYPE_SKB
> logic in check_kfunc_args(). This can be addressed later.
>
> Signed-off-by: Song Liu <song@xxxxxxxxxx>
> ---
> kernel/bpf/verifier.c | 25 ++++++----------------
> net/core/filter.c | 49 +++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 51 insertions(+), 23 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 2188b6674266..55e710e318e5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11750,6 +11750,7 @@ enum special_kfunc_type {
> KF_bpf_rbtree_add_impl,
> KF_bpf_rbtree_first,
> KF_bpf_dynptr_from_skb,
> + KF_bpf_dynptr_from_skb_rdonly,
> KF_bpf_dynptr_from_xdp,
> KF_bpf_dynptr_slice,
> KF_bpf_dynptr_slice_rdwr,
> @@ -11785,6 +11786,7 @@ BTF_ID(func, bpf_rbtree_add_impl)
> BTF_ID(func, bpf_rbtree_first)
> #ifdef CONFIG_NET
> BTF_ID(func, bpf_dynptr_from_skb)
> +BTF_ID(func, bpf_dynptr_from_skb_rdonly)
> BTF_ID(func, bpf_dynptr_from_xdp)
> #endif
> BTF_ID(func, bpf_dynptr_slice)
> @@ -11816,10 +11818,12 @@ BTF_ID(func, bpf_rbtree_add_impl)
> BTF_ID(func, bpf_rbtree_first)
> #ifdef CONFIG_NET
> BTF_ID(func, bpf_dynptr_from_skb)
> +BTF_ID(func, bpf_dynptr_from_skb_rdonly)
> BTF_ID(func, bpf_dynptr_from_xdp)
> #else
> BTF_ID_UNUSED
> BTF_ID_UNUSED
> +BTF_ID_UNUSED
> #endif
> BTF_ID(func, bpf_dynptr_slice)
> BTF_ID(func, bpf_dynptr_slice_rdwr)
> @@ -12741,7 +12745,8 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
> if (is_kfunc_arg_uninit(btf, &args[i]))
> dynptr_arg_type |= MEM_UNINIT;
>
> - if (meta->func_id == special_kfunc_list[KF_bpf_dynptr_from_skb]) {
> + if (meta->func_id == special_kfunc_list[KF_bpf_dynptr_from_skb] ||
> + meta->func_id == special_kfunc_list[KF_bpf_dynptr_from_skb_rdonly]) {
> dynptr_arg_type |= DYNPTR_TYPE_SKB;
> } else if (meta->func_id == special_kfunc_list[KF_bpf_dynptr_from_xdp]) {
> dynptr_arg_type |= DYNPTR_TYPE_XDP;
> @@ -20898,9 +20903,7 @@ static void specialize_kfunc(struct bpf_verifier_env *env,
> u32 func_id, u16 offset, unsigned long *addr)
> {
> struct bpf_prog *prog = env->prog;
> - bool seen_direct_write;
> void *xdp_kfunc;
> - bool is_rdonly;
>
> if (bpf_dev_bound_kfunc_id(func_id)) {
> xdp_kfunc = bpf_dev_bound_resolve_kfunc(prog, func_id);
> @@ -20910,22 +20913,6 @@ static void specialize_kfunc(struct bpf_verifier_env *env,
> }
> /* fallback to default kfunc when not supported by netdev */
> }
> -
> - if (offset)
> - return;
> -
> - if (func_id == special_kfunc_list[KF_bpf_dynptr_from_skb]) {
> - seen_direct_write = env->seen_direct_write;
> - is_rdonly = !may_access_direct_pkt_data(env, NULL, BPF_WRITE);
> -
> - if (is_rdonly)
> - *addr = (unsigned long)bpf_dynptr_from_skb_rdonly;
> -
> - /* restore env->seen_direct_write to its original value, since
> - * may_access_direct_pkt_data mutates it
> - */
> - env->seen_direct_write = seen_direct_write;
> - }
> }
>
> static void __fixup_collection_insert_kfunc(struct bpf_insn_aux_data *insn_aux,
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 2ec162dd83c4..6416689e3976 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -12062,10 +12062,8 @@ __bpf_kfunc int bpf_sk_assign_tcp_reqsk(struct __sk_buff *s, struct sock *sk,
> #endif
> }
>
> -__bpf_kfunc_end_defs();
> -
> -int bpf_dynptr_from_skb_rdonly(struct __sk_buff *skb, u64 flags,
> - struct bpf_dynptr *ptr__uninit)
> +__bpf_kfunc int bpf_dynptr_from_skb_rdonly(struct __sk_buff *skb, u64 flags,
> + struct bpf_dynptr *ptr__uninit)
> {
> struct bpf_dynptr_kern *ptr = (struct bpf_dynptr_kern *)ptr__uninit;
> int err;
> @@ -12079,10 +12077,16 @@ int bpf_dynptr_from_skb_rdonly(struct __sk_buff *skb, u64 flags,
> return 0;
> }
>
> +__bpf_kfunc_end_defs();
> +
> BTF_KFUNCS_START(bpf_kfunc_check_set_skb)
> BTF_ID_FLAGS(func, bpf_dynptr_from_skb, KF_TRUSTED_ARGS)
> BTF_KFUNCS_END(bpf_kfunc_check_set_skb)
>
> +BTF_HIDDEN_KFUNCS_START(bpf_kfunc_check_hidden_set_skb)
> +BTF_ID_FLAGS(func, bpf_dynptr_from_skb_rdonly, KF_TRUSTED_ARGS)
> +BTF_KFUNCS_END(bpf_kfunc_check_hidden_set_skb)
> +
> BTF_KFUNCS_START(bpf_kfunc_check_set_xdp)
> BTF_ID_FLAGS(func, bpf_dynptr_from_xdp)
> BTF_KFUNCS_END(bpf_kfunc_check_set_xdp)
> @@ -12095,9 +12099,46 @@ BTF_KFUNCS_START(bpf_kfunc_check_set_tcp_reqsk)
> BTF_ID_FLAGS(func, bpf_sk_assign_tcp_reqsk, KF_TRUSTED_ARGS)
> BTF_KFUNCS_END(bpf_kfunc_check_set_tcp_reqsk)
>
> +BTF_ID_LIST(bpf_dynptr_from_skb_list)
> +BTF_ID(func, bpf_dynptr_from_skb)
> +BTF_ID(func, bpf_dynptr_from_skb_rdonly)
> +
> +static u32 bpf_kfunc_set_skb_remap(const struct bpf_prog *prog, u32 kfunc_id)
> +{
> + if (kfunc_id != bpf_dynptr_from_skb_list[0])
> + return 0;
> +
> + switch (resolve_prog_type(prog)) {
> + /* Program types only with direct read access go here! */
> + case BPF_PROG_TYPE_LWT_IN:
> + case BPF_PROG_TYPE_LWT_OUT:
> + case BPF_PROG_TYPE_LWT_SEG6LOCAL:
> + case BPF_PROG_TYPE_SK_REUSEPORT:
> + case BPF_PROG_TYPE_FLOW_DISSECTOR:
> + case BPF_PROG_TYPE_CGROUP_SKB:
This copy pastes the logic from may_access_direct_pkt_data(),
so any future change to that helper would need to update
this one as well.
> + return bpf_dynptr_from_skb_list[1];
The [0] and [1] stuff is quite error prone.
> +
> + /* Program types with direct read + write access go here! */
> + case BPF_PROG_TYPE_SCHED_CLS:
> + case BPF_PROG_TYPE_SCHED_ACT:
> + case BPF_PROG_TYPE_XDP:
> + case BPF_PROG_TYPE_LWT_XMIT:
> + case BPF_PROG_TYPE_SK_SKB:
> + case BPF_PROG_TYPE_SK_MSG:
> + case BPF_PROG_TYPE_CGROUP_SOCKOPT:
> + return kfunc_id;
> +
> + default:
> + break;
> + }
> + return bpf_dynptr_from_skb_list[1];
> +}
> +
> static const struct btf_kfunc_id_set bpf_kfunc_set_skb = {
> .owner = THIS_MODULE,
> .set = &bpf_kfunc_check_set_skb,
> + .hidden_set = &bpf_kfunc_check_hidden_set_skb,
If I'm reading it correctly the hidden_set serves no additional purpose.
It splits the set into two, but patch 4 just adds them together.
> + .remap = &bpf_kfunc_set_skb_remap,
I'm not a fan of callbacks in general.
The makes everything harder to follow.
For all these reasons I don't like this approach.
This "generality" doesn't make it cleaner or easier to extend.
For the patch 6... just repeat what specialize_kfunc()
currently does for dynptr ?
pw-bot: cr
