Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] The `rv_uprobe_unregister_nosync` API destroys state needed for safe batch cleanup, leading to resource leaks or double-frees. - [High] The `DECLARE_RV_UPROBE` macro fails to zero-initialize the structure, risking execution of uninitialized function pointers. - [Medium] The commit message explicitly contradicts the code regarding path release in `rv_uprobe_unregister_nosync`. --
--- Patch [2]: [PATCH v5 2/9] rv: Add generic uprobe infrastructure for RV monitors --- commit 51a0e9ee4d8b7c0315a129cc3c664429f117eeb4 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. > The path is released in > rv_uprobe_unregister_nosync() after the consumer has been removed. [Severity: Medium] The commit message states that the path is released in rv_uprobe_unregister_nosync() after the consumer has been removed. Is this description accurate? Looking at the code for rv_uprobe_unregister_nosync(), it explicitly defers releasing the path to the caller. Could the commit message be updated to match the actual implementation? > diff --git a/include/rv/rv_uprobe.h b/include/rv/rv_uprobe.h > index 0000000000000..d0a9079ac5be4 > --- /dev/null > +++ b/include/rv/rv_uprobe.h [ ... ] > +/* Embed a named rv_uprobe inside a caller struct */ > +#define DECLARE_RV_UPROBE(name) struct rv_uprobe name [Severity: High] Does this macro zero-initialize the structure? Since it simply expands to a struct declaration, local variables using this macro might contain stack garbage. If a caller only sets p->uc.handler, could this leave other uprobe_consumer fields like filter or ret_handler uninitialized, causing the uprobe subsystem to execute random memory when triggered? > diff --git a/kernel/trace/rv/rv_uprobe.c b/kernel/trace/rv/rv_uprobe.c > index 0000000000000..b412a8e28a6ec > --- /dev/null > +++ b/kernel/trace/rv/rv_uprobe.c [ ... ] > +int rv_uprobe_register(const char *binpath, loff_t offset, struct rv_uprobe > *p) > +{ > + struct inode *inode; > + int ret; [ ... ] > + 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; > +} [Severity: High] When uprobe_register() fails, path_put() is called, but p->path is not cleared. If a caller blindly loops over an array of probes to clean up paths later, could this leave dangling pointers that result in a double-free? [ ... ] > +void rv_uprobe_unregister(struct rv_uprobe *p) > +{ > + if (!p || !p->uprobe) > + return; > + > + uprobe_unregister_nosync(p->uprobe, &p->uc); > + p->uprobe = NULL; > + rv_uprobe_sync(); > + path_put(&p->path); > +} [ ... ] > +void rv_uprobe_unregister_nosync(struct rv_uprobe *p) > +{ > + if (!p || !p->uprobe) > + return; > + > + uprobe_unregister_nosync(p->uprobe, &p->uc); > + p->uprobe = NULL; > + /* path held; caller must call rv_uprobe_sync() then path_put(&p->path) > */ > +} [Severity: High] By setting p->uprobe to NULL here, does rv_uprobe_unregister_nosync() destroy the state needed to safely perform the path cleanup? If a caller attempts to use rv_uprobe_unregister() to clean up paths after a batch of nosync operations, it will return early because p->uprobe is NULL, silently leaking the VFS path reference: rv_uprobe_unregister() { if (!p || !p->uprobe) return; ... } Alternatively, without external shadow state, how can a caller distinguish between a successfully unregistered probe that needs a path_put() and a probe that failed registration initially? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
