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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `rv_uprobe_unregister_nosync` API destroys state needed for safe batch cleanup, leading to resource leaks or double-frees.
- [High] The `DECLARE_RV_UPROBE` macro fails to zero-initialize the structure, risking execution of uninitialized function pointers.
- [Medium] The commit message explicitly contradicts the code regarding path release in `rv_uprobe_unregister_nosync`.
--

--- Patch [2]: [PATCH v5 2/9] rv: Add generic uprobe infrastructure for RV monitors ---
commit 51a0e9ee4d8b7c0315a129cc3c664429f117eeb4
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.

>     The path is released in
>     rv_uprobe_unregister_nosync() after the consumer has been removed.

[Severity: Medium]
The commit message states that the path is released in
rv_uprobe_unregister_nosync() after the consumer has been removed.
Is this description accurate?

Looking at the code for rv_uprobe_unregister_nosync(), it explicitly
defers releasing the path to the caller. Could the commit message be
updated to match the actual implementation?

> diff --git a/include/rv/rv_uprobe.h b/include/rv/rv_uprobe.h
> index 0000000000000..d0a9079ac5be4
> --- /dev/null
> +++ b/include/rv/rv_uprobe.h

[ ... ]

> +/* Embed a named rv_uprobe inside a caller struct */
> +#define DECLARE_RV_UPROBE(name)		struct rv_uprobe name

[Severity: High]
Does this macro zero-initialize the structure? 

Since it simply expands to a struct declaration, local variables using
this macro might contain stack garbage. If a caller only sets p->uc.handler,
could this leave other uprobe_consumer fields like filter or ret_handler
uninitialized, causing the uprobe subsystem to execute random memory when
triggered?

> diff --git a/kernel/trace/rv/rv_uprobe.c b/kernel/trace/rv/rv_uprobe.c
> index 0000000000000..b412a8e28a6ec
> --- /dev/null
> +++ b/kernel/trace/rv/rv_uprobe.c

[ ... ]

> +int rv_uprobe_register(const char *binpath, loff_t offset, struct rv_uprobe *p)
> +{
> +	struct inode *inode;
> +	int ret;

[ ... ]

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

[Severity: High]
When uprobe_register() fails, path_put() is called, but p->path is not
cleared. If a caller blindly loops over an array of probes to clean up
paths later, could this leave dangling pointers that result in a
double-free?

[ ... ]

> +void rv_uprobe_unregister(struct rv_uprobe *p)
> +{
> +	if (!p || !p->uprobe)
> +		return;
> +
> +	uprobe_unregister_nosync(p->uprobe, &p->uc);
> +	p->uprobe = NULL;
> +	rv_uprobe_sync();
> +	path_put(&p->path);
> +}

[ ... ]

> +void rv_uprobe_unregister_nosync(struct rv_uprobe *p)
> +{
> +	if (!p || !p->uprobe)
> +		return;
> +
> +	uprobe_unregister_nosync(p->uprobe, &p->uc);
> +	p->uprobe = NULL;
> +	/* path held; caller must call rv_uprobe_sync() then path_put(&p->path) */
> +}

[Severity: High]
By setting p->uprobe to NULL here, does rv_uprobe_unregister_nosync()
destroy the state needed to safely perform the path cleanup?

If a caller attempts to use rv_uprobe_unregister() to clean up paths
after a batch of nosync operations, it will return early because
p->uprobe is NULL, silently leaking the VFS path reference:

rv_uprobe_unregister() {
	if (!p || !p->uprobe)
		return;
    ...
}

Alternatively, without external shadow state, how can a caller
distinguish between a successfully unregistered probe that needs a
path_put() and a probe that failed registration initially?

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