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

["Yadav, Arvind" <arvind.yadav@intel.com>]

On 27-07-2026 07:16, 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.
>
>> 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.



Thanks Zi Yan, this makes sense.
After the split, each page is a separate order-0 folio so setting nr = 1 
lets each folio go through folio_free_swap() and mapping lookup 
independently. I will fold this into v3.

Regards,
Arvind

>
>