Re: [PATCH v19 31/40] dept: assign unique dept_key to each distinct wait_for_completion() caller

[sashiko-bot@kernel.org]

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 <byungchul@sk.com>

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/20260706061928.66713-1-byungchul@sk.com?part=31