Re: [PATCH v7 09/14] rv: Add generic uprobe infrastructure for RV monitors
flat view
From: sashiko-bot@kernel.org
Date: 2026-10-11 19:49:10
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+/* + * 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. [ ... ]
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+/* + * 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?
quoted hunk ↗ jump to hunk
+ 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