Re: (sashiko review) [RFC PATCH] mm/damon: reset thread status parameters upon kdamond termination

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

> [...]
> > @@ -2941,6 +2944,14 @@ static void kdamond_init_ctx(struct damon_ctx *ctx)
> >  	}
> >  }
> >  
> > +static void damon_update_thread_status(struct damon_ctx *ctx)
> > +{
> > +	if (ctx->thread_status.kdamond_pid)
> > +		*ctx->thread_status.kdamond_pid = -1;
> > +	if (ctx->thread_status.enabled)
> > +		*ctx->thread_status.enabled = false;
> 
> Can this write race with the user enabling the module? If a user writes 'Y'
> to 'enabled', the store function spawns kdamond and prepares to set 'enabled'
> to true. If kdamond exits immediately (e.g., due to invalid targets), could
> this line asynchronously set 'enabled' to false before the store function
> overwrites it with true?
> 
> If so, the system would be left in a state where the thread is dead but
> 'enabled' is true. Subsequent attempts to write 'N' to 'enabled' would fail
> on damon_stop(), leaving the module permanently locked.

You are right. I now see the potential race between kdamond exiting and
the enabled_store(). While it seems unlikely, the window exists and
could lead to an inconsistent state. I'm sitll thinking about the way to
synchronize this without introducing new issues. I will try to address
this is next-version once I have a solid plan.

> > +}
> > +
> >  /*
> >   * The monitoring daemon that runs as a kernel thread
> >   */
> 
> [ ... ]
> 
> > @@ -3065,17 +3076,23 @@ static int kdamond_fn(void *data)
> >  	kdamond_call(ctx, true);
> >  	damos_walk_cancel(ctx);
> >  
> > -	pr_debug("kdamond (%d) finishes\n", current->pid);
> >  	mutex_lock(&ctx->kdamond_lock);
> >  	ctx->kdamond = NULL;
> >  	mutex_unlock(&ctx->kdamond_lock);
> >  
> > +	if (ctx->thread_status.enabled && *ctx->thread_status.enabled)
> 
> Can this access freed memory? If the kdamond_lock is dropped and
> ctx->kdamond is NULL, damon_is_running(ctx) becomes false. If a concurrent
> sysfs operation removes the context, could ctx be freed by damon_destroy_ctx()
> before these lines execute, causing a use-after-free and memory corruption in
> damon_update_thread_status(ctx)?

I have performed tests with KASAN enabled on virtme-ng. During multiple
start/stop/fail cycles, KASAN did not report any UAF.

> > +		pr_debug("kdamond (%d) crashed\n", current->pid);
> 
> Does this log normal user-requested shutdowns as crashes? When a user stops
> the module by writing 'N' to 'enabled', the parameter store blocks on
> damon_stop(). Since the global 'enabled' variable is still true at this
> point, won't this incorrectly print a crash message instead of finishing
> normally?

Thank you for reminder. This logic is indeed redundant and pontentially
confusing to users. I will restore the original output in next-version.
:>

> [...]

Best regards,
Rui Yan