On Thu, 2 Jun 2022 12:37:06 -0700
Song Liu <song@xxxxxxxxxx> wrote:
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -27,6 +27,44 @@ static struct hlist_head trampoline_table[TRAMPOLINE_TABLE_SIZE];
> /* serializes access to trampoline_table */
> static DEFINE_MUTEX(trampoline_mutex);
>
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> +static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mutex);
> +
> +static int bpf_tramp_ftrace_ops_func(struct ftrace_ops *ops, enum ftrace_ops_cmd cmd)
> +{
> + struct bpf_trampoline *tr = ops->private;
> + int ret;
> +
> + /*
> + * The normal locking order is
> + * tr->mutex => direct_mutex (ftrace.c) => ftrace_lock (ftrace.c)
> + *
> + * This is called from prepare_direct_functions_for_ipmodify, with
> + * direct_mutex locked. Use mutex_trylock() to avoid dead lock.
> + * Also, bpf_trampoline_update here should not lock direct_mutex.
> + */
> + if (!mutex_trylock(&tr->mutex))
Can you comment here that returning -EAGAIN will not cause this to repeat.
That it will change things where the next try will not return -EGAIN?
> + return -EAGAIN;
> +
> + switch (cmd) {
> + case FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY:
> + tr->indirect_call = true;
> + ret = bpf_trampoline_update(tr, false /* lock_direct_mutex */);
> + break;
> + case FTRACE_OPS_CMD_DISABLE_SHARE_IPMODIFY:
> + tr->indirect_call = false;
> + tr->fops->flags &= ~FTRACE_OPS_FL_SHARE_IPMODIFY;
> + ret = bpf_trampoline_update(tr, false /* lock_direct_mutex */);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + };
> + mutex_unlock(&tr->mutex);
> + return ret;
> +}
> +#endif
> +
>
> @@ -330,7 +387,7 @@ 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;
> @@ -363,20 +420,45 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
> if (ip_arg)
> flags |= BPF_TRAMP_F_IP_ARG;
>
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> +again:
> + if (tr->indirect_call)
> + 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.addr);
> if (err < 0)
> goto out;
>
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> + if (tr->indirect_call)
> + tr->fops->flags |= FTRACE_OPS_FL_SHARE_IPMODIFY;
> +#endif
> +
> WARN_ON(tr->cur_image && tr->selector == 0);
> 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) {
> + if (WARN_ON_ONCE(tr->indirect_call))
> + goto out;
> + /* should only retry on the first register */
> + if (WARN_ON_ONCE(tr->cur_image))
> + goto out;
> + tr->indirect_call = true;
> + tr->fops->func = NULL;
> + tr->fops->trampoline = 0;
> + goto again;
I'm assuming that the above will prevent a return of -EAGAIN again. As if
it can, then this could turn into a dead lock.
Can you please comment that?
Thanks,
-- Steve
> + }
> +#endif
> if (err)
> goto out;
> if (tr->cur_image)
> @@ -460,7 +542,7 @@ int bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline
>
> 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 +569,7 @@ int bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_trampolin
> }
> hlist_del_init(&link->tramp_hlist);
> tr->progs_cnt[kind]--;
> - err = bpf_trampoline_update(tr);
> + err = bpf_trampoline_update(tr, true /* lock_direct_mutex */);
> out:
> mutex_unlock(&tr->mutex);
> return err;
> @@ -535,6 +617,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);
