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

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

[Aaron Yang]

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.

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.

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

No functional change for non-empty regions; only hardens error/edge
case handling.

Signed-off-by: Aaron Yang <yangqixiao@inspur.com>
---
 mm/damon/paddr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
index 07a8aead439e..32d8024d130e 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -212,7 +212,7 @@ static inline unsigned long damon_pa_mark_accessed_or_deactivate(
 		unsigned long *sz_filter_passed)
 {
 	phys_addr_t addr, applied = 0;
-	struct folio *folio;
+	struct folio *folio = NULL;
 
 	addr = damon_pa_phys_addr(r->ar.start, addr_unit);
 	while (addr < damon_pa_phys_addr(r->ar.end, addr_unit)) {
-- 
2.47.3

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

[SeongJae Park]

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.

So, if you found a case that allows zero-length DAMON regions, could you please
share the reproduction steps of it, or fix of it?

If you didn't find a real case that allows zero-length DAMON regions but was
thinking it might be theoretically possible, maybe the poor documentation of
DAMON has confused you to believe so.  If that's the case, how about making it
clear on the documentation?  Specifically, I think damon_region kernel-doc
comment in include/linux/damon.h could be a good place to make this explicitly
explained.


Thanks,
SJ

[...]

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

[...]