Page MenuHomeFreeBSD

Bug 238960: panic in vm_pageout_collect_batch() with QUEUE_MACRO_DEBUG_TRASH enabled
ClosedPublic

Authored by dgmorris_earthlink.net on Jul 3 2019, 4:08 PM.

Details

Summary

Investigation of a panic upon entering light memory pressure (causing inactive page scan) in an environment where QUEUE_MACRO_DEBUG_TRASH is enabled by default exposed an issue in vm_pageout_collect_batch().
The while loop iterates over the pages in the pagequeue provided, but when dequeue is enabled [as for inactive] the next pointer is technically invalid. This is hidden without the DEBUG option lit as the stale pointers are kept and the pagequeue lock should keep the next element on the list, but with the DEBUG option setting the pointers to -1, a panic ensues.

I would also argue that programatically, relying on the now-stale next pointer is effectively relying on the TAILQ implementation details (it could move to an underlying data structure which could pack elements or other methods where the original pointer could be freed) and should be avoided.

Test Plan
  1. Instrumented inactive batch walk to expose that we were attempting NEXT of -1 prior and that the page walk proceeded with the change (obviously QUEUE_MACRO_DEBUG_TRASH is on by default).
  1. Booted and looped memory pressure-inducing internal test application for 18+ continuous hours of operation. Without fix, panic happened on first invocation of the internal tests.

I believe this is sufficient given the defect is algorithmically evident.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj added inline comments.
sys/vm/vm_pageout.c
269 ↗(On Diff #59346)

n should come after marker.

This revision is now accepted and ready to land.Jul 3 2019, 4:10 PM
cem accepted this revision.EditedJul 3 2019, 4:23 PM

LGTM. For future reference, please upload future patches with more context. That could mean using the arc command line tool, which simplifies the process somewhat once it is set up, or diff -U99999 if you prefer not to use the arc ("Arcanist") tool.

Moved new local variable n to appropriate spot in multiple declarations.

This revision now requires review to proceed.Jul 3 2019, 4:23 PM
This revision is now accepted and ready to land.Jul 3 2019, 4:28 PM