hyperv/hn: Fix ifnet hwassist setup.
ClosedPublic
Actions

Authored by sepherosa_gmail.com on Sep 19 2016, 6:08 AM.

Details

Reviewers

howard0su_gmail.com
honzhan_microsoft.com
decui_microsoft.com
delphij
royger

Commits

rS306013: hyperv/hn: Fix ifnet hwassist setup.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

sepherosa_gmail.com updated this revision to Diff 20474.Sep 19 2016, 6:08 AM

sepherosa_gmail.com retitled this revision from to hyperv/hn: Fix ifnet hwassist setup..

sepherosa_gmail.com updated this object.

sepherosa_gmail.com edited the test plan for this revision. (Show Details)

sepherosa_gmail.com added reviewers: delphij, royger, decui_microsoft.com, honzhan_microsoft.com, howard0su_gmail.com.

Closed by commit rS306013: hyperv/hn: Fix ifnet hwassist setup. (authored by sephe). · Explain WhySep 20 2016, 5:26 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

sys/

dev/

hyperv/

netvsc/

hv_netvsc_drv_freebsd.c

161 lines

Diff 20474

View Options

sys/dev/hyperv/netvsc/hv_netvsc_drv_freebsd.c

This file was added.

				/*-
				* Copyright 2018 Aniket Pandey
				* All rights reserved.
				*
				* Redistribution and use in source and binary forms, with or without
				* modification, are permitted provided that the following conditions
				* are met:
				* 1. Redistributions of source code must retain the above copyright
				* notice, this list of conditions and the following disclaimer.
				* 2. Redistributions in binary form must reproduce the above copyright
				* notice, this list of conditions and the following disclaimer in the
				* documentation and/or other materials provided with the distribution.
				*
				* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
				* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
				* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
				* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
				* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
				* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
				* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
				* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
				* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
				* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
				* SUCH DAMAGE.
				*
				* $FreeBSD$
				*/

				#include <sys/ioctl.h>

				#include <time.h>
				#include <errno.h>
				#include <fcntl.h>
				#include <atf-c.h>
				#include <string.h>
				#include <stdlib.h>
				#include <unistd.h>
				#include <bsm/libbsm.h>
				#include <security/audit/audit_ioctl.h>

				#include "utils.h"

				/*
				* Checks the presence of "auditregex" in auditpipe(4) after the
				* corresponding system call has been triggered.
				*/
				static bool
				get_records(const char auditregex, FILE pipestream)
				{
				uint8_t *buff;
				tokenstr_t token;
				ssize_t size = 1024;
				char membuff[size];
				char del[] = ",";
				int reclen, bytes = 0;
				FILE *memstream;

				/*
				* Open a stream on 'membuff' (address to memory buffer) for storing
				* the audit records in the default mode.'reclen' is the length of the
				* available records from auditpipe which is passed to the functions
				* au_fetch_tok(3) and au_print_flags_tok(3) for further use.
				*/
				memstream = fmemopen(membuff, size, "w");
				reclen = au_read_rec(pipestream, &buff);

				asomersUnsubmitted Done Inline Actions You should check for failure of both fmemopen and au_read_rec. asomers: You should check for failure of both fmemopen and au_read_rec.
				/*
				* Iterate through each BSM token, extracting the bits that are
				* required to start processing the token sequences.
				*/
				while (bytes < reclen) {
				if (au_fetch_tok(&token, buff + bytes, reclen - bytes) == -1) {
				atf_tc_fail("Incomplete audit record");
				asomersUnsubmitted Done Inline Actions Misindentation asomers: Misindentation
				asomersUnsubmitted Done Inline Actions This change looks wrong to me. Can you explain it? asomers: This change looks wrong to me. Can you explain it?
				aniketp_iitk.ac.inAuthorUnsubmitted Done Inline Actions In a case when `reclen - bytes < token.len` we would not have the entire audit token which looked as if it caused the "incomplete audit record" issue. Also, why subtract `bytes` from `reclen` if there is a chance we might miss some records? Letting it be `reclen` seems to ensure that complete tokens are read. aniketp_iitk.ac.in: In a case when `reclen - bytes < token.len` we would not have the entire audit token which…
				asomersUnsubmitted Done Inline Actions `au_fetch_tok`'s man page says that reclen is supposed to be the length of the supplied buffer. When `bytes > 0`, the buffer you're actually passing will be smaller than `reclen`. asomers: `au_fetch_tok`'s man page says that reclen is supposed to be the length of the supplied buffer.
				aniketp_iitk.ac.inAuthorUnsubmitted Done Inline Actions I switched back to `reclen - bytes` from `reclen` and the intermittent issue seems to recur after a while. My take is, subtracting `bytes` from the original `reclen` causes the `len` parameter to eventually become less than `token.len` in some test-cases, which causes "incomplete audit record" issue Also, since I'm comparing against a regex which includes `return,success/failure` which is essentially at the end of the audit record, this might explain why some time the tests timeout. (As this part of the regex would be absent from buffer) aniketp_iitk.ac.in: I switched back to `reclen - bytes` from `reclen` and the intermittent issue seems to recur…
				asomersUnsubmitted Done Inline Actions No, the token must be completely contained within the buffer (where else would it be?). And the third argument to `au_fetch_tok` must be the length of the buffer. It's never ok to overrun a buffer. asomers: No, the token must be completely contained within the buffer (where else would it be?). And…
				};
				asomersUnsubmitted Done Inline Actions You should also print errno here. You can do it like this: perror("au_read_rec"); atf_tc_fail("Incomplete audit record") asomers: You should also print errno here. You can do it like this: ``` perror("au_read_rec")…
				/* Print the tokens as they are obtained, in the default form */
				au_print_flags_tok(memstream, &token, (char *)del, AU_OFLAG_NONE);
				bytes += token.len;
				}

				asomersUnsubmitted Done Inline Actions 80 characters per line asomers: 80 characters per line
				free(buff);
				fclose(memstream);
				return (atf_utils_grep_string("%s", membuff, auditregex));
				}

				/*
				* Override the system-wide audit mask settings in /etc/security/audit_control
				*/
				static void
				set_preselect_mode(int filedesc, au_mask_t *fmask)
				{
				int fmode = AUDITPIPE_PRESELECT_MODE_LOCAL;

				/* Set local preselection mode for auditing */
				if (ioctl(filedesc, AUDITPIPE_SET_PRESELECT_MODE, &fmode) < 0) {
				atf_tc_fail("Preselection mode: %s", strerror(errno));
				}

				/* Set local preselection flag corresponding to the audit_event */
				if (ioctl(filedesc, AUDITPIPE_SET_PRESELECT_FLAGS, fmask) < 0) {
				asomersUnsubmitted Done Inline Actions Standard style(9) says that single-line `if` statements don't get curly braces. There are exceptions where the braces are needed for clariy, like if the `else` statement is multiline, or if there's a long comment above the single line. But those exceptions mostly don't apply in this file. asomers: Standard style(9) says that single-line `if` statements don't get curly braces. There are…
				atf_tc_fail("Preselection flag: %s", strerror(errno));
				}

				/* Set local preselection flag for non-attributable audit_events */
				if (ioctl(filedesc, AUDITPIPE_SET_PRESELECT_NAFLAGS, fmask) < 0) {
				asomersUnsubmitted Done Inline Actions This change seems to have fixed the intermittency problems. asomers: This change seems to have fixed the intermittency problems.
				atf_tc_fail("Preselection flag: %s", strerror(errno));
				}

				/* This removes any outstanding record on the auditpipe */
				if (ioctl(filedesc, AUDITPIPE_FLUSH) < 0) {
				atf_tc_fail("Auditpipe flush: %s", strerror(errno));
				}
				}
				asomersUnsubmitted Done Inline Actions Why did you add all the curly braces back in? asomers: Why did you add all the curly braces back in?
				aniketp_iitk.ac.inAuthorUnsubmitted Done Inline Actions Oh, apologies. I somehow edited different file than the one present in `tests/sys/audit` aniketp_iitk.ac.in: Oh, apologies. I somehow edited different file than the one present in `tests/sys/audit`

				/*
				* Check if the auditd(8) startup was properly received at the auditpipe(4)
				*/
				static void
				check_audit_startup(struct pollfd fd[], FILE *pipestream)
				{
				int timeout = 2000;
				const char *auditstring = "audit startup";

				if (poll(fd, 1, timeout) < 0) {
				atf_tc_fail("Poll: %s", strerror(errno));
				} else {
				asomersUnsubmitted Done Inline Actions misindentation asomers: misindentation
				asomersUnsubmitted Done Inline Actions This line is failing because "audit startup" is not necessarily the first event to come out of the audit pipe. You should loop here, just as you do in `check_audit` asomers: This line is failing because "audit startup" is not necessarily the first event to come out of…
				if (fd[0].revents & POLLIN) {
				asomersUnsubmitted Done Inline Actions Probably don't need the `am_failure` line. asomers: Probably don't need the `am_failure` line.
				/* We now have a proof that auditd started smoothly */
				ATF_REQUIRE(get_records(auditstring, pipestream));
				} else {
				/* revents is not POLLIN */
				asomersUnsubmitted Done Inline Actions This line has spaces instead of tabs. There are a few others that do, too. You must use tabs throughout. asomers: This line has spaces instead of tabs. There are a few others that do, too. You must use tabs…
				atf_tc_fail("Auditpipe returned an unknown event %#x",
				fd[0].revents);
				}
				}
				}

				/*
				asomersUnsubmitted Done Inline Actions You should choose a more descriptive name for the `flag` variable. asomers: You should choose a more descriptive name for the `flag` variable.
				* Get the corresponding audit_mask for class-name "name" then set the
				* success and failure bits for fmask to be used as the ioctl argument
				*/
				static au_mask_t
				get_audit_mask(const char *name)
				{
				au_mask_t fmask;
				au_class_ent_t *class;

				ATF_REQUIRE((class = getauclassnam(name)) != NULL);
				asomersUnsubmitted Done Inline Actions style(9) actually says that `for(;;)` is preferred over `while (true)` asomers: style(9) actually says that `for(;;)` is preferred over `while (true)`
				fmask.am_success = class->ac_class;
				fmask.am_failure = class->ac_class;
				return (fmask);
				}

				/*
				* Loop until the auditpipe returns something, check if it is what
				* we want, else repeat the procedure until ppoll(2) times out.
				*/
				void
				asomersUnsubmitted Done Inline Actions A more idiomatic way to do this would be to use an endless loop and a `break` statement. asomers: A more idiomatic way to do this would be to use an endless loop and a `break` statement.
				check_audit(struct pollfd fd[], const char auditrgx, FILE pipestream)
				{
				struct timespec currtime, endtime, timeout;
				asomersUnsubmitted Done Inline Actions This error message isn't quite right. Auditpipe may have returned something, just not what we were looking for. Also, the error message does not indicate whether we we missed the audit record for the syscall under test, or whether we failed to observe auditd startup. Could you please make that more clear? It looks to me like most if not all remaining test failures are due to missing auditd startup. asomers: This error message isn't quite right. Auditpipe may have returned something, just not what we…

				/* Set the expire time for poll(2) while waiting for syscall audit */
				ATF_REQUIRE_EQ(0, clock_gettime(CLOCK_MONOTONIC, &endtime));
				endtime.tv_sec += 5;
				timeout.tv_nsec = endtime.tv_nsec;

				asomersUnsubmitted Done Inline Actions spaces -> tabs asomers: spaces -> tabs
				while (true) {
				/* Update the time left for auditpipe to return any event */
				ATF_REQUIRE_EQ(0, clock_gettime(CLOCK_MONOTONIC, &currtime));
				timeout.tv_sec = endtime.tv_sec - currtime.tv_sec;

				switch (ppoll(fd, 1, &timeout, NULL)) {
				/* ppoll(2) returns, check if it's what we want */
				case 1:
				if (fd[0].revents & POLLIN) {
				asomersUnsubmitted Done Inline Actions Misindentation asomers: Misindentation
				if (get_records(auditrgx, pipestream)) {
				/* Syscall's audit is confirmed */
				atf_tc_pass();
				}
				} else {
				asomersUnsubmitted Done Inline Actions spaces -> tabs asomers: spaces -> tabs
				atf_tc_fail("Auditpipe returned an "
				"unknown event %#x", fd[0].revents);
				asomersUnsubmitted Done Inline Actions s/an unknown event/too many file descriptors/ asomers: s/an unknown event/too many file descriptors/
				}
				break;

				/* poll(2) timed out */
				case 0:
				atf_tc_fail("Auditpipe did not return anything "
				"within the time limit");
				break;

				asomersUnsubmitted Done Inline Actions 80 characters per line asomers: 80 characters per line
				/* poll(2) standard error */
				case -1:
				atf_tc_fail("Poll: %s", strerror(errno));
				break;

				default:
				atf_tc_fail("Poll returned an unknown event");
				}
				asomersUnsubmitted Done Inline Actions It would probably make sense just to move the cleanup steps into `check_audit`, and dispense with the flag for `check_auditpipe` asomers: It would probably make sense just to move the cleanup steps into `check_audit`, and dispense…
				}

				/* Cleanup */
				fclose(pipestream);
				close(fd[0].fd);
				}

				FILE
				setup(struct pollfd fd[], const char name)
				{
				au_mask_t fmask;
				fmask = get_audit_mask(name);
				FILE *pipestream;

				fd[0].fd = open("/dev/auditpipe", O_RDONLY);
				fd[0].events = POLLIN;
				asomersUnsubmitted Done Inline Actions `set_preselect_mode` must be called before starting auditd. Otherwise there's a race. asomers: `set_preselect_mode` must be called before starting auditd. Otherwise there's a race.
				pipestream = fdopen(fd[0].fd, "r");

				asomersUnsubmitted Done Inline Actions Don't dereference an uninitialized variable. asomers: Don't dereference an uninitialized variable.
				ATF_REQUIRE_EQ(0, system("service auditd onestatus \|\| \
				{ service auditd onestart && touch started_auditd ; }"));

				/* If 'started_auditd' exists, that means we started auditd */
				asomersUnsubmitted Done Inline Actions This will probably work in the default case. However, if somebody has enabled too many classes in /etc/security/audit_control, then the audit queue might overflow before getting the audit startup message. To prevent that, you can set the audit class before starting auditd. asomers: This will probably work in the default case. However, if somebody has enabled too many classes…
				if (atf_utils_file_exists("started_auditd")) {
				check_audit_startup(fd, pipestream);
				}
				set_preselect_mode(fd[0].fd, &fmask);
				return (pipestream);
				}

				void
				cleanup(void)
				{
				system("[ -f started_auditd ] && service auditd onestop > \
				/dev/null 2>&1");
				asomersUnsubmitted Done Inline Actions This line would be a little bit faster if you move the check from sh to C by using atf_utils_file_exists. asomers: This line would be a little bit faster if you move the check from sh to C by using…
				}

hyperv/hn: Fix ifnet hwassist setup.ClosedPublicActions