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 [off-list ref]
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?
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..b412a8e28a6ec
--- /dev/null
+++ b/kernel/trace/rv/rv_uprobe.c
[ ... ]
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+
+ 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;
+}
[ ... ]
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+
+ 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