Re: [PATCH v3 2/2] mm/damon/core: Copy only installed probe in damon_split_region_at()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mm: damon: memory leaks in kunit tests due to unreachable cleanup code after kunit_skip()
- [Low] mm: dead code and memory leaks in DAMON kunit tests on error paths
--

commit 0052b3fffc74be07964d42e4abe4023a18442771
Author: Suhaas Joshi <suhaas@s-joshi.in>

mm/damon/core: Copy only installed probe in damon_split_region_at()

This commit updates damon_split_region_at() to take a struct damon_ctx
pointer, iterating only over installed probes when copying probe hits. 
It also updates all callsites and KUnit tests to supply the context.

> diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> index 385d32901650e..61ce085549a66 100644
> --- a/mm/damon/tests/core-kunit.h
> +++ b/mm/damon/tests/core-kunit.h
[ ... ]
> @@ -150,13 +152,25 @@ static void damon_test_split_at(struct kunit *test)
>  		damon_free_target(t);
>  		kunit_skip(test, "region alloc fail");
>  	}
> +	p = damon_new_probe();
> +	if (!p)
> +		kunit_skip(test, "probe alloc fail");

[Severity: Medium]
Since kunit_skip() internally calls kunit_try_catch_throw() which is marked
as __noreturn, execution will halt here.

Will this leak the previously allocated target 't' and region 'r', as they
are not freed before calling kunit_skip()?

> +	ctx = damon_new_ctx();
> +	if (!ctx) {
> +		damon_destroy_probe(p);
> +		damon_free_target(t);
> +		kunit_skip(test, "context alloc fail");
> +	}

[Severity: Medium]
Will this leak the allocated region 'r'?

In damon_test_split_at(), 'r' is allocated early, but it is not attached to 
the target 't' until damon_add_region(r, t) is called later. Calling
damon_free_target(t) here won't free 'r'.

[ ... ]
> @@ -206,7 +222,21 @@ static void damon_test_merge_two(struct kunit *test)
>  	r2->age = 21;
>  	damon_add_region(r2, t);
>  
> -	damon_merge_two_regions(t, r, r2);
> +	p = damon_new_probe();
> +	if (!p) {
> +		kunit_skip(test, "probe alloc fail");
> +		damon_free_target(t);
> +	}

[Severity: Low]
Is this damon_free_target(t) call unreachable dead code?

Because kunit_skip() does not return, any cleanup code placed after it will
never execute, which results in leaking 't' and its attached regions.

[ ... ]
> @@ -1802,28 +1834,42 @@ static void damos_test_filter_out(struct kunit *test)
>  	}
>  	damon_add_region(r, t);
>  
> +	p = damon_new_probe();
> +	if (!p) {
> +		kunit_skip(test, "probe alloc fail");
> +		damon_free_target(t);
> +	}

[Severity: Low]
Does this have the same unreachable dead code issue as the one in
damon_test_merge_two()? The damon_free_target(t) is placed after the
__noreturn kunit_skip() call.

> +	ctx = damon_new_ctx();
> +	if (!ctx) {
> +		damon_destroy_probe(p);
> +		damon_free_target(t);
> +		kunit_skip(test, "context alloc fail");
> +	}
[ ... ]
> @@ -2468,7 +2515,14 @@ static void damon_test_last_region_quota_reset(struct kunit *test)
>  	s.quota.charge_target_from = t;
>  	s.quota.charge_addr_from = r1->ar.end;
>  
> -	skipped = damos_skip_charged_region(t, r2, &s, min_region_sz);
> +	ctx = damon_new_ctx();
> +	if (!ctx) {
> +		kunit_skip(test, "ctx alloc failed");
> +		damon_free_target(t);
> +	}

[Severity: Low]
Again, is this damon_free_target(t) unreachable because kunit_skip() aborts
the execution?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012145636.168436-1-suhaas@s-joshi.in?part=2