On Tue, Aug 2, 2022 at 3:07 AM Roberto Sassu <roberto.sassu@xxxxxxxxxx> wrote: > > > From: Alexei Starovoitov [mailto:alexei.starovoitov@xxxxxxxxx] > > Sent: Tuesday, August 2, 2022 6:46 AM > > > > On Thu, Jul 28, 2022 at 2:02 AM Kumar Kartikeya Dwivedi > > <memxor@xxxxxxxxx> wrote: > > > > > > On Thu, 28 Jul 2022 at 10:45, Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> > > wrote: > > > > > > > > On Thu, 28 Jul 2022 at 10:18, Roberto Sassu <roberto.sassu@xxxxxxxxxx> > > wrote: > > > > > > > > > > > From: Roberto Sassu [mailto:roberto.sassu@xxxxxxxxxx] > > > > > > Sent: Thursday, July 28, 2022 9:46 AM > > > > > > > From: Kumar Kartikeya Dwivedi [mailto:memxor@xxxxxxxxx] > > > > > > > Sent: Wednesday, July 27, 2022 10:16 AM > > > > > > > Similar to how we detect mem, size pairs in kfunc, teach verifier to > > > > > > > treat __ref suffix on argument name to imply that it must be a trusted > > > > > > > arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS > > flag > > > > > > > but limited to the specific parameter. This is required to ensure that > > > > > > > kfunc that operate on some object only work on acquired pointers and > > not > > > > > > > normal PTR_TO_BTF_ID with same type which can be obtained by > > pointer > > > > > > > walking. Release functions need not specify such suffix on release > > > > > > > arguments as they are already expected to receive one referenced > > > > > > > argument. > > > > > > > > > > > > Thanks, Kumar. I will try it. > > > > > > > > > > Uhm. I realized that I was already using another suffix, > > > > > __maybe_null, to indicate that a caller can pass NULL as > > > > > argument. > > > > > > > > > > Wouldn't probably work well with two suffixes. > > > > > > > > > > > > > Then you can maybe extend it to parse two suffixes at most (for now > > atleast)? > > > > > > > > > Have you considered to extend BTF_ID_FLAGS to take five > > > > > extra arguments, to set flags for each kfunc parameter? > > > > > > > > > > > > > I didn't understand this. Flags parameter is an OR of the flags you > > > > set, why would we want to extend it to take 5 args? > > > > You can just or f1 | f2 | f3 | f4 | f5, as many as you want. > > > > > > Oh, so you mean having 5 more args to indicate flags on each > > > parameter? It is possible, but I think the scheme for now works ok. If > > > you extend it to parse two suffixes, it should be fine. Yes, the > > > variable name would be ugly, but you can just make a copy into a > > > properly named one. This is the best we can do without switching to > > > BTF tags. We can revisit this when we start having 4 or 5 tags on a > > > single parameter. > > > > > > To make it a bit less verbose you could probably call maybe_null just null? > > > > Thank you for posting the patch. > > It still feels that this extra flexibility gets convoluted. > > I'm not sure Roberto's kfunc actually needs __ref. > > All pointers should be pointers. Hacking -1 and -2 into a pointer > > is something that key infra did, but it doesn't mean that > > we have to carry over it into bpf kfunc. > > There is a separate parameter for the keyring IDs that only > verify_pkcs7_signature() understands. Type casting is done > internally in the bpf_verify_pkcs7_signature() kfunc. > > The other is always a valid struct key pointer or NULL, coming > from bpf_lookup_user_key() (acquire function). I extended > Kumar's patch further to annotate the struct key parameter > with the _ref_null suffix, to accept a referenced pointer or NULL, > instead of just referenced. I don't think it's a good tradeoff complexity wise. !=null check can be done in runtime by the helper. The type cast is a sign of something fishy in the design.
