On 2026/4/14 17:14 Masami Hiramatsu (Google) <[email protected]> write:
> From: Masami Hiramatsu (Google) <[email protected]>
>
> When register_fprobe_ips() fails, it tries to remove a list of
> fprobe_hash_node from fprobe_ip_table, but it missed to remove
> fprobe itself from fprobe_table. Moreover, when removing
> the fprobe_hash_node which is added to rhltable once, it must
> use kfree_rcu() after removing from rhltable.
>
> To fix these issues, this reuses unregister_fprobe() internal
> code to rollback the half-way registered fprobe.
>
> Fixes: 4346ba160409 ("fprobe: Rewrite fprobe on function-graph tracer")
> Cc: [email protected]
> Signed-off-by: Masami Hiramatsu (Google) <[email protected]>
> ---
[...]
>
> +static int unregister_fprobe_nolock(struct fprobe *fp, bool force);
> +
> /**
> * register_fprobe_ips() - Register fprobe to ftrace by address.
> * @fp: A fprobe data structure to be registered.
> @@ -847,29 +855,26 @@ int register_fprobe_ips(struct fprobe *fp, unsigned
> long *addrs, int num)
> if (ret)
> return ret;
Hi, Masami. The logic of unregister_fprobe_nolock() looks a little
messy. How about we make the logic here like this:
for (i = 0; i < hlist_array->size; i++) {
// The node->fp is NULL, so it's safe to add the node before
// fprobe_ftrace_add_ips(), right?
ret = insert_fprobe_node(&hlist_array->array[i], fp);
if (ret)
goto fallback_err;
}
if (fprobe_is_ftrace(fp))
ret = fprobe_ftrace_add_ips(addrs, num);
else
ret = fprobe_graph_add_ips(addrs, num);
if (ret)
goto fallback_err;
add_fprobe_hash(fp);
for (i = 0; i < hlist_array->size; i++)
WRITE_ONCE(hlist_array->array[i].fp, fp);
return 0;
fallback_err:
for (i--; i >= 0; i--)
delete_fprobe_node(&hlist_array->array[i]);
fprobe_fail_cleanup(fp);
return ret;
Then, we don't need to change unregister_fprobe_nolock and
insert_fprobe_node.
Thanks!
Menglong Dong
>
> - hlist_array = fp->hlist_array;
> if (fprobe_is_ftrace(fp))
> ret = fprobe_ftrace_add_ips(addrs, num);
> else
> ret = fprobe_graph_add_ips(addrs, num);
> + if (ret) {
> + fprobe_fail_cleanup(fp);
> + return ret;
> + }
>
> - if (!ret) {
> - add_fprobe_hash(fp);
> - for (i = 0; i < hlist_array->size; i++) {
> - ret = insert_fprobe_node(&hlist_array->array[i]);
> - if (ret)
> - break;
> - }
> - /* fallback on insert error */
> + hlist_array = fp->hlist_array;
> + add_fprobe_hash(fp);
> + for (i = 0; i < hlist_array->size; i++) {
> + ret = insert_fprobe_node(&hlist_array->array[i], fp);
> if (ret) {
> - for (i--; i >= 0; i--)
> - delete_fprobe_node(&hlist_array->array[i]);
> + if (unregister_fprobe_nolock(fp, true))
> + pr_warn("Failed to cleanup fprobe after
> insertion failure.\n");
> + break;
> }
> }
>
> - if (ret)
> - fprobe_fail_cleanup(fp);
> -
> return ret;
> }
> EXPORT_SYMBOL_GPL(register_fprobe_ips);
> @@ -912,37 +917,29 @@ bool fprobe_is_registered(struct fprobe *fp)
> return true;
> }
>
> -/**
> - * unregister_fprobe() - Unregister fprobe.
> - * @fp: A fprobe data structure to be unregistered.
> - *
> - * Unregister fprobe (and remove ftrace hooks from the function entries).
> - *
> - * Return 0 if @fp is unregistered successfully, -errno if not.
> - */
> -int unregister_fprobe(struct fprobe *fp)
> +static int unregister_fprobe_nolock(struct fprobe *fp, bool force)
> {
> - struct fprobe_hlist *hlist_array;
> + struct fprobe_hlist *hlist_array = fp->hlist_array;
> unsigned long *addrs = NULL;
> - int ret = 0, i, count;
> + int i, count;
>
> - mutex_lock(&fprobe_mutex);
> - if (!fp || !fprobe_registered(fp)) {
> - ret = -EINVAL;
> - goto out;
> - }
> -
> - hlist_array = fp->hlist_array;
> addrs = kcalloc(hlist_array->size, sizeof(unsigned long), GFP_KERNEL);
> - if (!addrs) {
> - ret = -ENOMEM; /* TODO: Fallback to one-by-one loop */
> - goto out;
> - }
> + if (!addrs && !force)
> + return -ENOMEM;
> + /*
> + * If @force is set, this function will remove fprobe_hash_node
> + * from the hash table even if memory allocation fails. However,
> + * ftrace_ops will not be updated. Anyway, when the last fprobe
> + * is unregistered, ftrace_ops is also unregistered.
> + */
>
> /* Remove non-synonim ips from table and hash */
> count = 0;
> for (i = 0; i < hlist_array->size; i++) {
> - if (!delete_fprobe_node(&hlist_array->array[i]))
> + if (delete_fprobe_node(&hlist_array->array[i]))
> + continue;
> +
> + if (addrs)
> addrs[count++] = hlist_array->array[i].addr;
> }
> del_fprobe_hash(fp);
> @@ -951,15 +948,35 @@ int unregister_fprobe(struct fprobe *fp)
> fprobe_ftrace_remove_ips(addrs, count);
> else
> fprobe_graph_remove_ips(addrs, count);
> + /*
> + * If count == 0, instead of calling ftrace_set_filter_ips(),
> + * we must wait for RCU grace period to finish del_fprobe_hash().
> + */
> + if (!count)
> + synchronize_rcu();
>
> kfree_rcu(hlist_array, rcu);
> fp->hlist_array = NULL;
> + kfree(addrs);
>
> -out:
> - mutex_unlock(&fprobe_mutex);
> + return !addrs ? -ENOMEM : 0;
> +}
>
> - kfree(addrs);
> - return ret;
> +/**
> + * unregister_fprobe() - Unregister fprobe.
> + * @fp: A fprobe data structure to be unregistered.
> + *
> + * Unregister fprobe (and remove ftrace hooks from the function entries).
> + *
> + * Return 0 if @fp is unregistered successfully, -errno if not.
> + */
> +int unregister_fprobe(struct fprobe *fp)
> +{
> + guard(mutex)(&fprobe_mutex);
> + if (!fp || !fprobe_registered(fp))
> + return -EINVAL;
> +
> + return unregister_fprobe_nolock(fp, false);
> }
> EXPORT_SYMBOL_GPL(unregister_fprobe);
>
>
>
>
