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 <[email protected]> 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? > 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 > @@ -0,0 +1,91 @@ [ ... ] > +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? > + > + 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; > +} [ ... ] > +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. > + > + 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/[email protected]?part=2
