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

[Zi Yan]

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@xxxxxxxxx> 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@xxxxxxxxx
>>>
>>> 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.

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

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.

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.



-- 
Best Regards,
Yan, Zi