Re: [RFC PATCH 2/4] rust: damon: add basic DAMON abstractions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] damon: data race in damon_add_target() and damon_add_scheme() via shared reference
- [High] damon: PID refcount leak in damon_add_target() when cleanup_target is NULL
- [High] damon: memory leak of struct damos due to missing Drop
- [Low] damon: missing #[inline] on C binding wrappers like damon_initialized()
--

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

rust: damon: add basic DAMON abstractions

Wrap the basic DAMON C API in a new rust/kernel/damon module, exposing
contexts, targets, schemes, and the types that configure them, so Rust
modules can drive DAMON without calling the raw C functions directly.

diff --git a/rust/kernel/damon.rs b/rust/kernel/damon.rs
new file mode 100644
index 0000000000000..80e6609132e75
--- /dev/null
+++ b/rust/kernel/damon.rs
@@ -0,0 +1,292 @@

[ ... ]

> +/// Return if DAMON is ready to be used.
> +pub fn damon_initialized() -> bool {
> +    // SAFETY: It is a simple pure query function.
> +    unsafe { bindings::damon_initialized() }
> +}

[Severity: Low]
This isn't a bug, but should this C binding wrapper have an #[inline]
annotation? The Rust subsystem guidelines recommend that small abstraction
functions forwarding to C bindings use #[inline].

[ ... ]

> +pub struct Damos {
> +    damos: NonNull<bindings::damos>,
> +}
> +
> +impl Damos {
> +    /// Create new Damos.
> +    pub fn new(
> +        pattern: &mut DamosAccessPattern,
> +        action: DamosAction,
> +        apply_interval_us: usize,
> +        quota: &mut DamosQuota,
> +        wmarks: &mut DamosWatermarks,
> +        target_nid: i32,
> +    ) -> Result<Self> {
> +        // SAFETY: All pointer arguments come from local variables that
> +        // stay alive during the call; the rest are just numbers.  The
> +        // return value is checked for NULL right after.
> +        let ptr = unsafe {
> +            bindings::damon_new_scheme(
> +                &mut pattern.damos_access_pattern,
> +                action.damos_action,
> +                apply_interval_us,
> +                &mut quota.damos_quota,
> +                &mut wmarks.damos_watermarks,
> +                target_nid,
> +            )
> +        };
> +        NonNull::new(ptr).map(|p| Self { damos: p }).ok_or(ENOMEM)

[Severity: High]
Does this code leak the allocated struct damos?

If a user calls Damos::new() but returns early (for example, via the ? operator
during error handling) before passing ownership to DamonCtx::damon_add_scheme(),
it appears the memory allocated by damon_new_scheme() is never freed because
the Damos struct does not implement the Drop trait.

> +    }
> +}

[ ... ]

> +    /// Add a new DAMON monitoring target.
> +    pub fn damon_add_target(&self, target: Target) {
> +        // SAFETY: Both arguments are valid: self.ctx is non-null by
> +        // construction, and target.target points to an unique owned
> +        // damon_target.  The C function takes ownership of the target
> +        // that is why rust slide should forget it.
> +        unsafe { bindings::damon_add_target(self.ctx.as_ptr(), target.target.as_ptr()) };
> +        core::mem::forget(target);

[Severity: High]
Can this data race with the kdamond thread iterating over the same lists
locklessly?

Because damon_add_target() takes a shared &self reference, safe Rust code
can call it concurrently, or after damon_start() spawns the background C kernel
thread. This does not enforce mutual exclusion.

[Severity: High]
Does this code leak the struct pid refcount when cleanup_target is NULL?

Target::damon_set_target_pid() takes a refcount on a struct pid. Here,
core::mem::forget(target) bypasses the Rust Drop handler. If a user drops the
context without successfully selecting operations (like OpsID::VADDR), the C
fallback damon_destroy_ctx() leaves cleanup_target as NULL and put_pid() is
never called, leaking the refcount.

> +    }
> +
> +    /// Add a scheme to context.
> +    pub fn damon_add_scheme(&self, scheme: Damos) {
> +        // SAFETY: self.ctx is a valid 'damon_ctx *' and scheme.damos is a
> +        // valid 'damos *', both by the type invariants of Ctx and Damos.
> +        // damon_add_scheme takes ownership of the scheme, so the rust
> +        // side should forget it to avoid a double free.
> +        unsafe { bindings::damon_add_scheme(self.ctx.as_ptr(), scheme.damos.as_ptr()) };

[Severity: High]
Can this data race with the background thread for the same reasons as
damon_add_target()?

This method also takes a shared &self reference, allowing concurrent mutation
of the context's scheme list while the kthread locklessly accesses it.

> +        core::mem::forget(scheme);
> +    }
> +
> +    /// Starts the monitorings for a given group of contexts.
> +    pub fn damon_start(&self) -> Result {
> +        let mut ctxs = [self.ctx.as_ptr()];
> +        // SAFETY: ctxs is a stack-allocated array of length 1 whose only
> +        // element is a valid 'damon_ctx *'.  The pointer remains valid
> +        // for the duration of the call.  The length argument 1 matches the
> +        // array size, and true is a valid bool value for the exclusive
> +        // parameter.
> +        let ret = unsafe { bindings::damon_start(ctxs.as_mut_ptr(), 1, true) };

[Severity: High]
Can this allow data races by only requiring a shared reference?

damon_start() spawns the kdamond background thread. Because it takes &self
instead of an exclusive reference or consuming the context, it fails to
statically prevent safe Rust code from calling mutating methods like
damon_add_target() or damon_add_scheme() concurrently.

> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(())
> +        }
> +    }

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