Re: [PATCH 2/4] mm/huge_memory: skip zone device folios in madvise_free_huge_pmd()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] madvise_cold_or_pageout_pte_range() fails to filter out zone device PMDs, risking invalid operations on device memory.
- [High] madvise_cold_or_pageout_pte_range() triggers a refcount livelock by elevating the folio refcount before acquiring the folio lock on the PMD path.
--

--- Patch [2]: [PATCH 2/4] mm/huge_memory: skip zone device folios in madvise_free_huge_pmd() ---
commit 624f2a18069c771c887a2d3f875451eb3bd7e41d
Author: Gregory Price <[email protected]>
Subject: mm/huge_memory: skip zone device folios in madvise_free_huge_pmd()

This commit addresses an issue in madvise_free_huge_pmd() where zone device
folios were being marked as lazyfree without proper checks. It adds a bailout
condition to skip the operation if the folio is a zone device folio.

> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 73522f296cee2..0b48112574882 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2338,6 +2338,10 @@ bool madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
>  	}
>  
>  	folio = pmd_folio(orig_pmd);
> +
> +	if (folio_is_zone_device(folio))
> +		goto out;
> +
>  	/*
>  	 * If other processes are mapping this folio, we couldn't discard

While reviewing the zone device filtering added here, I noticed a couple of
pre-existing issues in the related PMD handling path in mm/madvise.c.

mm/madvise.c:madvise_cold_or_pageout_pte_range() {
[ ... ]
		folio = pmd_folio(orig_pmd);

		/* Do not interfere with other mappings of this folio */
		if (folio_maybe_mapped_shared(folio))
			goto huge_unlock;

[Severity: Critical]
This is a pre-existing issue, but is it possible for a zone device PMD to be
processed here?

The folio is resolved from the PMD but lacks the folio_is_zone_device(folio)
filter (unlike the PTE path). If a DAX or device coherent THP is partially
advised, the split path could leave the pgmap pointer uninitialized on the
split folios, potentially causing a kernel panic when accessed. Could we add a
similar bailout here?


mm/madvise.c:madvise_cold_or_pageout_pte_range() {
[ ... ]
		if (next - addr != HPAGE_PMD_SIZE) {
			int err;

			folio_get(folio);
			spin_unlock(ptl);
			folio_lock(folio);
			err = split_folio(folio);

[Severity: High]
This isn't a bug introduced by this patch, but could this locking order create
a refcount livelock?

Calling folio_get() before blocking on folio_lock() elevates the folio's
refcount. When split_folio() later checks the expected refcount against
folio_ref_count(), wouldn't it fail with -EAGAIN if other tasks are also
blocked trying to lock the folio? Should the refcount be elevated only after
acquiring the folio lock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2