Re: [PATCH v2 0/2] mm/damon: reset thread status parameters upon kdamond termination

[Liew Rui Yan <aethernet65535@gmail.com>]

Hi SeongJae,

I've reviewed the Sashiko report on [PATCH v2 1/2] and [2/2]. Since the
issues are essentially the same, I want to reply to them all in one
email.

# PATCH v2 1/2
> > diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> > index 554559d729760..96c8d0dfcafd2 100644
> > --- a/mm/damon/lru_sort.c
> > +++ b/mm/damon/lru_sort.c
> > @@ -344,6 +344,10 @@ static int damon_lru_sort_apply_parameters(void)
> >  	if (err)
> >  		goto out;
> >  	err = damon_commit_ctx(ctx, param_ctx);
> > +	if (err) {
> > +		enabled = false;
> > +		kdamond_pid = -1;
> > +	}
> 
> Does updating the module parameters here require holding kernel_param_lock?
> 
> Since damon_lru_sort_apply_parameters() can be executed asynchronously by the
> kdamond thread when a user writes to the commit_inputs parameter, changing
> these variables locklessly might introduce a race condition.
> 
> If enabled is set to false here while kdamond is still preparing to terminate,
> could a concurrent sysfs write (echo Y > enabled) read the false state and
> proceed to call damon_lru_sort_turn(true) because it incorrectly assumes the
> worker has completely stopped?
> 
> If so, damon_lru_sort_turn(true) would call damon_commit_ctx(ctx, ...) and
> modify the shared ctx structures while the exiting kdamond worker thread is
> concurrently executing its cleanup block, such as damon_destroy_targets(ctx).
> 
> Can this concurrent modification of the context lists lead to use-after-free
> issues or list corruption?

# PATCH v2 2/2
> > diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> > index 86da147786583..e3e148fd80f97 100644
> > --- a/mm/damon/reclaim.c
> > +++ b/mm/damon/reclaim.c
> [ ... ]
> > @@ -250,6 +250,10 @@ static int damon_reclaim_apply_parameters(void)
> >  	if (err)
> >  		goto out;
> >  	err = damon_commit_ctx(ctx, param_ctx);
> > +	if (err) {
> > +		enabled = false;
> > +		kdamond_pid = -1;
> > +	}
> >  out:
> >  	damon_destroy_ctx(param_ctx);
> >  	return err;
> 
> Can prematurely resetting enabled to false here introduce a race condition
> leading to a use-after-free of the DAMON context structures?
> 
> If damon_reclaim_apply_parameters() is invoked from the kdamond worker thread
> (for example, when applying commit_inputs) and damon_commit_ctx() fails,
> ctx->maybe_corrupted is set to true. This signals the kdamond thread to
> break its main loop and begin its teardown phase, such as executing
> damon_destroy_targets() to free lists.
> 
> Because enabled is set to false asynchronously here, a concurrent sysfs write
> of 'Y' to enabled will succeed and immediately trigger
> damon_reclaim_turn(true). This unconditionally calls
> damon_reclaim_apply_parameters() and executes damon_commit_ctx() from the
> sysfs thread.
> 
> Since damon_commit_ctx() locklessly mutates and frees items in
> ctx->adaptive_targets and ctx->schemes, would this race directly with the
> still-exiting kdamond thread traversing and freeing those exact same lists,
> resulting in list corruption and a use-after-free?

The core issue is - modifying 'enabled' and 'kdamond_pid' in the error
path of damon_commit_ctx() is racy.

My plan for v3:
- Remove the reset code in damon_*_apply_parameters()
- Keep only the fix in damon_*_turn(false)

This resolves the restart issue without introducing new races.
Please let me know if this direction looks good.

Small changes for v3:
- Delete a "=" at the bottom of "Problem" (commit message):

      Problem
-     ========
+     =======

Best regards,
Rui Yan