Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] The `wait_for_completion_*_timeout` macros evaluate their timeout argument `t` multiple times, creating a side-effect vulnerability. - [High] The `wait_for_completion_*` wrapper macros declare local variables (`__ret` and `__m`) that shadow variables in the caller's scope, leading to uninitialized memory usage. - [High] The `wait_for_completion` macros now implicitly depend on GPL-only exported symbols, breaking non-GPL module compilation when CONFIG_DEPT is enabled. - [Medium] The C helper function `rust_helper_wait_for_completion` defeats the primary goal of the patch for the Rust subsystem by mapping all Rust callers to a single `dept_key`. --
commit 18fd6796eb79fb0d0201e71c6791aa80955604b2 Author: Byungchul Park <[email protected]> dept: assign unique dept_key to each distinct wait_for_completion() caller This patch assigns a unique dept_key to each distinct wait_for_completion() caller to avoid false positive reports from DEPT. It also adds a C helper for the Rust subsystem to avoid build errors. > diff --git a/include/linux/completion.h b/include/linux/completion.h > --- a/include/linux/completion.h > +++ b/include/linux/completion.h [ ... ] > +#define wait_for_completion(x) > \ > +({ \ > + sdt_might_sleep_start_timeout(NULL, -1L); \ > + __wait_for_completion(x); \ > + sdt_might_sleep_end(); \ > +}) [Severity: High] Will these macros implicitly depend on GPL-only exported symbols, breaking non-GPL module compilation when CONFIG_DEPT is enabled? The original wait_for_completion() functions are exported via EXPORT_SYMBOL, making them available to non-GPL modules. The new macros wrap these primitives with calls to dept_stage_wait() and dept_clean_stage() via sdt_might_sleep_start_timeout(). Since those tracker functions are exported via EXPORT_SYMBOL_GPL, could this cause linkage failures for non-GPL modules using the macro? [ ... ] > +#define wait_for_completion_timeout(x, t) \ > +({ \ > + unsigned long __ret; \ > + \ > + sdt_might_sleep_start_timeout(NULL, t); \ > + __ret = __wait_for_completion_timeout(x, t); \ > + sdt_might_sleep_end(); \ > + __ret; \ > +}) [Severity: High] Does this macro evaluate the timeout argument t twice? If a caller passes an expression with side effects (like a stateful function call or a pre/post-increment) as the timeout argument, the expression will be executed multiple times. Would this lead to incorrect timeouts or potential kernel crashes if the expression modifies sensitive state? [Severity: High] Could the internal __ret variable shadow a variable in the caller's scope? If a caller passes an expression for the timeout t or the completion x that relies on a variable named __ret in their own scope, the macro's inner uninitialized __ret might shadow the caller's variable. Would this result in the underlying wait functions receiving uninitialized stack memory instead of the intended parameter? [ ... ] > diff --git a/rust/helpers/completion.c b/rust/helpers/completion.c > --- a/rust/helpers/completion.c > +++ b/rust/helpers/completion.c [ ... ] > +void rust_helper_wait_for_completion(struct completion *x) > +{ > + wait_for_completion(x); > +} [Severity: Medium] Does this C helper function defeat the primary goal of the patch for the Rust subsystem? The patch explicitly changes wait_for_completion() to a macro so that a unique static dept_key is instantiated at each distinct call site, avoiding false positive dependency reports. However, by wrapping the macro into a single out-of-line C function, wouldn't all Rust callers of completions share the exact same dept_key, reintroducing the false positive reports? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=31
