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

[Shakeel Butt <shakeel.butt@linux.dev>]

On Tue, Sep 01, 2026 at 06:27:06PM -0700, Hugh Dickins wrote:
> On Tue, 1 Sep 2026, Shakeel Butt wrote:
> 
> > 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.
> 
> 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.
> 
> > 
> > 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: <stable@vger.kernel.org>
> 
> Okay: just a wrong stat, but it ought to go back to 0, so Cc stable yes.
> 
> > Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
> 
> Acked-by: Hugh Dickins <hughd@google.com>
> 
> But I do think you (or Andrew :-) should include
> 
> Reported-by: syzbot+cd2073ee6d958a8d0fcd@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/linux-mm/6a931c5a.08e933ee.dbf97.0093.GAE@google.com/
> 
> 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.
> 

Thanks a lot Hugh for taking a look and Andrew has already added the syzbot
tags.