From: David Vernet <void@manifault.com>
To: Joanne Koong <joannelkoong@gmail.com>
Cc: bpf@vger.kernel.org, kafai@fb.com, jolsa@kernel.org,
haoluo@google.com, andrii@kernel.org, daniel@iogearbox.net,
ast@kernel.org
Subject: Re: [PATCH bpf-next v2 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier
Date: Mon, 8 Aug 2022 14:14:33 -0700 [thread overview]
Message-ID: <20220808211433.oz3fuvtayfdwrnwi@dev0025.ash9.facebook.com> (raw)
In-Reply-To: <20220722175807.4038317-1-joannelkoong@gmail.com>
On Fri, Jul 22, 2022 at 10:58:06AM -0700, Joanne Koong wrote:
> When a data slice is obtained from a dynptr (through the bpf_dynptr_data API),
> the ref obj id of the dynptr must be found and then associated with the data
> slice.
>
> The ref obj id of the dynptr must be found *before* the caller saved regs are
> reset. Without this fix, the ref obj id tracking is not correct for
> dynptrs that are at an offset from the frame pointer.
>
> Please also note that the data slice's ref obj id must be assigned after the
> ret types are parsed, since RET_PTR_TO_ALLOC_MEM-type return regs get
> zero-marked.
>
> Fixes: 34d4ef5775f776ec4b0d53a02d588bf3195cada6 ("bpf: Add dynptr data slices");
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
Hi Joanne,
Overall this looks great, thanks. Just a couple small comments / questions.
> kernel/bpf/verifier.c | 62 ++++++++++++++++++++-----------------------
> 1 file changed, 29 insertions(+), 33 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c59c3df0fea6..29987b2ea26f 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5830,7 +5830,8 @@ static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state
>
> static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> struct bpf_call_arg_meta *meta,
> - const struct bpf_func_proto *fn)
> + const struct bpf_func_proto *fn,
> + int func_id)
Can we get the func_id from meta instead of adding another argument? It
looks like the func_id is stored there before we call check_func_arg.
> {
> u32 regno = BPF_REG_1 + arg;
> struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno];
> @@ -6040,23 +6041,33 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> }
>
> meta->uninit_dynptr_regno = regno;
> - } else if (!is_dynptr_reg_valid_init(env, reg, arg_type)) {
> - const char *err_extra = "";
> + } else {
> + if (!is_dynptr_reg_valid_init(env, reg, arg_type)) {
> + const char *err_extra = "";
>
> - switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
> - case DYNPTR_TYPE_LOCAL:
> - err_extra = "local ";
> - break;
> - case DYNPTR_TYPE_RINGBUF:
> - err_extra = "ringbuf ";
> - break;
> - default:
> - break;
> - }
> + switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
> + case DYNPTR_TYPE_LOCAL:
> + err_extra = "local ";
> + break;
> + case DYNPTR_TYPE_RINGBUF:
> + err_extra = "ringbuf ";
> + break;
> + default:
> + break;
> + }
>
> - verbose(env, "Expected an initialized %sdynptr as arg #%d\n",
> - err_extra, arg + 1);
> - return -EINVAL;
> + verbose(env, "Expected an initialized %sdynptr as arg #%d\n",
> + err_extra, arg + 1);
> + return -EINVAL;
> + }
> + if (func_id == BPF_FUNC_dynptr_data) {
> + if (meta->ref_obj_id) {
> + verbose(env, "verifier internal error: multiple refcounted args in BPF_FUNC_dynptr_data");
> + return -EFAULT;
> + }
> + /* Find the id of the dynptr we're tracking the reference of */
> + meta->ref_obj_id = stack_slot_get_id(env, reg);
> + }
> }
> break;
> case ARG_CONST_ALLOC_SIZE_OR_ZERO:
> @@ -7227,7 +7238,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> meta.func_id = func_id;
> /* check args */
> for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
> - err = check_func_arg(env, i, &meta, fn);
> + err = check_func_arg(env, i, &meta, fn, func_id);
> if (err)
> return err;
> }
> @@ -7457,7 +7468,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> if (type_may_be_null(regs[BPF_REG_0].type))
> regs[BPF_REG_0].id = ++env->id_gen;
>
> - if (is_ptr_cast_function(func_id)) {
> + if (is_ptr_cast_function(func_id) || func_id == BPF_FUNC_dynptr_data) {
Just a nit and my two cents, but IMO, is_ptr_cast_function() feels like a
bit of an unclear function name. It's only used for this specific if
statement, so maybe we should change that function name to something like
is_meta_stored_ref() and just add BPF_FUNC_dynptr_data to that list?
> /* For release_reference() */
> regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id;
> } else if (is_acquire_function(func_id, meta.map_ptr)) {
> @@ -7469,21 +7480,6 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> regs[BPF_REG_0].id = id;
> /* For release_reference() */
> regs[BPF_REG_0].ref_obj_id = id;
> - } else if (func_id == BPF_FUNC_dynptr_data) {
> - int dynptr_id = 0, i;
> -
> - /* Find the id of the dynptr we're acquiring a reference to */
> - for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
> - if (arg_type_is_dynptr(fn->arg_type[i])) {
> - if (dynptr_id) {
> - verbose(env, "verifier internal error: multiple dynptr args in func\n");
> - return -EFAULT;
> - }
> - dynptr_id = stack_slot_get_id(env, ®s[BPF_REG_1 + i]);
> - }
> - }
> - /* For release_reference() */
> - regs[BPF_REG_0].ref_obj_id = dynptr_id;
> }
>
> do_refine_retval_range(regs, fn->ret_type, func_id, &meta);
> --
> 2.30.2
>
Looks good otherwise, as mentioned above.
Thanks,
David
next prev parent reply other threads:[~2022-08-08 21:14 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-22 17:58 [PATCH bpf-next v2 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier Joanne Koong
2022-07-22 17:58 ` [PATCH bpf-next v2 2/2] selftests/bpf: add extra test for using dynptr data slice after release Joanne Koong
2022-07-25 19:15 ` Martin KaFai Lau
2022-07-25 19:10 ` [PATCH bpf-next v2 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier Martin KaFai Lau
2022-07-25 21:52 ` Joanne Koong
2022-08-08 21:14 ` David Vernet [this message]
2022-08-08 23:11 ` Joanne Koong
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=20220808211433.oz3fuvtayfdwrnwi@dev0025.ash9.facebook.com \
--to=void@manifault.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=haoluo@google.com \
--cc=joannelkoong@gmail.com \
--cc=jolsa@kernel.org \
--cc=kafai@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).