Fix a swap block allocation race.
ClosedPublic
Actions

Authored by markj on Feb 13 2020, 5:24 PM.

Details

Reviewers

alc
dougm
kib
jeff

Commits

rS358025: Fix a swap block allocation race.

Summary

putpages' allocation of swap blocks is done under the global sw_dev
lock. Previously it would drop that lock before inserting the allocated
blocks into the object's trie, creating a window in which swap blocks
are allocated but are not visible to swapoff. This can cause
swp_pager_strategy() to fail and panic the system.

Fix the problem bluntly, by allocating swap blocks under the object
lock.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Feb 13 2020, 5:24 PM

Harbormaster completed remote builds in B29351: Diff 68257.Feb 13 2020, 5:24 PM

markj added reviewers: alc, dougm, kib, jeff.Feb 13 2020, 5:26 PM

Wake up sleepers when incrementing nsw_wcount_async.

Harbormaster completed remote builds in B29356: Diff 68263.Feb 13 2020, 6:28 PM

jeff accepted this revision.Feb 13 2020, 11:03 PM

jeff added inline comments.

sys/vm/swap_pager.c
1457 ↗	(On Diff #68263)	We should generalize my lockless limit/counting semaphore in uma.
1482–1483 ↗	(On Diff #68263)	Unrelated but it's unfortunate that this is required. How many places actually inspect that we can't pass NULL?
1502 ↗	(On Diff #68263)	Wouldn't need the double loop if we allocated the buf first. Why did it need to move?

This revision is now accepted and ready to land.Feb 13 2020, 11:03 PM

markj added inline comments.Feb 14 2020, 3:35 PM

sys/vm/swap_pager.c
1482–1483 ↗	(On Diff #68263)	I'm actually not sure that this is really required, though at least nfs_strategy() assumes that the creds are not equal to NOCRED (NFSNEWCRED will trigger a page fault), and yes it is in principle possible to swap over NFS. The buffer cache seems to handle null credentials. I don't see why we bother setting b_rcred though.
1502 ↗	(On Diff #68263)	It doesn't really need to move. It is just more cleanup to do if the swap block allocation fails and I liked having all of the initialization happen in one block. Maybe I will change it to avoid the extra loop.