From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: Alexei Starovoitov <ast@kernel.org>,
john fastabend <john.fastabend@gmail.com>,
Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH bpf-next 5/8] bpf: add poke dependency tracking for prog array maps
Date: Wed, 20 Nov 2019 10:56:27 -0800 [thread overview]
Message-ID: <CAEf4BzaVvPa5dLfPCfiXik9tnbXZaW2omxXiFwdJbFb7s1Z=PQ@mail.gmail.com> (raw)
In-Reply-To: <41436f9a5d5dd8ef5e88e05b1439e68428fdf2a3.1574126683.git.daniel@iogearbox.net>
On Mon, Nov 18, 2019 at 5:38 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> This work adds program tracking to prog array maps. This is needed such
> that upon prog array updates/deletions we can fix up all programs which
> make use of this tail call map. We add ops->map_poke_{un,}track() helpers
> to maps to maintain the list of programs and ops->map_poke_run() for
> triggering the actual update. bpf_array_aux is extended to contain the
> list head and poke_mutex in order to serialize program patching during
> updates/deletions. bpf_free_used_maps() will untrack the program shortly
> before dropping the reference to the map.
>
> The prog_array_map_poke_run() is triggered during updates/deletions and
> walks the maintained prog list. It checks in their poke_tabs whether the
> map and key is matching and runs the actual bpf_arch_text_poke() for
> patching in the nop or new jmp location. Depending on the type of update,
> we use one of BPF_MOD_{NOP_TO_JUMP,JUMP_TO_NOP,JUMP_TO_JUMP}.
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
> include/linux/bpf.h | 36 ++++++++++
> kernel/bpf/arraymap.c | 151 +++++++++++++++++++++++++++++++++++++++++-
> kernel/bpf/core.c | 9 ++-
> 3 files changed, 193 insertions(+), 3 deletions(-)
>
[...]
> #endif /* _LINUX_BPF_H */
> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index 5be12db129cc..d2b559c6659e 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -586,10 +586,14 @@ int bpf_fd_array_map_update_elem(struct bpf_map *map, struct file *map_file,
> if (IS_ERR(new_ptr))
> return PTR_ERR(new_ptr);
>
> + bpf_map_poke_lock(map);
> old_ptr = xchg(array->ptrs + index, new_ptr);
> + if (map->ops->map_poke_run)
> + map->ops->map_poke_run(map, index, old_ptr, new_ptr);
> + bpf_map_poke_unlock(map);
so this is a bit subtle, if I understand correctly. I was originally
going to suggest that if no map->ops->map_poke_run is set, then
bpf_map_pole_{lock,unlock} shouldn't be called at all. But then I
realized that this creates a race, where xchg can happen in different
order than map_poke_runs. Am I right?
If yes, I wonder if it will be better to express this logic more
explicitly as below, to avoid someone else "optimizing" the code
later:
if (map->ops->map_poke_run) {
bpf_map_poke_lock(map);
old_ptr = xchg(array->ptrs + index, new_ptr);
bpf_map_poke_unlock(map);
} else {
old_ptr = xchg(array->ptrs + index, new_ptr);
}
This will make it more apparent that something different is happing
when poke tracking is supported by a map.
Am I overthinking this?
> +
> if (old_ptr)
> map->ops->map_fd_put_ptr(old_ptr);
> -
> return 0;
> }
>
[...]
> +static void prog_array_map_poke_untrack(struct bpf_map *map,
> + struct bpf_prog_aux *prog_aux)
> +{
> + struct prog_poke_elem *elem, *tmp;
> + struct bpf_array_aux *aux;
> +
> + aux = container_of(map, struct bpf_array, map)->aux;
> + mutex_lock(&aux->poke_mutex);
> + list_for_each_entry_safe(elem, tmp, &aux->poke_progs, list) {
> + if (elem->aux == prog_aux) {
> + list_del_init(&elem->list);
> + kfree(elem);
break; ?
> + }
> + }
> + mutex_unlock(&aux->poke_mutex);
> +}
> +
[...]
> +
> + ret = bpf_arch_text_poke(poke->ip, type,
> + old ? (u8 *)old->bpf_func +
> + poke->adj_off : NULL,
> + new ? (u8 *)new->bpf_func +
> + poke->adj_off : NULL);
nit: extract old/new address calculation, so it's not multi-line
wrapped? It's a bit hard to follow.
> + BUG_ON(ret < 0 && ret != -EINVAL);
> + }
> + }
> +}
> +
[...]
next prev parent reply other threads:[~2019-11-20 18:56 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-19 1:38 [PATCH bpf-next 0/8] Optimize BPF tail calls for direct jumps Daniel Borkmann
2019-11-19 1:38 ` [PATCH bpf-next 1/8] bpf, x86: generalize and extend bpf_arch_text_poke " Daniel Borkmann
2019-11-20 18:11 ` Andrii Nakryiko
2019-11-19 1:38 ` [PATCH bpf-next 2/8] bpf: move bpf_free_used_maps into sleepable section Daniel Borkmann
2019-11-19 1:38 ` [PATCH bpf-next 3/8] bpf: move owner type,jited info into array auxiliary data Daniel Borkmann
2019-11-19 1:38 ` [PATCH bpf-next 4/8] bpf: add initial poke descriptor table for jit images Daniel Borkmann
2019-11-20 18:34 ` Andrii Nakryiko
2019-11-19 1:38 ` [PATCH bpf-next 5/8] bpf: add poke dependency tracking for prog array maps Daniel Borkmann
2019-11-20 18:56 ` Andrii Nakryiko [this message]
2019-11-19 1:38 ` [PATCH bpf-next 6/8] bpf: constant map key tracking for prog array pokes Daniel Borkmann
2019-11-20 19:02 ` Andrii Nakryiko
2019-11-19 1:38 ` [PATCH bpf-next 7/8] bpf, x86: emit patchable direct jump as tail call Daniel Borkmann
2019-11-20 19:16 ` Andrii Nakryiko
2019-11-19 1:38 ` [PATCH bpf-next 8/8] bpf, testing: add various tail call test cases Daniel Borkmann
2019-11-20 19:26 ` Andrii Nakryiko
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='CAEf4BzaVvPa5dLfPCfiXik9tnbXZaW2omxXiFwdJbFb7s1Z=PQ@mail.gmail.com' \
--to=andrii.nakryiko@gmail.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=john.fastabend@gmail.com \
--cc=netdev@vger.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;
as well as URLs for NNTP newsgroup(s).