Re: [PATCH v7 09/14] 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] uprobes: missing vfsmount reference leads to VFS BUG on unmount
- [Medium] uprobes: double registration leaks inode reference and corrupts consumer list
- [Low] uprobes: missing <linux/export.h> inclusion
--

commit 27eb3c739b9acf445ce512a1c9be0a7dca8bace0
Author: Wen Yang <wen.yang@linux.dev>

rv: Add generic uprobe infrastructure for RV monitors

Monitors that instrument user-space function boundaries need to register
uprobes and deregister them safely. Provide a thin wrapper so monitors
share a single implementation of this boilerplate.

> diff --git a/include/rv/rv_uprobe.h b/include/rv/rv_uprobe.h
> new file mode 100644
> index 0000000000000..b93c2a81bc228
> --- /dev/null
> +++ b/include/rv/rv_uprobe.h
[ ... ]
> +/*
> + * Free the containing struct only after rv_uprobe_unregister_nosync() +
> + * rv_uprobe_unregister_sync() + iput(p->inode).
> + *
> + * The inode reference does not pin the dentry, so callers that re-resolve the
> + * probed path must hold a struct path: 9p cache=none remints an inode per
> + * lookup.
> + */
> +struct rv_uprobe {
> +	struct uprobe_consumer	uc;
> +	struct uprobe		*uprobe;
> +	struct inode		*inode;
> +};

[Severity: High]
Does this struct need to hold a reference to the vfsmount as well?

The core uprobes API documents that the caller of uprobe_register() is
required to keep the inode and the containing mount referenced. 

If the infrastructure only pins the inode via igrab(), and the caller does
not independently hold a mount reference, user-space could unmount the
filesystem while the monitor is active.

During unmount, the elevated inode i_count from igrab prevents eviction,
which causes the VFS superblock shutdown sequence in generic_shutdown_super()
to trigger a bug for busy inodes after unmount.

[ ... ]
> diff --git a/kernel/trace/rv/rv_uprobe.c b/kernel/trace/rv/rv_uprobe.c
> new file mode 100644
> index 0000000000000..1c7f8466aeedd
> --- /dev/null
> +++ b/kernel/trace/rv/rv_uprobe.c
[ ... ]
> +/*
> + * uprobe_register() takes no inode reference of its own, so grab one here and
> + * drop it only after rv_uprobe_unregister_sync().
> + */
> +int rv_uprobe_register(struct inode *inode, loff_t offset, struct rv_uprobe *p)
> +{
> +	int ret;
> +
> +	p->inode = igrab(inode);

[Severity: Medium]
What happens if rv_uprobe_register() is called on an already registered
struct rv_uprobe?

Since p->inode is unconditionally overwritten with igrab(inode), the previous
inode reference seems to be leaked without calling iput().

Additionally, passing an actively linked uprobe_consumer to uprobe_register()
again could corrupt the uprobe subsystem's internal consumer lists.

Would it be safer to check if p->inode is already set before proceeding?

> +	if (!p->inode)
> +		return -ESTALE;
> +
> +	p->uprobe = uprobe_register(inode, offset, 0, &p->uc);
> +	if (IS_ERR(p->uprobe)) {
> +		ret = PTR_ERR(p->uprobe);
> +		p->uprobe = NULL;
> +		iput(p->inode);
> +		p->inode = NULL;
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(rv_uprobe_register);

[Severity: Low]
Could this EXPORT_SYMBOL_GPL usage cause build failures on some architectures?

The source file uses EXPORT_SYMBOL_GPL but does not explicitly include
<linux/export.h>. Relying on implicit transitive inclusion of this header
can lead to build breakages on alternative configurations.

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