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 > For the numbers iterator, for instance, this PTR_TRUSTED makes no sense I see no conflict. It's a trusted pointer to u32.
