diff mbox series

[v11,bpf-next,5/7] bpf: Use btf_kfunc_id_set.remap logic for bpf_dynptr_from_skb

Message ID 20250129205957.2457655-6-song@kernel.org (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Enable writing xattr from BPF programs | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 10 maintainers not CCed: sdf@fomichev.me edumazet@google.com jolsa@kernel.org yonghong.song@linux.dev horms@kernel.org kuba@kernel.org john.fastabend@gmail.com haoluo@google.com netdev@vger.kernel.org pabeni@redhat.com
netdev/build_clang success Errors and warnings before: 149 this patch: 149
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 35 this patch: 35
netdev/checkpatch warning WARNING: line length of 90 exceeds 80 columns WARNING: line length of 97 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 6 this patch: 6
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-11 success Logs for aarch64-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for s390x-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-19 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-gcc / veristat-kernel / x86_64-gcc veristat_kernel
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-gcc / veristat-meta / x86_64-gcc veristat_meta
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-17 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-17 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-44 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-45 success Logs for x86_64-llvm-18 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-46 success Logs for x86_64-llvm-18 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-43 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18

Commit Message

Song Liu Jan. 29, 2025, 8:59 p.m. UTC 
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@kernel.org>
---
 kernel/bpf/verifier.c | 25 ++++++----------------
 net/core/filter.c     | 49 +++++++++++++++++++++++++++++++++++++++----
 2 files changed, 51 insertions(+), 23 deletions(-)

Comments

Alexei Starovoitov Jan. 30, 2025, 2:32 a.m. UTC | #1 
On Wed, Jan 29, 2025 at 1:00 PM Song Liu <song@kernel.org> 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@kernel.org>
> ---
>  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
Song Liu Jan. 30, 2025, 5:49 p.m. UTC | #2 
Hi Alexei, 

Thanks for the review!

> On Jan 29, 2025, at 6:32 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

[...]

>> 
>> +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.

We can probably improve this with some helpers/macros. 

> 
>> +               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.

hidden_set does not have BTF_SET8_KFUNCS, so pahole will not export 
these kfuncs to vmlinux.h. 

> 
>> +       .remap = &bpf_kfunc_set_skb_remap,
> 
> I'm not a fan of callbacks in general.
> The makes everything harder to follow.

This motivation here is to move polymorphism logic from verifier
core to kfuncs owners. I guess we will need some callback to 
achieve this goal. Of course, we don't have to do it in this set. 
 

> 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 ?

Yes, specialize_kfunc() can handle this. But we will need to use
d_inode_locked_hooks from 6/7 in specialize_kfunc(). It works, 
but it is not clean (to me). 

I will revise this set so that the polymorphism logic in handled
in specialize_kfunc(). For longer term, maybe we should discuss 
"move some logic from verifier core to kfuncs" in the upcoming 
LSF/MM/BPF? 

Thanks,
Song
Alexei Starovoitov Jan. 30, 2025, 8:23 p.m. UTC | #3 
On Thu, Jan 30, 2025 at 9:49 AM Song Liu <songliubraving@meta.com> wrote:
>
> Hi Alexei,
>
> Thanks for the review!
>
> > On Jan 29, 2025, at 6:32 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>
> [...]
>
> >>
> >> +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.
>
> We can probably improve this with some helpers/macros.
>
> >
> >> +               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.
>
> hidden_set does not have BTF_SET8_KFUNCS, so pahole will not export
> these kfuncs to vmlinux.h.
>
> >
> >> +       .remap = &bpf_kfunc_set_skb_remap,
> >
> > I'm not a fan of callbacks in general.
> > The makes everything harder to follow.
>
> This motivation here is to move polymorphism logic from verifier
> core to kfuncs owners. I guess we will need some callback to
> achieve this goal. Of course, we don't have to do it in this set.
>
>
> > 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 ?
>
> Yes, specialize_kfunc() can handle this. But we will need to use
> d_inode_locked_hooks from 6/7 in specialize_kfunc(). It works,
> but it is not clean (to me).

I'm missing why that would be necessary to cross the layers
so much. I guess the code will tell.
Pls send an rfc to illustrate the unclean part.

> I will revise this set so that the polymorphism logic in handled
> in specialize_kfunc(). For longer term, maybe we should discuss
> "move some logic from verifier core to kfuncs" in the upcoming
> LSF/MM/BPF?

imo such topic is too narrow and detail oriented.
There is not much to gain from discussing it at lsfmm.
email works well for such discussions.
Song Liu Jan. 30, 2025, 9:24 p.m. UTC | #4 
> On Jan 30, 2025, at 12:23 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

[...]

>>> 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 ?
>> 
>> Yes, specialize_kfunc() can handle this. But we will need to use
>> d_inode_locked_hooks from 6/7 in specialize_kfunc(). It works,
>> but it is not clean (to me).
> 
> I'm missing why that would be necessary to cross the layers
> so much. I guess the code will tell.
> Pls send an rfc to illustrate the unclean part.

The actual code is actually a lot cleaner than I thought. We just
need to use the bpf_lsm_has_d_inode_locked() helper in verifier.c. 

Thanks,
Song

> 
>> I will revise this set so that the polymorphism logic in handled
>> in specialize_kfunc(). For longer term, maybe we should discuss
>> "move some logic from verifier core to kfuncs" in the upcoming
>> LSF/MM/BPF?
> 
> imo such topic is too narrow and detail oriented.
> There is not much to gain from discussing it at lsfmm.
> email works well for such discussions.
diff mbox series

Patch

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:
+		return bpf_dynptr_from_skb_list[1];
+
+	/* 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,
+	.remap = &bpf_kfunc_set_skb_remap,
 };
 
 static const struct btf_kfunc_id_set bpf_kfunc_set_xdp = {