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 v8 1/3] bpf: Allow trusted pointers to be passed to KF_TRUSTED_ARGS kfuncs
Date: Sat, 19 Nov 2022 14:02:46 -0800 [thread overview]
Message-ID: <20221119220246.k4i3zp5wmsm6g2al@macbook-pro-5.dhcp.thefacebook.com> (raw)
In-Reply-To: <20221119210748.3325667-2-void@manifault.com>
On Sat, Nov 19, 2022 at 03:07:46PM -0600, David Vernet wrote:
> @@ -6887,6 +6895,7 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
> }
>
> reg->type = PTR_TO_MEM | PTR_MAYBE_NULL;
> +
No need to add empty line here.
> reg->id = ++env->id_gen;
>
> continue;
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 195d24316750..3a90a1c7613f 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -557,7 +557,7 @@ static bool is_cmpxchg_insn(const struct bpf_insn *insn)
> static const char *reg_type_str(struct bpf_verifier_env *env,
> enum bpf_reg_type type)
> {
> - char postfix[16] = {0}, prefix[32] = {0};
> + char postfix[16] = {0}, prefix[64] = {0};
> static const char * const str[] = {
> [NOT_INIT] = "?",
> [SCALAR_VALUE] = "scalar",
> @@ -589,16 +589,14 @@ static const char *reg_type_str(struct bpf_verifier_env *env,
> strncpy(postfix, "_or_null", 16);
> }
>
> - if (type & MEM_RDONLY)
> - strncpy(prefix, "rdonly_", 32);
> - if (type & MEM_RINGBUF)
> - strncpy(prefix, "ringbuf_", 32);
> - if (type & MEM_USER)
> - strncpy(prefix, "user_", 32);
> - if (type & MEM_PERCPU)
> - strncpy(prefix, "percpu_", 32);
> - if (type & PTR_UNTRUSTED)
> - strncpy(prefix, "untrusted_", 32);
> + snprintf(prefix, sizeof(prefix), "%s%s%s%s%s%s",
> + type & MEM_RDONLY ? "rdonly_" : "",
> + type & MEM_RINGBUF ? "ringbuf_" : "",
> + type & MEM_USER ? "user_" : "",
> + type & MEM_PERCPU ? "percpu_" : "",
> + type & PTR_UNTRUSTED ? "untrusted_" : "",
> + type & PTR_TRUSTED ? "trusted_" : ""
> + );
Nice. Could have been a separate patch, but ok.
>
> found:
> - if (reg->type == PTR_TO_BTF_ID) {
> + if (reg->type == PTR_TO_BTF_ID || (reg->type & PTR_TRUSTED)) {
No need for (). The operator precedence is pretty clear.
> /* For bpf_sk_release, it needs to match against first member
> * 'struct sock_common', hence make an exception for it. This
> * allows bpf_sk_release to work for multiple socket types.
> @@ -6058,6 +6070,8 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
> */
> case PTR_TO_BTF_ID:
> case PTR_TO_BTF_ID | MEM_ALLOC:
> + case PTR_TO_BTF_ID | PTR_TRUSTED:
> + case PTR_TO_BTF_ID | MEM_ALLOC | PTR_TRUSTED:
> /* When referenced PTR_TO_BTF_ID is passed to release function,
> * it's fixed offset must be 0. In the other cases, fixed offset
> * can be non-zero.
> @@ -7942,6 +7956,25 @@ static bool is_kfunc_arg_kptr_get(struct bpf_kfunc_call_arg_meta *meta, int arg)
> return arg == 0 && (meta->kfunc_flags & KF_KPTR_GET);
> }
>
> +static bool is_trusted_reg(const struct bpf_reg_state *reg)
> +{
> + /* A referenced register is always trusted. */
> + if (reg->ref_obj_id)
> + return true;
> +
> + /* If a register is not referenced, it is trusted if it has either the
> + * MEM_ALLOC or PTR_TRUSTED type modifiers, and no others. Some of the
> + * other type modifiers may be safe, but we elect to take an opt-in
> + * approach here as some (e.g. PTR_UNTRUSTED and PTR_MAYBE_NULL) are
> + * not.
> + *
> + * Eventually, we should make PTR_TRUSTED the single source of truth
> + * for whether a register is trusted.
> + */
> + return (type_flag(reg->type) & BPF_REG_TRUSTED_MODIFIERS) &&
No need for ().
> + !bpf_type_has_unsafe_modifiers(reg->type);
> +}
> +
...
> - if (is_kfunc_release(meta) && reg->ref_obj_id)
> + if (is_kfunc_release(meta) && reg->ref_obj_id) {
> arg_type |= OBJ_RELEASE;
> + if (bpf_type_has_unsafe_modifiers(reg->type)) {
> + verbose(env, "R%d release reg has unsafe modifiers\n", i);
> + return -EINVAL;
> + }
This part is a bit controversial, sicne it messes up the verifier messages.
Meaning that doing the check that early is losing important context.
> + }
> ret = check_func_arg_reg_off(env, reg, regno, arg_type);
> if (ret < 0)
> return ret;
> @@ -8705,7 +8745,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
> break;
> case KF_ARG_PTR_TO_BTF_ID:
> /* Only base_type is checked, further checks are done here */
> - if (reg->type != PTR_TO_BTF_ID &&
> + if (base_type(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);
With base_type() addition maybe the bpf_type_has_unsafe_modifiers() check
should be done here ?
Then test_verifier wouldn't need to change.
It's not the change itself that is a concern, but the loss of context in the messages.
I guess one can argue that erroring on PTR_TO_BTF_ID | PTR_MAYBE_NULL
with "reg has unsafe modifiers" is just as correct as saying
"expected pointer to btf or socket" a bit later.
Both could be improved.
If we keep it early while doing is_kfunc_release(meta) && reg->ref_obj_id
we could say:
"%s is not allowed in release function"
reg_type_str(env,reg->type)
Which for verifier/calls.c test case will be:
"ptr_prog_test_ref_kfunc_or_null is not allowed in release function"
If we do it later here it could be:
"arg#$d is %s. Expected %s or socket",
reg_type_str(env,reg->type)
reg_type_str(env,base_type(reg->type) | type_flag(reg->type) & ~BPF_REG_TRUSTED_MODIFIERS)
"arg#0 is ptr_prog_test_ref_kfunc_or_null. Expected ptr_prog_test_ref_kfunc or socket"
which is even better and it will make it easier for user to fix the code.
wdyt?
next prev parent reply other threads:[~2022-11-19 22:03 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-19 21:07 [PATCH bpf-next v8 0/3] Support storing struct task_struct objects as kptrs David Vernet
2022-11-19 21:07 ` [PATCH bpf-next v8 1/3] bpf: Allow trusted pointers to be passed to KF_TRUSTED_ARGS kfuncs David Vernet
2022-11-19 22:02 ` Alexei Starovoitov [this message]
2022-11-19 22:20 ` David Vernet
2022-11-19 21:07 ` [PATCH bpf-next v8 2/3] bpf: Add kfuncs for storing struct task_struct * as a kptr David Vernet
2022-11-19 21:07 ` [PATCH bpf-next v8 3/3] bpf/selftests: Add selftests for new task kfuncs David Vernet
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221119220246.k4i3zp5wmsm6g2al@macbook-pro-5.dhcp.thefacebook.com \
--to=alexei.starovoitov@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=haoluo@google.com \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kernel-team@fb.com \
--cc=kpsingh@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=martin.lau@linux.dev \
--cc=memxor@gmail.com \
--cc=sdf@google.com \
--cc=song@kernel.org \
--cc=tj@kernel.org \
--cc=void@manifault.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).