Page MenuHomeFreeBSD
Differential D4817

mlx5en: Allow RX and TX pause frames to be set through ifconfig
ClosedPublic
Actions

Authored by markb_mellanox.com on Jan 7 2016, 2:34 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 14, 6:20 AM
Unknown Object (File)
Mon, Apr 14, 3:35 AM
Unknown Object (File)
Mon, Apr 14, 2:42 AM
Unknown Object (File)
Mon, Apr 14, 2:35 AM
Unknown Object (File)
Mon, Apr 14, 2:19 AM
Unknown Object (File)
Tue, Mar 25, 9:02 AM
Unknown Object (File)
Fri, Mar 21, 3:24 AM
Unknown Object (File)
Mar 12 2025, 10:40 AM
View All Files
Subscribers
gallatin
imp

Details

Reviewers
erj
• hselasky
np
gnn
Commits
rS294917: MFC r294314:
rS294314: Allow RX and TX pause frames to be set through ifconfig.
Summary

Allow setting RX/TX pause frames via ifconfig

Submitted by: Hans Petter Selasky <hps@selasky.org>
Sponsored by: Mellanox Technologies

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

markb_mellanox.com updated this revision to Diff 11998.Jan 7 2016, 2:34 PM
markb_mellanox.com retitled this revision from to mlx5en: Allow RX and TX pause frames to be set through ifconfig.
markb_mellanox.com updated this object.
markb_mellanox.com edited the test plan for this revision. (Show Details)
markb_mellanox.com added reviewers: gnn, • hselasky, np, erj.
markb_mellanox.com set the repository for this revision to rS FreeBSD src repository - subversion.
Herald added a subscriber: imp. · View Herald TranscriptJan 7 2016, 2:34 PM
markb_mellanox.com added a subscriber: gallatin.Jan 7 2016, 2:35 PM
gallatin added a comment.Jan 7 2016, 3:51 PM

I may be misreading this, but it appears we are loosing the ability to set the initial pause state via a loader tunable. Would it be possible to preserve this feature by masking the IFM_ETHER_*PAUSE bits with the user's desired pause sate in the initial ifmedia_set() call?

• hselasky edited edge metadata.Jan 7 2016, 3:53 PM

Yes, this patch removes the sysctl way to set RX/TX pause frames. It is possible to have both sysctl and ifconfig.

markb_mellanox.com updated this revision to Diff 12311.Jan 14 2016, 1:48 PM
markb_mellanox.com updated this object.
markb_mellanox.com edited edge metadata.

Add tunnable option

markb_mellanox.com added a child revision: D4939: mlx5en: Fix declaration of variables.Jan 14 2016, 1:50 PM
gnn accepted this revision.Jan 14 2016, 8:48 PM
gnn edited edge metadata.

This is

This revision is now accepted and ready to land.Jan 14 2016, 8:48 PM
gnn accepted this revision.Jan 14 2016, 8:49 PM
Closed by commit rS294314: Allow RX and TX pause frames to be set through ifconfig. (authored by • hselasky). · Explain WhyJan 19 2016, 10:10 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
sys/
dev/
mlx5/
mlx5_en/
4 lines
15 lines
120 lines

Diff 12311

View Options

sys/dev/mlx5/mlx5_en/en.h

  • 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…
}
View Options

sys/dev/mlx5/mlx5_en/mlx5_en_ethtool.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…
}
View Options

sys/dev/mlx5/mlx5_en/mlx5_en_main.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…
}