Page MenuHomeFreeBSD

clarify reserv_test_config
AbandonedPublic

Authored by dougm on Jun 12 2019, 6:52 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 16, 1:23 AM
Unknown Object (File)
Feb 26 2024, 12:18 PM
Unknown Object (File)
Feb 11 2024, 6:32 AM
Unknown Object (File)
Dec 20 2023, 1:56 AM
Unknown Object (File)
Nov 18 2023, 2:09 PM
Unknown Object (File)
Nov 18 2023, 1:06 PM
Unknown Object (File)
Nov 18 2023, 12:30 PM
Unknown Object (File)
Nov 18 2023, 12:23 PM
Subscribers
None

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
Tests Skipped

Event Timeline

This revision is now accepted and ready to land.Jun 12 2019, 7:12 PM
vm_reserv.c
1238

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.

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
This revision is now accepted and ready to land.Jun 13 2019, 3:43 PM
vm_reserv.c
1174

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

1195–1196

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

1224–1225

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

1232–1233

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

1266

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.

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
vm_reserv.c
1256–1257

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.)

Update comments in accordance with suggestions from alc.

This revision is now accepted and ready to land.Jun 17 2019, 4:03 PM
dougm removed a subscriber: alc.
This revision now requires review to proceed.Jun 27 2019, 7:25 PM
vm_reserv.c
1181

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.

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

vm_reserv.c
1181

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

The code this modifies has been substantially changed now, and the benefits of this change no longer apply.