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

[SeongJae Park <sj@kernel.org>]

Hello Junxi,


When you send a new version of a patch, please send it as a new mail with
changes log [1], rather than as a reply to the old version.

On Mon, 20 Apr 2026 20:54:05 +0800 Junxi Qian <qjx1298677004@gmail.com> wrote:

> memcg_path_store() and path_store() free and replace their respective
> string pointers without holding damon_sysfs_lock. This creates a race
> with the kdamond thread, which reads these pointers in
> damon_sysfs_add_scheme_filters() and damos_sysfs_add_quota_score()
> via damon_call() during a "commit" operation.
> 
> The race window is as follows:
>   Thread A (commit):  holds damon_sysfs_lock, triggers damon_call()
>   kdamond thread:     reads sysfs_filter->memcg_path in
>                       damon_sysfs_memcg_path_to_id()
>   Thread B (sysfs):   calls memcg_path_store() WITHOUT lock,
>                       kfree(filter->memcg_path), replaces pointer
> 
> Since Thread B does not hold damon_sysfs_lock, the kdamond thread can
> observe a freed pointer, resulting in a use-after-free when
> damon_sysfs_memcg_path_eq() dereferences it for string comparison.

Thank you for finding and sharing this bug!
> 
> Similarly, memcg_path_show() and path_show() read the string pointers
> without holding the lock, so a concurrent store can free the string
> just before sysfs_emit() dereferences it, causing a use-after-free.

Is this correct?  Isn't sysfs prohibiting such concurrent read/write using
kernfs_open_file->mutex?  Were you able to trigger this?

> 
> KASAN reports the following on v7.0.0-rc5 with CONFIG_KASAN=y:
> 
>   ==================================================================
>   BUG: KASAN: double-free in memcg_path_store+0x6e/0xc0
> 
>   Free of addr ffff888100a086c0 by task exp/149
> 
>   CPU: 1 UID: 0 PID: 149 Comm: exp Not tainted 7.0.0-rc5-gd38efd7c139a #18 PREEMPT(lazy)
>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
>   Call Trace:
>    <TASK>
>    dump_stack_lvl+0x55/0x70
>    print_report+0xcb/0x5d0
>    kasan_report_invalid_free+0x94/0xc0
>    check_slab_allocation+0xde/0x110
>    kfree+0x114/0x3b0
>    memcg_path_store+0x6e/0xc0
>    kernfs_fop_write_iter+0x2fc/0x490
>    vfs_write+0x8e1/0xcc0
>    ksys_write+0xee/0x1c0
>    do_syscall_64+0xfc/0x580
>    entry_SYSCALL_64_after_hwframe+0x77/0x7f
>    </TASK>
> 
>   Allocated by task 147 on cpu 0 at 12.364295s:
>    kasan_save_stack+0x24/0x50
>    kasan_save_track+0x17/0x60
>    __kasan_kmalloc+0x7f/0x90
>    __kmalloc_noprof+0x191/0x490
>    memcg_path_store+0x32/0xc0
>    kernfs_fop_write_iter+0x2fc/0x490
>    vfs_write+0x8e1/0xcc0
>    ksys_write+0xee/0x1c0
> 
>   Freed by task 150 on cpu 0 at 13.373810s:
>    kasan_save_stack+0x24/0x50
>    kasan_save_track+0x17/0x60
>    kasan_save_free_info+0x3b/0x60
>    __kasan_slab_free+0x43/0x70
>    kfree+0x137/0x3b0
>    memcg_path_store+0x6e/0xc0
>    kernfs_fop_write_iter+0x2fc/0x490
>    vfs_write+0x8e1/0xcc0
>    ksys_write+0xee/0x1c0
> 
>   The buggy address belongs to the object at ffff888100a086c0
>    which belongs to the cache kmalloc-8 of size 8
>   ==================================================================
> 
> Fix this by holding damon_sysfs_lock while swapping the path pointer
> in both memcg_path_store() and path_store(),

I'd like to avoid use of the lock in long term.  But this sounds good and
simple for hotfixes.

> and while reading the
> path pointer in memcg_path_show() and path_show().

As I commented above, I'm not sure if this is really needed.

> The actual kfree()
> is moved outside the lock since the old pointer is no longer reachable
> once replaced.
> 
> Fixes: 490a43d07f16 ("mm/damon/sysfs-schemes: free old damon_sysfs_scheme_filter->memcg_path on write")

I think this deserves Cc-ing stable@.

Also, this patch is fixing two bugs.  For making backport of the fixes easy,
could we split this patch into two patches, one for memcg_path_{show,store}(),
and the other one for path_{show,store}()?

> Signed-off-by: Junxi Qian <qjx1298677004@gmail.com>
> 
> ---
> Changes in v2:
> - Also protect memcg_path_show() and path_show() with damon_sysfs_lock,
>   since sysfs show and store callbacks can run concurrently and a
>   lockless read in show can race with the kfree in store. (Sashiko AI)

As I abovely commented, I'm not sure that Sashiko comment is correct.

> 
>  mm/damon/sysfs-schemes.c | 36 ++++++++++++++++++++++++++++--------
>  1 file changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index 5186966da..1a890e2a4 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);

I read Sashiko is commenting [2] about possible ABBA deadlock, and suggesting
mutex_trylock().  I think that makes sense.  damon_sysfs_lock is always only
mutex_trylock()-ed for the reason.

> +	len = sysfs_emit(buf, "%s\n",
>  			filter->memcg_path ? filter->memcg_path : "");
> +	mutex_unlock(&damon_sysfs_lock);
> +
> +	return len;
>  }

And I'm unsure if this change for memcg_path_show() is really needed.

>  
>  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);

Let's use mutex_trylock().

> +	old = filter->memcg_path;
>  	filter->memcg_path = path;
> +	mutex_unlock(&damon_sysfs_lock);
> +
> +	kfree(old);
>  	return count;
>  }

Same comments for path_show() and path_store() changes.

[1] https://docs.kernel.org/process/submitting-patches.html#commentary
[2] https://lore.kernel.org/20260420183146.0DA7AC2BCB0@smtp.kernel.org


Thanks,
SJ

[...]