From: Jiri Olsa <olsajiri@gmail.com>
To: Andrii Nakryiko <andrii@kernel.org>
Cc: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
martin.lau@kernel.org, viro@kernel.org,
linux-fsdevel@vger.kernel.org, brauner@kernel.org,
torvalds@linux-foundation.org
Subject: Re: [PATCH bpf-next 3/8] bpf: factor out fetching bpf_map from FD and adding it to used_maps list
Date: Wed, 14 Aug 2024 23:39:10 +0200 [thread overview]
Message-ID: <Zr0j_mYCtM-P-vlK@krava> (raw)
In-Reply-To: <20240813230300.915127-4-andrii@kernel.org>
On Tue, Aug 13, 2024 at 04:02:55PM -0700, Andrii Nakryiko wrote:
> Factor out the logic to extract bpf_map instances from FD embedded in
> bpf_insns, adding it to the list of used_maps (unless it's already
> there, in which case we just reuse map's index). This simplifies the
> logic in resolve_pseudo_ldimm64(), especially around `struct fd`
> handling, as all that is now neatly contained in the helper and doesn't
> leak into a dozen error handling paths.
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
> kernel/bpf/verifier.c | 115 ++++++++++++++++++++++++------------------
> 1 file changed, 66 insertions(+), 49 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index df3be12096cf..14e4ef687a59 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -18865,6 +18865,58 @@ static bool bpf_map_is_cgroup_storage(struct bpf_map *map)
> map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE);
> }
>
> +/* Add map behind fd to used maps list, if it's not already there, and return
> + * its index. Also set *reused to true if this map was already in the list of
> + * used maps.
> + * Returns <0 on error, or >= 0 index, on success.
> + */
> +static int add_used_map_from_fd(struct bpf_verifier_env *env, int fd, bool *reused)
> +{
> + struct fd f = fdget(fd);
using 'CLASS(fd, f)(fd)' would remove few fdput lines below?
jirka
> + struct bpf_map *map;
> + int i;
> +
> + map = __bpf_map_get(f);
> + if (IS_ERR(map)) {
> + verbose(env, "fd %d is not pointing to valid bpf_map\n", fd);
> + return PTR_ERR(map);
> + }
> +
> + /* check whether we recorded this map already */
> + for (i = 0; i < env->used_map_cnt; i++) {
> + if (env->used_maps[i] == map) {
> + *reused = true;
> + fdput(f);
> + return i;
> + }
> + }
> +
> + if (env->used_map_cnt >= MAX_USED_MAPS) {
> + verbose(env, "The total number of maps per program has reached the limit of %u\n",
> + MAX_USED_MAPS);
> + fdput(f);
> + return -E2BIG;
> + }
> +
> + if (env->prog->sleepable)
> + atomic64_inc(&map->sleepable_refcnt);
> +
> + /* hold the map. If the program is rejected by verifier,
> + * the map will be released by release_maps() or it
> + * will be used by the valid program until it's unloaded
> + * and all maps are released in bpf_free_used_maps()
> + */
> + bpf_map_inc(map);
> +
> + *reused = false;
> + env->used_maps[env->used_map_cnt++] = map;
> +
> + fdput(f);
> +
> + return env->used_map_cnt - 1;
> +
> +}
> +
> /* find and rewrite pseudo imm in ld_imm64 instructions:
> *
> * 1. if it accesses map FD, replace it with actual map pointer.
> @@ -18876,7 +18928,7 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
> {
> struct bpf_insn *insn = env->prog->insnsi;
> int insn_cnt = env->prog->len;
> - int i, j, err;
> + int i, err;
>
> err = bpf_prog_calc_tag(env->prog);
> if (err)
> @@ -18893,9 +18945,10 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
> if (insn[0].code == (BPF_LD | BPF_IMM | BPF_DW)) {
> struct bpf_insn_aux_data *aux;
> struct bpf_map *map;
> - struct fd f;
> + int map_idx;
> u64 addr;
> u32 fd;
> + bool reused;
>
> if (i == insn_cnt - 1 || insn[1].code != 0 ||
> insn[1].dst_reg != 0 || insn[1].src_reg != 0 ||
> @@ -18956,20 +19009,18 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
> break;
> }
>
> - f = fdget(fd);
> - map = __bpf_map_get(f);
> - if (IS_ERR(map)) {
> - verbose(env, "fd %d is not pointing to valid bpf_map\n", fd);
> - return PTR_ERR(map);
> - }
> + map_idx = add_used_map_from_fd(env, fd, &reused);
> + if (map_idx < 0)
> + return map_idx;
> + map = env->used_maps[map_idx];
> +
> + aux = &env->insn_aux_data[i];
> + aux->map_index = map_idx;
>
> err = check_map_prog_compatibility(env, map, env->prog);
> - if (err) {
> - fdput(f);
> + if (err)
> return err;
> - }
>
> - aux = &env->insn_aux_data[i];
> if (insn[0].src_reg == BPF_PSEUDO_MAP_FD ||
> insn[0].src_reg == BPF_PSEUDO_MAP_IDX) {
> addr = (unsigned long)map;
> @@ -18978,13 +19029,11 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
>
> if (off >= BPF_MAX_VAR_OFF) {
> verbose(env, "direct value offset of %u is not allowed\n", off);
> - fdput(f);
> return -EINVAL;
> }
>
> if (!map->ops->map_direct_value_addr) {
> verbose(env, "no direct value access support for this map type\n");
> - fdput(f);
> return -EINVAL;
> }
>
> @@ -18992,7 +19041,6 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
> if (err) {
> verbose(env, "invalid access to map value pointer, value_size=%u off=%u\n",
> map->value_size, off);
> - fdput(f);
> return err;
> }
>
> @@ -19003,70 +19051,39 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
> insn[0].imm = (u32)addr;
> insn[1].imm = addr >> 32;
>
> - /* check whether we recorded this map already */
> - for (j = 0; j < env->used_map_cnt; j++) {
> - if (env->used_maps[j] == map) {
> - aux->map_index = j;
> - fdput(f);
> - goto next_insn;
> - }
> - }
> -
> - if (env->used_map_cnt >= MAX_USED_MAPS) {
> - verbose(env, "The total number of maps per program has reached the limit of %u\n",
> - MAX_USED_MAPS);
> - fdput(f);
> - return -E2BIG;
> - }
> -
> - if (env->prog->sleepable)
> - atomic64_inc(&map->sleepable_refcnt);
> - /* hold the map. If the program is rejected by verifier,
> - * the map will be released by release_maps() or it
> - * will be used by the valid program until it's unloaded
> - * and all maps are released in bpf_free_used_maps()
> - */
> - bpf_map_inc(map);
> -
> - aux->map_index = env->used_map_cnt;
> - env->used_maps[env->used_map_cnt++] = map;
> + /* proceed with extra checks only if its newly added used map */
> + if (reused)
> + goto next_insn;
>
> if (bpf_map_is_cgroup_storage(map) &&
> bpf_cgroup_storage_assign(env->prog->aux, map)) {
> verbose(env, "only one cgroup storage of each type is allowed\n");
> - fdput(f);
> return -EBUSY;
> }
> if (map->map_type == BPF_MAP_TYPE_ARENA) {
> if (env->prog->aux->arena) {
> verbose(env, "Only one arena per program\n");
> - fdput(f);
> return -EBUSY;
> }
> if (!env->allow_ptr_leaks || !env->bpf_capable) {
> verbose(env, "CAP_BPF and CAP_PERFMON are required to use arena\n");
> - fdput(f);
> return -EPERM;
> }
> if (!env->prog->jit_requested) {
> verbose(env, "JIT is required to use arena\n");
> - fdput(f);
> return -EOPNOTSUPP;
> }
> if (!bpf_jit_supports_arena()) {
> verbose(env, "JIT doesn't support arena\n");
> - fdput(f);
> return -EOPNOTSUPP;
> }
> env->prog->aux->arena = (void *)map;
> if (!bpf_arena_get_user_vm_start(env->prog->aux->arena)) {
> verbose(env, "arena's user address must be set via map_extra or mmap()\n");
> - fdput(f);
> return -EINVAL;
> }
> }
>
> - fdput(f);
> next_insn:
> insn++;
> i++;
> --
> 2.43.5
>
>
next prev parent reply other threads:[~2024-08-14 21:39 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-08-13 23:02 [PATCH bpf-next 0/8] BPF follow ups to struct fd refactorings Andrii Nakryiko
2024-08-13 23:02 ` [PATCH bpf-next 1/8] bpf: convert __bpf_prog_get() to CLASS(fd, ...) Andrii Nakryiko
2024-08-13 23:02 ` [PATCH bpf-next 2/8] bpf: switch fdget_raw() uses to CLASS(fd_raw, ...) Andrii Nakryiko
2024-08-13 23:02 ` [PATCH bpf-next 3/8] bpf: factor out fetching bpf_map from FD and adding it to used_maps list Andrii Nakryiko
2024-08-14 21:39 ` Jiri Olsa [this message]
2024-08-14 23:17 ` Andrii Nakryiko
2024-08-13 23:02 ` [PATCH bpf-next 4/8] bpf: switch maps to CLASS(fd, ...) Andrii Nakryiko
2024-08-13 23:02 ` [PATCH bpf-next 5/8] bpf: trivial conversions for fdget() Andrii Nakryiko
2024-08-13 23:02 ` [PATCH bpf-next 6/8] bpf: more trivial fdget() conversions Andrii Nakryiko
2024-08-13 23:02 ` [PATCH bpf-next 7/8] security,bpf: constify struct path in bpf_token_create() LSM hook Andrii Nakryiko
2024-08-27 23:02 ` Andrii Nakryiko
2024-08-27 23:20 ` Paul Moore
2024-08-27 23:30 ` Andrii Nakryiko
2024-08-13 23:03 ` [PATCH bpf-next 8/8] bpf: convert bpf_token_create() to CLASS(fd, ...) Andrii Nakryiko
2024-08-27 22:55 ` [PATCH bpf-next 0/8] BPF follow ups to struct fd refactorings Andrii Nakryiko
2024-09-12 23:57 ` Al Viro
2024-09-13 0:10 ` Andrii Nakryiko
2024-09-13 0:18 ` Al Viro
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=Zr0j_mYCtM-P-vlK@krava \
--to=olsajiri@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=brauner@kernel.org \
--cc=daniel@iogearbox.net \
--cc=linux-fsdevel@vger.kernel.org \
--cc=martin.lau@kernel.org \
--cc=torvalds@linux-foundation.org \
--cc=viro@kernel.org \
/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