[RFC PATCH] mm/damon/sysfs-schemes: hold damon_sysfs_lock in target_value_store()

[RFC PATCH] mm/damon/sysfs-schemes: hold damon_sysfs_lock in target_value_store()

[SJ Park]

Remove a race condition under certain use cases that can trigger
divide-by-zero.

damon_sysfs_add_quota_score() setup damos quota goal as user configured
and pass to the core layer.  Because it could have a race with
damos_sysfs_quota_goals_rm_dirs(), which destructs the files that carry
user configuration, both are called under damon_sysfs_lock protection.

The damos quota goal setup disallows zero target_value parameter because
it can be used as a denominator later in the core layer function,
damos_quota_score().  Otherwise, divide-by-zero can happen.

For the zero value check and final assignment,
damon_sysfs_add_quota_score() reads damos_sysfs_quota_goal->target_value
twice.  And user can update damos_sysfs_quota_goal->target_value via
target_value_store(), which doesn't hold damon_sysfs_lock.  As a result,
a time-of-check-to time-of-use (toctou) race can happen.  If the user
sets the value to zero between the zero value check and final
target_value assignment, zero target_value is passed to the core layer
and results in divide-by-zero.

Fix by holding damon_sysfs_lock in target_value_store() while saving the
value.  The read side, damon_sysfs_add_quota_score(), is already called
while holding the same mutex lock, so it avoids the race.

Users could reproduce the race by writing zero to the target_value sysfs
file while DAMON is starting, or online parameters update is ongoing.
The issue will only rarely happen in the real use cases.  It requires
sysfs write permission.  The race window is short.  Sane users wouldn't
write zero target_value.  Especially, they wouldn't do that while DAMON
enablement and/or online parameters update is ongoing.  That said, the
probability is not zero, and the consequence (divide-by-zero) is quite
bad.

Closes: https://sashiko.dev/#/bug/linux-7f814c9a-986f-4103-a55a-ef9537dcf62b
Fixes: 8b549a4fd3c5 ("mm/damon/sysfs-schemes: commit damos quota goals user input to DAMOS")
Cc: <stable@vger.kernel.org> # 6.8.x
Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/sysfs-schemes.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
index 8f083611741fd..55298a17b653a 100644
--- a/mm/damon/sysfs-schemes.c
+++ b/mm/damon/sysfs-schemes.c
@@ -1355,7 +1355,13 @@ static ssize_t target_value_store(struct kobject *kobj,
 {
 	struct damos_sysfs_quota_goal *goal = container_of(kobj, struct
 			damos_sysfs_quota_goal, kobj);
-	int err = kstrtoul(buf, 0, &goal->target_value);
+	unsigned long target_value;
+	int err = kstrtoul(buf, 0, &target_value);
+
+	if (!mutex_trylock(&damon_sysfs_lock))
+		return -EBUSY;
+	goal->target_value = target_value;
+	mutex_unlock(&damon_sysfs_lock);
 
 	return err ? err : count;
 }

base-commit: 33560dfbb2df9b2d1525fd51ceeca3c1167431e3
-- 
2.47.3

Re: [RFC PATCH] mm/damon/sysfs-schemes: hold damon_sysfs_lock in target_value_store()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm/damon/sysfs-schemes: uninitialized stack read in target_value_store()
--

commit f43fea0015fcb1917385f72d150c59603a9ae43c
Author: SJ Park <sj@kernel.org>

mm/damon/sysfs-schemes: hold damon_sysfs_lock in target_value_store()

This commit holds damon_sysfs_lock in target_value_store() while updating
goal->target_value to prevent a time-of-check-to-time-of-use race with
damon_sysfs_add_quota_score() that could result in a divide-by-zero.

> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index 8f083611741fd..55298a17b653a 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
> @@ -1355,7 +1355,13 @@ static ssize_t target_value_store(struct kobject *kobj,
>  {
>  	struct damos_sysfs_quota_goal *goal = container_of(kobj, struct
>  			damos_sysfs_quota_goal, kobj);
> -	int err = kstrtoul(buf, 0, &goal->target_value);
> +	unsigned long target_value;
> +	int err = kstrtoul(buf, 0, &target_value);
> +
> +	if (!mutex_trylock(&damon_sysfs_lock))
> +		return -EBUSY;
> +	goal->target_value = target_value;
> +	mutex_unlock(&damon_sysfs_lock);
>  
>  	return err ? err : count;
>  }

[Severity: High]
What happens in target_value_store() if kstrtoul() fails (for example,
returning -EINVAL or -ERANGE on malformed input) while damon_sysfs_lock is
not contended?

Since kstrtoul() leaves target_value untouched on error and err is not
checked before acquiring the lock, wouldn't this overwrite
goal->target_value with an uninitialized stack value (which could also be
read back via target_value_show())? Should err be checked and returned
immediately after kstrtoul()?

[]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020144726.88978-1-sj@kernel.org?part=1

Re: [RFC PATCH] mm/damon/sysfs-schemes: hold damon_sysfs_lock in target_value_store()

[SJ Park]

On Tue, 20 Oct 2026 14:54:16 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] mm/damon/sysfs-schemes: uninitialized stack read in target_value_store()
> --
> 
> commit f43fea0015fcb1917385f72d150c59603a9ae43c
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/sysfs-schemes: hold damon_sysfs_lock in target_value_store()
> 
> This commit holds damon_sysfs_lock in target_value_store() while updating
> goal->target_value to prevent a time-of-check-to-time-of-use race with
> damon_sysfs_add_quota_score() that could result in a divide-by-zero.
> 
> > diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> > index 8f083611741fd..55298a17b653a 100644
> > --- a/mm/damon/sysfs-schemes.c
> > +++ b/mm/damon/sysfs-schemes.c
> > @@ -1355,7 +1355,13 @@ static ssize_t target_value_store(struct kobject *kobj,
> >  {
> >  	struct damos_sysfs_quota_goal *goal = container_of(kobj, struct
> >  			damos_sysfs_quota_goal, kobj);
> > -	int err = kstrtoul(buf, 0, &goal->target_value);
> > +	unsigned long target_value;
> > +	int err = kstrtoul(buf, 0, &target_value);
> > +
> > +	if (!mutex_trylock(&damon_sysfs_lock))
> > +		return -EBUSY;
> > +	goal->target_value = target_value;
> > +	mutex_unlock(&damon_sysfs_lock);
> >  
> >  	return err ? err : count;
> >  }
> 
> [Severity: High]
> What happens in target_value_store() if kstrtoul() fails (for example,
> returning -EINVAL or -ERANGE on malformed input) while damon_sysfs_lock is
> not contended?
> 
> Since kstrtoul() leaves target_value untouched on error and err is not
> checked before acquiring the lock, wouldn't this overwrite
> goal->target_value with an uninitialized stack value (which could also be
> read back via target_value_show())? Should err be checked and returned
> immediately after kstrtoul()?

Good catch.  I will check kstrtorul() error and return -EINVAL in the next
version.

> 
> []
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261020144726.88978-1-sj@kernel.org?part=1


Thanks,
SJ

Re: [RFC PATCH] mm/damon/sysfs-schemes: hold damon_sysfs_lock in target_value_store()

[SJ Park]

On Tue, 20 Oct 2026 07:47:25 -0700 SJ Park <sj@kernel.org> wrote:

> Remove a race condition under certain use cases that can trigger
> divide-by-zero.
> 
> damon_sysfs_add_quota_score() setup damos quota goal as user configured
> and pass to the core layer.  Because it could have a race with
> damos_sysfs_quota_goals_rm_dirs(), which destructs the files that carry
> user configuration, both are called under damon_sysfs_lock protection.
> 
> The damos quota goal setup disallows zero target_value parameter because
> it can be used as a denominator later in the core layer function,
> damos_quota_score().  Otherwise, divide-by-zero can happen.

This is no more true due to another patch [1].  The patch added the zero
target_value case in the core layer central validation pass.  But it forgot
removing the redundant check in sysfs.

> 
> For the zero value check and final assignment,
> damon_sysfs_add_quota_score() reads damos_sysfs_quota_goal->target_value
> twice.  And user can update damos_sysfs_quota_goal->target_value via
> target_value_store(), which doesn't hold damon_sysfs_lock.  As a result,
> a time-of-check-to time-of-use (toctou) race can happen.  If the user
> sets the value to zero between the zero value check and final
> target_value assignment, zero target_value is passed to the core layer
> and results in divide-by-zero.

So toctou race exists, but it cannot result in divide-by-zero.

> 
> Fix by holding damon_sysfs_lock in target_value_store() while saving the
> value.  The read side, damon_sysfs_add_quota_score(), is already called
> while holding the same mutex lock, so it avoids the race.
> 
> Users could reproduce the race by writing zero to the target_value sysfs
> file while DAMON is starting, or online parameters update is ongoing.
> The issue will only rarely happen in the real use cases.  It requires
> sysfs write permission.  The race window is short.  Sane users wouldn't
> write zero target_value.  Especially, they wouldn't do that while DAMON
> enablement and/or online parameters update is ongoing.  That said, the
> probability is not zero, and the consequence (divide-by-zero) is quite
> bad.
> 
> Closes: https://sashiko.dev/#/bug/linux-7f814c9a-986f-4103-a55a-ef9537dcf62b

Maybe Sashiko found the issue before the other patch [1] is applied.


> Fixes: 8b549a4fd3c5 ("mm/damon/sysfs-schemes: commit damos quota goals user input to DAMOS")
> Cc: <stable@vger.kernel.org> # 6.8.x

So this patch is unnecessary for the mainline.  But still need to be added to
older kernels.  Backporting the mainline fix [1] would be one simple way.  The
backporting would be not that straightforward because the core layer parameters
validation was introduced on 7.3 via commit b90408ef1163 ("mm/damon/core:
safely validate src on damon_commit_ctx()").  Hopefully doable.  I will take
time to do backports for relavant LTS and stable kernels.

[1] https://lore.kernel.org/20260903010722.94244-2-sj@kernel.org


Thanks,
SJ

[...]

Re: [RFC PATCH] mm/damon/sysfs-schemes: hold damon_sysfs_lock in target_value_store()

[SJ Park]

On Tue, 20 Oct 2026 17:52:33 -0700 SJ Park <sj@kernel.org> wrote:

> On Tue, 20 Oct 2026 07:47:25 -0700 SJ Park <sj@kernel.org> wrote:
> 
> > Remove a race condition under certain use cases that can trigger
> > divide-by-zero.
> > 
> > damon_sysfs_add_quota_score() setup damos quota goal as user configured
> > and pass to the core layer.  Because it could have a race with
> > damos_sysfs_quota_goals_rm_dirs(), which destructs the files that carry
> > user configuration, both are called under damon_sysfs_lock protection.
> > 
> > The damos quota goal setup disallows zero target_value parameter because
> > it can be used as a denominator later in the core layer function,
> > damos_quota_score().  Otherwise, divide-by-zero can happen.
> 
> This is no more true due to another patch [1].  The patch added the zero
> target_value case in the core layer central validation pass.  But it forgot
> removing the redundant check in sysfs.
> 
> > 
> > For the zero value check and final assignment,
> > damon_sysfs_add_quota_score() reads damos_sysfs_quota_goal->target_value
> > twice.  And user can update damos_sysfs_quota_goal->target_value via
> > target_value_store(), which doesn't hold damon_sysfs_lock.  As a result,
> > a time-of-check-to time-of-use (toctou) race can happen.  If the user
> > sets the value to zero between the zero value check and final
> > target_value assignment, zero target_value is passed to the core layer
> > and results in divide-by-zero.
> 
> So toctou race exists, but it cannot result in divide-by-zero.

And the toctou doesn't matter because the final value based same check will be
made in the core layer.  But the toctou makes readers easy to be confused.  I
will post another patch removing the unnecessary zero value check.


Thanks,
SJ

[...]