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:   Sun, 20 Nov 2022 09:28:15 -0800
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
To:     David Vernet <void@manifault.com>
Cc:     ast@kernel.org, andrii@kernel.org, daniel@iogearbox.net,
        martin.lau@linux.dev, yhs@fb.com, song@kernel.org, sdf@google.com,
        john.fastabend@gmail.com, haoluo@google.com, jolsa@kernel.org,
        kpsingh@kernel.org, memxor@gmail.com, tj@kernel.org,
        bpf@vger.kernel.org, linux-kernel@vger.kernel.org,
        kernel-team@fb.com
Subject: Re: [PATCH bpf-next v9 2/4] bpf: Allow trusted pointers to be passed
 to KF_TRUSTED_ARGS kfuncs
Message-ID: <20221120172815.godn2rt22yk7j22z@macbook-pro-5.dhcp.thefacebook.com>
References: <20221120051004.3605026-1-void@manifault.com>
 <20221120051004.3605026-3-void@manifault.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221120051004.3605026-3-void@manifault.com>
Precedence: bulk

On Sat, Nov 19, 2022 at 11:10:02PM -0600, David Vernet wrote:
>  		case KF_ARG_PTR_TO_BTF_ID:
>  			/* Only base_type is checked, further checks are done here */
> -			if (reg->type != PTR_TO_BTF_ID &&
> -			    (!reg2btf_ids[base_type(reg->type)] || type_flag(reg->type))) {
> -				verbose(env, "arg#%d expected pointer to btf or socket\n", i);
> +			if ((base_type(reg->type) != PTR_TO_BTF_ID ||
> +			     bpf_type_has_unsafe_modifiers(reg->type)) &&
> +			    !reg2btf_ids[base_type(reg->type)]) {
> +				verbose(env, "arg#%d is %s ", i, reg_type_str(env, reg->type));
> +				verbose(env, "expected %s or socket\n",
> +					reg_type_str(env, base_type(reg->type) |
> +							  (type_flag(reg->type) & BPF_REG_TRUSTED_MODIFIERS)));
...
> diff --git a/tools/testing/selftests/bpf/verifier/calls.c b/tools/testing/selftests/bpf/verifier/calls.c
> index 86d6fef2e3b4..3193915c5ee6 100644
> --- a/tools/testing/selftests/bpf/verifier/calls.c
> +++ b/tools/testing/selftests/bpf/verifier/calls.c
> @@ -109,7 +109,7 @@
>  	},
>  	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
>  	.result = REJECT,
> -	.errstr = "arg#0 expected pointer to btf or socket",
> +	.errstr = "arg#0 is ptr_or_null_ expected ptr_ or socket",

Nice.
I missed the fact that reg_type_str() prints only the type.
We see more verbose prints in print_verifier_state():
  verbose(env, "%s", reg_type_str(env, t));
  if (base_type(t) == PTR_TO_BTF_ID)
          verbose(env, "%s", kernel_type_name(reg->btf, reg->btf_id));
Since reg_type_str() prints into a buffer maybe we can enhance it with
struct name printing too?
Not urgent.
The set looks great. Applied.
There is an odd arm64 failure in bonding test reported by CI, but looks unrelated.