Re: [PATCH v6 2/9] rv: Add generic uprobe infrastructure for RV monitors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] API state tracking design in `rv_uprobe` leaves stale pointers on error and breaks state validation for batch cleanup, trapping callers into a double-free or memory leak.
- [Medium] The commit message explicitly states a factually incorrect resource lifecycle that contradicts the code and creates an RCU use-after-free hazard if implemented as described.
--

--- Patch [2]: [PATCH v6 2/9] rv: Add generic uprobe infrastructure for RV monitors ---
commit be3e5812586424192ba2d5028d63b1a9b959099b
Author: Wen Yang <wen.yang@linux.dev>

rv: Add generic uprobe infrastructure for RV monitors
    
Monitors that instrument user-space function boundaries need to resolve
paths, register uprobes, and deregister them safely.  Provide a thin
wrapper so monitors share a single implementation of this boilerplate.

> struct rv_uprobe embeds struct uprobe_consumer directly, avoiding a
> separate heap allocation per probe.  The struct holds a struct path for
> the probed binary so that the inode and its mount remain referenced for
> the full uprobe lifetime; uprobe_register() does not take its own
> reference to the inode.  The path is released in
> rv_uprobe_unregister_nosync() after the consumer has been removed.

[Severity: Medium]
This isn't a bug in the code, but this statement contradicts the actual
implementation. Looking at the code, rv_uprobe_unregister_nosync() explicitly
leaves the path held to prevent RCU use-after-free hazards, relying on the
caller to release it after rv_uprobe_sync(). Could this description be
updated to match the code?

> diff --git a/kernel/trace/rv/rv_uprobe.c b/kernel/trace/rv/rv_uprobe.c
> new file mode 100644
> index 0000000000000..b412a8e28a6ec
> --- /dev/null
> +++ b/kernel/trace/rv/rv_uprobe.c
> @@ -0,0 +1,91 @@
[ ... ]
> +int rv_uprobe_register(const char *binpath, loff_t offset, struct rv_uprobe *p)
> +{
> +	struct inode *inode;
> +	int ret;
> +
> +	ret = kern_path(binpath, LOOKUP_FOLLOW, &p->path);
> +	if (ret)
> +		return ret;
> +
> +	if (!d_is_reg(p->path.dentry)) {
> +		path_put(&p->path);
> +		return -EINVAL;
> +	}

[Severity: Medium]
Do these error paths leave a stale pointer in the caller's struct?

When path_put() is called, p->path.dentry is not cleared. If a caller is
using an array of struct rv_uprobe and tries to batch clean up failed
registrations, could they accidentally use this stale pointer and cause a
double-free?

> +
> +	inode = d_real_inode(p->path.dentry);
> +
> +	/* uprobe_register() takes no inode reference; the path is held in p->path */
> +	p->uprobe = uprobe_register(inode, offset, 0, &p->uc);
> +	if (IS_ERR(p->uprobe)) {
> +		ret = PTR_ERR(p->uprobe);
> +		p->uprobe = NULL;
> +		path_put(&p->path);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +void rv_uprobe_unregister(struct rv_uprobe *p)
> +{
> +	if (!p || !p->uprobe)
> +		return;

[Severity: Medium]
Does this early return cause a memory leak if the caller previously used
rv_uprobe_unregister_nosync()?

Since rv_uprobe_unregister_nosync() clears p->uprobe to NULL, a caller
attempting to use rv_uprobe_unregister() on it later will hit this early
return. This skips path_put(&p->path), permanently leaking the path
reference.

> +
> +	uprobe_unregister_nosync(p->uprobe, &p->uc);
> +	p->uprobe = NULL;
> +	rv_uprobe_sync();
> +	path_put(&p->path);
> +}
> +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;

[Severity: Medium]
If p->uprobe is cleared here, how does the caller know which paths need to be
released later?

The documentation suggests batching deregistrations. However, if p->uprobe is
set to NULL here, any subsequent iteration over an array to conditionally call
path_put() has no reliable state indicator left to determine if the path was
actually held, potentially leading to incorrect cleanup.

> +	/* path held; caller must call rv_uprobe_sync() then path_put(&p->path) */
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787243842.git.wen.yang@linux.dev?part=2