Page MenuHomeFreeBSD
Differential D15898

Add tests for miscellaneous administrative system calls
ClosedPublic
Actions

Authored by aniketp on Jun 19 2018, 6:07 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 28, 8:56 PM
Unknown Object (File)
Thu, Nov 28, 8:56 PM
Unknown Object (File)
Thu, Nov 28, 8:55 PM
Unknown Object (File)
Thu, Nov 28, 8:54 PM
Unknown Object (File)
Thu, Nov 28, 8:54 PM
Unknown Object (File)
Thu, Nov 28, 8:54 PM
Unknown Object (File)
Thu, Nov 28, 8:53 PM
Unknown Object (File)
Thu, Nov 28, 8:53 PM
View All Files
Subscribers
imp

Details

Reviewers
asomers
rwatson
gnn
Commits
rS339092: MFC r335792, r336564, r336579
rS335792: audit(4): add tests for several more administrative syscalls
Summary

This revision introduces atf-c(3) tests for many unrelated administrative system calls.
Note: Most the syscalls are too critical to be tested in success mode.

List of Syscalls:

  • ntp_adjtime(2)
  • auditctl(2)
  • acct(2)
  • auditon(2)
  • clock_settime(2)

// Only in failure mode

  • reboot(2)
  • quotactl(2)
  • mount(2)
  • nmount(2)
  • swapon(2)
  • swapoff(2)
Test Plan

Execute make && make install from test/sys/audit.
Execute kyua test from /usr/tests/sys/audit. All testcases should succeed.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

aniketp created this revision.Jun 19 2018, 6:07 AM
Harbormaster completed remote builds in B17437: Diff 44061.Jun 19 2018, 6:07 AM
asomers requested changes to this revision.Jun 19 2018, 4:42 PM
asomers added inline comments.
tests/sys/audit/administrative.c
171 ↗(On Diff #44061)

timebuff is used uninitialized.

267 ↗(On Diff #44061)

What happens to the system's audit trail after this command? Does it keep recording the stuff in /var/audit?

656 ↗(On Diff #44061)

Can't you test quotactl successfully using the non-destructive Q_GETQUOTASIZE, Q_GETQUOTA, or Q_SYNC commands?

685 ↗(On Diff #44061)

Actually, you can mount a filesystem. nullfs would work, for example. But be sure to unmount it during the cleanup!

This revision now requires changes to proceed.Jun 19 2018, 4:42 PM
aniketp added inline comments.Jun 20 2018, 11:06 AM
tests/sys/audit/administrative.c
171 ↗(On Diff #44061)

Is it not supposed to be filled by ntp_adjtime(2). That is what man-page suggested.

267 ↗(On Diff #44061)

After I call auditctl(2) myself, auditing starts in my configured path rather than /var/audit.
But when the audit daemon is stopped and started. Process resumes in /var/audit

Actually:
When the auditd(8) is not already running, everything works fine after running the tests. But on the other side, auditing in /var/audit stops.

So basically, to make everything work normally, we'll have to stop and start auditd8) again. (I verified this behaviour using an independent program).

656 ↗(On Diff #44061)
int size;
if (quotactl("/", Q_GETQUOTASIZE, 0, &size) < 0)
        perror("quotactl");

outputs:
quotactl: Invalid argument

From the man-page, it looks like the error is because the filesystem quota is not already enabled on my system

MAN
The cmd argument or the command type is invalid.  In
Q_GETQUOTASIZE, Q_GETQUOTA, Q_SETQUOTA, and Q_SETUSE,
quotas are not currently enabled for this file system.
asomers added inline comments.Jun 20 2018, 3:20 PM
tests/sys/audit/administrative.c
171 ↗(On Diff #44061)

No, you're misreading it. That argument is provided from userland to the kernel.

267 ↗(On Diff #44061)

Tough call. Do we skip the test if auditd is already running? Or do we run it anyway, but make sure to restart auditd in the cleanup? I'm leaning toward the latter, even though it will cause a short disruption to the system-wide audit trail. My reasoning is that none of the ATF tests should be run in a production environment, so a brief audit-trail disruption should be ok.

628 ↗(On Diff #44061)

If we're going to temporarily disrupt the audit trail, then we may as well disrupt process accounting, too. You can check whether accounting is enabled with the kern.acct_configured sysctl. Then, during cleanup, either do "service accounting onestop" (if it wasn't enabled) or "service accounting restart" (if it was).

656 ↗(On Diff #44061)

Does that even happen for Q_SYNC?

aniketp updated this revision to Diff 44203.Jun 21 2018, 8:51 AM
aniketp marked 10 inline comments as done.
  • Add test for acct_success. Use sysctlbyname(3) to retrieve the accounting status
  • Minor changes in auditctl_success test case cleanup
Harbormaster completed remote builds in B17509: Diff 44203.Jun 21 2018, 8:51 AM
asomers requested changes to this revision.Jun 21 2018, 2:47 PM
asomers added inline comments.
tests/sys/audit/administrative.c
346 ↗(On Diff #44203)

Why go to so much effort not to enable accounting? It would be simpler to make acctpath a constant. It will be deleted anyway at the end of the test.

357 ↗(On Diff #44203)

You can't do cleanup in the test body, because it won't run if the test fails. You need to move this to the cleanup function.

This revision now requires changes to proceed.Jun 21 2018, 2:47 PM
aniketp updated this revision to Diff 44237.Jun 21 2018, 4:01 PM

Update the test case for acct_success

Harbormaster completed remote builds in B17515: Diff 44237.Jun 21 2018, 4:01 PM
asomers requested changes to this revision.Jun 21 2018, 7:21 PM
asomers added inline comments.
tests/sys/audit/administrative.c
335 ↗(On Diff #44237)

It's not useful to document that the file is conditional; the condition one line above makes that clear. If you're going to document it, it should be to highlight its role in the cleanup function.

356 ↗(On Diff #44237)

Pro tip: if you initialize filedesc2 to -1, then you can unconditionally close it and ignore the return value. However, in this case it would be better to move the close statement up to line 336, immediately after opening it.

This revision now requires changes to proceed.Jun 21 2018, 7:21 PM
aniketp updated this revision to Diff 44272.Jun 21 2018, 11:25 PM

Shift the location of close(filedesc2) to right after opening it

Harbormaster completed remote builds in B17527: Diff 44272.Jun 21 2018, 11:25 PM
asomers added inline comments.Jun 22 2018, 2:57 AM
tests/sys/audit/administrative.c
685 ↗(On Diff #44061)

Are you still working on mounting a filesystem?

aniketp added inline comments.Jun 22 2018, 4:27 AM
tests/sys/audit/administrative.c
685 ↗(On Diff #44061)

I've been trying out numerous variations of commands to see if they execute successfully for mount(2) and nmount(2).

Whenever I get the "type" right, i.e either "nullfs" or "tmpfs", Any other set of command outputs Operation not supported.
For the wrong types, I expectedly get invalid argument

aniketp edited the summary of this revision. (Show Details)Jun 22 2018, 4:27 AM
aniketp updated this revision to Diff 44525.Jun 27 2018, 1:59 PM
  • Add tests for auditon(2), swapon(2) and swapoff(2)

Note: swapon(2) and swapoff(2) require block device, hence can't be tested
in success case since block devices are gone (from FreeBSD)

Harbormaster completed remote builds in B17664: Diff 44525.Jun 27 2018, 1:59 PM
aniketp edited the summary of this revision. (Show Details)Jun 27 2018, 2:00 PM
aniketp set the repository for this revision to rS FreeBSD src repository - subversion.
Herald added a subscriber: imp. · View Herald TranscriptJun 27 2018, 2:00 PM
aniketp updated this revision to Diff 44527.Jun 27 2018, 2:53 PM
  • Add tests for clock_settime(2)
Harbormaster completed remote builds in B17665: Diff 44527.Jun 27 2018, 2:53 PM
aniketp edited the summary of this revision. (Show Details)Jun 27 2018, 3:04 PM
asomers requested changes to this revision.Jun 27 2018, 3:39 PM

If you intend to add successful test cases for mount, swapon, etc later, then you shouldn't indicate them as TODOs in the comments. Remove the text about how mount can't be tested in success mode.

This revision now requires changes to proceed.Jun 27 2018, 3:39 PM
aniketp updated this revision to Diff 44531.Jun 27 2018, 4:19 PM
  • Remove placeholder comment about not testing mount(2), nmount(2) in success case

These will be dealt later on

Harbormaster completed remote builds in B17667: Diff 44531.Jun 27 2018, 4:19 PM
asomers requested changes to this revision.Jun 27 2018, 4:44 PM
asomers added inline comments.
tests/sys/audit/administrative.c
385 ↗(On Diff #44531)

evclass is used uninitialized here.

This revision now requires changes to proceed.Jun 27 2018, 4:44 PM
aniketp updated this revision to Diff 44541.Jun 27 2018, 5:11 PM

Initialize evclass to get the event-class mapping for auditon(2)

Harbormaster completed remote builds in B17673: Diff 44541.Jun 27 2018, 5:11 PM
asomers requested changes to this revision.Jun 27 2018, 5:20 PM
asomers added inline comments.
tests/sys/audit/administrative.c
386 ↗(On Diff #44541)

You should initialize ec_class too, even though current kernel code doesn't use it. Otherwise, tools like Coverity will detect this as a use of uninitialized data. Anything will work. Either bzero, or an explicit value.

This revision now requires changes to proceed.Jun 27 2018, 5:20 PM
aniketp updated this revision to Diff 44545.Jun 27 2018, 5:28 PM
  • Initialize ec_class to 0
Harbormaster completed remote builds in B17675: Diff 44545.Jun 27 2018, 5:28 PM
aniketp marked 6 inline comments as done.Jun 27 2018, 5:30 PM
asomers accepted this revision.Jun 29 2018, 4:40 AM
This revision is now accepted and ready to land.Jun 29 2018, 4:40 AM
Closed by commit rS335792: audit(4): add tests for several more administrative syscalls (authored by asomers). · Explain WhyJun 29 2018, 4:52 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
head/
tests/
sys/
audit/
468 lines

Diff 44620

View Options

head/tests/sys/audit/administrative.c

Loading...