[bpf-next,v1,0/5] Extend struct_ops support for operators

Message

Amery Hung Feb. 14, 2025, 4:45 p.m. UTC

Hi,

I am splitting the bpf qdisc patchset into smaller landable sets and
this is the first part.

This patchset supports struct_ops operators that acquire kptrs through
arguments and operators that return a kptr. A coming new struct_ops use
case, bpf qdisc [0], has two operators that are not yet supported by
current struct_ops infrastructure. Qdisc_ops::enqueue requires getting
referenced skb kptr from the argument; Qdisc_ops::dequeue needs to return
a referenced skb kptr. This patchset will allow bpf qdisc and other
potential struct_ops implementers to do so.

For struct_ops implementers:

- To get a kptr from an argument, a struct_ops implementer needs to
  annotate the argument name in the stub function with "__ref" suffix.

- The kptr return will automatically work as we now allow operators that
  return a struct pointer.

- The verifier allows returning a null pointer. More control can be
  added later if there is a future struct_ops implementer only expecting
  valid pointers.

For struct_ops users:

- The referenced kptr acquired through the argument needs to be released
  or xchged into maps just like ones acquired via kfuncs.

- To return a referenced kptr in struct_ops,
  1) The type of the pointer must matches the return type
  2) The pointer must comes from the kernel (not locally allocated), and
  3) The pointer must be in its unmodified form


[0] https://lore.kernel.org/bpf/[e-mail]/

---
v1
- Fix missing kfree for ctx_arg_info


Amery Hung (5):
  bpf: Make every prog keep a copy of ctx_arg_info
  bpf: Support getting referenced kptr from struct_ops argument
  selftests/bpf: Test referenced kptr arguments of struct_ops programs
  bpf: Allow struct_ops prog to return referenced kptr
  selftests/bpf: Test returning referenced kptr from struct_ops programs

 include/linux/bpf.h                           | 10 +-
 kernel/bpf/bpf_iter.c                         | 13 ++-
 kernel/bpf/bpf_struct_ops.c                   | 38 ++++++--
 kernel/bpf/btf.c                              |  1 +
 kernel/bpf/syscall.c                          |  2 +
 kernel/bpf/verifier.c                         | 96 +++++++++++++++----
 .../prog_tests/test_struct_ops_kptr_return.c  | 16 ++++
 .../prog_tests/test_struct_ops_refcounted.c   | 12 +++
 .../bpf/progs/struct_ops_kptr_return.c        | 30 ++++++
 ...uct_ops_kptr_return_fail__invalid_scalar.c | 26 +++++
 .../struct_ops_kptr_return_fail__local_kptr.c | 34 +++++++
 ...uct_ops_kptr_return_fail__nonzero_offset.c | 25 +++++
 .../struct_ops_kptr_return_fail__wrong_type.c | 30 ++++++
 .../bpf/progs/struct_ops_refcounted.c         | 31 ++++++
 ...ruct_ops_refcounted_fail__global_subprog.c | 39 ++++++++
 .../struct_ops_refcounted_fail__ref_leak.c    | 22 +++++
 .../selftests/bpf/test_kmods/bpf_testmod.c    | 15 +++
 .../selftests/bpf/test_kmods/bpf_testmod.h    |  6 ++
 18 files changed, 413 insertions(+), 33 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/test_struct_ops_kptr_return.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/test_struct_ops_refcounted.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_kptr_return.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_kptr_return_fail__invalid_scalar.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_kptr_return_fail__local_kptr.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_kptr_return_fail__nonzero_offset.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_kptr_return_fail__wrong_type.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_refcounted.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_refcounted_fail__global_subprog.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_refcounted_fail__ref_leak.c

Comments

Martin KaFai Lau Feb. 15, 2025, 2:09 a.m. UTC | #1

On 2/14/25 8:45 AM, Amery Hung wrote:
> Hi,
> 
> I am splitting the bpf qdisc patchset into smaller landable sets and
> this is the first part.
> 
> This patchset supports struct_ops operators that acquire kptrs through
> arguments and operators that return a kptr. A coming new struct_ops use
> case, bpf qdisc [0], has two operators that are not yet supported by
> current struct_ops infrastructure. Qdisc_ops::enqueue requires getting
> referenced skb kptr from the argument; Qdisc_ops::dequeue needs to return
> a referenced skb kptr. This patchset will allow bpf qdisc and other
> potential struct_ops implementers to do so.
> 
> For struct_ops implementers:
> 
> - To get a kptr from an argument, a struct_ops implementer needs to
>    annotate the argument name in the stub function with "__ref" suffix.
> 
> - The kptr return will automatically work as we now allow operators that
>    return a struct pointer.
> 
> - The verifier allows returning a null pointer. More control can be
>    added later if there is a future struct_ops implementer only expecting
>    valid pointers.
> 
> For struct_ops users:
> 
> - The referenced kptr acquired through the argument needs to be released
>    or xchged into maps just like ones acquired via kfuncs.
> 
> - To return a referenced kptr in struct_ops,
>    1) The type of the pointer must matches the return type
>    2) The pointer must comes from the kernel (not locally allocated), and
>    3) The pointer must be in its unmodified form

I only left some minor comments in the patches.

A few other thoughts:

I think https://lore.kernel.org/bpf/[e-mail]/
should be a good addition also. A new subtest in the prog_tests/pro_epilogue.c
should do for testing it.

Another thing is disabling tail call in the bpf_qdisc_get_func_proto in
https://lore.kernel.org/bpf/[e-mail]/
I am wondering if it can be done in a generic way for all struct_ops in
check_struct_ops_btf_id(). At that point, we should know all the "has_tail_call".
I meant only for ops with __ref arg such that it won't break the existing
struct_ops.

I think both of them could be a followup.

Amery Hung Feb. 18, 2025, 10:20 p.m. UTC | #2

On Fri, Feb 14, 2025 at 6:09 PM Martin KaFai Lau <[e-mail]> wrote:
>
> On 2/14/25 8:45 AM, Amery Hung wrote:
> > Hi,
> >
> > I am splitting the bpf qdisc patchset into smaller landable sets and
> > this is the first part.
> >
> > This patchset supports struct_ops operators that acquire kptrs through
> > arguments and operators that return a kptr. A coming new struct_ops use
> > case, bpf qdisc [0], has two operators that are not yet supported by
> > current struct_ops infrastructure. Qdisc_ops::enqueue requires getting
> > referenced skb kptr from the argument; Qdisc_ops::dequeue needs to return
> > a referenced skb kptr. This patchset will allow bpf qdisc and other
> > potential struct_ops implementers to do so.
> >
> > For struct_ops implementers:
> >
> > - To get a kptr from an argument, a struct_ops implementer needs to
> >    annotate the argument name in the stub function with "__ref" suffix.
> >
> > - The kptr return will automatically work as we now allow operators that
> >    return a struct pointer.
> >
> > - The verifier allows returning a null pointer. More control can be
> >    added later if there is a future struct_ops implementer only expecting
> >    valid pointers.
> >
> > For struct_ops users:
> >
> > - The referenced kptr acquired through the argument needs to be released
> >    or xchged into maps just like ones acquired via kfuncs.
> >
> > - To return a referenced kptr in struct_ops,
> >    1) The type of the pointer must matches the return type
> >    2) The pointer must comes from the kernel (not locally allocated), and
> >    3) The pointer must be in its unmodified form
>
> I only left some minor comments in the patches.
>
> A few other thoughts:
>
> I think https://lore.kernel.org/bpf/[e-mail]/
> should be a good addition also. A new subtest in the prog_tests/pro_epilogue.c
> should do for testing it.
>
> Another thing is disabling tail call in the bpf_qdisc_get_func_proto in
> https://lore.kernel.org/bpf/[e-mail]/
> I am wondering if it can be done in a generic way for all struct_ops in
> check_struct_ops_btf_id(). At that point, we should know all the "has_tail_call".
> I meant only for ops with __ref arg such that it won't break the existing
> struct_ops.
>
> I think both of them could be a followup.

Your suggestions make sense. I will follow-up on these two items with patches.

Thanks,
Amery