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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.