Re: [PATCH 13/17] mm/damon/sysfs: pass preps to DAMON core

[SJ Park <sj@kernel.org>]

On Tue, 01 Sep 2026 04:53:14 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] Data race on `sys_prep->action` due to lockless read and write
> --
> 
> commit 09b46cd761363fb28603bd1d27adb637a6c16a7d
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/sysfs: pass preps to DAMON core
>     
> DAMON sysfs interface provides the files for setting DAMON probe preps.
> But the underlying code is not really passing the user-set values to
> DAMON core.  Pass those.
> 
> > diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> > index c0130843223d7..d02f5f8ceae5b 100644
> > --- a/mm/damon/sysfs.c
> > +++ b/mm/damon/sysfs.c
> > @@ -2176,6 +2176,23 @@ static int damon_sysfs_set_attrs(struct damon_ctx *ctx,
> >  	return damon_set_attrs(ctx, &attrs);
> >  }
> >  
> > +static int damon_sysfs_set_preps(struct damon_probe *probe,
> > +		struct damon_sysfs_preps *sys_preps)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < sys_preps->nr; i++) {
> > +		struct damon_sysfs_prep *sys_prep = sys_preps->preps_arr[i];
> > +		struct damon_prep *prep;
> > +
> > +		prep = damon_new_prep(sys_prep->action);
> 
> [Severity: Low]
> Could this result in a data race on sys_prep->action?
> 
> During DAMON context building, this locklessly reads the action enum while a
> concurrent root user could be writing to the /sys/.../prep_action attribute.
> 
> A concurrent write to prep_action could happen while another process issues a
> commit command to the state file, which runs damon_sysfs_commit_input
> asynchronously on the kdamond thread without holding damon_sysfs_lock.
> 
> Would it be better to use READ_ONCE() here, along with a corresponding
> WRITE_ONCE() on the writer side, to adhere to the Linux Kernel Memory Model
> guidelines and avoid potential KCSAN warnings?

Good catch.  A few similar call paths protect stores with damon_sysfs_lock.  I
will protect prep->action writing with the lock.  Because the context build is
done while holding the lock, it should avoid the race, like below.

'''
--- a/mm/damon/sysfs.c
+++ b/mm/damon/sysfs.c
@@ -827,7 +827,10 @@ static ssize_t prep_action_store(struct kobject *kobj,

                action_name = &damon_sysfs_prep_action_names[i];
                if (sysfs_streq(buf, action_name->name)) {
+                       if (!mutex_trylock(&damon_sysfs_lock))
+                               return -EBUSY;
                        prep->action = action_name->action;
+                       mutex_unlock(&damon_sysfs_lock);
                        ret = count;
                        break;
                }
'''

> 
> > +		if (!prep)
> > +			return -ENOMEM;
> > +		damon_add_prep(probe, prep);
> > +	}
> > +	return 0;
> > +}
> > +
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260901043417.2165-1-sj@kernel.org?part=13


Thanks,
SJ