Re: [PATCH v5 1/3] bpf: Allow trusted pointers to be passed to KF_TRUSTED_ARGS kfuncs

[Kumar Kartikeya Dwivedi]

On Thu, Oct 20, 2022 at 11:41:41AM IST, David Vernet wrote:
> [...]
> Apologies, as mentioned below I pasted the wrong if-check on accident.
> If I had actually meant to paste that one, then saying I "misunderstand
> this a bit" would have been a very generous understatment :-)
>
> > When you have task from tracing ctx arg:
> > r1 = ctx;
> > r1 = *(r1 + ...); // PTR_TO_BTF_ID, task_struct, off=0
> > // r1 = task->next
> > r1 = *(r1 + offsetof(task_struct, next)); // PTR_TO_BTF_ID | PTR_WALKED, task_struct, off = 0
> >
> > We loaded a pointer from task_struct into r1.
> > Now r1 still points to a task_struct, so that check above won't fail for r1.
>
> I meant to paste the if-condition _above_ that one. This is the if-check
> we'll fail due to the presence of a type modifier (PTR_WALKED):
>
> 	} else if (is_kfunc && (reg->type == PTR_TO_BTF_ID ||
> 		   (reg2btf_ids[base_type(reg->type)] && !type_flag(reg->type)))) {
> 		const struct btf_type *reg_ref_t;
> 		const struct btf *reg_btf;
> 		const char *reg_ref_tname;
> 		u32 reg_ref_id;
>
> So we'll never even get to the if check I originally pasted because
> reg->type == PTR_TO_BTF_ID will fail for a PTR_WALKED reg. And then
> below we'll eventually fail later on here:
>
> 	/* Permit pointer to mem, but only when argument
> 	 * type is pointer to scalar, or struct composed
> 	 * (recursively) of scalars.
> 	 * When arg_mem_size is true, the pointer can be
> 	 * void *.
> 	 * Also permit initialized local dynamic pointers.
> 	 */
> 	if (!btf_type_is_scalar(ref_t) &&
> 	    !__btf_type_is_scalar_struct(log, btf, ref_t, 0) &&
> 	    !arg_dynptr &&
> 	    (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) {
> 		bpf_log(log,
> 			"arg#%d pointer type %s %s must point to %sscalar, or struct with scalar\n",
> 			i, btf_type_str(ref_t), ref_tname, arg_mem_size ? "void, " : "");
> 		return -EINVAL;
> 	}
>
> Appreciate the explanation, sorry to have made you type it.
>

Ah, I see. Your analysis is right, but the error in CI comes from
check_func_arg_reg_off invocation in check_helper_call, this code is for kfuncs.

Since you have this to preserve backwards compat:
+static const struct bpf_reg_types btf_ptr_types = {
+	.types = {
+		PTR_TO_BTF_ID,
+		PTR_TO_BTF_ID | PTR_NESTED
+	},
+};

It allows passing those with PTR_NESTED to stable helpers.