Re: [PATCH v2] mm/damon/sysfs-schemes: fix use-after-free on memcg_path and goal path

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [High] Using `mutex_lock(&damon_sysfs_lock)` in sysfs `.show` and `.store` callbacks introduces a classic ABBA deadlock against concurrent directory removal.
--

commit 01b23742d072b6f9a33d05db48195e9adf5cf0a9
Author: Junxi Qian <qjx1298677004@gmail.com>

mm/damon/sysfs-schemes: fix use-after-free on memcg_path and goal path

This commit wraps the string pointer reads and writes in memcg_path_show(),
memcg_path_store(), path_show(), and path_store() with damon_sysfs_lock
to prevent use-after-free bugs when racing with the kdamond thread.

> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index 5186966dafb35..1a890e2a4eb9c 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
> @@ -533,9 +533,14 @@ static ssize_t memcg_path_show(struct kobject *kobj,
>  {
>  	struct damon_sysfs_scheme_filter *filter = container_of(kobj,
>  			struct damon_sysfs_scheme_filter, kobj);
> +	ssize_t len;
>  
> -	return sysfs_emit(buf, "%s\n",
> +	mutex_lock(&damon_sysfs_lock);

Could this introduce an ABBA deadlock against concurrent directory removal?

When a user reads a sysfs file, kernfs acquires an active reference
(s_active) before executing the show callback, which now blocks
unconditionally on damon_sysfs_lock.

If another thread simultaneously writes to a file that removes directories
(like nr_filters_store), it will acquire damon_sysfs_lock and call
kobject_put(). The teardown path then invokes kernfs_drain(), which
synchronously waits for s_active to drop.

This leaves the teardown thread holding the lock and waiting for s_active,
while the read thread holds s_active and waits for the lock.

Should this use mutex_trylock(&damon_sysfs_lock) and return -EBUSY on
failure, similar to other sysfs callbacks in this subsystem?

> +	len = sysfs_emit(buf, "%s\n",
>  			filter->memcg_path ? filter->memcg_path : "");
> +	mutex_unlock(&damon_sysfs_lock);
> +
> +	return len;
>  }
>  
>  static ssize_t memcg_path_store(struct kobject *kobj,
> @@ -543,15 +548,20 @@ static ssize_t memcg_path_store(struct kobject *kobj,
>  {
>  	struct damon_sysfs_scheme_filter *filter = container_of(kobj,
>  			struct damon_sysfs_scheme_filter, kobj);
> -	char *path = kmalloc_array(size_add(count, 1), sizeof(*path),
> -				   GFP_KERNEL);
> +	char *path, *old;
>  
> +	path = kmalloc_array(size_add(count, 1), sizeof(*path), GFP_KERNEL);
>  	if (!path)
>  		return -ENOMEM;
>  
>  	strscpy(path, buf, count + 1);
> -	kfree(filter->memcg_path);
> +
> +	mutex_lock(&damon_sysfs_lock);

Does this face the same deadlock risk as the show function? 

If converted to mutex_trylock(), would we need to free the newly allocated
path buffer before returning -EBUSY?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260420125405.362137-1-qjx1298677004@gmail.com?part=1