On 11/26/24 11:56 AM, Amery Hung wrote:
I have a use case where I would like to store sk_buff pointers as kptrs in eBPF map. To do so, I am borrowing skb kfuncs for acquire/release/destroy from Amery Hung's bpf qdisc set [0], but they are registered for BPF_PROG_TYPE_SCHED_CLS programs. TL;DR - due to following callstack: do_check() check_kfunc_call() check_kfunc_args() get_kfunc_ptr_arg_type() btf_is_prog_ctx_type() btf_is_projection_of() -- return true sk_buff argument is being interpreted as KF_ARG_PTR_TO_CTX, but what we have there is KF_ARG_PTR_TO_BTF_ID. Verifier is unhappy about it. Should
I don't think I fully understand "what we have there is KF_ARG_PTR_TO_BTF_ID". I am trying to guess you meant what we have there in the reg->type is in (PTR_TO_BTF_ID | PTR_TRUSTED).
It makes sense to have "struct sk_buff __kptr *" instead of "struct __sk_buff __kptr *". However, the get_kfunc_ptr_arg_type() is expecting KF_ARG_PTR_TO_CTX because the prog type is BPF_PROG_TYPE_SCHED_CLS.
From a very quick look, under the "case KF_ARG_PTR_TO_CTX:" in check_kfunc_args(), I think it needs to teach the verifier that the reg->type with a trusted PTR_TO_BTF_ID ("struct sk_buff *") can be used as the PTR_TO_CTX.
this be workarounded via some typedef or adding mentioned kfuncs to special_kfunc_list ? If the latter, then what else needs to be handled? Commenting out sk_buff part from btf_is_projection_of() makes it work, but that probably is not a solution:) Another question is in case bpf qdisc set lands, could we have these kfuncs not being limited to BPF_PROG_TYPE_STRUCT_OPS ?
Similar to Amery's comment. Please share the patch and user case. It will be easier to discuss.
In bpf qdisc case, we are still working on releasing skb kptrs in maps or graphs automatically when .reset is called so that we don't hold the resources forever.
Regarding specifically the bpf qdisc case, the .reset should do the right thing to release the queued skb. imo, after sleeping on it, if the bpf prog missed releasing the skb, it is fine to depend on the map destruction to finally release them. It is the same as other kptrs type stored in the map which will also be finally released during map_free.
In the future, for the struct_ops case, it can be improved by allowing to define the sch->privdata. May be allow to define the layout of this privdata, e.g. the whole privdata is a one element map backed by a btf id. The implementation will need to be generic enough for any bpf_struct_ops instead of something specific to the bpf-qdisc. This can be a follow up improvement as a more seamless per sch instance cleanup after the core bpf-qdisc pieces landed.
