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

[Yadav, Arvind]

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



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

>