From: Jiri Olsa <olsajiri@gmail.com>
To: Song Liu <song@kernel.org>
Cc: bpf@vger.kernel.org, linux-kernel@vger.kernel.org,
live-patching@vger.kernel.org, daniel@iogearbox.net,
kernel-team@fb.com, rostedt@goodmis.org
Subject: Re: [PATCH v5 bpf-next 4/4] bpf: Support bpf_trampoline on functions with IPMODIFY (e.g. livepatch)
Date: Fri, 22 Jul 2022 09:31:05 +0200 [thread overview]
Message-ID: <YtpSOTlpHbKOKTDJ@krava> (raw)
In-Reply-To: <20220720002126.803253-5-song@kernel.org>
On Tue, Jul 19, 2022 at 05:21:26PM -0700, Song Liu wrote:
SNIP
> + tr->flags |= BPF_TRAMP_F_SHARE_IPMODIFY;
> +
> + if ((tr->flags & BPF_TRAMP_F_CALL_ORIG) &&
> + !(tr->flags & BPF_TRAMP_F_ORIG_STACK))
> + ret = bpf_trampoline_update(tr, false /* lock_direct_mutex */);
> + break;
> + case FTRACE_OPS_CMD_DISABLE_SHARE_IPMODIFY_PEER:
> + tr->flags &= ~BPF_TRAMP_F_SHARE_IPMODIFY;
> +
> + if (tr->flags & BPF_TRAMP_F_ORIG_STACK)
> + ret = bpf_trampoline_update(tr, false /* lock_direct_mutex */);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + };
> +
> + mutex_unlock(&tr->mutex);
> + return ret;
> +}
> +#endif
> +
> bool bpf_prog_has_trampoline(const struct bpf_prog *prog)
> {
> enum bpf_attach_type eatype = prog->expected_attach_type;
> @@ -89,6 +165,16 @@ static struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
> tr = kzalloc(sizeof(*tr), GFP_KERNEL);
> if (!tr)
> goto out;
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> + tr->fops = kzalloc(sizeof(struct ftrace_ops), GFP_KERNEL);
> + if (!tr->fops) {
> + kfree(tr);
> + tr = NULL;
> + goto out;
> + }
would it be easier to put ftrace_ops directly to bpf_trampoline,
not just pointer.. it's allocated and freed at the same point
I recall there were some include issues when I tried that long
time ago [1], but could make the change bit simpler
[1] https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git/commit/?h=bpf/batch&id=52a1d4acdf55df41e99ca2cea51865e6821036ce
jirka
> + tr->fops->private = tr;
> + tr->fops->ops_func = bpf_tramp_ftrace_ops_func;
> +#endif
>
> tr->key = key;
> INIT_HLIST_NODE(&tr->hlist);
> @@ -128,7 +214,7 @@ static int unregister_fentry(struct bpf_trampoline *tr, void *old_addr)
> int ret;
>
> if (tr->func.ftrace_managed)
> - ret = unregister_ftrace_direct((long)ip, (long)old_addr);
> + ret = unregister_ftrace_direct_multi(tr->fops, (long)old_addr);
> else
> ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, old_addr, NULL);
>
> @@ -137,15 +223,20 @@ static int unregister_fentry(struct bpf_trampoline *tr, void *old_addr)
> return ret;
> }
>
> -static int modify_fentry(struct bpf_trampoline *tr, void *old_addr, void *new_addr)
> +static int modify_fentry(struct bpf_trampoline *tr, void *old_addr, void *new_addr,
> + bool lock_direct_mutex)
> {
> void *ip = tr->func.addr;
> int ret;
>
> - if (tr->func.ftrace_managed)
> - ret = modify_ftrace_direct((long)ip, (long)old_addr, (long)new_addr);
> - else
> + if (tr->func.ftrace_managed) {
> + if (lock_direct_mutex)
> + ret = modify_ftrace_direct_multi(tr->fops, (long)new_addr);
> + else
> + ret = modify_ftrace_direct_multi_nolock(tr->fops, (long)new_addr);
> + } else {
> ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, old_addr, new_addr);
> + }
> return ret;
> }
>
> @@ -163,10 +254,12 @@ static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
> if (bpf_trampoline_module_get(tr))
> return -ENOENT;
>
> - if (tr->func.ftrace_managed)
> - ret = register_ftrace_direct((long)ip, (long)new_addr);
> - else
> + if (tr->func.ftrace_managed) {
> + ftrace_set_filter_ip(tr->fops, (unsigned long)ip, 0, 0);
> + ret = register_ftrace_direct_multi(tr->fops, (long)new_addr);
> + } else {
> ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, NULL, new_addr);
> + }
>
> if (ret)
> bpf_trampoline_module_put(tr);
> @@ -332,11 +425,11 @@ static struct bpf_tramp_image *bpf_tramp_image_alloc(u64 key, u32 idx)
> return ERR_PTR(err);
> }
>
> -static int bpf_trampoline_update(struct bpf_trampoline *tr)
> +static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mutex)
> {
> struct bpf_tramp_image *im;
> struct bpf_tramp_links *tlinks;
> - u32 flags = BPF_TRAMP_F_RESTORE_REGS;
> + u32 orig_flags = tr->flags;
> bool ip_arg = false;
> int err, total;
>
> @@ -358,18 +451,31 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
> goto out;
> }
>
> + /* clear all bits except SHARE_IPMODIFY */
> + tr->flags &= BPF_TRAMP_F_SHARE_IPMODIFY;
> +
> if (tlinks[BPF_TRAMP_FEXIT].nr_links ||
> - tlinks[BPF_TRAMP_MODIFY_RETURN].nr_links)
> + tlinks[BPF_TRAMP_MODIFY_RETURN].nr_links) {
> /* NOTE: BPF_TRAMP_F_RESTORE_REGS and BPF_TRAMP_F_SKIP_FRAME
> * should not be set together.
> */
> - flags = BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_SKIP_FRAME;
> + tr->flags |= BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_SKIP_FRAME;
> + } else {
> + tr->flags |= BPF_TRAMP_F_RESTORE_REGS;
> + }
>
> if (ip_arg)
> - flags |= BPF_TRAMP_F_IP_ARG;
> + tr->flags |= BPF_TRAMP_F_IP_ARG;
> +
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> +again:
> + if ((tr->flags & BPF_TRAMP_F_SHARE_IPMODIFY) &&
> + (tr->flags & BPF_TRAMP_F_CALL_ORIG))
> + tr->flags |= BPF_TRAMP_F_ORIG_STACK;
> +#endif
>
> err = arch_prepare_bpf_trampoline(im, im->image, im->image + PAGE_SIZE,
> - &tr->func.model, flags, tlinks,
> + &tr->func.model, tr->flags, tlinks,
> tr->func.addr);
> if (err < 0)
> goto out;
> @@ -378,17 +484,34 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
> WARN_ON(!tr->cur_image && tr->selector);
> if (tr->cur_image)
> /* progs already running at this address */
> - err = modify_fentry(tr, tr->cur_image->image, im->image);
> + err = modify_fentry(tr, tr->cur_image->image, im->image, lock_direct_mutex);
> else
> /* first time registering */
> err = register_fentry(tr, im->image);
> +
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> + if (err == -EAGAIN) {
> + /* -EAGAIN from bpf_tramp_ftrace_ops_func. Now
> + * BPF_TRAMP_F_SHARE_IPMODIFY is set, we can generate the
> + * trampoline again, and retry register.
> + */
> + /* reset fops->func and fops->trampoline for re-register */
> + tr->fops->func = NULL;
> + tr->fops->trampoline = 0;
> + goto again;
> + }
> +#endif
> if (err)
> goto out;
> +
> if (tr->cur_image)
> bpf_tramp_image_put(tr->cur_image);
> tr->cur_image = im;
> tr->selector++;
> out:
> + /* If any error happens, restore previous flags */
> + if (err)
> + tr->flags = orig_flags;
> kfree(tlinks);
> return err;
> }
> @@ -454,7 +577,7 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr
>
> hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]);
> tr->progs_cnt[kind]++;
> - err = bpf_trampoline_update(tr);
> + err = bpf_trampoline_update(tr, true /* lock_direct_mutex */);
> if (err) {
> hlist_del_init(&link->tramp_hlist);
> tr->progs_cnt[kind]--;
> @@ -487,7 +610,7 @@ static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_
> }
> hlist_del_init(&link->tramp_hlist);
> tr->progs_cnt[kind]--;
> - return bpf_trampoline_update(tr);
> + return bpf_trampoline_update(tr, true /* lock_direct_mutex */);
> }
>
> /* bpf_trampoline_unlink_prog() should never fail. */
> @@ -715,6 +838,7 @@ void bpf_trampoline_put(struct bpf_trampoline *tr)
> * multiple rcu callbacks.
> */
> hlist_del(&tr->hlist);
> + kfree(tr->fops);
> kfree(tr);
> out:
> mutex_unlock(&trampoline_mutex);
> --
> 2.30.2
>
next prev parent reply other threads:[~2022-07-22 7:31 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-20 0:21 [PATCH v5 bpf-next 0/4] ftrace: host klp and bpf trampoline together Song Liu
2022-07-20 0:21 ` [PATCH v5 bpf-next 1/4] ftrace: Add modify_ftrace_direct_multi_nolock Song Liu
2022-07-20 0:21 ` [PATCH v5 bpf-next 2/4] ftrace: Allow IPMODIFY and DIRECT ops on the same function Song Liu
2022-09-09 11:58 ` Naveen N. Rao
2022-09-09 14:05 ` Song Liu
2022-09-09 18:13 ` Naveen N. Rao
2022-07-20 0:21 ` [PATCH v5 bpf-next 3/4] bpf, x64: Allow to use caller address from stack Song Liu
2022-07-20 0:21 ` [PATCH v5 bpf-next 4/4] bpf: Support bpf_trampoline on functions with IPMODIFY (e.g. livepatch) Song Liu
2022-07-22 7:31 ` Jiri Olsa [this message]
2022-07-22 15:58 ` Song Liu
2022-07-21 22:59 ` [PATCH v5 bpf-next 0/4] ftrace: host klp and bpf trampoline together Song Liu
2022-07-22 16:56 ` Steven Rostedt
2022-07-22 18:02 ` Song Liu
2022-07-22 20:10 ` patchwork-bot+netdevbpf
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=YtpSOTlpHbKOKTDJ@krava \
--to=olsajiri@gmail.com \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@fb.com \
--cc=linux-kernel@vger.kernel.org \
--cc=live-patching@vger.kernel.org \
--cc=rostedt@goodmis.org \
--cc=song@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).