On Fri, Aug 16, 2024 at 8:39 AM Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: > > On Fri, Aug 16, 2024 at 3:43 PM Juntong Deng <juntong.deng@xxxxxxxxxxx> wrote: > > > > On 8/15/24 18:15, Andrii Nakryiko wrote: > > > On Thu, Aug 15, 2024 at 9:11 AM Juntong Deng <juntong.deng@xxxxxxxxxxx> wrote: > > >> > > >> Currently we cannot pass the pointer returned by iter next method as > > >> argument to KF_TRUSTED_ARGS kfuncs, because the pointer returned by > > >> iter next method is not "valid". > > >> > > >> This patch sets the pointer returned by iter next method to be valid. > > >> > > >> This is based on the fact that if the iterator is implemented correctly, > > >> then the pointer returned from the iter next method should be valid. > > >> > > >> This does not make NULL pointer valid. If the iter next method has > > >> KF_RET_NULL flag, then the verifier will ask the ebpf program to > > >> check NULL pointer. > > >> > > >> Signed-off-by: Juntong Deng <juntong.deng@xxxxxxxxxxx> > > >> --- > > >> kernel/bpf/verifier.c | 4 ++++ > > >> 1 file changed, 4 insertions(+) > > >> > > >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > >> index ebec74c28ae3..35a7b7c6679c 100644 > > >> --- a/kernel/bpf/verifier.c > > >> +++ b/kernel/bpf/verifier.c > > >> @@ -12832,6 +12832,10 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > > >> /* For mark_ptr_or_null_reg, see 93c230e3f5bd6 */ > > >> regs[BPF_REG_0].id = ++env->id_gen; > > >> } > > >> + > > >> + if (is_iter_next_kfunc(&meta)) > > >> + regs[BPF_REG_0].type |= PTR_TRUSTED; > > >> + > > > > > > It seems a bit too generic to always assign PTR_TRUSTED to anything > > > returned from any iterator. Let's maybe add KF_RET_TRUSTED or > > > KF_ITER_TRUSTED or something along those lines to mark such iter_next > > > kfuncs explicitly? > > > > > > For the numbers iterator, for instance, this PTR_TRUSTED makes no sense. > > > > > > > I had the same idea (KF_RET_TRUSTED) before, but Kumar thought it should > > be avoided and pointers returned by iter next method should be trusted > > by default [0]. > > > > The following are previous related discussions: > > > > >> For iter_next(), I currently have an idea to add new flags to allow > > >> iter_next() to decide whether the return value is trusted or not, > > >> such as KF_RET_TRUSTED. > > >> > > >> What do you think? > > > > > > Why shouldn't the return value always be trusted? > > > We eventually want to switch over to trusted by default everywhere. > > > It would be nice not to go further in the opposite direction (i.e. > > > having to manually annotate trusted) if we can avoid it. > > > > [0]: > > https://lore.kernel.org/bpf/CAP01T75na=fz7EhrP4Aw0WZ33R7jTbZ4BcmY56S1xTWczxHXWw@xxxxxxxxxxxxxx/ > > > > Maybe we can have more discussion? > > > > (This email has been CC Kumar) > > +1 > pointer from iterator should always be trusted except > the case of KF_RCU_PROTECTED iterators. > Those iters clear iter itself outside of RCU CS, > so a pointer returned from iter_next should probably be > PTR_TO_BTF_ID | MEM_RCU | PTR_MAYBE_NULL. > > For all other iters it should be safe to return > PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL > Ok, but we at some point might need to return a non-RCU/non-trusted pointer, so I guess we'll have to add yet another flag to opt-out of "trustedness"? > > For the numbers iterator, for instance, this PTR_TRUSTED makes no sense > > I see no conflict. It's a trusted pointer to u32.