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

[Wen Yang]

On 7/20/26 23:22, Gabriele Monaco wrote:
> On Wed, 2026-07-08 at 23:38 +0800, wen.yang@xxxxxxxxx wrote:
> > From: Wen Yang <wen.yang@xxxxxxxxx>
> >
> > +++ 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?

Thanks, the bool config stays (it gates the obj build for monitors that 
need it)but the invisible help text is dropped in v5; the entry is now just:

  config RV_UPROBE
        bool
        depends on RV && UPROBES


> > +
> >   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.
Done in v5: struct rv_uprobe now keeps a struct path to the probed
binary.  rv_uprobe_register() resolves it via kern_path() and does not
drop it after uprobe_register(); rv_uprobe_unregister() releases it with
path_put() only after rv_uprobe_sync() has drained in-flight handlers,
so no inode reference outlives the probe.

--
Best wishes,
Wen

> > +	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);