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)
mark_btf_func_reg_size(env, BPF_REG_0, sizeof(void *));
if (is_kfunc_acquire(&meta)) {
int id = acquire_reference_state(env, insn_idx);
--
2.39.2
