Re: [RFC PATCH 3/4] samples/damon: add Rust sample for DAMON access-aware proactive reclamation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] damon/rust_prcl: concurrent writes to 'enabled' debugfs file leak threads
- [Low] damon/rust_prcl: custom boolean parsing rejects standard kernel bool strings
--

commit 195de0b55eeb64d2bfdbc4cb35a867a97366c03f
Author: Enze Li <lienze@kylinos.cn>

samples/damon: add Rust sample for DAMON access-aware proactive reclamation

This commit adds a Rust implementation of the DAMON-based proactive
reclamation sample module. It allows monitoring a target process's virtual
address space and proactively reclaiming cold memory via debugfs endpoints.

> diff --git a/samples/damon/rust_prcl.rs b/samples/damon/rust_prcl.rs
> new file mode 100644
> index 0000000000000..1e6c820311e23
> --- /dev/null
> +++ b/samples/damon/rust_prcl.rs
[ ... ]
> +fn damon_sample_rust_prcl_start(target_pid: i32) -> Result {
> +    let ctx = damon::DamonCtx::damon_new_ctx()?;
[ ... ]
> +    let ret = ctx.damon_start();
> +    match ret {
> +        Ok(_) => {
> +            CTX.store(ctx.into_raw(), Relaxed);
> +        }

[Severity: High]
Can this leak the original damon_ctx and its running thread if
damon_sample_rust_prcl_start() is called concurrently?

Concurrent callers to start() will overwrite the global CTX pointer here,
causing the first context to be permanently leaked along with its thread.

> +        Err(e) => {
> +            pr_err!("damon_start failed, error: {:?}\n", e);
> +            return Err(e);
> +        }
> +    }
> +    Ok(())
> +}
> +
> +fn damon_sample_rust_prcl_stop() {
> +    pr_info!("stop\n");
> +    let raw = CTX.xchg(null_mut(), Relaxed);

[Severity: High]
If stop() is called in a race with start(), could it read null_mut() here
before start() has recorded the context?

A rapid start/stop sequence could result in stop() incorrectly finding a
null pointer and failing to terminate the newly-starting thread.

[ ... ]
> +fn damon_sample_rust_parse_bool(s: &str) -> Result<bool> {
> +    let s = s.trim();
> +    if s.eq_ignore_ascii_case("y") {
> +        Ok(true)
> +    } else if s.eq_ignore_ascii_case("n") {
> +        Ok(false)
> +    } else {
> +        Err(EINVAL)
> +    }
> +}

[Severity: Low]
Does this custom parsing reject standard boolean representations that the
kernel conventionally accepts?

This function only accepts 'y', 'Y', 'n', and 'N'. Standard kernel boolean
parsing typically supports inputs like '1', '0', 'on', and 'off' via
mechanisms similar to kstrtobool(). This might provide a confusing example
for future Rust modules.

[ ... ]
> +fn enabled_write(data: &ModuleData, reader: &mut UserSliceReader) -> Result {
[ ... ]
> +    let enabled = damon_sample_rust_parse_bool(s)?;
> +
> +    let is_enabled = data.enabled.load(Relaxed);
> +    if enabled == is_enabled {
> +        return Ok(());
> +    }
> +
> +    if !damon::damon_initialized() {
> +        return Ok(());
> +    }
> +
> +    data.enabled.store(enabled, Relaxed);

[Severity: High]
Does this lockless read-and-update of the boolean flag allow multiple
concurrent writers to trigger complex state changes simultaneously?

Because the read-and-check of data.enabled lacks locking, a TOCTOU race
exists. Two processes writing 'y' concurrently will both bypass the
check and call damon_sample_rust_prcl_start(), which triggers the thread
and context leaks mentioned earlier.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260927072812.2393456-1-lienze@kylinos.cn?part=3