Re: [PATCH v6 2/5] tracing/fprobe: Remove fprobe from hash in failure path

[Menglong Dong]

On 2026/4/14 17:14 Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> write:
> From: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> 
> 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: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> ---
[...]
>  
> +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);
>  
> 
> 
>