* [bpf-next v3 07/12] tools headers: Adopt compiletime_assert from kernel sources
From: Krzesimir Nowak @ 2019-07-08 16:31 UTC (permalink / raw)
To: linux-kernel
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, netdev, bpf, xdp-newbies,
Krzesimir Nowak
In-Reply-To: <20190708163121.18477-1-krzesimir@kinvolk.io>
This will come in handy to verify that the hardcoded size of the
context data in bpf_test struct is high enough to hold some struct.
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
tools/include/linux/compiler.h | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/tools/include/linux/compiler.h b/tools/include/linux/compiler.h
index 1827c2f973f9..b4e97751000a 100644
--- a/tools/include/linux/compiler.h
+++ b/tools/include/linux/compiler.h
@@ -172,4 +172,32 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
# define __fallthrough
#endif
+
+#ifdef __OPTIMIZE__
+# define __compiletime_assert(condition, msg, prefix, suffix) \
+ do { \
+ extern void prefix ## suffix(void) __compiletime_error(msg); \
+ if (!(condition)) \
+ prefix ## suffix(); \
+ } while (0)
+#else
+# define __compiletime_assert(condition, msg, prefix, suffix) do { } while (0)
+#endif
+
+#define _compiletime_assert(condition, msg, prefix, suffix) \
+ __compiletime_assert(condition, msg, prefix, suffix)
+
+/**
+ * compiletime_assert - break build and emit msg if condition is false
+ * @condition: a compile-time constant condition to check
+ * @msg: a message to emit if condition is false
+ *
+ * In tradition of POSIX assert, this macro will break the build if the
+ * supplied condition is *false*, emitting the supplied error message if the
+ * compiler has support to do so.
+ */
+#define compiletime_assert(condition, msg) \
+ _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
+
+
#endif /* _TOOLS_LINUX_COMPILER_H */
--
2.20.1
^ permalink raw reply related
* [bpf-next v3 05/12] selftests/bpf: Allow passing more information to BPF prog test run
From: Krzesimir Nowak @ 2019-07-08 16:31 UTC (permalink / raw)
To: linux-kernel
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, netdev, bpf, xdp-newbies,
Krzesimir Nowak
In-Reply-To: <20190708163121.18477-1-krzesimir@kinvolk.io>
The test case can now specify a custom length of the data member,
context data and its length, which will be passed to
bpf_prog_test_run_xattr. For backward compatilibity, if the data
length is 0 (which is what will happen when the field is left
unspecified in the designated initializer of a struct), then the
length passed to the bpf_prog_test_run_xattr is TEST_DATA_LEN.
Also for backward compatilibity, if context data length is 0, NULL is
passed as a context to bpf_prog_test_run_xattr. This is to avoid
breaking other tests, where context data being NULL and context data
length being 0 is handled differently from the case where context data
is not NULL and context data length is 0.
Custom lengths still can't be greater than hardcoded 64 bytes for data
and 192 for context data.
192 for context data was picked to allow passing struct
bpf_perf_event_data as a context for perf event programs. The struct
is quite large, because it contains struct pt_regs.
Test runs for perf event programs will not allow the copying the data
back to data_out buffer, so they require data_out_size to be zero and
data_out to be NULL. Since test_verifier hardcodes it, make it
possible to override the size. Overriding the size to zero will cause
the buffer to be NULL.
Changes since v2:
- Allow overriding the data out size and buffer.
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
tools/testing/selftests/bpf/test_verifier.c | 105 +++++++++++++++++---
1 file changed, 93 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 1640ba9f12c1..6f124cc4ee34 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -54,6 +54,7 @@
#define MAX_TEST_RUNS 8
#define POINTER_VALUE 0xcafe4all
#define TEST_DATA_LEN 64
+#define TEST_CTX_LEN 192
#define F_NEEDS_EFFICIENT_UNALIGNED_ACCESS (1 << 0)
#define F_LOAD_WITH_STRICT_ALIGNMENT (1 << 1)
@@ -96,7 +97,12 @@ struct bpf_test {
enum bpf_prog_type prog_type;
uint8_t flags;
__u8 data[TEST_DATA_LEN];
+ __u32 data_len;
+ __u8 ctx[TEST_CTX_LEN];
+ __u32 ctx_len;
void (*fill_helper)(struct bpf_test *self);
+ bool override_data_out_len;
+ __u32 overridden_data_out_len;
uint8_t runs;
struct {
uint32_t retval, retval_unpriv;
@@ -104,6 +110,9 @@ struct bpf_test {
__u8 data[TEST_DATA_LEN];
__u64 data64[TEST_DATA_LEN / 8];
};
+ __u32 data_len;
+ __u8 ctx[TEST_CTX_LEN];
+ __u32 ctx_len;
} retvals[MAX_TEST_RUNS];
};
@@ -818,21 +827,35 @@ static int set_admin(bool admin)
}
static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
- void *data, size_t size_data)
+ void *data, size_t size_data, void *ctx,
+ size_t size_ctx, u32 *overridden_data_out_size)
{
- __u8 tmp[TEST_DATA_LEN << 2];
- __u32 size_tmp = sizeof(tmp);
- int saved_errno;
- int err;
struct bpf_prog_test_run_attr attr = {
.prog_fd = fd_prog,
.repeat = 1,
.data_in = data,
.data_size_in = size_data,
- .data_out = tmp,
- .data_size_out = size_tmp,
+ .ctx_in = ctx,
+ .ctx_size_in = size_ctx,
};
+ __u8 tmp[TEST_DATA_LEN << 2];
+ __u32 size_tmp = sizeof(tmp);
+ __u32 size_buf = size_tmp;
+ __u8 *buf = tmp;
+ int saved_errno;
+ int err;
+ if (overridden_data_out_size)
+ size_buf = *overridden_data_out_size;
+ if (size_buf > size_tmp) {
+ printf("FAIL: out data size (%d) greater than a buffer size (%d) ",
+ size_buf, size_tmp);
+ return -EINVAL;
+ }
+ if (!size_buf)
+ buf = NULL;
+ attr.data_size_out = size_buf;
+ attr.data_out = buf;
if (unpriv)
set_admin(true);
err = bpf_prog_test_run_xattr(&attr);
@@ -956,13 +979,45 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
if (!alignment_prevented_execution && fd_prog >= 0) {
uint32_t expected_val;
int i;
+ __u32 size_data;
+ __u32 size_ctx;
+ bool bad_size;
+ void *ctx;
+ __u32 *overridden_data_out_size;
if (!test->runs) {
+ if (test->data_len > 0)
+ size_data = test->data_len;
+ else
+ size_data = sizeof(test->data);
+ if (test->override_data_out_len)
+ overridden_data_out_size = &test->overridden_data_out_len;
+ else
+ overridden_data_out_size = NULL;
+ size_ctx = test->ctx_len;
+ bad_size = false;
expected_val = unpriv && test->retval_unpriv ?
test->retval_unpriv : test->retval;
- err = do_prog_test_run(fd_prog, unpriv, expected_val,
- test->data, sizeof(test->data));
+ if (size_data > sizeof(test->data)) {
+ printf("FAIL: data size (%u) greater than TEST_DATA_LEN (%lu) ", size_data, sizeof(test->data));
+ bad_size = true;
+ }
+ if (size_ctx > sizeof(test->ctx)) {
+ printf("FAIL: ctx size (%u) greater than TEST_CTX_LEN (%lu) ", size_ctx, sizeof(test->ctx));
+ bad_size = true;
+ }
+ if (size_ctx)
+ ctx = test->ctx;
+ else
+ ctx = NULL;
+ if (bad_size)
+ err = 1;
+ else
+ err = do_prog_test_run(fd_prog, unpriv, expected_val,
+ test->data, size_data,
+ ctx, size_ctx,
+ overridden_data_out_size);
if (err)
run_errs++;
else
@@ -970,14 +1025,40 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
}
for (i = 0; i < test->runs; i++) {
+ if (test->retvals[i].data_len > 0)
+ size_data = test->retvals[i].data_len;
+ else
+ size_data = sizeof(test->retvals[i].data);
+ if (test->override_data_out_len)
+ overridden_data_out_size = &test->overridden_data_out_len;
+ else
+ overridden_data_out_size = NULL;
+ size_ctx = test->retvals[i].ctx_len;
+ bad_size = false;
if (unpriv && test->retvals[i].retval_unpriv)
expected_val = test->retvals[i].retval_unpriv;
else
expected_val = test->retvals[i].retval;
- err = do_prog_test_run(fd_prog, unpriv, expected_val,
- test->retvals[i].data,
- sizeof(test->retvals[i].data));
+ if (size_data > sizeof(test->retvals[i].data)) {
+ printf("FAIL: data size (%u) at run %i greater than TEST_DATA_LEN (%lu) ", size_data, i + 1, sizeof(test->retvals[i].data));
+ bad_size = true;
+ }
+ if (size_ctx > sizeof(test->retvals[i].ctx)) {
+ printf("FAIL: ctx size (%u) at run %i greater than TEST_CTX_LEN (%lu) ", size_ctx, i + 1, sizeof(test->retvals[i].ctx));
+ bad_size = true;
+ }
+ if (size_ctx)
+ ctx = test->retvals[i].ctx;
+ else
+ ctx = NULL;
+ if (bad_size)
+ err = 1;
+ else
+ err = do_prog_test_run(fd_prog, unpriv, expected_val,
+ test->retvals[i].data, size_data,
+ ctx, size_ctx,
+ overridden_data_out_size);
if (err) {
printf("(run %d/%d) ", i + 1, test->runs);
run_errs++;
--
2.20.1
^ permalink raw reply related
* [bpf-next v3 06/12] selftests/bpf: Make sure that preexisting tests for perf event work
From: Krzesimir Nowak @ 2019-07-08 16:31 UTC (permalink / raw)
To: linux-kernel
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, netdev, bpf, xdp-newbies,
Krzesimir Nowak
In-Reply-To: <20190708163121.18477-1-krzesimir@kinvolk.io>
We are going to introduce a test run implementation for perf event in
a later commit and it will not allow passing any data out or ctx out
to it, and requires their sizes to be specified to zero. To avoid test
failures when the feature is introduced, override the data out size to
zero. That will also cause NULL buffer to be sent to the kernel.
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
.../testing/selftests/bpf/verifier/perf_event_sample_period.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c b/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c
index 471c1a5950d8..19f5d824b275 100644
--- a/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c
+++ b/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c
@@ -13,6 +13,7 @@
},
.result = ACCEPT,
.prog_type = BPF_PROG_TYPE_PERF_EVENT,
+ .override_data_out_len = true,
},
{
"check bpf_perf_event_data->sample_period half load permitted",
@@ -29,6 +30,7 @@
},
.result = ACCEPT,
.prog_type = BPF_PROG_TYPE_PERF_EVENT,
+ .override_data_out_len = true,
},
{
"check bpf_perf_event_data->sample_period word load permitted",
@@ -45,6 +47,7 @@
},
.result = ACCEPT,
.prog_type = BPF_PROG_TYPE_PERF_EVENT,
+ .override_data_out_len = true,
},
{
"check bpf_perf_event_data->sample_period dword load permitted",
@@ -56,4 +59,5 @@
},
.result = ACCEPT,
.prog_type = BPF_PROG_TYPE_PERF_EVENT,
+ .override_data_out_len = true,
},
--
2.20.1
^ permalink raw reply related
* [bpf-next v3 04/12] selftests/bpf: Use bpf_prog_test_run_xattr
From: Krzesimir Nowak @ 2019-07-08 16:31 UTC (permalink / raw)
To: linux-kernel
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, netdev, bpf, xdp-newbies,
Krzesimir Nowak
In-Reply-To: <20190708163121.18477-1-krzesimir@kinvolk.io>
The bpf_prog_test_run_xattr function gives more options to set up a
test run of a BPF program than the bpf_prog_test_run function.
We will need this extra flexibility to pass ctx data later.
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
tools/testing/selftests/bpf/test_verifier.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index c7541f572932..1640ba9f12c1 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -822,14 +822,20 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
{
__u8 tmp[TEST_DATA_LEN << 2];
__u32 size_tmp = sizeof(tmp);
- uint32_t retval;
int saved_errno;
int err;
+ struct bpf_prog_test_run_attr attr = {
+ .prog_fd = fd_prog,
+ .repeat = 1,
+ .data_in = data,
+ .data_size_in = size_data,
+ .data_out = tmp,
+ .data_size_out = size_tmp,
+ };
if (unpriv)
set_admin(true);
- err = bpf_prog_test_run(fd_prog, 1, data, size_data,
- tmp, &size_tmp, &retval, NULL);
+ err = bpf_prog_test_run_xattr(&attr);
saved_errno = errno;
if (unpriv)
set_admin(false);
@@ -846,9 +852,9 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
return err;
}
}
- if (retval != expected_val &&
+ if (attr.retval != expected_val &&
expected_val != POINTER_VALUE) {
- printf("FAIL retval %d != %d ", retval, expected_val);
+ printf("FAIL retval %d != %d ", attr.retval, expected_val);
return 1;
}
--
2.20.1
^ permalink raw reply related
* [bpf-next v3 03/12] selftests/bpf: Avoid another case of errno clobbering
From: Krzesimir Nowak @ 2019-07-08 16:31 UTC (permalink / raw)
To: linux-kernel
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, netdev, bpf, xdp-newbies,
Krzesimir Nowak
In-Reply-To: <20190708163121.18477-1-krzesimir@kinvolk.io>
Commit 8184d44c9a57 ("selftests/bpf: skip verifier tests for
unsupported program types") added a check for an unsupported program
type. The function doing it changes errno, so test_verifier should
save it before calling it if test_verifier wants to print a reason why
verifying a BPF program of a supported type failed.
Changes since v2:
- Move the declaration to fit the reverse christmas tree style.
Fixes: 8184d44c9a57 ("selftests/bpf: skip verifier tests for unsupported program types")
Cc: Stanislav Fomichev <sdf@google.com>
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
tools/testing/selftests/bpf/test_verifier.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 3fe126e0083b..c7541f572932 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -864,6 +864,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
int run_errs, run_successes;
int map_fds[MAX_NR_MAPS];
const char *expected_err;
+ int saved_errno;
int fixup_skips;
__u32 pflags;
int i, err;
@@ -894,6 +895,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
pflags |= BPF_F_ANY_ALIGNMENT;
fd_prog = bpf_verify_program(prog_type, prog, prog_len, pflags,
"GPL", 0, bpf_vlog, sizeof(bpf_vlog), 4);
+ saved_errno = errno;
if (fd_prog < 0 && !bpf_probe_prog_type(prog_type, 0)) {
printf("SKIP (unsupported program type %d)\n", prog_type);
skips++;
@@ -910,7 +912,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
if (expected_ret == ACCEPT) {
if (fd_prog < 0) {
printf("FAIL\nFailed to load prog '%s'!\n",
- strerror(errno));
+ strerror(saved_errno));
goto fail_log;
}
#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
--
2.20.1
^ permalink raw reply related
* [bpf-next v3 01/12] selftests/bpf: Print a message when tester could not run a program
From: Krzesimir Nowak @ 2019-07-08 16:31 UTC (permalink / raw)
To: linux-kernel
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, netdev, bpf, xdp-newbies,
Krzesimir Nowak
In-Reply-To: <20190708163121.18477-1-krzesimir@kinvolk.io>
This prints a message when the error is about program type being not
supported by the test runner or because of permissions problem. This
is to see if the program we expected to run was actually executed.
The messages are open-coded because strerror(ENOTSUPP) returns
"Unknown error 524".
Changes since v2:
- Also print "FAIL" on an unexpected bpf_prog_test_run error, so there
is a corresponding "FAIL" message for each failed test.
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
tools/testing/selftests/bpf/test_verifier.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index c5514daf8865..b8d065623ead 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -831,11 +831,20 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
tmp, &size_tmp, &retval, NULL);
if (unpriv)
set_admin(false);
- if (err && errno != 524/*ENOTSUPP*/ && errno != EPERM) {
- printf("Unexpected bpf_prog_test_run error ");
- return err;
+ if (err) {
+ switch (errno) {
+ case 524/*ENOTSUPP*/:
+ printf("Did not run the program (not supported) ");
+ return 0;
+ case EPERM:
+ printf("Did not run the program (no permission) ");
+ return 0;
+ default:
+ printf("FAIL: Unexpected bpf_prog_test_run error (%s) ", strerror(saved_errno));
+ return err;
+ }
}
- if (!err && retval != expected_val &&
+ if (retval != expected_val &&
expected_val != POINTER_VALUE) {
printf("FAIL retval %d != %d ", retval, expected_val);
return 1;
--
2.20.1
^ permalink raw reply related
* [bpf-next v3 02/12] selftests/bpf: Avoid a clobbering of errno
From: Krzesimir Nowak @ 2019-07-08 16:31 UTC (permalink / raw)
To: linux-kernel
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, netdev, bpf, xdp-newbies,
Krzesimir Nowak
In-Reply-To: <20190708163121.18477-1-krzesimir@kinvolk.io>
Save errno right after bpf_prog_test_run returns, so we later check
the error code actually set by bpf_prog_test_run, not by some libcap
function.
Changes since v1:
- Fix the "Fixes:" tag to mention actual commit that introduced the
bug
Changes since v2:
- Move the declaration so it fits the reverse christmas tree style.
Cc: Daniel Borkmann <daniel@iogearbox.net>
Fixes: 832c6f2c29ec ("bpf: test make sure to run unpriv test cases in test_verifier")
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
tools/testing/selftests/bpf/test_verifier.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index b8d065623ead..3fe126e0083b 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -823,16 +823,18 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
__u8 tmp[TEST_DATA_LEN << 2];
__u32 size_tmp = sizeof(tmp);
uint32_t retval;
+ int saved_errno;
int err;
if (unpriv)
set_admin(true);
err = bpf_prog_test_run(fd_prog, 1, data, size_data,
tmp, &size_tmp, &retval, NULL);
+ saved_errno = errno;
if (unpriv)
set_admin(false);
if (err) {
- switch (errno) {
+ switch (saved_errno) {
case 524/*ENOTSUPP*/:
printf("Did not run the program (not supported) ");
return 0;
--
2.20.1
^ permalink raw reply related
* [bpf-next v3 00/12] Test the 32bit narrow reads
From: Krzesimir Nowak @ 2019-07-08 16:31 UTC (permalink / raw)
To: linux-kernel
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, netdev, bpf, xdp-newbies,
Krzesimir Nowak
These patches try to test the fix made in commit e2f7fc0ac695 ("bpf:
fix undefined behavior in narrow load handling"). The problem existed
in the generated BPF bytecode that was doing a 32bit narrow read of a
64bit field, so to test it the code would need to be executed.
Currently the only such field exists in BPF_PROG_TYPE_PERF_EVENT,
which was not supported by bpf_prog_test_run().
I'm sending these patches to bpf-next now as they introduce a new
feature. But maybe some of those patches could go to the bpf branch?
There is a bit of yak shaving to do for the test to be run:
1. Print why the program could not be run (patch 1).
2. Some fixes for errno clobbering (patches 2 and 3).
3. Using bpf_prog_test_run_xattr, so I can pass ctx_in stuff too
(patch 4).
4. Adding ctx stuff and data out size override to struct bpf_test, and
use them for the perf event tests (patches 5 and 6).
5. Some tools headers syncing (patches 7 and 8).
6. Split out some useful functions for bpf_prog_test_run
implementation out of the net/bpf/test_run.c (patch 9)
7. Implement bpf_prog_test_run for perf event programs and test it
(patches 10 and 11).
The last point is where I'm least sure how things should be done
properly:
1. There is a bunch of stuff to prepare for the
bpf_perf_prog_read_value to work, and that stuff is very hacky. I
would welcome some hints about how to set up the perf_event and
perf_sample_data structs in a way that is a bit more future-proof
than just setting some fields in a specific way, so some other code
won't use some other fields (like setting event.oncpu to -1 to
avoid event.pmu to be used for reading the value of the event).
2. The tests try to see if the test run for perf event sets up the
context properly, so they verify the struct pt_regs contents. They
way it is currently written Works For Me, but surely it won't work
on other arch. So what would be the way forward? Just put the test
case inside #ifdef __x86_64__?
3. Another thing in tests - I'm trying to make sure that the
bpf_perf_prog_read_value helper works as it seems to be the only
bpf perf helper that takes the ctx as a parameter. Is that enough
or I should test other helpers too?
About the test itself - I'm not sure if it will work on a big endian
machine. I think it should, but I don't have anything handy here to
verify it.
Krzesimir Nowak (12):
selftests/bpf: Print a message when tester could not run a program
selftests/bpf: Avoid a clobbering of errno
selftests/bpf: Avoid another case of errno clobbering
selftests/bpf: Use bpf_prog_test_run_xattr
selftests/bpf: Allow passing more information to BPF prog test run
selftests/bpf: Make sure that preexisting tests for perf event work
tools headers: Adopt compiletime_assert from kernel sources
tools headers: Sync struct bpf_perf_event_data
bpf: Split out some helper functions
bpf: Implement bpf_prog_test_run for perf event programs
selftests/bpf: Add tests for bpf_prog_test_run for perf events progs
selftests/bpf: Test correctness of narrow 32bit read on 64bit field
include/linux/bpf.h | 28 ++
kernel/bpf/Makefile | 1 +
kernel/bpf/test_run.c | 212 ++++++++++++++
kernel/trace/bpf_trace.c | 60 ++++
net/bpf/test_run.c | 263 +++++-------------
tools/include/linux/compiler.h | 28 ++
tools/include/uapi/linux/bpf_perf_event.h | 1 +
tools/testing/selftests/bpf/test_verifier.c | 197 ++++++++++++-
.../selftests/bpf/verifier/perf_event_run.c | 96 +++++++
.../bpf/verifier/perf_event_sample_period.c | 4 +
.../testing/selftests/bpf/verifier/var_off.c | 21 ++
11 files changed, 700 insertions(+), 211 deletions(-)
create mode 100644 kernel/bpf/test_run.c
create mode 100644 tools/testing/selftests/bpf/verifier/perf_event_run.c
--
2.20.1
^ permalink raw reply
* Re: [PATCH 1/1] tools/dtrace: initial implementation of DTrace
From: Kris Van Hees @ 2019-07-08 16:38 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Kris Van Hees, netdev, bpf, dtrace-devel, linux-kernel, rostedt,
mhiramat, acme, ast, daniel, Chris Mason
In-Reply-To: <20190704130509.GO3402@hirez.programming.kicks-ass.net>
On Thu, Jul 04, 2019 at 03:05:09PM +0200, Peter Zijlstra wrote:
> On Wed, Jul 03, 2019 at 08:14:30PM -0700, Kris Van Hees wrote:
> > +int dt_bpf_attach(int event_id, int bpf_fd)
> > +{
> > + int event_fd;
> > + int rc;
> > + struct perf_event_attr attr = {};
> > +
> > + attr.type = PERF_TYPE_TRACEPOINT;
> > + attr.sample_type = PERF_SAMPLE_RAW;
> > + attr.sample_period = 1;
> > + attr.wakeup_events = 1;
> > + attr.config = event_id;
> > +
> > + /* Register the event (based on its id), and obtain a fd. */
> > + event_fd = perf_event_open(&attr, -1, 0, -1, 0);
> > + if (event_fd < 0) {
> > + perror("sys_perf_event_open");
> > + return -1;
> > + }
> > +
> > + /* Enable the probe. */
> > + rc = ioctl(event_fd, PERF_EVENT_IOC_ENABLE, 0);
>
> AFAICT you didn't use attr.disabled = 1, so this IOC_ENABLE is
> completely superfluous.
Oh yes, good point (and the same applies to the dt_buffer.c code where I set
up the events that own each buffer - no point in doing an explicit enable there
eiteher).
Thanks for catching this!
> > + if (rc < 0) {
> > + perror("PERF_EVENT_IOC_ENABLE");
> > + return -1;
> > + }
> > +
> > + /* Associate the BPF program with the event. */
> > + rc = ioctl(event_fd, PERF_EVENT_IOC_SET_BPF, bpf_fd);
> > + if (rc < 0) {
> > + perror("PERF_EVENT_IOC_SET_BPF");
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
^ permalink raw reply
* Re: [bpf-next v3 09/12] bpf: Split out some helper functions
From: Krzesimir Nowak @ 2019-07-08 16:40 UTC (permalink / raw)
To: linux-kernel
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
John Fastabend, Stanislav Fomichev, netdev, bpf, xdp-newbies
In-Reply-To: <20190708163121.18477-10-krzesimir@kinvolk.io>
On Mon, Jul 8, 2019 at 6:31 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
>
> The moved functions are generally useful for implementing
> bpf_prog_test_run for other types of BPF programs - they don't have
> any network-specific stuff in them, so I can use them in a test run
> implementation for perf event BPF program too.
>
> Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
This patch has some whitespace issues (indentation with spaces instead
of tabs in some places), will fix them for v4. Sorry about that.
> ---
> include/linux/bpf.h | 28 +++++
> kernel/bpf/Makefile | 1 +
> kernel/bpf/test_run.c | 212 ++++++++++++++++++++++++++++++++++
> net/bpf/test_run.c | 263 +++++++++++-------------------------------
> 4 files changed, 308 insertions(+), 196 deletions(-)
> create mode 100644 kernel/bpf/test_run.c
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 18f4cc2c6acd..28db8ba57bc3 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1143,4 +1143,32 @@ static inline u32 bpf_xdp_sock_convert_ctx_access(enum bpf_access_type type,
> }
> #endif /* CONFIG_INET */
>
> +/* Helper functions for bpf_prog_test_run implementations */
> +typedef u32 bpf_prog_run_helper_t(struct bpf_prog *prog, void *ctx,
> + void *private_data);
> +
> +enum bpf_test_run_flags {
> + BPF_TEST_RUN_PLAIN = 0,
> + BPF_TEST_RUN_SETUP_CGROUP_STORAGE = 1 << 0,
> +};
> +
> +int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat, u32 flags,
> + u32 *retval, u32 *duration);
> +
> +int bpf_test_run_cb(struct bpf_prog *prog, void *ctx, u32 repeat, u32 flags,
> + bpf_prog_run_helper_t run_prog, void *private_data,
> + u32 *retval, u32 *duration);
> +
> +int bpf_test_finish(union bpf_attr __user *uattr, u32 retval, u32 duration);
> +
> +void *bpf_receive_ctx(const union bpf_attr *kattr, u32 max_size);
> +
> +int bpf_send_ctx(const union bpf_attr *kattr, union bpf_attr __user *uattr,
> + const void *data, u32 size);
> +
> +void *bpf_receive_data(const union bpf_attr *kattr, u32 max_size);
> +
> +int bpf_send_data(const union bpf_attr *kattr, union bpf_attr __user *uattr,
> + const void *data, u32 size);
> +
> #endif /* _LINUX_BPF_H */
> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> index 29d781061cd5..570fd40288f4 100644
> --- a/kernel/bpf/Makefile
> +++ b/kernel/bpf/Makefile
> @@ -22,3 +22,4 @@ obj-$(CONFIG_CGROUP_BPF) += cgroup.o
> ifeq ($(CONFIG_INET),y)
> obj-$(CONFIG_BPF_SYSCALL) += reuseport_array.o
> endif
> +obj-$(CONFIG_BPF_SYSCALL) += test_run.o
> diff --git a/kernel/bpf/test_run.c b/kernel/bpf/test_run.c
> new file mode 100644
> index 000000000000..0481373da8be
> --- /dev/null
> +++ b/kernel/bpf/test_run.c
> @@ -0,0 +1,212 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2017 Facebook
> + * Copyright (c) 2019 Tigera, Inc
> + */
> +
> +#include <asm/div64.h>
> +
> +#include <linux/bpf-cgroup.h>
> +#include <linux/bpf.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/filter.h>
> +#include <linux/gfp.h>
> +#include <linux/kernel.h>
> +#include <linux/limits.h>
> +#include <linux/preempt.h>
> +#include <linux/rcupdate.h>
> +#include <linux/sched.h>
> +#include <linux/sched/signal.h>
> +#include <linux/slab.h>
> +#include <linux/timekeeping.h>
> +#include <linux/uaccess.h>
> +
> +static void teardown_cgroup_storage(struct bpf_cgroup_storage **storage)
> +{
> + enum bpf_cgroup_storage_type stype;
> +
> + if (!storage)
> + return;
> + for_each_cgroup_storage_type(stype)
> + bpf_cgroup_storage_free(storage[stype]);
> + kfree(storage);
> +}
> +
> +static struct bpf_cgroup_storage **setup_cgroup_storage(struct bpf_prog *prog)
> +{
> + enum bpf_cgroup_storage_type stype;
> + struct bpf_cgroup_storage **storage;
> + size_t size = MAX_BPF_CGROUP_STORAGE_TYPE;
> +
> + size *= sizeof(struct bpf_cgroup_storage *);
> + storage = kzalloc(size, GFP_KERNEL);
> + for_each_cgroup_storage_type(stype) {
> + storage[stype] = bpf_cgroup_storage_alloc(prog, stype);
> + if (IS_ERR(storage[stype])) {
> + storage[stype] = NULL;
> + teardown_cgroup_storage(storage);
> + return ERR_PTR(-ENOMEM);
> + }
> + }
> + return storage;
> +}
> +
> +static u32 run_bpf_prog(struct bpf_prog *prog, void *ctx, void *private_data)
> +{
> + return BPF_PROG_RUN(prog, ctx);
> +}
> +
> +int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat, u32 flags,
> + u32 *retval, u32 *duration)
> +{
> + return bpf_test_run_cb(prog, ctx, repeat, flags, run_bpf_prog, NULL,
> + retval, duration);
> +}
> +
> +int bpf_test_run_cb(struct bpf_prog *prog, void *ctx, u32 repeat, u32 flags,
> + bpf_prog_run_helper_t run_prog, void *private_data,
> + u32 *retval, u32 *duration)
> +{
> + struct bpf_cgroup_storage **storage = NULL;
> + u64 time_start, time_spent = 0;
> + int ret = 0;
> + u32 i;
> +
> + if (flags & BPF_TEST_RUN_SETUP_CGROUP_STORAGE) {
> + storage = setup_cgroup_storage(prog);
> + if (IS_ERR(storage))
> + return PTR_ERR(storage);
> + }
> +
> + if (!repeat)
> + repeat = 1;
> +
> + rcu_read_lock();
> + preempt_disable();
> + time_start = ktime_get_ns();
> + for (i = 0; i < repeat; i++) {
> + if (storage)
> + bpf_cgroup_storage_set(storage);
> + *retval = run_prog(prog, ctx, private_data);
> +
> + if (signal_pending(current)) {
> + preempt_enable();
> + rcu_read_unlock();
> + teardown_cgroup_storage(storage);
> + return -EINTR;
> + }
> +
> + if (need_resched()) {
> + time_spent += ktime_get_ns() - time_start;
> + preempt_enable();
> + rcu_read_unlock();
> +
> + cond_resched();
> +
> + rcu_read_lock();
> + preempt_disable();
> + time_start = ktime_get_ns();
> + }
> + }
> + time_spent += ktime_get_ns() - time_start;
> + preempt_enable();
> + rcu_read_unlock();
> +
> + do_div(time_spent, repeat);
> + *duration = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
> +
> + teardown_cgroup_storage(storage);
> +
> + return ret;
> +}
> +
> +int bpf_test_finish(union bpf_attr __user *uattr, u32 retval, u32 duration)
> +{
> + if (copy_to_user(&uattr->test.retval, &retval, sizeof(retval)))
> + return -EFAULT;
> + if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration)))
> + return -EFAULT;
> + return 0;
> +}
> +
> +static void *bpf_receive_mem(u64 in, u32 in_size, u32 max_size)
> +{
> + void __user *data_in = u64_to_user_ptr(in);
> + void *data;
> + int err;
> +
> + if (!data_in && in_size)
> + return ERR_PTR(-EINVAL);
> + data = kzalloc(max_size, GFP_USER);
> + if (!data)
> + return ERR_PTR(-ENOMEM);
> +
> + if (data_in) {
> + err = bpf_check_uarg_tail_zero(data_in, max_size, in_size);
> + if (err) {
> + kfree(data);
> + return ERR_PTR(err);
> + }
> +
> + in_size = min_t(u32, max_size, in_size);
> + if (copy_from_user(data, data_in, in_size)) {
> + kfree(data);
> + return ERR_PTR(-EFAULT);
> + }
> + }
> + return data;
> +}
> +
> +static int bpf_send_mem(u64 out, u32 out_size, u32 *out_size_write,
> + const void *data, u32 data_size)
> +{
> + void __user *data_out = u64_to_user_ptr(out);
> + int err = -EFAULT;
> + u32 copy_size = data_size;
> +
> + if (!data_out && out_size)
> + return -EINVAL;
> +
> + if (!data || !data_out)
> + return 0;
> +
> + if (copy_size > out_size) {
> + copy_size = out_size;
> + err = -ENOSPC;
> + }
> +
> + if (copy_to_user(data_out, data, copy_size))
> + goto out;
> + if (copy_to_user(out_size_write, &data_size, sizeof(data_size)))
> + goto out;
> + if (err != -ENOSPC)
> + err = 0;
> +out:
> + return err;
> +}
> +
> +void *bpf_receive_data(const union bpf_attr *kattr, u32 max_size)
> +{
> + return bpf_receive_mem(kattr->test.data_in, kattr->test.data_size_in,
> + max_size);
> +}
> +
> +int bpf_send_data(const union bpf_attr *kattr, union bpf_attr __user *uattr,
> + const void *data, u32 size)
> +{
> + return bpf_send_mem(kattr->test.data_out, kattr->test.data_size_out,
> + &uattr->test.data_size_out, data, size);
> +}
> +
> +void *bpf_receive_ctx(const union bpf_attr *kattr, u32 max_size)
> +{
> + return bpf_receive_mem(kattr->test.ctx_in, kattr->test.ctx_size_in,
> + max_size);
> +}
> +
> +int bpf_send_ctx(const union bpf_attr *kattr, union bpf_attr __user *uattr,
> + const void *data, u32 size)
> +{
> + return bpf_send_mem(kattr->test.ctx_out, kattr->test.ctx_size_out,
> + &uattr->test.ctx_size_out, data, size);
> +}
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 80e6f3a6864d..fe6b7b1af0cc 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -14,97 +14,6 @@
> #define CREATE_TRACE_POINTS
> #include <trace/events/bpf_test_run.h>
>
> -static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
> - u32 *retval, u32 *time)
> -{
> - struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = { NULL };
> - enum bpf_cgroup_storage_type stype;
> - u64 time_start, time_spent = 0;
> - int ret = 0;
> - u32 i;
> -
> - for_each_cgroup_storage_type(stype) {
> - storage[stype] = bpf_cgroup_storage_alloc(prog, stype);
> - if (IS_ERR(storage[stype])) {
> - storage[stype] = NULL;
> - for_each_cgroup_storage_type(stype)
> - bpf_cgroup_storage_free(storage[stype]);
> - return -ENOMEM;
> - }
> - }
> -
> - if (!repeat)
> - repeat = 1;
> -
> - rcu_read_lock();
> - preempt_disable();
> - time_start = ktime_get_ns();
> - for (i = 0; i < repeat; i++) {
> - bpf_cgroup_storage_set(storage);
> - *retval = BPF_PROG_RUN(prog, ctx);
> -
> - if (signal_pending(current)) {
> - ret = -EINTR;
> - break;
> - }
> -
> - if (need_resched()) {
> - time_spent += ktime_get_ns() - time_start;
> - preempt_enable();
> - rcu_read_unlock();
> -
> - cond_resched();
> -
> - rcu_read_lock();
> - preempt_disable();
> - time_start = ktime_get_ns();
> - }
> - }
> - time_spent += ktime_get_ns() - time_start;
> - preempt_enable();
> - rcu_read_unlock();
> -
> - do_div(time_spent, repeat);
> - *time = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
> -
> - for_each_cgroup_storage_type(stype)
> - bpf_cgroup_storage_free(storage[stype]);
> -
> - return ret;
> -}
> -
> -static int bpf_test_finish(const union bpf_attr *kattr,
> - union bpf_attr __user *uattr, const void *data,
> - u32 size, u32 retval, u32 duration)
> -{
> - void __user *data_out = u64_to_user_ptr(kattr->test.data_out);
> - int err = -EFAULT;
> - u32 copy_size = size;
> -
> - /* Clamp copy if the user has provided a size hint, but copy the full
> - * buffer if not to retain old behaviour.
> - */
> - if (kattr->test.data_size_out &&
> - copy_size > kattr->test.data_size_out) {
> - copy_size = kattr->test.data_size_out;
> - err = -ENOSPC;
> - }
> -
> - if (data_out && copy_to_user(data_out, data, copy_size))
> - goto out;
> - if (copy_to_user(&uattr->test.data_size_out, &size, sizeof(size)))
> - goto out;
> - if (copy_to_user(&uattr->test.retval, &retval, sizeof(retval)))
> - goto out;
> - if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration)))
> - goto out;
> - if (err != -ENOSPC)
> - err = 0;
> -out:
> - trace_bpf_test_finish(&err);
> - return err;
> -}
> -
> static void *bpf_test_init(const union bpf_attr *kattr, u32 size,
> u32 headroom, u32 tailroom)
> {
> @@ -125,63 +34,6 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 size,
> return data;
> }
>
> -static void *bpf_ctx_init(const union bpf_attr *kattr, u32 max_size)
> -{
> - void __user *data_in = u64_to_user_ptr(kattr->test.ctx_in);
> - void __user *data_out = u64_to_user_ptr(kattr->test.ctx_out);
> - u32 size = kattr->test.ctx_size_in;
> - void *data;
> - int err;
> -
> - if (!data_in && !data_out)
> - return NULL;
> -
> - data = kzalloc(max_size, GFP_USER);
> - if (!data)
> - return ERR_PTR(-ENOMEM);
> -
> - if (data_in) {
> - err = bpf_check_uarg_tail_zero(data_in, max_size, size);
> - if (err) {
> - kfree(data);
> - return ERR_PTR(err);
> - }
> -
> - size = min_t(u32, max_size, size);
> - if (copy_from_user(data, data_in, size)) {
> - kfree(data);
> - return ERR_PTR(-EFAULT);
> - }
> - }
> - return data;
> -}
> -
> -static int bpf_ctx_finish(const union bpf_attr *kattr,
> - union bpf_attr __user *uattr, const void *data,
> - u32 size)
> -{
> - void __user *data_out = u64_to_user_ptr(kattr->test.ctx_out);
> - int err = -EFAULT;
> - u32 copy_size = size;
> -
> - if (!data || !data_out)
> - return 0;
> -
> - if (copy_size > kattr->test.ctx_size_out) {
> - copy_size = kattr->test.ctx_size_out;
> - err = -ENOSPC;
> - }
> -
> - if (copy_to_user(data_out, data, copy_size))
> - goto out;
> - if (copy_to_user(&uattr->test.ctx_size_out, &size, sizeof(size)))
> - goto out;
> - if (err != -ENOSPC)
> - err = 0;
> -out:
> - return err;
> -}
> -
> /**
> * range_is_zero - test whether buffer is initialized
> * @buf: buffer to check
> @@ -238,6 +90,36 @@ static void convert_skb_to___skb(struct sk_buff *skb, struct __sk_buff *__skb)
> memcpy(__skb->cb, &cb->data, QDISC_CB_PRIV_LEN);
> }
>
> +static int bpf_net_prog_test_run_finish(const union bpf_attr *kattr,
> + union bpf_attr __user *uattr,
> + const void *data, u32 data_size,
> + const void *ctx, u32 ctx_size,
> + u32 retval, u32 duration)
> +{
> + int ret;
> + union bpf_attr fixed_kattr;
> + const union bpf_attr *kattr_ptr = kattr;
> +
> + /* Clamp copy (in bpf_send_mem) if the user has provided a
> + * size hint, but copy the full buffer if not to retain old
> + * behaviour.
> + */
> + if (!kattr->test.data_size_out && kattr->test.data_out) {
> + fixed_kattr = *kattr;
> + fixed_kattr.test.data_size_out = U32_MAX;
> + kattr_ptr = &fixed_kattr;
> + }
> +
> + ret = bpf_send_data(kattr_ptr, uattr, data, data_size);
> + if (!ret) {
> + ret = bpf_test_finish(uattr, retval, duration);
> + if (!ret && ctx)
> + ret = bpf_send_ctx(kattr_ptr, uattr, ctx, ctx_size);
> + }
> + trace_bpf_test_finish(&ret);
> + return ret;
> +}
> +
> int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> union bpf_attr __user *uattr)
> {
> @@ -257,7 +139,7 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> if (IS_ERR(data))
> return PTR_ERR(data);
>
> - ctx = bpf_ctx_init(kattr, sizeof(struct __sk_buff));
> + ctx = bpf_receive_ctx(kattr, sizeof(struct __sk_buff));
> if (IS_ERR(ctx)) {
> kfree(data);
> return PTR_ERR(ctx);
> @@ -307,7 +189,8 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> ret = convert___skb_to_skb(skb, ctx);
> if (ret)
> goto out;
> - ret = bpf_test_run(prog, skb, repeat, &retval, &duration);
> + ret = bpf_test_run(prog, skb, repeat, BPF_TEST_RUN_SETUP_CGROUP_STORAGE,
> + &retval, &duration);
> if (ret)
> goto out;
> if (!is_l2) {
> @@ -327,10 +210,9 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> /* bpf program can never convert linear skb to non-linear */
> if (WARN_ON_ONCE(skb_is_nonlinear(skb)))
> size = skb_headlen(skb);
> - ret = bpf_test_finish(kattr, uattr, skb->data, size, retval, duration);
> - if (!ret)
> - ret = bpf_ctx_finish(kattr, uattr, ctx,
> - sizeof(struct __sk_buff));
> + ret = bpf_net_prog_test_run_finish(kattr, uattr, skb->data, size,
> + ctx, sizeof(struct __sk_buff),
> + retval, duration);
> out:
> kfree_skb(skb);
> bpf_sk_storage_free(sk);
> @@ -365,32 +247,48 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
> rxqueue = __netif_get_rx_queue(current->nsproxy->net_ns->loopback_dev, 0);
> xdp.rxq = &rxqueue->xdp_rxq;
>
> - ret = bpf_test_run(prog, &xdp, repeat, &retval, &duration);
> + ret = bpf_test_run(prog, &xdp, repeat,
> + BPF_TEST_RUN_SETUP_CGROUP_STORAGE,
> + &retval, &duration);
> if (ret)
> goto out;
> if (xdp.data != data + XDP_PACKET_HEADROOM + NET_IP_ALIGN ||
> xdp.data_end != xdp.data + size)
> size = xdp.data_end - xdp.data;
> - ret = bpf_test_finish(kattr, uattr, xdp.data, size, retval, duration);
> + ret = bpf_net_prog_test_run_finish(kattr, uattr, xdp.data, size,
> + NULL, 0, retval, duration);
> out:
> kfree(data);
> return ret;
> }
>
> +struct bpf_flow_dissect_run_data {
> + __be16 proto;
> + int nhoff;
> + int hlen;
> +};
> +
> +static u32 bpf_flow_dissect_run(struct bpf_prog *prog, void *ctx,
> + void *private_data)
> +{
> + struct bpf_flow_dissect_run_data *data = private_data;
> +
> + return bpf_flow_dissect(prog, ctx, data->proto, data->nhoff, data->hlen);
> +}
> +
> int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
> const union bpf_attr *kattr,
> union bpf_attr __user *uattr)
> {
> + struct bpf_flow_dissect_run_data run_data = {};
> u32 size = kattr->test.data_size_in;
> struct bpf_flow_dissector ctx = {};
> u32 repeat = kattr->test.repeat;
> struct bpf_flow_keys flow_keys;
> - u64 time_start, time_spent = 0;
> const struct ethhdr *eth;
> u32 retval, duration;
> void *data;
> int ret;
> - u32 i;
>
> if (prog->type != BPF_PROG_TYPE_FLOW_DISSECTOR)
> return -EINVAL;
> @@ -407,49 +305,22 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
>
> eth = (struct ethhdr *)data;
>
> - if (!repeat)
> - repeat = 1;
> -
> ctx.flow_keys = &flow_keys;
> ctx.data = data;
> ctx.data_end = (__u8 *)data + size;
>
> - rcu_read_lock();
> - preempt_disable();
> - time_start = ktime_get_ns();
> - for (i = 0; i < repeat; i++) {
> - retval = bpf_flow_dissect(prog, &ctx, eth->h_proto, ETH_HLEN,
> - size);
> -
> - if (signal_pending(current)) {
> - preempt_enable();
> - rcu_read_unlock();
> -
> - ret = -EINTR;
> - goto out;
> - }
> -
> - if (need_resched()) {
> - time_spent += ktime_get_ns() - time_start;
> - preempt_enable();
> - rcu_read_unlock();
> -
> - cond_resched();
> -
> - rcu_read_lock();
> - preempt_disable();
> - time_start = ktime_get_ns();
> - }
> - }
> - time_spent += ktime_get_ns() - time_start;
> - preempt_enable();
> - rcu_read_unlock();
> -
> - do_div(time_spent, repeat);
> - duration = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
> + run_data.proto = eth->h_proto;
> + run_data.nhoff = ETH_HLEN;
> + run_data.hlen = size;
> + ret = bpf_test_run_cb(prog, &ctx, repeat, BPF_TEST_RUN_PLAIN,
> + bpf_flow_dissect_run, &run_data,
> + &retval, &duration);
> + if (!ret)
> + goto out;
>
> - ret = bpf_test_finish(kattr, uattr, &flow_keys, sizeof(flow_keys),
> - retval, duration);
> + ret = bpf_net_prog_test_run_finish(kattr, uattr, &flow_keys,
> + sizeof(flow_keys), NULL, 0,
> + retval, duration);
>
> out:
> kfree(data);
> --
> 2.20.1
>
--
Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364
Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras
Registergericht/Court of registration: Amtsgericht Charlottenburg
Registernummer/Registration number: HRB 171414 B
Ust-ID-Nummer/VAT ID number: DE302207000
^ permalink raw reply
* Re: [PATCH v2 net-next 1/3] tc-testing: Add JSON verification to tdc
From: Lucas Bates @ 2019-07-08 16:48 UTC (permalink / raw)
To: Alexander Aring
Cc: David Miller, Linux Kernel Network Developers, Jamal Hadi Salim,
Cong Wang, Jiri Pirko, Marcelo Ricardo Leitner, Vlad Buslov,
Davide Caratti, kernel
In-Reply-To: <20190704202130.tv2ivy5tjj7pjasj@x220t>
On Thu, Jul 4, 2019 at 4:21 PM Alexander Aring <aring@mojatatu.com> wrote:
> why you just use eval() as pattern matching operation and let the user
> define how to declare a matching mechanism instead you introduce another
> static matching scheme based on a json description?
>
> Whereas in eval() you could directly use the python bool expression
> parser to make whatever you want.
>
> I don't know, I see at some points you will hit limitations what you can
> express with this matchFOO and we need to introduce another matchBAR,
> whereas in providing the code it should be no problem expression
> anything. If you want smaller shortcuts writing matching patterns you
> can implement them and using in your eval() operation.
Regarding hitting limitations: quite possibly, yes.
Using eval() to provide code for matching is going to put more of a
dependency on the test writer knowing Python. I know it's not a
terribly difficult language to pick up, but it's still setting a
higher barrier to entry. This is the primary reason I scrapped the
work I had presented at Netdev 1.2 in Tokyo, where all the tests were
coded using Python's unittest framework - I want to be sure it's as
easy as possible for people to use tdc and write tests for it.
Unless I'm off-base here?
Lucas
^ permalink raw reply
* Re: [PATCH bpf-next v3 3/6] xdp: Add devmap_hash map type for looking up devices by hashed index
From: Yonghong Song @ 2019-07-08 16:47 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, Daniel Borkmann
Cc: Alexei Starovoitov, netdev@vger.kernel.org, David Miller,
Jesper Dangaard Brouer, Jakub Kicinski, Björn Töpel
In-Reply-To: <156258334732.1664.10214955962271992722.stgit@alrua-x1>
On 7/8/19 3:55 AM, Toke Høiland-Jørgensen wrote:
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> A common pattern when using xdp_redirect_map() is to create a device map
> where the lookup key is simply ifindex. Because device maps are arrays,
> this leaves holes in the map, and the map has to be sized to fit the
> largest ifindex, regardless of how many devices actually are actually
> needed in the map.
>
> This patch adds a second type of device map where the key is looked up
> using a hashmap, instead of being used as an array index. This allows maps
> to be densely packed, so they can be smaller.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
Acked-by: Yonghong Song <yhs@fb.com>
> ---
> include/linux/bpf.h | 7 ++
> include/linux/bpf_types.h | 1
> include/trace/events/xdp.h | 3 -
> include/uapi/linux/bpf.h | 1
> kernel/bpf/devmap.c | 194 ++++++++++++++++++++++++++++++++++++++++++++
> kernel/bpf/verifier.c | 2
> net/core/filter.c | 9 ++
> 7 files changed, 214 insertions(+), 3 deletions(-)
^ permalink raw reply
* Re: [PATCH bpf-next v3 4/6] tools/include/uapi: Add devmap_hash BPF map type
From: Yonghong Song @ 2019-07-08 16:47 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, Daniel Borkmann
Cc: Alexei Starovoitov, netdev@vger.kernel.org, David Miller,
Jesper Dangaard Brouer, Jakub Kicinski, Björn Töpel
In-Reply-To: <156258334740.1664.18295003114988159871.stgit@alrua-x1>
On 7/8/19 3:55 AM, Toke Høiland-Jørgensen wrote:
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> This adds the devmap_hash BPF map type to the uapi headers in tools/.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
Acked-by: Yonghong Song <yhs@fb.com>
> ---
> tools/include/uapi/linux/bpf.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index cecf42c871d4..8afaa0a19c67 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -134,6 +134,7 @@ enum bpf_map_type {
> BPF_MAP_TYPE_QUEUE,
> BPF_MAP_TYPE_STACK,
> BPF_MAP_TYPE_SK_STORAGE,
> + BPF_MAP_TYPE_DEVMAP_HASH,
> };
>
> /* Note that tracing related programs such as
>
^ permalink raw reply
* Re: [PATCH bpf-next v3 5/6] tools/libbpf_probes: Add new devmap_hash type
From: Yonghong Song @ 2019-07-08 16:48 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, Daniel Borkmann
Cc: Alexei Starovoitov, netdev@vger.kernel.org, David Miller,
Jesper Dangaard Brouer, Jakub Kicinski, Björn Töpel
In-Reply-To: <156258334745.1664.1686759894096070590.stgit@alrua-x1>
On 7/8/19 3:55 AM, Toke Høiland-Jørgensen wrote:
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> This adds the definition for BPF_MAP_TYPE_DEVMAP_HASH to libbpf_probes.c in
> tools/lib/bpf.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
Acked-by: Yonghong Song <yhs@fb.com>
> ---
> tools/lib/bpf/libbpf_probes.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
> index ace1a0708d99..4b0b0364f5fc 100644
> --- a/tools/lib/bpf/libbpf_probes.c
> +++ b/tools/lib/bpf/libbpf_probes.c
> @@ -244,6 +244,7 @@ bool bpf_probe_map_type(enum bpf_map_type map_type, __u32 ifindex)
> case BPF_MAP_TYPE_ARRAY_OF_MAPS:
> case BPF_MAP_TYPE_HASH_OF_MAPS:
> case BPF_MAP_TYPE_DEVMAP:
> + case BPF_MAP_TYPE_DEVMAP_HASH:
> case BPF_MAP_TYPE_SOCKMAP:
> case BPF_MAP_TYPE_CPUMAP:
> case BPF_MAP_TYPE_XSKMAP:
>
^ permalink raw reply
* Re: [PATCH 1/1] tools/dtrace: initial implementation of DTrace
From: Kris Van Hees @ 2019-07-08 16:48 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Kris Van Hees, netdev, bpf, dtrace-devel, linux-kernel, rostedt,
mhiramat, acme, ast, daniel, Chris Mason
In-Reply-To: <20190704130336.GN3402@hirez.programming.kicks-ass.net>
On Thu, Jul 04, 2019 at 03:03:36PM +0200, Peter Zijlstra wrote:
> On Wed, Jul 03, 2019 at 08:14:30PM -0700, Kris Van Hees wrote:
> > +/*
> > + * Read the data_head offset from the header page of the ring buffer. The
> > + * argument is declared 'volatile' because it references a memory mapped page
> > + * that the kernel may be writing to while we access it here.
> > + */
> > +static u64 read_rb_head(volatile struct perf_event_mmap_page *rb_page)
> > +{
> > + u64 head = rb_page->data_head;
> > +
> > + asm volatile("" ::: "memory");
> > +
> > + return head;
> > +}
> > +
> > +/*
> > + * Write the data_tail offset in the header page of the ring buffer. The
> > + * argument is declared 'volatile' because it references a memory mapped page
> > + * that the kernel may be writing to while we access it here.
>
> s/writing/reading/
Thanks!
> > + */
> > +static void write_rb_tail(volatile struct perf_event_mmap_page *rb_page,
> > + u64 tail)
> > +{
> > + asm volatile("" ::: "memory");
> > +
> > + rb_page->data_tail = tail;
> > +}
>
> That volatile usage is atrocious (kernel style would have you use
> {READ,WRITE}_ONCE()). Also your comments fail to mark these as
> load_acquire and store_release. And by only using a compiler barrier
> you're hard assuming TSO, which is somewhat fragile at best.
>
> Alternatively, you can use the C11 bits and write:
>
> return __atomic_load_n(&rb_page->data_head, __ATOMIC_ACQUIRE);
>
> __atomic_store_n(&rb_page->data_tail, tail, __ATOMIC_RELEASE);
Perhaps I should just use ring_buffer_read_head() and ring_buffer_write_tail()
since they are provided in tools/include/linux/ring_buffer.h? I expect that
would be even more preferable over __atomic_load_n() and __atomic_store_n()?
> > +/*
> > + * Process and output the probe data at the supplied address.
> > + */
> > +static int output_event(int cpu, u64 *buf)
> > +{
> > + u8 *data = (u8 *)buf;
> > + struct perf_event_header *hdr;
> > +
> > + hdr = (struct perf_event_header *)data;
> > + data += sizeof(struct perf_event_header);
> > +
> > + if (hdr->type == PERF_RECORD_SAMPLE) {
> > + u8 *ptr = data;
> > + u32 i, size, probe_id;
> > +
> > + /*
> > + * struct {
> > + * struct perf_event_header header;
> > + * u32 size;
> > + * u32 probe_id;
> > + * u32 gap;
> > + * u64 data[n];
> > + * }
> > + * and data points to the 'size' member at this point.
> > + */
> > + if (ptr > (u8 *)buf + hdr->size) {
> > + fprintf(stderr, "BAD: corrupted sample header\n");
> > + goto out;
> > + }
> > +
> > + size = *(u32 *)data;
> > + data += sizeof(size);
> > + ptr += sizeof(size) + size;
> > + if (ptr != (u8 *)buf + hdr->size) {
> > + fprintf(stderr, "BAD: invalid sample size\n");
> > + goto out;
> > + }
> > +
> > + probe_id = *(u32 *)data;
> > + data += sizeof(probe_id);
> > + size -= sizeof(probe_id);
> > + data += sizeof(u32); /* skip 32-bit gap */
> > + size -= sizeof(u32);
> > + buf = (u64 *)data;
> > +
> > + printf("%3d %6d ", cpu, probe_id);
> > + for (i = 0, size /= sizeof(u64); i < size; i++)
> > + printf("%#016lx ", buf[i]);
> > + printf("\n");
> > + } else if (hdr->type == PERF_RECORD_LOST) {
> > + u64 lost;
> > +
> > + /*
> > + * struct {
> > + * struct perf_event_header header;
> > + * u64 id;
> > + * u64 lost;
> > + * }
> > + * and data points to the 'id' member at this point.
> > + */
> > + lost = *(u64 *)(data + sizeof(u64));
> > +
> > + printf("[%ld probes dropped]\n", lost);
> > + } else
> > + fprintf(stderr, "UNKNOWN: record type %d\n", hdr->type);
> > +
> > +out:
> > + return hdr->size;
> > +}
>
> I see a distinct lack of wrapping support. AFAICT when buf+hdr->size
> wraps you're doing out-of-bounds accesses.
Yes, that is correct. I'm actually trying to figure out why it didn't actually
cause a SEGV when I tested this because I'm clearly reading past the end of
the mmap'd memory. Thank you for noticing this - I was trying to be too
minimal in the code I was putting out and really didn't pay attention to this.
Fixed in the V2 I am preparing.
> > +/*
> > + * Process the available probe data in the given buffer.
> > + */
> > +static void process_data(struct dtrace_buffer *buf)
> > +{
> > + /* This is volatile because the kernel may be updating the content. */
> > + volatile struct perf_event_mmap_page *rb_page = buf->base;
> > + u8 *base = (u8 *)buf->base +
> > + buf->page_size;
> > + u64 head = read_rb_head(rb_page);
> > +
> > + while (rb_page->data_tail != head) {
> > + u64 tail = rb_page->data_tail;
> > + u64 *ptr = (u64 *)(base + tail % buf->data_size);
> > + int len;
> > +
> > + len = output_event(buf->cpu, ptr);
> > +
> > + write_rb_tail(rb_page, tail + len);
> > + head = read_rb_head(rb_page);
> > + }
> > +}
>
> more volatile yuck.
>
> Also:
>
> for (;;) {
> head = __atomic_load_n(&rb_page->data_head, __ATOMIC_ACQUIRE);
> tail = __atomic_load_n(&rb_page->data_tail, __ATOMIC_RELAXED);
>
> if (head == tail)
> break;
>
> do {
> hdr = buf->base + (tail & ((1UL << buf->data_shift) - 1));
> if ((tail >> buf->data_shift) !=
> ((tail + hdr->size) >> buf->data_shift))
> /* handle wrap case */
> else
> /* normal case */
>
> tail += hdr->size;
> } while (tail != head);
>
> __atomic_store_n(&rb_page->data_tail, tail, __ATOMIC_RELEASE);
> }
>
> Or something.
Thank you for this suggestion. As mentioned above, I lean towards using the
provided ring_buffer_(read_head,write_tail) implementations since that is the
'other end' of the ring buffer head/tail mechanism that is going to be kept
in sync with any changes that might happen on the kernel side, right?
> > +/*
> > + * Wait for data to become available in any of the buffers.
> > + */
> > +int dt_buffer_poll(int epoll_fd, int timeout)
> > +{
> > + struct epoll_event events[dt_numcpus];
> > + int i, cnt;
> > +
> > + cnt = epoll_wait(epoll_fd, events, dt_numcpus, timeout);
> > + if (cnt < 0)
> > + return -errno;
> > +
> > + for (i = 0; i < cnt; i++)
> > + process_data((struct dtrace_buffer *)events[i].data.ptr);
> > +
> > + return cnt;
> > +}
>
> Or make sure to read on the CPU by having a poll thread per CPU, then
> you can do away with the memory barriers.
That is definitely something for the todo list for future optimizations.
Thanks for your review and code suggestions.
Kris
^ permalink raw reply
* Re: [PATCH bpf-next 0/3] xdp: Add devmap_hash map type
From: Jonathan Lemon @ 2019-07-08 16:50 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Daniel Borkmann, Alexei Starovoitov, netdev, David Miller,
Jesper Dangaard Brouer, Jakub Kicinski, Björn Töpel
In-Reply-To: <87bly4zg8n.fsf@toke.dk>
On 8 Jul 2019, at 8:40, Toke Høiland-Jørgensen wrote:
> "Jonathan Lemon" <jlemon@flugsvamp.com> writes:
>
>> On 5 Jul 2019, at 10:56, Toke Høiland-Jørgensen wrote:
>>
>>> This series adds a new map type, devmap_hash, that works like the
>>> existing
>>> devmap type, but using a hash-based indexing scheme. This is useful
>>> for the use
>>> case where a devmap is indexed by ifindex (for instance for use with
>>> the routing
>>> table lookup helper). For this use case, the regular devmap needs to
>>> be sized
>>> after the maximum ifindex number, not the number of devices in it. A
>>> hash-based
>>> indexing scheme makes it possible to size the map after the number of
>>> devices it
>>> should contain instead.
>>
>> This device hash map is sized at NETDEV_HASHENTRIES == 2^8 == 256. Is
>> this actually smaller than an array? What ifindex values are you
>> seeing?
>
> Well, not in all cases, certainly. But machines with lots of virtual
> interfaces (e.g., container hosts) can easily exceed that. Also, for a
> devmap we charge the full size of max_entries * struct bpf_dtab_netdev
> towards the locked memory cost on map creation. And since sizeof(struct
> bpf_dtab_netdev) is 64, the size of the hashmap only corresponds to 32
> entries...
>
> But more importantly, it's a UI issue: Say you want to create a simple
> program that uses the fib_lookup helper (something like the xdp_fwd
> example under samples/bpf/). You know that you only want to route
> between a couple of interfaces, so you naturally create a devmap that
> can hold, say, 8 entries (just to be sure). This works fine on your
> initial test, where the machine only has a couple of physical interfaces
> brought up at boot. But then you try to run the same program on your
> production server, where the interfaces you need to use just happen to
> have ifindexes higher than 8, and now it breaks for no discernible
> reason. Or even worse, if you remove and re-add an interface, you may no
> longer be able to insert it into your map because the ifindex changed...
Thanks for the explanation, that makes sense.
--
Jonathan
^ permalink raw reply
* Re: [bpf-next v2 08/10] bpf: Implement bpf_prog_test_run for perf event programs
From: Krzesimir Nowak @ 2019-07-08 16:51 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: netdev, Alban Crequy, Iago López Galeiras,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, linux-kernel, bpf
In-Reply-To: <20190626161231.GA4866@mini-arch>
On Wed, Jun 26, 2019 at 6:12 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> On 06/26, Krzesimir Nowak wrote:
> > On Tue, Jun 25, 2019 at 10:12 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > On 06/25, Krzesimir Nowak wrote:
> > > > As an input, test run for perf event program takes struct
> > > > bpf_perf_event_data as ctx_in and struct bpf_perf_event_value as
> > > > data_in. For an output, it basically ignores ctx_out and data_out.
> > > >
> > > > The implementation sets an instance of struct bpf_perf_event_data_kern
> > > > in such a way that the BPF program reading data from context will
> > > > receive what we passed to the bpf prog test run in ctx_in. Also BPF
> > > > program can call bpf_perf_prog_read_value to receive what was passed
> > > > in data_in.
> > > >
> > > > Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> > > > ---
> > > > kernel/trace/bpf_trace.c | 107 ++++++++++++++++++
> > > > .../bpf/verifier/perf_event_sample_period.c | 8 ++
> > > > 2 files changed, 115 insertions(+)
> > > >
> > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > > index c102c240bb0b..2fa49ea8a475 100644
> > > > --- a/kernel/trace/bpf_trace.c
> > > > +++ b/kernel/trace/bpf_trace.c
> > > > @@ -16,6 +16,8 @@
> > > >
> > > > #include <asm/tlb.h>
> > > >
> > > > +#include <trace/events/bpf_test_run.h>
> > > > +
> > > > #include "trace_probe.h"
> > > > #include "trace.h"
> > > >
> > > > @@ -1160,7 +1162,112 @@ const struct bpf_verifier_ops perf_event_verifier_ops = {
> > > > .convert_ctx_access = pe_prog_convert_ctx_access,
> > > > };
> > > >
> > > > +static int pe_prog_test_run(struct bpf_prog *prog,
> > > > + const union bpf_attr *kattr,
> > > > + union bpf_attr __user *uattr)
> > > > +{
> > > > + void __user *ctx_in = u64_to_user_ptr(kattr->test.ctx_in);
> > > > + void __user *data_in = u64_to_user_ptr(kattr->test.data_in);
> > > > + u32 data_size_in = kattr->test.data_size_in;
> > > > + u32 ctx_size_in = kattr->test.ctx_size_in;
> > > > + u32 repeat = kattr->test.repeat;
> > > > + u32 retval = 0, duration = 0;
> > > > + int err = -EINVAL;
> > > > + u64 time_start, time_spent = 0;
> > > > + int i;
> > > > + struct perf_sample_data sample_data = {0, };
> > > > + struct perf_event event = {0, };
> > > > + struct bpf_perf_event_data_kern real_ctx = {0, };
> > > > + struct bpf_perf_event_data fake_ctx = {0, };
> > > > + struct bpf_perf_event_value value = {0, };
> > > > +
> > > > + if (ctx_size_in != sizeof(fake_ctx))
> > > > + goto out;
> > > > + if (data_size_in != sizeof(value))
> > > > + goto out;
> > > > +
> > > > + if (copy_from_user(&fake_ctx, ctx_in, ctx_size_in)) {
> > > > + err = -EFAULT;
> > > > + goto out;
> > > > + }
> > > Move this to net/bpf/test_run.c? I have a bpf_ctx_init helper to deal
> > > with ctx input, might save you some code above wrt ctx size/etc.
> >
> > My impression about net/bpf/test_run.c was that it was a collection of
> > helpers for test runs of the network-related BPF programs, because
> > they are so similar to each other. So kernel/trace/bpf_trace.c looked
> > like an obvious place for the test_run implementation since other perf
> > trace BPF stuff was already there.
> Maybe net/bpf/test_run.c should be renamed to kernel/bpf/test_run.c?
Just sent another version of this patch series. I went with slightly
different approach - moved some functions to kernel/bpf/test_run.c and
left the network specific stuff in net/bpf/test_run.c.
>
> > And about bpf_ctx_init - looks useful as it seems to me that it
> > handles the scenario where the size of the ctx struct grows, but still
> > allows passing older version of the struct (thus smaller) from
> > userspace for compatibility. Maybe that checking and copying part of
> > the function could be moved into some non-static helper function, so I
> > could use it and still skip the need for allocating memory for the
> > context?
> You can always make bpf_ctx_init non-static and export it.
> But, again, consider adding your stuff to the net/bpf/test_run.c
> and exporting only pe_prog_test_run. That way you can reuse
> bpf_ctx_init and bpf_test_run.
>
> Why do you care about memory allocation though? It's a one time
> operation and doesn't affect the performance measurements.
>
> > > > + if (copy_from_user(&value, data_in, data_size_in)) {
> > > > + err = -EFAULT;
> > > > + goto out;
> > > > + }
> > > > +
> > > > + real_ctx.regs = &fake_ctx.regs;
> > > > + real_ctx.data = &sample_data;
> > > > + real_ctx.event = &event;
> > > > + perf_sample_data_init(&sample_data, fake_ctx.addr,
> > > > + fake_ctx.sample_period);
> > > > + event.cpu = smp_processor_id();
> > > > + event.oncpu = -1;
> > > > + event.state = PERF_EVENT_STATE_OFF;
> > > > + local64_set(&event.count, value.counter);
> > > > + event.total_time_enabled = value.enabled;
> > > > + event.total_time_running = value.running;
> > > > + /* make self as a leader - it is used only for checking the
> > > > + * state field
> > > > + */
> > > > + event.group_leader = &event;
> > > > +
> > > > + /* slightly changed copy pasta from bpf_test_run() in
> > > > + * net/bpf/test_run.c
> > > > + */
> > > > + if (!repeat)
> > > > + repeat = 1;
> > > > +
> > > > + rcu_read_lock();
> > > > + preempt_disable();
> > > > + time_start = ktime_get_ns();
> > > > + for (i = 0; i < repeat; i++) {
> > > Any reason for not using bpf_test_run?
> >
> > Two, mostly. One was that it is a static function and my code was
> > elsewhere. Second was that it does some cgroup storage setup and I'm
> > not sure if the perf event BPF program needs that.
> You can always make it non-static.
>
> Regarding cgroup storage: do we care? If you can see it affecting
> your performance numbers, then yes, but you can try to measure to see
> if it gives you any noticeable overhead. Maybe add an argument to
> bpf_test_run to skip cgroup storage stuff?
>
> > > > + retval = BPF_PROG_RUN(prog, &real_ctx);
> > > > +
> > > > + if (signal_pending(current)) {
> > > > + err = -EINTR;
> > > > + preempt_enable();
> > > > + rcu_read_unlock();
> > > > + goto out;
> > > > + }
> > > > +
> > > > + if (need_resched()) {
> > > > + time_spent += ktime_get_ns() - time_start;
> > > > + preempt_enable();
> > > > + rcu_read_unlock();
> > > > +
> > > > + cond_resched();
> > > > +
> > > > + rcu_read_lock();
> > > > + preempt_disable();
> > > > + time_start = ktime_get_ns();
> > > > + }
> > > > + }
> > > > + time_spent += ktime_get_ns() - time_start;
> > > > + preempt_enable();
> > > > + rcu_read_unlock();
> > > > +
> > > > + do_div(time_spent, repeat);
> > > > + duration = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
> > > > + /* end of slightly changed copy pasta from bpf_test_run() in
> > > > + * net/bpf/test_run.c
> > > > + */
> > > > +
> > > > + if (copy_to_user(&uattr->test.retval, &retval, sizeof(retval))) {
> > > > + err = -EFAULT;
> > > > + goto out;
> > > > + }
> > > > + if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration))) {
> > > > + err = -EFAULT;
> > > > + goto out;
> > > > + }
> > > Can BPF program modify fake_ctx? Do we need/want to copy it back?
> >
> > Reading the pe_prog_is_valid_access function tells me that it's not
> > possible - the only type of valid access is read. So maybe I should be
> > stricter about the requirements for the data_out and ctx_out sizes
> > (should be zero or return -EINVAL).
> Yes, better to explicitly prohibit anything that we don't support.
>
> > > > + err = 0;
> > > > +out:
> > > > + trace_bpf_test_finish(&err);
> > > > + return err;
> > > > +}
> > > > +
> > > > const struct bpf_prog_ops perf_event_prog_ops = {
> > > > + .test_run = pe_prog_test_run,
> > > > };
> > > >
> > > > static DEFINE_MUTEX(bpf_event_mutex);
> > > > diff --git a/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c b/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c
> > > > index 471c1a5950d8..16e9e5824d14 100644
> > > > --- a/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c
> > > > +++ b/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c
> > > This should probably go in another patch.
> >
> > Yeah, I was wondering about it. These changes are here to avoid
> > breaking the tests, since perf event program can actually be run now
> > and the test_run for perf event required certain sizes for ctx and
> > data.
> You need to make sure the context is optional, that way you don't break
> any existing tests out in the wild and can move those changes to
> another patch.
>
> > So, I will either move them to a separate patch or rework the test_run
> > for perf event to accept the size between 0 and sizeof(struct
> > something), so the changes in tests maybe will not be necessary.
> >
> > >
> > > > @@ -13,6 +13,8 @@
> > > > },
> > > > .result = ACCEPT,
> > > > .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> > > > + .ctx_len = sizeof(struct bpf_perf_event_data),
> > > > + .data_len = sizeof(struct bpf_perf_event_value),
> > > > },
> > > > {
> > > > "check bpf_perf_event_data->sample_period half load permitted",
> > > > @@ -29,6 +31,8 @@
> > > > },
> > > > .result = ACCEPT,
> > > > .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> > > > + .ctx_len = sizeof(struct bpf_perf_event_data),
> > > > + .data_len = sizeof(struct bpf_perf_event_value),
> > > > },
> > > > {
> > > > "check bpf_perf_event_data->sample_period word load permitted",
> > > > @@ -45,6 +49,8 @@
> > > > },
> > > > .result = ACCEPT,
> > > > .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> > > > + .ctx_len = sizeof(struct bpf_perf_event_data),
> > > > + .data_len = sizeof(struct bpf_perf_event_value),
> > > > },
> > > > {
> > > > "check bpf_perf_event_data->sample_period dword load permitted",
> > > > @@ -56,4 +62,6 @@
> > > > },
> > > > .result = ACCEPT,
> > > > .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> > > > + .ctx_len = sizeof(struct bpf_perf_event_data),
> > > > + .data_len = sizeof(struct bpf_perf_event_value),
> > > > },
> > > > --
> > > > 2.20.1
> > > >
> >
> >
> >
> > --
> > Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364
> > Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras
> > Registergericht/Court of registration: Amtsgericht Charlottenburg
> > Registernummer/Registration number: HRB 171414 B
> > Ust-ID-Nummer/VAT ID number: DE302207000
--
Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364
Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras
Registergericht/Court of registration: Amtsgericht Charlottenburg
Registernummer/Registration number: HRB 171414 B
Ust-ID-Nummer/VAT ID number: DE302207000
^ permalink raw reply
* [PATCH net-next 0/4] sctp: tidy up some ep and asoc feature flags
From: Xin Long @ 2019-07-08 16:57 UTC (permalink / raw)
To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem
This patchset is to remove some unnecessary feature flags from
sctp_assocation and move some others to the right places.
Xin Long (4):
sctp: remove reconf_enable from asoc
sctp: remove prsctp_enable from asoc
sctp: rename asoc intl_enable to asoc peer.intl_capable
sctp: rename sp strm_interleave to ep intl_enable
include/net/sctp/structs.h | 37 ++++++++++++++++++-------------------
net/sctp/associola.c | 2 --
net/sctp/sm_make_chunk.c | 21 ++++++++++-----------
net/sctp/socket.c | 19 ++++++++-----------
net/sctp/stream_interleave.c | 4 ++--
net/sctp/stream_sched.c | 2 +-
6 files changed, 39 insertions(+), 46 deletions(-)
--
2.1.0
^ permalink raw reply
* [PATCH net-next 1/4] sctp: remove reconf_enable from asoc
From: Xin Long @ 2019-07-08 16:57 UTC (permalink / raw)
To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem
In-Reply-To: <cover.1562604972.git.lucien.xin@gmail.com>
asoc's reconf support is actually decided by the 4-shakehand negotiation,
not something that users can set by sockopt. asoc->peer.reconf_capable is
working for this. So remove it from asoc.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
include/net/sctp/structs.h | 3 +--
net/sctp/associola.c | 1 -
net/sctp/sm_make_chunk.c | 5 ++---
net/sctp/socket.c | 7 ++-----
4 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 0767701..d9e0e1a 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -2051,8 +2051,7 @@ struct sctp_association {
temp:1, /* Is it a temporary association? */
force_delay:1,
intl_enable:1,
- prsctp_enable:1,
- reconf_enable:1;
+ prsctp_enable:1;
__u8 strreset_enable;
__u8 strreset_outstanding; /* request param count on the fly */
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 1999237..321c199 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -262,7 +262,6 @@ static struct sctp_association *sctp_association_init(
asoc->active_key_id = ep->active_key_id;
asoc->prsctp_enable = ep->prsctp_enable;
- asoc->reconf_enable = ep->reconf_enable;
asoc->strreset_enable = ep->strreset_enable;
/* Save the hmacs and chunks list into this association */
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 9b0e5b0..d784dc1 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -261,7 +261,7 @@ struct sctp_chunk *sctp_make_init(const struct sctp_association *asoc,
num_ext += 2;
}
- if (asoc->reconf_enable) {
+ if (asoc->ep->reconf_enable) {
extensions[num_ext] = SCTP_CID_RECONF;
num_ext += 1;
}
@@ -2007,8 +2007,7 @@ static void sctp_process_ext_param(struct sctp_association *asoc,
for (i = 0; i < num_ext; i++) {
switch (param.ext->chunks[i]) {
case SCTP_CID_RECONF:
- if (asoc->reconf_enable &&
- !asoc->peer.reconf_capable)
+ if (asoc->ep->reconf_enable)
asoc->peer.reconf_capable = 1;
break;
case SCTP_CID_FWD_TSN:
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 39ea0a3..0424876 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -4226,10 +4226,7 @@ static int sctp_setsockopt_reconfig_supported(struct sock *sk,
sctp_style(sk, UDP))
goto out;
- if (asoc)
- asoc->reconf_enable = !!params.assoc_value;
- else
- sctp_sk(sk)->ep->reconf_enable = !!params.assoc_value;
+ sctp_sk(sk)->ep->reconf_enable = !!params.assoc_value;
retval = 0;
@@ -7554,7 +7551,7 @@ static int sctp_getsockopt_reconfig_supported(struct sock *sk, int len,
goto out;
}
- params.assoc_value = asoc ? asoc->reconf_enable
+ params.assoc_value = asoc ? asoc->peer.reconf_capable
: sctp_sk(sk)->ep->reconf_enable;
if (put_user(len, optlen))
--
2.1.0
^ permalink raw reply related
* [PATCH net-next 2/4] sctp: remove prsctp_enable from asoc
From: Xin Long @ 2019-07-08 16:57 UTC (permalink / raw)
To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem
In-Reply-To: <cover.1562604972.git.lucien.xin@gmail.com>
Like reconf_enable, prsctp_enable should also be removed from asoc,
as asoc->peer.prsctp_capable has taken its job.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
include/net/sctp/structs.h | 3 +--
net/sctp/associola.c | 1 -
net/sctp/sm_make_chunk.c | 8 ++++----
net/sctp/socket.c | 2 +-
4 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index d9e0e1a..7f35b8e 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -2050,8 +2050,7 @@ struct sctp_association {
__u8 need_ecne:1, /* Need to send an ECNE Chunk? */
temp:1, /* Is it a temporary association? */
force_delay:1,
- intl_enable:1,
- prsctp_enable:1;
+ intl_enable:1;
__u8 strreset_enable;
__u8 strreset_outstanding; /* request param count on the fly */
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 321c199..5010cce 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -261,7 +261,6 @@ static struct sctp_association *sctp_association_init(
goto stream_free;
asoc->active_key_id = ep->active_key_id;
- asoc->prsctp_enable = ep->prsctp_enable;
asoc->strreset_enable = ep->strreset_enable;
/* Save the hmacs and chunks list into this association */
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index d784dc1..227bbac 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -247,7 +247,7 @@ struct sctp_chunk *sctp_make_init(const struct sctp_association *asoc,
chunksize += SCTP_PAD4(SCTP_SAT_LEN(num_types));
chunksize += sizeof(ecap_param);
- if (asoc->prsctp_enable)
+ if (asoc->ep->prsctp_enable)
chunksize += sizeof(prsctp_param);
/* ADDIP: Section 4.2.7:
@@ -348,7 +348,7 @@ struct sctp_chunk *sctp_make_init(const struct sctp_association *asoc,
sctp_addto_param(retval, num_ext, extensions);
}
- if (asoc->prsctp_enable)
+ if (asoc->ep->prsctp_enable)
sctp_addto_chunk(retval, sizeof(prsctp_param), &prsctp_param);
if (sp->adaptation_ind) {
@@ -2011,7 +2011,7 @@ static void sctp_process_ext_param(struct sctp_association *asoc,
asoc->peer.reconf_capable = 1;
break;
case SCTP_CID_FWD_TSN:
- if (asoc->prsctp_enable && !asoc->peer.prsctp_capable)
+ if (asoc->ep->prsctp_enable)
asoc->peer.prsctp_capable = 1;
break;
case SCTP_CID_AUTH:
@@ -2636,7 +2636,7 @@ static int sctp_process_param(struct sctp_association *asoc,
break;
case SCTP_PARAM_FWD_TSN_SUPPORT:
- if (asoc->prsctp_enable) {
+ if (asoc->ep->prsctp_enable) {
asoc->peer.prsctp_capable = 1;
break;
}
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 0424876..da2a3c2 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -7343,7 +7343,7 @@ static int sctp_getsockopt_pr_supported(struct sock *sk, int len,
goto out;
}
- params.assoc_value = asoc ? asoc->prsctp_enable
+ params.assoc_value = asoc ? asoc->peer.prsctp_capable
: sctp_sk(sk)->ep->prsctp_enable;
if (put_user(len, optlen))
--
2.1.0
^ permalink raw reply related
* [PATCH net-next 3/4] sctp: rename asoc intl_enable to asoc peer.intl_capable
From: Xin Long @ 2019-07-08 16:57 UTC (permalink / raw)
To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem
In-Reply-To: <cover.1562604972.git.lucien.xin@gmail.com>
To keep consistent with other asoc features, we move intl_enable
to peer.intl_capable in asoc.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
include/net/sctp/structs.h | 33 +++++++++++++++++----------------
net/sctp/sm_make_chunk.c | 4 ++--
net/sctp/socket.c | 2 +-
net/sctp/stream_interleave.c | 4 ++--
net/sctp/stream_sched.c | 2 +-
5 files changed, 23 insertions(+), 22 deletions(-)
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 7f35b8e..c41b57b 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1679,28 +1679,30 @@ struct sctp_association {
__be16 addip_disabled_mask;
/* These are capabilities which our peer advertised. */
- __u8 ecn_capable:1, /* Can peer do ECN? */
+ __u16 ecn_capable:1, /* Can peer do ECN? */
ipv4_address:1, /* Peer understands IPv4 addresses? */
ipv6_address:1, /* Peer understands IPv6 addresses? */
hostname_address:1, /* Peer understands DNS addresses? */
asconf_capable:1, /* Does peer support ADDIP? */
prsctp_capable:1, /* Can peer do PR-SCTP? */
reconf_capable:1, /* Can peer do RE-CONFIG? */
- auth_capable:1; /* Is peer doing SCTP-AUTH? */
-
- /* sack_needed : This flag indicates if the next received
- * : packet is to be responded to with a
- * : SACK. This is initialized to 0. When a packet
- * : is received sack_cnt is incremented. If this value
- * : reaches 2 or more, a SACK is sent and the
- * : value is reset to 0. Note: This is used only
- * : when no DATA chunks are received out of
- * : order. When DATA chunks are out of order,
- * : SACK's are not delayed (see Section 6).
- */
- __u8 sack_needed:1, /* Do we need to sack the peer? */
+ intl_capable:1, /* Can peer do INTERLEAVE */
+ auth_capable:1, /* Is peer doing SCTP-AUTH? */
+ /* sack_needed:
+ * This flag indicates if the next received
+ * packet is to be responded to with a
+ * SACK. This is initialized to 0. When a packet
+ * is received sack_cnt is incremented. If this value
+ * reaches 2 or more, a SACK is sent and the
+ * value is reset to 0. Note: This is used only
+ * when no DATA chunks are received out of
+ * order. When DATA chunks are out of order,
+ * SACK's are not delayed (see Section 6).
+ */
+ sack_needed:1, /* Do we need to sack the peer? */
sack_generation:1,
zero_window_announced:1;
+
__u32 sack_cnt;
__u32 adaptation_ind; /* Adaptation Code point. */
@@ -2049,8 +2051,7 @@ struct sctp_association {
__u8 need_ecne:1, /* Need to send an ECNE Chunk? */
temp:1, /* Is it a temporary association? */
- force_delay:1,
- intl_enable:1;
+ force_delay:1;
__u8 strreset_enable;
__u8 strreset_outstanding; /* request param count on the fly */
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 227bbac..31ab2c6 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -438,7 +438,7 @@ struct sctp_chunk *sctp_make_init_ack(const struct sctp_association *asoc,
if (sp->adaptation_ind)
chunksize += sizeof(aiparam);
- if (asoc->intl_enable) {
+ if (asoc->peer.intl_capable) {
extensions[num_ext] = SCTP_CID_I_DATA;
num_ext += 1;
}
@@ -2028,7 +2028,7 @@ static void sctp_process_ext_param(struct sctp_association *asoc,
break;
case SCTP_CID_I_DATA:
if (sctp_sk(asoc->base.sk)->strm_interleave)
- asoc->intl_enable = 1;
+ asoc->peer.intl_capable = 1;
break;
default:
break;
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index da2a3c2..b679b61 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -7710,7 +7710,7 @@ static int sctp_getsockopt_interleaving_supported(struct sock *sk, int len,
goto out;
}
- params.assoc_value = asoc ? asoc->intl_enable
+ params.assoc_value = asoc ? asoc->peer.intl_capable
: sctp_sk(sk)->strm_interleave;
if (put_user(len, optlen))
diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c
index afbf122..40c40be 100644
--- a/net/sctp/stream_interleave.c
+++ b/net/sctp/stream_interleave.c
@@ -1358,6 +1358,6 @@ void sctp_stream_interleave_init(struct sctp_stream *stream)
struct sctp_association *asoc;
asoc = container_of(stream, struct sctp_association, stream);
- stream->si = asoc->intl_enable ? &sctp_stream_interleave_1
- : &sctp_stream_interleave_0;
+ stream->si = asoc->peer.intl_capable ? &sctp_stream_interleave_1
+ : &sctp_stream_interleave_0;
}
diff --git a/net/sctp/stream_sched.c b/net/sctp/stream_sched.c
index b8fa7ab..99e5f69 100644
--- a/net/sctp/stream_sched.c
+++ b/net/sctp/stream_sched.c
@@ -228,7 +228,7 @@ int sctp_sched_get_value(struct sctp_association *asoc, __u16 sid,
void sctp_sched_dequeue_done(struct sctp_outq *q, struct sctp_chunk *ch)
{
if (!list_is_last(&ch->frag_list, &ch->msg->chunks) &&
- !q->asoc->intl_enable) {
+ !q->asoc->peer.intl_capable) {
struct sctp_stream_out *sout;
__u16 sid;
--
2.1.0
^ permalink raw reply related
* [PATCH net-next 4/4] sctp: rename sp strm_interleave to ep intl_enable
From: Xin Long @ 2019-07-08 16:57 UTC (permalink / raw)
To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem
In-Reply-To: <cover.1562604972.git.lucien.xin@gmail.com>
Like other endpoint features, strm_interleave should be moved to
sctp_endpoint and renamed to intl_enable.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
include/net/sctp/structs.h | 2 +-
net/sctp/sm_make_chunk.c | 4 ++--
net/sctp/socket.c | 8 ++++----
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index c41b57b..ba5c4f6 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -219,7 +219,6 @@ struct sctp_sock {
disable_fragments:1,
v4mapped:1,
frag_interleave:1,
- strm_interleave:1,
recvrcvinfo:1,
recvnxtinfo:1,
data_ready_signalled:1;
@@ -1324,6 +1323,7 @@ struct sctp_endpoint {
struct list_head endpoint_shared_keys;
__u16 active_key_id;
__u8 auth_enable:1,
+ intl_enable:1,
prsctp_enable:1,
reconf_enable:1;
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 31ab2c6..ed39396 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -269,7 +269,7 @@ struct sctp_chunk *sctp_make_init(const struct sctp_association *asoc,
if (sp->adaptation_ind)
chunksize += sizeof(aiparam);
- if (sp->strm_interleave) {
+ if (asoc->ep->intl_enable) {
extensions[num_ext] = SCTP_CID_I_DATA;
num_ext += 1;
}
@@ -2027,7 +2027,7 @@ static void sctp_process_ext_param(struct sctp_association *asoc,
asoc->peer.asconf_capable = 1;
break;
case SCTP_CID_I_DATA:
- if (sctp_sk(asoc->base.sk)->strm_interleave)
+ if (asoc->ep->intl_enable)
asoc->peer.intl_capable = 1;
break;
default:
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index b679b61..0fc5b90 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1913,7 +1913,7 @@ static int sctp_sendmsg_to_asoc(struct sctp_association *asoc,
if (err)
goto err;
- if (sp->strm_interleave) {
+ if (asoc->ep->intl_enable) {
timeo = sock_sndtimeo(sk, 0);
err = sctp_wait_for_connect(asoc, &timeo);
if (err) {
@@ -3581,7 +3581,7 @@ static int sctp_setsockopt_fragment_interleave(struct sock *sk,
sctp_sk(sk)->frag_interleave = !!val;
if (!sctp_sk(sk)->frag_interleave)
- sctp_sk(sk)->strm_interleave = 0;
+ sctp_sk(sk)->ep->intl_enable = 0;
return 0;
}
@@ -4484,7 +4484,7 @@ static int sctp_setsockopt_interleaving_supported(struct sock *sk,
goto out;
}
- sp->strm_interleave = !!params.assoc_value;
+ sp->ep->intl_enable = !!params.assoc_value;
retval = 0;
@@ -7711,7 +7711,7 @@ static int sctp_getsockopt_interleaving_supported(struct sock *sk, int len,
}
params.assoc_value = asoc ? asoc->peer.intl_capable
- : sctp_sk(sk)->strm_interleave;
+ : sctp_sk(sk)->ep->intl_enable;
if (put_user(len, optlen))
goto out;
--
2.1.0
^ permalink raw reply related
* Re: [PATCH rdma-next 0/2] DEVX VHCA tunnel support
From: Jason Gunthorpe @ 2019-07-08 16:58 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Max Gurtovoy,
Yishai Hadas, Saeed Mahameed, linux-netdev
In-Reply-To: <20190701181402.25286-1-leon@kernel.org>
On Mon, Jul 01, 2019 at 09:14:00PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
>
> Hi,
>
> Those two patches introduce VHCA tunnel mechanism to DEVX interface
> needed for Bluefield SOC. See extensive commit messages for more
> information.
>
> Thanks
>
> Max Gurtovoy (2):
> net/mlx5: Introduce VHCA tunnel device capability
> IB/mlx5: Implement VHCA tunnel mechanism in DEVX
Thanks, applied to for-next
Jason
^ permalink raw reply
* [PATCH net-next] sctp: remove rcu_read_lock from sctp_bind_addr_state
From: Xin Long @ 2019-07-08 16:59 UTC (permalink / raw)
To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem
sctp_bind_addr_state() is called either in packet rcv path or
by sctp_copy_local_addr_list(), which are under rcu_read_lock.
So there's no need to call it again in sctp_bind_addr_state().
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/sctp/bind_addr.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index f54333c..53bc615 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -393,24 +393,19 @@ int sctp_bind_addr_state(const struct sctp_bind_addr *bp,
{
struct sctp_sockaddr_entry *laddr;
struct sctp_af *af;
- int state = -1;
af = sctp_get_af_specific(addr->sa.sa_family);
if (unlikely(!af))
- return state;
+ return -1;
- rcu_read_lock();
list_for_each_entry_rcu(laddr, &bp->address_list, list) {
if (!laddr->valid)
continue;
- if (af->cmp_addr(&laddr->a, addr)) {
- state = laddr->state;
- break;
- }
+ if (af->cmp_addr(&laddr->a, addr))
+ return laddr->state;
}
- rcu_read_unlock();
- return state;
+ return -1;
}
/* Find the first address in the bind address list that is not present in
--
2.1.0
^ permalink raw reply related
* Re: [PATCH 1/1] tools/dtrace: initial implementation of DTrace
From: Arnaldo Carvalho de Melo @ 2019-07-08 17:15 UTC (permalink / raw)
To: Kris Van Hees
Cc: netdev, bpf, dtrace-devel, linux-kernel, rostedt, mhiramat, ast,
daniel, Peter Zijlstra, Chris Mason
In-Reply-To: <201907040314.x643EUoA017906@aserv0122.oracle.com>
Em Wed, Jul 03, 2019 at 08:14:30PM -0700, Kris Van Hees escreveu:
> This initial implementation of a tiny subset of DTrace functionality
> provides the following options:
>
> dtrace [-lvV] [-b bufsz] -s script
> -b set trace buffer size
> -l list probes (only works with '-s script' for now)
> -s enable or list probes for the specified BPF program
> -V report DTrace API version
>
> The patch comprises quite a bit of code due to DTrace requiring a few
> crucial components, even in its most basic form.
>
> The code is structured around the command line interface implemented in
> dtrace.c. It provides option parsing and drives the three modes of
> operation that are currently implemented:
>
> 1. Report DTrace API version information.
> Report the version information and terminate.
>
> 2. List probes in BPF programs.
> Initialize the list of probes that DTrace recognizes, load BPF
> programs, parse all BPF ELF section names, resolve them into
> known probes, and emit the probe names. Then terminate.
>
> 3. Load BPF programs and collect tracing data.
> Initialize the list of probes that DTrace recognizes, load BPF
> programs and attach them to their corresponding probes, set up
> perf event output buffers, and start processing tracing data.
>
> This implementation makes extensive use of BPF (handled by dt_bpf.c) and
> the perf event output ring buffer (handled by dt_buffer.c). DTrace-style
> probe handling (dt_probe.c) offers an interface to probes that hides the
> implementation details of the individual probe types by provider (dt_fbt.c
> and dt_syscall.c). Probe lookup by name uses a hashtable implementation
> (dt_hash.c). The dt_utils.c code populates a list of online CPU ids, so
> we know what CPUs we can obtain tracing data from.
>
> Building the tool is trivial because its only dependency (libbpf) is in
> the kernel tree under tools/lib/bpf. A simple 'make' in the tools/dtrace
> directory suffices.
>
> The 'dtrace' executable needs to run as root because BPF programs cannot
> be loaded by non-root users.
>
> Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> Reviewed-by: David Mc Lean <david.mclean@oracle.com>
> Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
> ---
> MAINTAINERS | 6 +
> tools/dtrace/Makefile | 88 ++++++++++
> tools/dtrace/bpf_sample.c | 145 ++++++++++++++++
> tools/dtrace/dt_bpf.c | 188 +++++++++++++++++++++
> tools/dtrace/dt_buffer.c | 331 +++++++++++++++++++++++++++++++++++++
> tools/dtrace/dt_fbt.c | 201 ++++++++++++++++++++++
> tools/dtrace/dt_hash.c | 211 +++++++++++++++++++++++
> tools/dtrace/dt_probe.c | 230 ++++++++++++++++++++++++++
> tools/dtrace/dt_syscall.c | 179 ++++++++++++++++++++
> tools/dtrace/dt_utils.c | 132 +++++++++++++++
> tools/dtrace/dtrace.c | 249 ++++++++++++++++++++++++++++
> tools/dtrace/dtrace.h | 13 ++
> tools/dtrace/dtrace_impl.h | 101 +++++++++++
> 13 files changed, 2074 insertions(+)
> create mode 100644 tools/dtrace/Makefile
> create mode 100644 tools/dtrace/bpf_sample.c
> create mode 100644 tools/dtrace/dt_bpf.c
> create mode 100644 tools/dtrace/dt_buffer.c
> create mode 100644 tools/dtrace/dt_fbt.c
> create mode 100644 tools/dtrace/dt_hash.c
> create mode 100644 tools/dtrace/dt_probe.c
> create mode 100644 tools/dtrace/dt_syscall.c
> create mode 100644 tools/dtrace/dt_utils.c
> create mode 100644 tools/dtrace/dtrace.c
> create mode 100644 tools/dtrace/dtrace.h
> create mode 100644 tools/dtrace/dtrace_impl.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 606d1f80bc49..668468834865 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5474,6 +5474,12 @@ W: https://linuxtv.org
> S: Odd Fixes
> F: drivers/media/pci/dt3155/
>
> +DTRACE
> +M: Kris Van Hees <kris.van.hees@oracle.com>
> +L: dtrace-devel@oss.oracle.com
> +S: Maintained
> +F: tools/dtrace/
> +
> DVB_USB_AF9015 MEDIA DRIVER
> M: Antti Palosaari <crope@iki.fi>
> L: linux-media@vger.kernel.org
> diff --git a/tools/dtrace/Makefile b/tools/dtrace/Makefile
> new file mode 100644
> index 000000000000..99fd0f9dd1d6
> --- /dev/null
> +++ b/tools/dtrace/Makefile
> @@ -0,0 +1,88 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# This Makefile is based on samples/bpf.
> +#
> +# Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
> +
> +DT_VERSION := 2.0.0
> +DT_GIT_VERSION := $(shell git rev-parse HEAD 2>/dev/null || \
> + echo Unknown)
> +
> +DTRACE_PATH ?= $(abspath $(srctree)/$(src))
> +TOOLS_PATH := $(DTRACE_PATH)/..
> +SAMPLES_PATH := $(DTRACE_PATH)/../../samples
> +
> +hostprogs-y := dtrace
> +
> +LIBBPF := $(TOOLS_PATH)/lib/bpf/libbpf.a
> +OBJS := dt_bpf.o dt_buffer.o dt_utils.o dt_probe.o \
> + dt_hash.o \
> + dt_fbt.o dt_syscall.o
> +
> +dtrace-objs := $(OBJS) dtrace.o
> +
> +always := $(hostprogs-y)
> +always += bpf_sample.o
> +
> +KBUILD_HOSTCFLAGS += -DDT_VERSION=\"$(DT_VERSION)\"
> +KBUILD_HOSTCFLAGS += -DDT_GIT_VERSION=\"$(DT_GIT_VERSION)\"
> +KBUILD_HOSTCFLAGS += -I$(srctree)/tools/lib
> +KBUILD_HOSTCFLAGS += -I$(srctree)/tools/perf
Interesting, what are you using from tools/perf/? So that we can move to
tools/{include,lib,arch}.
> +KBUILD_HOSTCFLAGS += -I$(srctree)/tools/include/uapi
> +KBUILD_HOSTCFLAGS += -I$(srctree)/tools/include/
> +KBUILD_HOSTCFLAGS += -I$(srctree)/usr/include
> +
> +KBUILD_HOSTLDLIBS := $(LIBBPF) -lelf
> +
> +LLC ?= llc
> +CLANG ?= clang
> +LLVM_OBJCOPY ?= llvm-objcopy
> +
> +ifdef CROSS_COMPILE
> +HOSTCC = $(CROSS_COMPILE)gcc
> +CLANG_ARCH_ARGS = -target $(ARCH)
> +endif
> +
> +all:
> + $(MAKE) -C ../../ $(CURDIR)/ DTRACE_PATH=$(CURDIR)
> +
> +clean:
> + $(MAKE) -C ../../ M=$(CURDIR) clean
> + @rm -f *~
> +
> +$(LIBBPF): FORCE
> + $(MAKE) -C $(dir $@) RM='rm -rf' LDFLAGS= srctree=$(DTRACE_PATH)/../../ O=
> +
> +FORCE:
> +
> +.PHONY: verify_cmds verify_target_bpf $(CLANG) $(LLC)
> +
> +verify_cmds: $(CLANG) $(LLC)
> + @for TOOL in $^ ; do \
> + if ! (which -- "$${TOOL}" > /dev/null 2>&1); then \
> + echo "*** ERROR: Cannot find LLVM tool $${TOOL}" ;\
> + exit 1; \
> + else true; fi; \
> + done
> +
> +verify_target_bpf: verify_cmds
> + @if ! (${LLC} -march=bpf -mattr=help > /dev/null 2>&1); then \
> + echo "*** ERROR: LLVM (${LLC}) does not support 'bpf' target" ;\
> + echo " NOTICE: LLVM version >= 3.7.1 required" ;\
> + exit 2; \
> + else true; fi
> +
> +$(DTRACE_PATH)/*.c: verify_target_bpf $(LIBBPF)
> +$(src)/*.c: verify_target_bpf $(LIBBPF)
> +
> +$(obj)/%.o: $(src)/%.c
> + @echo " CLANG-bpf " $@
> + $(Q)$(CLANG) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) $(EXTRA_CFLAGS) -I$(obj) \
> + -I$(srctree)/tools/testing/selftests/bpf/ \
> + -D__KERNEL__ -D__BPF_TRACING__ -Wno-unused-value -Wno-pointer-sign \
> + -D__TARGET_ARCH_$(ARCH) -Wno-compare-distinct-pointer-types \
> + -Wno-gnu-variable-sized-type-not-at-end \
> + -Wno-address-of-packed-member -Wno-tautological-compare \
> + -Wno-unknown-warning-option $(CLANG_ARCH_ARGS) \
> + -I$(srctree)/samples/bpf/ -include asm_goto_workaround.h \
> + -O2 -emit-llvm -c $< -o -| $(LLC) -march=bpf $(LLC_FLAGS) -filetype=obj -o $@
We have the above in tools/perf/util/llvm-utils.c, perhaps we need to
move it to some place in lib/ to share?
> diff --git a/tools/dtrace/bpf_sample.c b/tools/dtrace/bpf_sample.c
> new file mode 100644
> index 000000000000..49f350390b5f
> --- /dev/null
> +++ b/tools/dtrace/bpf_sample.c
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * This sample DTrace BPF tracing program demonstrates how actions can be
> + * associated with different probe types.
> + *
> + * The kprobe/ksys_write probe is a Function Boundary Tracing (FBT) entry probe
> + * on the ksys_write(fd, buf, count) function in the kernel. Arguments to the
> + * function can be retrieved from the CPU registers (struct pt_regs).
> + *
> + * The tracepoint/syscalls/sys_enter_write probe is a System Call entry probe
> + * for the write(d, buf, count) system call. Arguments to the system call can
> + * be retrieved from the tracepoint data passed to the BPF program as context
> + * struct syscall_data) when the probe fires.
> + *
> + * The BPF program associated with each probe prepares a DTrace BPF context
> + * (struct dt_bpf_context) that stores the probe ID and up to 10 arguments.
> + * Only 3 arguments are used in this sample. Then the prorgams call a shared
> + * BPF function (bpf_action) that implements the actual action to be taken when
> + * a probe fires. It prepares a data record to be stored in the tracing buffer
> + * and submits it to the buffer. The data in the data record is obtained from
> + * the DTrace BPF context.
> + *
> + * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
> + */
> +#include <uapi/linux/bpf.h>
> +#include <linux/ptrace.h>
> +#include <linux/version.h>
> +#include <uapi/linux/unistd.h>
> +#include "bpf_helpers.h"
> +
> +#include "dtrace.h"
> +
> +struct syscall_data {
> + struct pt_regs *regs;
> + long syscall_nr;
> + long arg[6];
> +};
> +
> +struct bpf_map_def SEC("maps") buffers = {
> + .type = BPF_MAP_TYPE_PERF_EVENT_ARRAY,
> + .key_size = sizeof(u32),
> + .value_size = sizeof(u32),
> + .max_entries = NR_CPUS,
> +};
> +
> +#if defined(__amd64)
> +# define GET_REGS_ARG0(regs) ((regs)->di)
> +# define GET_REGS_ARG1(regs) ((regs)->si)
> +# define GET_REGS_ARG2(regs) ((regs)->dx)
> +# define GET_REGS_ARG3(regs) ((regs)->cx)
> +# define GET_REGS_ARG4(regs) ((regs)->r8)
> +# define GET_REGS_ARG5(regs) ((regs)->r9)
> +#else
> +# warning Argument retrieval from pt_regs is not supported yet on this arch.
> +# define GET_REGS_ARG0(regs) 0
> +# define GET_REGS_ARG1(regs) 0
> +# define GET_REGS_ARG2(regs) 0
> +# define GET_REGS_ARG3(regs) 0
> +# define GET_REGS_ARG4(regs) 0
> +# define GET_REGS_ARG5(regs) 0
> +#endif
We have this in tools/testing/selftests/bpf/bpf_helpers.h, probably need
to move to some other place in tools/include/ where this can be shared.
- Arnaldo
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox