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.
