Page MenuHomeFreeBSD

Tests for copyin(9) handling of kernel addresses
ClosedPublic

Authored by kib on Oct 17 2015, 10:41 AM.

Details

Summary

I wrote morally equivalent tests when I developed the arm patch r289372. The same issue is present on arm64.

Diff Detail

Repository
rS 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

kib updated this revision to Diff 9463.Oct 17 2015, 10:41 AM
kib retitled this revision from to Tests for copyin(9) handling of kernel addresses.
kib updated this object.
kib edited the test plan for this revision. (Show Details)
kib added a reviewer: emaste.
kib set the repository for this revision to rS FreeBSD src repository.
kib added a project: tests.
emaste edited edge metadata.Oct 19 2015, 4:01 PM

Is it worth checking VM_MAXUSER_ADDRESS + 1, 0?

emaste accepted this revision.Oct 19 2015, 4:04 PM
emaste edited edge metadata.
emaste added subscribers: ngie, jmmv.
This revision is now accepted and ready to land.Oct 19 2015, 4:06 PM
kib added a comment.Oct 19 2015, 4:21 PM
In D3925#81835, @emaste wrote:

Is it worth checking VM_MAXUSER_ADDRESS + 1, 0?

(VM_MAXUSER_ADDRESS, 0) is only considered valid because the short-circuit for length == 0 is taken before the interval validation is performed. So, knowing the implementations, I would say that it should be excessive. Still, since the question was raised, I will add the test as well.

kib updated this revision to Diff 9506.Oct 19 2015, 4:29 PM
kib edited edge metadata.

Add VM_MAXUSER + 1, 0 test.

This revision now requires review to proceed.Oct 19 2015, 4:29 PM
emaste accepted this revision.Oct 19 2015, 4:33 PM
emaste edited edge metadata.
This revision is now accepted and ready to land.Oct 19 2015, 4:33 PM
ngie added inline comments.Oct 19 2015, 6:11 PM
tests/sys/kern/kern_copyin.c
60–64 ↗(On Diff #9506)

Creating a temporary file in /tmp violates ATF/kyua's sandbox, even though you call unlink after the fact. Please use copyin.XXXXXX instead

66–78 ↗(On Diff #9506)

ATF_CHECK(copyin_checker... == 0); is the same thing. I don't know if jmmv had a preference but IIRC the more complex macros were going to be removed, maybe, someday (yes, I am being wishy-washy on purpose here).

ngie requested changes to this revision.Oct 19 2015, 6:11 PM
ngie added a reviewer: ngie.
This revision now requires changes to proceed.Oct 19 2015, 6:11 PM
kib updated this revision to Diff 9508.Oct 19 2015, 6:35 PM
kib edited edge metadata.

Do not create temp file in /tmp. Use ATF_CHECK instead of ATF_CHECK_EQ.

ngie accepted this revision.EditedOct 19 2015, 6:40 PM
ngie edited edge metadata.

Sidenote: the only thing that's slightly painful with ATF that other test infrastructures seem to do better is that ATF_CHECK/ATF_REQUIRE don't print out the result of the expression, so if you need to dump out the result of the check, use ATF_CHECK_MSG/ATF_REQUIRE_MSG instead, like:

int rc;

ATF_CHECK_MSG((rc = foo()) == exp_result, "foo() failed. Expected: %d; got: %d");

LGTM!

This revision is now accepted and ready to land.Oct 19 2015, 6:40 PM
emaste accepted this revision.Oct 19 2015, 6:41 PM
emaste edited edge metadata.
kib added a comment.Oct 19 2015, 6:54 PM
In D3925#81898, @ngie wrote:
int rc;
ATF_CHECK_MSG((rc = foo()) == exp_result, "foo() failed. Expected: %d; got: %d");

This is extremely ugly. For my tests, the function is the same, but the arguments are what produce the test case. In essense, I have to either retype the same thing twice, or create a wrapper that would call ATF_CHECK_MSG() only in case of failure.

I decided to keep this as is, since the most likely outcome of the failing test is the kernel panic, anyway.

ngie added a comment.Oct 19 2015, 6:58 PM
In D3925#81904, @kib wrote:
In D3925#81898, @ngie wrote:
int rc;
ATF_CHECK_MSG((rc = foo()) == exp_result, "foo() failed. Expected: %d; got: %d");

This is extremely ugly. For my tests, the function is the same, but the arguments are what produce the test case. In essense, I have to either retype the same thing twice, or create a wrapper that would call ATF_CHECK_MSG() only in case of failure.

Yes, I wish it was more user friendly...

I decided to keep this as is, since the most likely outcome of the failing test is the kernel panic, anyway.

0-o...

This revision was automatically updated to reflect the committed changes.