> From: Alexei Starovoitov [mailto:alexei.starovoitov@xxxxxxxxx] > Sent: Tuesday, August 2, 2022 5:06 PM > 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. Not sure I follow. bpf_lookup_user_key() might not find the key you asked for, so it will return NULL. Did you mean that I should not set KF_RET_NULL for bpf_lookup_user_key()? That anyway won’t help if you use the system keyring, the alternative parameter. The user keyring is the other one, returned by bpf_lookup_user_key(). When you specify a system keyring, the user keyring should be NULL, to indicate that the parameter should not be used. This was suggested by both John and Daniel. So, it seems unavoidable to annotate the keyring parameter with __ref_null, or at least with __null. __ref_null would be better, to require a referenced parameter. > The type cast is a sign of something fishy in the design. Since this is what verify_pkcs7_signature() accepts, I guess it is the only option. Roberto
