Re: (sashiko review) [RFC PATCH 1/2] Docs/admin-guide/mm/damon/reclaim: warn commit_inputs vs param updates race

[SeongJae Park <sj@kernel.org>]

On Sat, 28 Mar 2026 10:34:45 -0700 SeongJae Park <sj@kernel.org> wrote:

> Forwarding Sashiko review for doing discussions via mails.
> 
> # review url: https://sashiko.dev/#/patchset/20260328172415.49940-2-sj@kernel.org
> # start of sashiko.dev inline review
> commit 8b189801fc104c5ecc412454a3d8db6a3faf27d6
> Author: SeongJae Park <sj@kernel.org>
>     
> Docs/admin-guide/mm/damon/reclaim: warn commit_inputs vs param updates race
> 
> This commit adds documentation warning users not to write to module
> parameters while commit_inputs is in progress. This warning is intended to
> help users avoid a race condition that can cause undefined kernel behavior.
> 
> Link: https://lore.kernel.org/20260319161620.189392-3-objecting@objecting.org
> 
> > diff --git a/Documentation/admin-guide/mm/damon/reclaim.rst b/Documentation/admin-guide/mm/damon/reclaim.rst
> > --- a/Documentation/admin-guide/mm/damon/reclaim.rst
> > +++ b/Documentation/admin-guide/mm/damon/reclaim.rst
> > @@ -71,6 +71,10 @@ of parametrs except ``enabled`` again.  Once the re-reading is done, this
> >  parameter is set as ``N``.  If invalid parameters are found while the
> >  re-reading, DAMON_RECLAIM will be disabled.
> >  
> > +Once ``Y`` is written to this parametr, the user must not write to any
> 
> This isn't a bug, but there is a typo ("parametr") in this line.

Good eye, I will fix this in the next revision.

> 
> > +parameters until reading ``commit_inputs`` again returns ``N``.  If users
> > +violate this rule, the kernel can do undefined behaviors.
> 
> Is it acceptable to document a race condition that leads to undefined
> behavior instead of fixing it?
> 
> Even if the module parameter interface is restricted to root, user space
> shouldn't be able to trigger kernel undefined behavior (which could lead to
> memory corruption or crashes) simply by writing to module parameters
> concurrently.
> 
> Should the underlying race condition in DAMON_RECLAIM be fixed with proper
> synchronization rather than relying on userspace adherence to a documentation
> warning?

Good callout.  In short, I agree for mainline kernel, but I'd argue this may be
enough for stable kernels.

That is, I was also thinking in a way similar to sashiko.  But, from the work I
realized the amount of changes for fixing those in stable-backporting-easy way
is not small.  Also, given the age of the broken commits (5.19!), I don't think
this is urgent.  And again arguably, as long as it is well documented, letting
users break kernel when they want (e.g., sysrq-trigger) is ok...?  Let me know
if other humans have different opinion, though.

But for future (mainline), surely this have to be fixed in the code.  And
Liew's patch [1] was initially started not for exactly this patch, but will fix
this together.  Cc-ing Liew.

So, I'm suggesting to add this documentation for stable kernels, and continue
helping Liew's patch for cleanly removing this awkward behavior in the mianline
and future kernels.

> 
> This isn't a bug, but the phrasing "can do undefined behaviors" is slightly
> awkward. Rephrasing to "may exhibit undefined behavior" might be more natural.

Good catch.  I will update so in the next version.

[1] https://lore.kernel.org/20260328084524.5451-1-aethernet65535@gmail.com


Thanks,
SJ

[...]