Re: [PATCH v4 2/8] rv: add generic uprobe infrastructure for RV monitors

On Wed, 2026-07-08 at 23:38 +0800, [email protected] wrote:
> From: Wen Yang <[email protected]>
>
> +++ b/kernel/trace/rv/Kconfig
> @@ -59,6 +59,13 @@ config RV_PER_TASK_MONITORS
>  	  This option configures the maximum number of per-task RV monitors
> that can run
>  	  simultaneously.
>  
> +config RV_UPROBE
> +	bool
> +	depends on RV && UPROBES
> +	help
> +	  Generic uprobe infrastructure for RV monitors.  Provides path
> +	  resolution, registration, and safe synchronous teardown.

This isn't exposed, it's selected automatically when required, I don't
even think the help text is visible (menuconfig doesn't show it), do we
really need it?

> +
>  source "kernel/trace/rv/monitors/wip/Kconfig"
>  source "kernel/trace/rv/monitors/wwnr/Kconfig"
>  

...

> +++ b/kernel/trace/rv/rv_uprobe.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Generic uprobe infrastructure for RV monitors.
> + *
> + * struct rv_uprobe embeds struct uprobe_consumer directly.  This is safe
> + * because rv_uprobe_sync() calls uprobe_unregister_sync(), which calls
> + * synchronize_rcu_tasks_trace().  handler_chain() runs under
> + * rcu_read_lock_trace(), so after synchronize_rcu_tasks_trace() returns,
> + * all in-flight handler_chain() iterations, including any pending
> + * uc->cons_node.next reads, have completed on all CPUs.  The caller may
> + * then free the struct containing rv_uprobe immediately.
> + */
> +#include <linux/dcache.h>
> +#include <linux/fs.h>
> +#include <linux/namei.h>
> +#include <linux/uprobes.h>
> +#include <rv/rv_uprobe.h>
> +
> +/**
> + * rv_uprobe_register - initialise and register an uprobe
> + */
> +int rv_uprobe_register(const char *binpath, loff_t offset, struct rv_uprobe
> *p)
> +{
> +	struct inode *inode;
> +	struct path path;
> +	int ret;
> +
> +	if (!p->uc.handler && !p->uc.ret_handler)
> +		return -EINVAL;

uprobe_register() does this already, do we need it here too?

> +
> +	ret = kern_path(binpath, LOOKUP_FOLLOW, &path);
> +	if (ret)
> +		return ret;
> +
> +	if (!d_is_reg(path.dentry)) {
> +		path_put(&path);
> +		return -EINVAL;
> +	}
> +
> +	inode = d_real_inode(path.dentry);
> +	p->inode = inode;
> +
> +	/*
> +	 * uprobe_register() requires the inode (and mount) to remain
> +	 * referenced across the call.  Keep the path alive until after
> +	 * uprobe_register() has stored its own reference, then release it.
> +	 */
> +	p->uprobe = uprobe_register(inode, offset, 0, &p->uc);
> +	path_put(&path);

I believe I was mistaken here, as sashiko pointed out, uprobe_register()
doesn't keep a reference to the inode, (explicitly stated in it's docs:
"Caller of uprobe_register() is required to keep @inode (and the
containing mount) referenced.").

We should probably revert back to holding path instead of inode and
putting it after synchronous cleanup. That's also what BPF does.

Thanks,
Gabriele

> +	if (IS_ERR(p->uprobe)) {
> +		ret = PTR_ERR(p->uprobe);
> +		p->uprobe = NULL;
> +		p->inode = NULL;
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(rv_uprobe_register);
> +
> +/**
> + * rv_uprobe_is_registered - test whether an uprobe is currently active
> + */
> +bool rv_uprobe_is_registered(const struct rv_uprobe *p)
> +{
> +	return p && p->uprobe;
> +}
> +EXPORT_SYMBOL_GPL(rv_uprobe_is_registered);
> +
> +/**
> + * rv_uprobe_unregister - synchronously unregister a uprobe
> + */
> +void rv_uprobe_unregister(struct rv_uprobe *p)
> +{
> +	if (!p || !p->uprobe)
> +		return;
> +
> +	rv_uprobe_unregister_nosync(p);
> +	rv_uprobe_sync();
> +}
> +EXPORT_SYMBOL_GPL(rv_uprobe_unregister);
> +
> +/**
> + * rv_uprobe_unregister_nosync - dequeue an uprobe without waiting
> + */
> +void rv_uprobe_unregister_nosync(struct rv_uprobe *p)
> +{
> +	if (!p || !p->uprobe)
> +		return;
> +
> +	uprobe_unregister_nosync(p->uprobe, &p->uc);
> +	p->uprobe = NULL;
> +	p->inode = NULL;
> +}
> +EXPORT_SYMBOL_GPL(rv_uprobe_unregister_nosync);
> +
> +/**
> + * rv_uprobe_sync - wait for all in-flight uprobe handlers to complete
> + */
> +void rv_uprobe_sync(void)
> +{
> +	uprobe_unregister_sync();
> +}
> +EXPORT_SYMBOL_GPL(rv_uprobe_sync);