Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 20 Oct 2022 11:52:29 +0530
From:   Kumar Kartikeya Dwivedi <memxor@gmail.com>
To:     David Vernet <void@manifault.com>
Cc:     bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
        andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
        yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org,
        sdf@google.com, haoluo@google.com, jolsa@kernel.org,
        linux-kernel@vger.kernel.org, kernel-team@fb.com, tj@kernel.org
Subject: Re: [PATCH v5 1/3] bpf: Allow trusted pointers to be passed to
 KF_TRUSTED_ARGS kfuncs
Message-ID: <20221020062229.b7e7r7lrnkszjoiy@apollo>
References: <20221014212133.2520531-1-void@manifault.com>
 <20221014212133.2520531-2-void@manifault.com>
 <CAP01T75FGW7F=Ho+oqoC6WgxK5uUir2=CUgiW_HwqNxmzmthBg@mail.gmail.com>
 <Y1BR5c6W4tgljA8q@maniforge.dhcp.thefacebook.com>
 <20221020055749.33lfipxtaubhnqbv@apollo>
 <Y1DmnQE0xuj1RDp7@maniforge.dhcp.thefacebook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y1DmnQE0xuj1RDp7@maniforge.dhcp.thefacebook.com>
Precedence: bulk

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.