On Thu, Nov 04, 2021 at 06:25:03PM +0530, Kumar Kartikeya Dwivedi wrote:

On Wed, Nov 03, 2021 at 04:46:42AM IST, Alexei Starovoitov wrote:

quoted

On Sat, Oct 30, 2021 at 08:16:03PM +0530, Kumar Kartikeya Dwivedi wrote:

quoted

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.

I needed a sentinel to skip return type checking (otherwise check that return
type and prototype match) since existing kfunc don't have a
get_kfunc_return_type callback, but if we add bpf_func_proto support to kfunc
then we can probably convert existing kfuncs to that as well and skip all this
logic. Mostly needed it for RET_PTR_TO_BTF_ID_OR_NULL.

So it's just to special case r0=PTR_TO_BTF_ID_OR_NULL instead of
PTR_TO_BTF_ID that it's doing by default now?
Then could you use a btf_id list to whitelist all such funcs that needs _OR_NULL
variant and just do a search in that list in check_kfunc_call() ?
Instead of adding get_kfunc_return_type() callback.

Extending to support bpf_func_proto seemed like a bit of work so I wanted to get
some feedback first on all this, before working on it.

No need to hack into bpf_func_proto. All kernel funcs have BTF. It's all we need.
The _OR_NULL part we will eventually be able to express with btf_tag when
it's supported by both gcc and clang.

quoted

quoted

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 ?

Sorry, that's poorly phrased. Current kfunc doesn't support PTR_TO_MEM. I meant
it would be allowed now, with the way I implemented things, but there also isn't
a way to signal whether PTR_TO_BTF_ID is expected (hence the question about
bpf_func_proto). I did not understand why that was not done originally (maybe it
was lack of usecase). PTR_TO_CTX works because the type is matched with prog
type, so you can't pass something else there. For other cases the type of
register is considered.

Right. btf_check_kfunc_arg_match doesn't allow ptr_to_mem yet.
There is no signalling needed.
All args passed by the program into kfunc have to be either exact
PTR_TO_BTF_ID or conversions from PTR_TO_SOCK*.

Passing rX=PTR_TO_CTX into kfunc should not work. If I'm reading the code
correctly it's not allowed. I'm not sure why you're saying it can be done.
It's possible to pass PTR_TO_CTX into another bpf prog's global function.
The same btf_check_func_arg_match() helper checks both cases (global funcs and kfuncs).
Maybe that's where the confusion comes from?

Same with if (ptr_to_mem_ok). It's only for passing PTR_TO_MEM
into bpf prog's global function.
We can extend the verifier and allow PTR_TO_MEM into kfunc that
has 'long *' prototype, for example.
But it doesn't sound like the use case you have in mind.