Re: (sashiko review) [PATCH v1 1/1] mm/damon: support MADV_COLLAPSE via DAMOS_COLLAPSE scheme action

[SeongJae Park]

Forwarding Sashiko review for doing discussions via mails.

# review url: https://sashiko.dev/#/patchset/20260330145758.2115502-1-gutierrez.asier@xxxxxxxxxxxxxxxxxxx
# start of sashiko.dev inline review
commit cd78854e55ee90102333f0331c58417c16e8d798
Author: Asier Gutierrez <gutierrez.asier@xxxxxxxxxxxxxxxxxxx>

mm/damon: support MADV_COLLAPSE via DAMOS_COLLAPSE scheme action

This commit introduces the DAMOS_COLLAPSE action to DAMON. It allows DAMON
to synchronously collapse memory regions into huge pages by leveraging the
existing MADV_COLLAPSE mechanism.

Link: https://lore.kernel.org/damon/20260313000816.79933-1-sj@xxxxxxxxxx/

[ ... ]

> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index b069dbc7e3d2..dd5f2d7027ac 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -903,6 +903,9 @@ static unsigned long damon_va_apply_scheme(struct damon_ctx *ctx,
>  	case DAMOS_NOHUGEPAGE:
>  		madv_action = MADV_NOHUGEPAGE;
>  		break;
> +	case DAMOS_COLLAPSE:
> +		madv_action = MADV_COLLAPSE;

Could exposing MADV_COLLAPSE to automated continuous execution via DAMOS
trigger a pre-existing use-after-free in the core madvise logic?

In madvise_collapse(), the lock_dropped boolean tracks whether the lock is
currently dropped and tells madvise_walk_vmas() if the lock was ever dropped.

If collapse_single_pmd() drops the mmap lock, *lock_dropped is set to true.
On the next loop iteration, madvise_collapse() re-acquires the lock,
validates the VMA, and resets *lock_dropped = false to avoid double-locking.
If a subsequent iteration does not drop the lock, the function returns to
madvise_walk_vmas() with lock_dropped set to false.

Because madvise_walk_vmas() sees false, it assumes the lock was continuously
held and the original VMA pointer is still safe. However, another thread
could have freed the original VMA while the lock was temporarily dropped,
leading to a use-after-free when madvise_walk_vmas() dereferences
vma->vm_end.

While this flaw pre-dates this patch, does introducing DAMOS_COLLAPSE expose
it to continuous background execution by kdamond, making the race condition
much more likely to occur in practice?

> +		break;
>  	case DAMOS_MIGRATE_HOT:
>  	case DAMOS_MIGRATE_COLD:


# end of sashiko.dev inline review
# review url: https://sashiko.dev/#/patchset/20260330145758.2115502-1-gutierrez.asier@xxxxxxxxxxxxxxxxxxx
#
# hkml [1] generated a draft of this mail.  It can be regenerated
# using below command:
#
#     hkml patch sashiko_dev --for_forwarding \
#             20260330145758.2115502-1-gutierrez.asier@xxxxxxxxxxxxxxxxxxx
#
# [1] https://github.com/sjp38/hackermail

Sent using hkml (https://github.com/sjp38/hackermail)