mm/mlock: use the IRQ-safe accessor for NR_MLOCK in __munlock_folio()

NR_MLOCK is updated from interrupt context. __free_pages_prepare() clears a stray PG_mlocked and adjusts NR_MLOCK, and a folio can reach it with the flag still set from a bio completion handler:

  __free_pages_ok+0x6af/0x7a0
  <IRQ>
  __bio_release_pages+0xde/0x260
  __iomap_dio_bio_end_io+0x16e/0x1a0
  blk_update_request+0x14b/0x3d0
  blk_mq_end_request+0x18/0x30
  blk_done_softirq+0x49/0x60

The folio gets there like this. A MAP_SHARED file mapping is mlocked, so its page cache folios carry PG_mlocked, and an O_DIRECT write sourced from that mapping GUP-pins those same folios. munlock() then runs mlock_vma_pages_range(), which clears VM_LOCKED before walking the page tables to munlock each folio. A concurrent hole punch reaches the folio through the rmap (i_mmap_rwsem, not mmap_lock) and can land inside that
window: __folio_remove_rmap() -> munlock_vma_folio() sees VM_LOCKED already clear, so it neither queues the folio on the mlock batch nor takes a reference, and the pte it clears makes the pending mlock_pte_range() walk skip the folio at its !pte_present() check. filemap_remove_folio() then drops the page cache reference, leaving the bio's pin as the last one, released from the completion handler above.

So __zone_stat_mod_folio() here needs interrupts disabled, not merely preemption, and __munlock_folio() has a path where they are not: when the folio has already been taken off the LRU by somebody else the function jumps straight to the counter update without taking the lruvec lock. The read-modify-write of the per-CPU NR_MLOCK diff can then be interrupted by the softirq above, and one of the two decrements is lost, leaving Mlocked in /proc/meminfo permanently overstated.

Use zone_stat_mod_folio(). mod_zone_state()'s this_cpu_try_cmpxchg() is atomic against a same-CPU interrupt and retries, and on the path where the lruvec lock is held its cost is negligible next to the lock itself.

The UNEVICTABLE_PG* events are deliberately left on the __ accessors:
they occupy different vm_event_states slots from the UNEVICTABLE_PGCLEARED that __free_pages_prepare() bumps, and nothing updates those two from interrupt context.

Fixes 2fbb0c10d1e8 ("mm/munlock: mlock_page() munlock_page() batch by pagevec")

Cc <[email protected]>

Signed-off-by Shakeel Butt <[email protected]>

---
 mm/mlock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/mlock.c +1 -1

index efa6716e4dfb..39215a3eab1f 100644
@@ -141,7 +141,7 @@ static struct lruvec *__munlock_folio(struct folio *folio, struct lruvec *lruvec
 
 munlock:
 	if (folio_test_clear_mlocked(folio)) {
-		__zone_stat_mod_folio(folio, NR_MLOCK, -nr_pages);
+		zone_stat_mod_folio(folio, NR_MLOCK, -nr_pages);
 		if (isolated || !folio_test_unevictable(folio))
 			__count_vm_events(UNEVICTABLE_PGMUNLOCKED, nr_pages);
 		else

Signature · 1 line

2.53.0-Meta

Quoted 12 lines · Shakeel Butt

NR_MLOCK is updated from interrupt context. __free_pages_prepare() clears a stray PG_mlocked and adjusts NR_MLOCK, and a folio can reach it with the flag still set from a bio completion handler:

  __free_pages_ok+0x6af/0x7a0
  <IRQ>
  __bio_release_pages+0xde/0x260
  __iomap_dio_bio_end_io+0x16e/0x1a0
  blk_update_request+0x14b/0x3d0
  blk_mq_end_request+0x18/0x30
  blk_done_softirq+0x49/0x60

Quoted 12 lines

The folio gets there like this. A MAP_SHARED file mapping is mlocked, so its page cache folios carry PG_mlocked, and an O_DIRECT write sourced from that mapping GUP-pins those same folios. munlock() then runs mlock_vma_pages_range(), which clears VM_LOCKED before walking the page tables to munlock each folio. A concurrent hole punch reaches the folio through the rmap (i_mmap_rwsem, not mmap_lock) and can land inside that
window: __folio_remove_rmap() -> munlock_vma_folio() sees VM_LOCKED already clear, so it neither queues the folio on the mlock batch nor takes a reference, and the pte it clears makes the pending mlock_pte_range() walk skip the folio at its !pte_present() check. filemap_remove_folio() then drops the page cache reference, leaving the bio's pin as the last one, released from the completion handler above.

I'm hardly ashamed to admit that I've not tried to digest that paragraph. You're writing about the rare fallback cases when PG_mlocked is cleared late, and unevictable_pgs_cleared incremented to notify us of that defect: yes, I accept that might happen at interrupt time, and so we ought not to take the __shortcut in __munlock_folio() which you fix below.

Quoted 13 lines

So __zone_stat_mod_folio() here needs interrupts disabled, not merely preemption, and __munlock_folio() has a path where they are not: when the folio has already been taken off the LRU by somebody else the function jumps straight to the counter update without taking the lruvec lock. The read-modify-write of the per-CPU NR_MLOCK diff can then be interrupted by the softirq above, and one of the two decrements is lost, leaving Mlocked in /proc/meminfo permanently overstated.

Use zone_stat_mod_folio(). mod_zone_state()'s this_cpu_try_cmpxchg() is atomic against a same-CPU interrupt and retries, and on the path where the lruvec lock is held its cost is negligible next to the lock itself.

Quoted 7 lines

The UNEVICTABLE_PG* events are deliberately left on the __ accessors:
they occupy different vm_event_states slots from the UNEVICTABLE_PGCLEARED that __free_pages_prepare() bumps, and nothing updates those two from interrupt context.

Fixes 2fbb0c10d1e8 ("mm/munlock: mlock_page() munlock_page() batch by pagevec")

Cc <[email protected]>

Okay: just a wrong stat, but it ought to go back to 0, so Cc stable yes.

Quoted 1 line

Signed-off-by Shakeel Butt <[email protected]>

Acked-by Hugh Dickins <[email protected]>

But I do think you (or Andrew :-) should include

Reported-by [email protected]

Closes https://lore.kernel.org/linux-mm/[email protected]/ open here

That was indeed reporting a different way to get a WARNING from this, when offlining a CPU: but it should be acknowledged for bringing you here, and we should tell syzbot it's fixed by this.

I've been trying to work out whether you're going to come back in a day or two, changing the __count_vm_events() too: and had raised in that thread the question of why lru_add_drain()'s __count_vm_events were not also reported by syzbot; but now I can see
* vm counters are allowed to be racy. Use raw_cpu_ops to avoid the
* local_irq_disable overhead.
and realize that they're not a problem; so this looks complete, we shouldn't need local_lock()ing in mlock_drain_remote() after all.