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 <[email protected]> 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/[email protected]?part=9
