Re: [PATCH v1] mm/damon/pa: initialize 'folio' to NULL to prevent use of uninitialized value

Re: [PATCH v1] mm/damon/pa: initialize 'folio' to NULL to prevent use of uninitialized value

[SeongJae Park]

+ damon@

On Tue, 30 Dec 2025 23:00:22 -0800 SeongJae Park <sj@kernel.org> wrote:

> Hello Aaron,
> 
> 
> Thank you for sending this patch!
> 
> For the patch subject, let's use 'mm/damon/paddr:' as the prefix of the
> subject, for making it consistent with other past commits for the paddr.c file.
> 
> On Wed, 31 Dec 2025 13:57:37 +0800 Aaron Yang <yangqixiao@inspur.com> wrote:
> 
> > In damon_pa_mark_accessed_or_deactivate(), the local variable 'folio'
> > is declared but not initialized. If the region [r->ar.start, r->ar.end)
> > is empty or invalid such that the while-loop body is never entered,
> > 'folio' retains an indeterminate (garbage) value. The function then
> > unconditionally assigns this uninitialized pointer to s->last_applied
> > (line 239), resulting in undefined behavior. Subsequent dereference or
> > folio_put() on s->last_applied may cause crashes or memory corruption.
> 
> Nice finding!
> 
> > 
> > Although DAMON regions are typically non-empty, zero-length regions
> > can arise during region merging/splitting or due to address unit
> > alignment — making this path reachable in practice.
> 
> But, can zero-length regions be made in real?  damon_ctx->min_sz_region
> disallows DAMON having regions of size less that it, and all DAMON API callers
> set it at least 1.  That is, DAMON code is at least intentionally designed to
> prohibit zero-length regions.  And many parts of DAMON including
> damon_pa_mark_accessed_or_deactivate() are written under the assumption that
> there is no zero-length regions.  If there is a case that allows zero-length
> regions, all such parts could trigger problems.
> 
> > 
> > Fix by initializing 'folio' to NULL. Assigning NULL to s->last_applied
> > is safe and semantically correct: it cleanly indicates "no folio was
> > processed in this invocation", and callers are expected to check for
> > NULL before use (as per common kernel practice).
> 
> For the above mentioned reason, I'd like to fix the code that allows
> zero-length regions, rather than making only this single function safe from the
> bug.

I still think in the above way.  But in my second thought, I also find no
reason to oppose to initializing 'folio' to NULL.  It adds overheads of
unnecessary initialization, but those seems quite negligible.  Meanwhile,
_arguably_ it makes the function more complete and easy to read in my humble
opinion.  The function might better to have further cleanup or complete
rewriting, but I think such complete rewriting is too much request, while
initializing the variable is a readability improvement for at least someone.

That said, to do the initialization, I think the commit message need to be
correctly updated.  Otherwise, it may only confuse people and encourage making
changes that allow zero-length regions.  Also, this pattern exists on
damon_pa_pageout(), damon_pa_migrate() and damon_pa_stat(), so better to update
those altogether for consistency.

So, Aaron, if you still williing to make this change, could you please send v2
after doing aobvely commented requests (updating commit message and making
changes to other functions)?


Thanks,
SJ

[...]