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

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

[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

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: <stable@vger.kernel.org>
Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 mm/mlock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/mlock.c b/mm/mlock.c
index efa6716e4dfb..39215a3eab1f 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -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
-- 
2.53.0-Meta

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

[Hugh Dickins]

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,
Hugh

> ---
>  mm/mlock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/mlock.c b/mm/mlock.c
> index efa6716e4dfb..39215a3eab1f 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -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
> -- 
> 2.53.0-Meta

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

[Shakeel Butt]

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.