Page MenuHomeFreeBSD

Tests for copyin(9) handling of kernel addresses
ClosedPublic

Authored by kib on Oct 17 2015, 10:41 AM.
Tags
Referenced Files
Unknown Object (File)
Feb 24 2024, 4:59 AM
Unknown Object (File)
Feb 22 2024, 11:51 PM
Unknown Object (File)
Jan 9 2024, 4:08 PM
Unknown Object (File)
Jan 3 2024, 9:07 PM
Unknown Object (File)
Dec 20 2023, 1:20 AM
Unknown Object (File)
Nov 13 2023, 10:44 AM
Unknown Object (File)
Oct 24 2023, 8:58 PM
Unknown Object (File)
Oct 10 2023, 9:56 AM
Subscribers

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 - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

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 - subversion.
kib added a project: tests.

Is it worth checking VM_MAXUSER_ADDRESS + 1, 0?

emaste edited edge metadata.
emaste added subscribers: ngie, jmmv.
This revision is now accepted and ready to land.Oct 19 2015, 4:06 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 edited edge metadata.

Add VM_MAXUSER + 1, 0 test.

This revision now requires review to proceed.Oct 19 2015, 4:29 PM
emaste edited edge metadata.
This revision is now accepted and ready to land.Oct 19 2015, 4:33 PM
tests/sys/kern/kern_copyin.c
61–65

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

67–79

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 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 edited edge metadata.
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.

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.