> From: Roberto Sassu [mailto:roberto.sassu@xxxxxxxxxx] > Sent: Tuesday, August 2, 2022 6:01 PM > > 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. I added this topic for the agenda of BPF office hours tomorrow. Roberto
