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

[David Vernet]

On Thu, Oct 20, 2022 at 11:52:29AM +0530, Kumar Kartikeya Dwivedi wrote:
> 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.

Yeah, in my defense, if you look back at [0] where I fat-fingered the
wrong if statement, I did say that I missed the case for helpers
specifically:

> Note that we also don't include PTR_TO_BTF_ID | PTR_UNTRUSTED here. The
> difference for PTR_TO_BTF_ID | PTR_WALK(ED) is of course that we also need to
> allow it to work properly for normal helper calls, so I'll make that change.

[0]: https://lore.kernel.org/all/Y1BR5c6W4tgljA8q@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/

Anyways, I think we're on the same page. I already have a local revision
that fixes this.

> 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.

I'd need to look over all of this code again to give useful suggestions
here (it's very difficult to keep it all paged in, there's just so much
context), but it'd be nice if we could somehow refactor some of this so
that check_helper_call() and check_kfunc_call() shared more code.
Obviously out of scope for this patch set.