Page MenuHomeFreeBSD

clarify reserv_test_config
Needs ReviewPublic

Authored by dougm on Jun 12 2019, 6:52 PM.

Details

Reviewers
jhb
markj
kib
alc
Summary

Add assertions to vm_reserv_test_config that are enforced by its caller. Change the type of variables 'hi' and 'lo', and change the a comparison to be in terms of pages, not bytes, to convince, perhaps, Coverity that nothing is amiss.

Test Plan

I would appreciate a report on how Coverity accepts this change.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

dougm created this revision.Jun 12 2019, 6:52 PM
kib accepted this revision.Jun 12 2019, 7:12 PM
This revision is now accepted and ready to land.Jun 12 2019, 7:12 PM
jhb added inline comments.Jun 12 2019, 7:51 PM
vm_reserv.c
1240

If 'lo' and 'hi' stayed as 'int', coverity would still complain due to 'npages' being u_long, but since lo and hi are vm_paddr_t, it won't complain. I do think that 'lo + npages <= hi' is actually more readable. You might consider leaving 'lo' and 'hi' as 'int' (or even 'u_int') and changing 'npages' to 'u_int' instead of u_long. It would seem that 'npages' is confined to the range (0, VM_LEVEL_0_NPAGES].

dougm marked an inline comment as done.Jun 12 2019, 8:44 PM
dougm updated this revision to Diff 58574.

Accept reviewer suggestions for changes to u_int for hi, lo and npages.

This revision now requires review to proceed.Jun 12 2019, 8:44 PM
markj accepted this revision.Jun 13 2019, 3:43 PM
This revision is now accepted and ready to land.Jun 13 2019, 3:43 PM
alc added inline comments.Jun 13 2019, 4:12 PM
vm_reserv.c
1174

This comment is no longer correct. The requirement is that the reservation is locked. See the below assertion.

1192–1193

I question the "+ PAGE_MASK" here. In other words, I suspect that "high" should be truncated, not rounded.

1226–1227

An absurdly large alignment could cause integer overflow here as well.

1234–1235

An absurdly large boundary could cause integer overflow here as well.

1268

Since this function's return type is still boolean_t, the spelling of false should have remained "FALSE".

dougm marked 5 inline comments as done.Jun 13 2019, 4:48 PM
dougm updated this revision to Diff 58591.

Change bool literals to boolean_t literals in some places. Change u_ints to u_longs, and add a check for overflow. Truncate in computing hi.

This revision now requires review to proceed.Jun 13 2019, 4:48 PM
alc added inline comments.Jun 13 2019, 11:29 PM
vm_reserv.c
1258–1259

Not here. Only in vm_reserv_test_contig(). This function acquires the reservation lock for vm_reserv_test_contig().

(And, to be clear, the original comment here "The free page queue lock must be held." is no longer correct and can be removed.)

dougm updated this revision to Diff 58605.Jun 14 2019, 1:40 AM

Update comments in accordance with suggestions from alc.

markj accepted this revision.Jun 17 2019, 4:03 PM
This revision is now accepted and ready to land.Jun 17 2019, 4:03 PM
jhb accepted this revision.Thu, Jun 27, 7:22 PM
dougm added a reviewer: alc.Thu, Jun 27, 7:25 PM
dougm removed a subscriber: alc.
This revision now requires review to proceed.Thu, Jun 27, 7:25 PM
alc added inline comments.Sat, Jul 13, 5:54 PM
vm_reserv.c
1180–1182

There exists at least one platform, i386 PAE, where vm_paddr_t is wider than u_long, so simply changing "hi" and "lo" to u_long won't address the hypothetical overflow problems. (In practice, there would never be a problem because i386 PAE won't use all the bits available in vm_paddr_t, but a static analysis tool, like Coverity, is not going to know that.)

1184

I would also assert that "npages < VM_LEVEL_0_NPAGES".

dougm marked 2 inline comments as done.Sat, Jul 13, 6:33 PM
dougm updated this revision to Diff 59729.

Add an assertion. Use vm_paddr_t variable to manage bounds checking at function start.

kib added inline comments.Sat, Jul 13, 7:21 PM
vm_reserv.c
1180–1182

I believe that 32bit powerpc also got a similar mode recently.