Page MenuHomeFreeBSD

Fix alignment issues in vm_reserv_reclaim_contig
ClosedPublic

Authored by dougm on Dec 8 2021, 9:49 PM.

Details

Summary

Fix misuse of alignment and boundary in vm_reserv_reclaim_contig.

Change the computations with alignment and boundary in vm_reserv_test_contig, since the author of the current computations no longer thinks that they are correct. Add assertions to test the effect of alignment and boundary in vm_reserv_test_contig. Add assertions to vm_reserv_test_contig to verify the conditions established by vm_reserv_reclaim_contig.

Fail vm_reserv_reclaim_contig immediately if size > boundary, since in that case, jumping the range start to the next multiple of boundary cannot satisfy the boundary condition; indeed, nothing can satisfy that condition.

Not for final check-in, add support for a sysctl to test vm_reserv_reclaim_contig, to see if the new assertions hold.
Currently:

  1. sysctl debug.vm_reserv_reclaim_contig=1

debug.vm_reserv_reclaim_contig: 0
cycles/break: 85
-> 0

Diff Detail

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

Event Timeline

dougm requested review of this revision.Dec 8 2021, 9:49 PM
dougm created this revision.

Don't bother checking a reservation if every address within it violates alignment.

Consequently, the page update to achieve alignment for a gap in a reservation can't jump to a page way way outside the reservation, but only as far as one past the last reservation.

Don't allow boundary to exceed reservation size, so that boundary arithmetic cannot be a problem.

Use roundup2() to hide the bit-twiddling in alignment and boundary adjustment. Work with the page number directly instead of going back to the address for calculation.

Make all the arithmetic in the helper function use ints, and let the caller handle conversion to int. Let the caller manage bounds calculation. Return the found page and let the caller check assertions to verify alignment.

sys/vm/vm_reserv.c
1247–1248

Don't use __FUNCTION__. It was a gcc extension. Use __func__. It is standard C.

1325–1326

Unless this is needed for correctness, which I don't think it is, I don't see the point of this check. It amounts to an optimization for the unexpected case.

1327–1328

This is incorrect. The units for boundary are bytes, not pages.

dougm marked 2 inline comments as done.

Shrink whitespace. Fix pages vs bytes error. Stop using FUNCTION.

sys/vm/vm_reserv.c
1325–1326

In the patch at present, when we find that the prospective range spans a multiple of boundary, we move the range start to the next multiple of boundary, and assume that the boundary constraint is satisfied. However, if the size is bigger than boundary, the boundary constraint cannot be satisfied and the code may return true anyway.

So I can ignore size>boundary and just be wrong in that case, or assert size<= boundary, or keep the code as is.

Rename a couple of parameters to vm_reserv_find_contig. Add assertions and comments to ensure/explain that arithmetic with bound and alignment cannot compute values beyond the number of pages in a reservation.

sys/vm/vm_reserv.c
1381

Remove + PAGE_MASK. You want to round down, not up, here. Otherwise, the allocated memory could go beyond high.

1389

Deindent this.

1392

Deindent this.

dougm marked 3 inline comments as done.

Accept 3 reviewer suggestions.

sys/vm/vm_reserv.c
1390

Drop the outermost ()'s.

dougm marked an inline comment as done.

Apply reviewer suggestion. Remove some upper bound checks on alignment and boundary. Add some zero and power-of-two kasserts on alignment and boundary.

Correct identifier name. Verify compilation.

sys/vm/vm_reserv.c
1253–1256

alignment and boundary are frequently zero.

dougm marked an inline comment as done.

Compute the shifted alignment and boundary values once, not with every find function call, and make sure they aren't zero-valued.

This revision is now accepted and ready to land.Dec 15 2021, 4:02 PM

In the commit message, I would somehow explain that the consequence of the bug was only that the wrong reservations might be broken, not that unaligned or inappropriately boundary crossing memory would be returned by contigmalloc(), et al.

sys/vm/vm_reserv.c
1283

What happens if ppn_align is 2^31, and, thus, a negative value since lo is a signed int? To be clear, we will probably never see such a value passed in our lifetimes, but a static analyzer might still flag this.

dougm marked an inline comment as done.

Drop debug code. Push ppn_align and ppn_bound into [1,VM_LEVEL_0_NPAGES] to avoid overflow problems.

This revision now requires review to proceed.Dec 15 2021, 7:00 PM
sys/vm/vm_reserv.c
1283

I have made a change to ensure that ppn_align and ppn_bound are in [1, VM_LEVEL_0_NPAGES].

markj added inline comments.
sys/vm/vm_reserv.c
1345

Extra whitespace after >>.

This revision is now accepted and ready to land.Dec 16 2021, 3:47 PM
dougm added inline comments.
sys/vm/vm_reserv.c
1346

alc (vocally):
use MIN, MAX macros here since sizeof(boundary) > sizeof(u_long) for some architectures.

Change stray tab to space.
Avoid ulmin, ulmax for vm_paddr_t boundary.

This revision now requires review to proceed.Dec 16 2021, 5:12 PM
This revision is now accepted and ready to land.Dec 16 2021, 6:07 PM