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