Re: [PATCH v2] mm/migrate_device: Clear stale mapping after freeing swapcache

[Balbir Singh <balbirs@nvidia.com>]

On 7/27/26 11:46 AM, Zi Yan wrote:
> On Sun Jul 26, 2026 at 8:38 PM EDT, Balbir Singh wrote:
>> On 7/26/26 7:05 AM, Zi Yan wrote:
>>> On Sat Jul 25, 2026 at 3:36 PM EDT, David Hildenbrand (Arm) wrote:
>>>> On 7/25/26 06:43, Andrew Morton wrote:
>>>>> On Fri, 24 Jul 2026 13:57:02 +0530 Arvind Yadav <arvind.yadav@intel.com> wrote:
>>>>>
>>>>>> __migrate_device_pages() reads the folio mapping before calling
>>>>>> folio_free_swap(). When folio_free_swap() succeeds, the folio is removed
>>>>>> from the swap cache, but the saved mapping still points to swap_space.
>>>>>>
>>>>>> Passing the stale mapping to folio_migrate_mapping() makes it take the
>>>>>> mapped-folio path after the swapcache reference has been dropped. This can
>>>>>> cause an invalid swap_space lock access followed by a folio reference
>>>>>> count BUG.
>>>>>>
>>>>>> Refresh the saved mapping after folio_free_swap() so the current folio
>>>>>> state is used during migration.
>>>>>>
>>>>>
>>>>> Thanks.  AI review might have found an issue with this.  And one
>>>>> possible pre-existing issue in the code which Alistair and Balbir
>>>>> worked on.
>>>>>
>>>>> 	https://sashiko.dev/#/patchset/20260724082702.2531024-1-arvind.yadav@intel.com
>>>>
>>>> Yeah, this might need another careful look.
>>>
>>> It seems that the pre-existing issue can be fixed by resetting nr to 1
>>> after split is successful. It should also complete this patch. Something
>>> like this:
>>>
>>>
>>> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
>>> index 18d097c388530..4a77b6c86ae4f 100644
>>> --- a/mm/migrate_device.c
>>> +++ b/mm/migrate_device.c
>>> @@ -1193,6 +1193,11 @@ static void __migrate_device_pages(unsigned long *src_pfns,
>>>  							 MIGRATE_PFN_COMPOUND);
>>>  					goto next;
>>>  				}
>>> +				/*
>>> +				 * reset nr so that only first after-split folio
>>> +				 * is processed below
>>> +				 */
>>> +				nr = 1;
>>>  			} else if ((src_pfns[i] & MIGRATE_PFN_MIGRATE) &&
>>>  				(dst_pfns[i] & MIGRATE_PFN_COMPOUND) &&
>>>  				!(src_pfns[i] & MIGRATE_PFN_COMPOUND)) {
>>>
>>>
>>
>> Hmm.. I don't this error condition possible, migrate_vma_split_unmapped_folio()
>> will VM_WARN_ON non anonymous folios, but the design contract is for anonymous
>> folios only. The enforcement comes from the callers of migrate_vma_pages() and
> 
> folio_test_anon() returns true for an anon folio in swapcache. So
> migrate_vma_split_unmapped_folio()'s VM_WARN_ON() does not prevent anon
> folios in swapcache.
> 

Agreed. Rhe warning is in folio_split_unmapped() it only works fornon-anon
folios. So it doesn't exclude the swapcache case.

And the !folio_test_anon(folio) || !folio_free_swap(folio) actually
handles a swapcache anon folio by dropping the swap, which is precisely
where the stale mapping originates. So you're right, my argument there
doesn't hold.

>> migrate_device_pages(). Also __folio_freeze_and_split_unmapped() checks if the
>> folio has a swapcache and mapping associated with it, prior to split. I think
>> this is a false positive
>>
> 
> NULL is passed as mapping to __folio_freeze_and_split_unmapped() by
> migrate_vma_split_unmapped_folio(), so the VM_WARN_ON_ONCE() there will
> not warn this.
> 

> None of the above arguments is valid.
> 

Agreed, mapping isn't passed all the way. The split still resolves the
swapcache correctly from the current folio rather than mapping - in
__folio_freeze_and_split_unmapped(), ci = swap_cluster_get_and_lock(folio)
guarded by folio_test_swapcache(folio), and each after-split folio is
fixed up via __swap_cache_replace_folio(ci, ...)

> One thing prevents large anon folios in swapcache from reaching to
> __migrate_device_pages() is migrate_vma_check_page(). When
> folio_mapping() is not NULL, extra pin count is only 1 +
> folio_has_private(), which happens to exclude large anon folios in
> swapcache.
> 


Yes, agreed - that's the reason why it's safe, and it kicks in during
migrate_vma_unmap() before any of the above. So the condition is not
reachable today for large folios; it's a false positive in that sense.

> Regardless, adding nr = 1 here still makes sense, since why should the
> code below process the old nr pages after split is successful? It might
> be an optimization for current large anon folio only case, but it is
> more like an issue in the future.
Agreed, and we could add it in as a correctness check. After a successful
split the inner loop still runs folio_migrate_mapping(mapping, ...) over
all nr after-split folios, but folio_free_swap() was only called on the
first one - so folios [i+1 .. i+nr) would be migrated as swapcache folios
against a single cached mapping.

Thanks for the correction!
Balbir