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

On 7/20/26 23:22, Gabriele Monaco wrote:
> 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?
> 

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