On Sat, Oct 30, 2021 at 08:16:03PM +0530, Kumar Kartikeya Dwivedi wrote: > This series adds unstable conntrack lookup helpers using BPF kfunc support. The > patch adding the lookup helper is based off of Maxim's recent patch to aid in > rebasing their series on top of this, all adjusted to work with kfunc support > [0]. > > This is an RFC series, as I'm unsure whether the reference tracking for > PTR_TO_BTF_ID will be accepted. Yes. The patches look good overall. Please don't do __BPF_RET_TYPE_MAX signalling. It's an ambiguous name. _MAX is typically used for a different purpose. Just give it an explicit name. I don't fully understand why that skip is needed though. Why it's not one of existing RET_*. Duplication of return and being lazy to propagate the correct ret value into get_kfunc_return_type ? > If not, we can go back to doing it the typical > way with PTR_TO_NF_CONN type, guarded with #if IS_ENABLED(CONFIG_NF_CONNTRACK). Please don't. We already have a ton of special and custom types in the verifier. refcnted PTR_TO_BTF_ID sounds as good way to scale it. > Also, I want to understand whether it would make sense to introduce > check_helper_call style bpf_func_proto based argument checking for kfuncs, or > continue with how it is right now, since it doesn't seem correct that PTR_TO_MEM > can be passed where PTR_TO_BTF_ID may be expected. Only PTR_TO_CTX is enforced. Do we really allow to pass PTR_TO_MEM argument into a function that expects PTR_TO_BTF_ID ? That sounds like a bug that we need to fix.
