Re: [PATCH bpf-next v1 0/5] Extend struct_ops support for operators

[Martin KaFai Lau <martin.lau@linux.dev>]

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/20250210174336.2024258-11-ameryhung@gmail.com/
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/20250210174336.2024258-9-ameryhung@gmail.com/
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.