* [bpf-next v2 09/10] selftests/bpf: Add tests for bpf_prog_test_run for perf events progs
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, Krzesimir Nowak
In-Reply-To: <20190625194215.14927-1-krzesimir@kinvolk.io>
The tests check if ctx and data are correctly prepared from ctx_in and
data_in, so accessing the ctx and using the bpf_perf_prog_read_value
work as expected.
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
tools/testing/selftests/bpf/test_verifier.c | 48 ++++++++++
.../selftests/bpf/verifier/perf_event_run.c | 93 +++++++++++++++++++
2 files changed, 141 insertions(+)
create mode 100644 tools/testing/selftests/bpf/verifier/perf_event_run.c
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 05bad54f481f..6fa962014b64 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -293,6 +293,54 @@ static void bpf_fill_scale(struct bpf_test *self)
}
}
+static void bpf_fill_perf_event_test_run_check(struct bpf_test *self)
+{
+ compiletime_assert(
+ sizeof(struct bpf_perf_event_data) <= TEST_CTX_LEN,
+ "buffer for ctx is too short to fit struct bpf_perf_event_data");
+ compiletime_assert(
+ sizeof(struct bpf_perf_event_value) <= TEST_DATA_LEN,
+ "buffer for data is too short to fit struct bpf_perf_event_value");
+
+ struct bpf_perf_event_data ctx = {
+ .regs = (bpf_user_pt_regs_t) {
+ .r15 = 1,
+ .r14 = 2,
+ .r13 = 3,
+ .r12 = 4,
+ .rbp = 5,
+ .rbx = 6,
+ .r11 = 7,
+ .r10 = 8,
+ .r9 = 9,
+ .r8 = 10,
+ .rax = 11,
+ .rcx = 12,
+ .rdx = 13,
+ .rsi = 14,
+ .rdi = 15,
+ .orig_rax = 16,
+ .rip = 17,
+ .cs = 18,
+ .eflags = 19,
+ .rsp = 20,
+ .ss = 21,
+ },
+ .sample_period = 1,
+ .addr = 2,
+ };
+ struct bpf_perf_event_value data = {
+ .counter = 1,
+ .enabled = 2,
+ .running = 3,
+ };
+
+ memcpy(self->ctx, &ctx, sizeof(ctx));
+ memcpy(self->data, &data, sizeof(data));
+ free(self->fill_insns);
+ self->fill_insns = NULL;
+}
+
/* BPF_SK_LOOKUP contains 13 instructions, if you need to fix up maps */
#define BPF_SK_LOOKUP(func) \
/* struct bpf_sock_tuple tuple = {} */ \
diff --git a/tools/testing/selftests/bpf/verifier/perf_event_run.c b/tools/testing/selftests/bpf/verifier/perf_event_run.c
new file mode 100644
index 000000000000..d451932a6fc0
--- /dev/null
+++ b/tools/testing/selftests/bpf/verifier/perf_event_run.c
@@ -0,0 +1,93 @@
+#define PER_LOAD_AND_CHECK_PTREG(PT_REG_FIELD, VALUE) \
+ PER_LOAD_AND_CHECK_CTX(offsetof(bpf_user_pt_regs_t, PT_REG_FIELD), VALUE)
+#define PER_LOAD_AND_CHECK_EVENT(PED_FIELD, VALUE) \
+ PER_LOAD_AND_CHECK_CTX(offsetof(struct bpf_perf_event_data, PED_FIELD), VALUE)
+#define PER_LOAD_AND_CHECK_CTX(OFFSET, VALUE) \
+ PER_LOAD_AND_CHECK_64(BPF_REG_4, BPF_REG_1, OFFSET, VALUE)
+#define PER_LOAD_AND_CHECK_VALUE(PEV_FIELD, VALUE) \
+ PER_LOAD_AND_CHECK_64(BPF_REG_7, BPF_REG_6, offsetof(struct bpf_perf_event_value, PEV_FIELD), VALUE)
+#define PER_LOAD_AND_CHECK_64(DST, SRC, OFFSET, VALUE) \
+ BPF_LDX_MEM(BPF_DW, DST, SRC, OFFSET), \
+ BPF_JMP_IMM(BPF_JEQ, DST, VALUE, 2), \
+ BPF_MOV64_IMM(BPF_REG_0, VALUE), \
+ BPF_EXIT_INSN()
+
+{
+ "check if regs contain expected values",
+ .insns = {
+ PER_LOAD_AND_CHECK_PTREG(r15, 1),
+ PER_LOAD_AND_CHECK_PTREG(r14, 2),
+ PER_LOAD_AND_CHECK_PTREG(r13, 3),
+ PER_LOAD_AND_CHECK_PTREG(r12, 4),
+ PER_LOAD_AND_CHECK_PTREG(rbp, 5),
+ PER_LOAD_AND_CHECK_PTREG(rbx, 6),
+ PER_LOAD_AND_CHECK_PTREG(r11, 7),
+ PER_LOAD_AND_CHECK_PTREG(r10, 8),
+ PER_LOAD_AND_CHECK_PTREG(r9, 9),
+ PER_LOAD_AND_CHECK_PTREG(r8, 10),
+ PER_LOAD_AND_CHECK_PTREG(rax, 11),
+ PER_LOAD_AND_CHECK_PTREG(rcx, 12),
+ PER_LOAD_AND_CHECK_PTREG(rdx, 13),
+ PER_LOAD_AND_CHECK_PTREG(rsi, 14),
+ PER_LOAD_AND_CHECK_PTREG(rdi, 15),
+ PER_LOAD_AND_CHECK_PTREG(orig_rax, 16),
+ PER_LOAD_AND_CHECK_PTREG(rip, 17),
+ PER_LOAD_AND_CHECK_PTREG(cs, 18),
+ PER_LOAD_AND_CHECK_PTREG(eflags, 19),
+ PER_LOAD_AND_CHECK_PTREG(rsp, 20),
+ PER_LOAD_AND_CHECK_PTREG(ss, 21),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .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),
+ .fill_helper = bpf_fill_perf_event_test_run_check,
+},
+{
+ "check if sample period and addr contain expected values",
+ .insns = {
+ PER_LOAD_AND_CHECK_EVENT(sample_period, 1),
+ PER_LOAD_AND_CHECK_EVENT(addr, 2),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .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),
+ .fill_helper = bpf_fill_perf_event_test_run_check,
+},
+{
+ "check if bpf_perf_prog_read_value returns expected data",
+ .insns = {
+ // allocate space for a struct bpf_perf_event_value
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, -(int)sizeof(struct bpf_perf_event_value)),
+ // prepare parameters for bpf_perf_prog_read_value(ctx, struct bpf_perf_event_value*, u32)
+ // BPF_REG_1 already contains the context
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_6),
+ BPF_MOV64_IMM(BPF_REG_3, sizeof(struct bpf_perf_event_value)),
+ BPF_EMIT_CALL(BPF_FUNC_perf_prog_read_value),
+ // check the return value
+ BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
+ BPF_EXIT_INSN(),
+ // check if the fields match the expected values
+ PER_LOAD_AND_CHECK_VALUE(counter, 1),
+ PER_LOAD_AND_CHECK_VALUE(enabled, 2),
+ PER_LOAD_AND_CHECK_VALUE(running, 3),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .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),
+ .fill_helper = bpf_fill_perf_event_test_run_check,
+},
+#undef PER_LOAD_AND_CHECK_64
+#undef PER_LOAD_AND_CHECK_VALUE
+#undef PER_LOAD_AND_CHECK_CTX
+#undef PER_LOAD_AND_CHECK_EVENT
+#undef PER_LOAD_AND_CHECK_PTREG
--
2.20.1
^ permalink raw reply related
* [bpf-next v2 08/10] bpf: Implement bpf_prog_test_run for perf event programs
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, Krzesimir Nowak
In-Reply-To: <20190625194215.14927-1-krzesimir@kinvolk.io>
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;
+ }
+ 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++) {
+ 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;
+ }
+ 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
@@ -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
^ permalink raw reply related
* [bpf-next v2 07/10] tools headers: sync struct bpf_perf_event_data
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, Krzesimir Nowak
In-Reply-To: <20190625194215.14927-1-krzesimir@kinvolk.io>
struct bpf_perf_event_data in kernel headers has the addr field, which
is missing in the tools version of the struct. This will be important
for the bpf prog test run implementation for perf events as it will
expect data to be an instance of struct bpf_perf_event_data, so the
size of the data needs to match sizeof(bpf_perf_event_data).
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
tools/include/uapi/linux/bpf_perf_event.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/include/uapi/linux/bpf_perf_event.h b/tools/include/uapi/linux/bpf_perf_event.h
index 8f95303f9d80..eb1b9d21250c 100644
--- a/tools/include/uapi/linux/bpf_perf_event.h
+++ b/tools/include/uapi/linux/bpf_perf_event.h
@@ -13,6 +13,7 @@
struct bpf_perf_event_data {
bpf_user_pt_regs_t regs;
__u64 sample_period;
+ __u64 addr;
};
#endif /* _UAPI__LINUX_BPF_PERF_EVENT_H__ */
--
2.20.1
^ permalink raw reply related
* [bpf-next v2 05/10] selftests/bpf: Allow passing more information to BPF prog test run
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, Krzesimir Nowak
In-Reply-To: <20190625194215.14927-1-krzesimir@kinvolk.io>
The test case can 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.
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
tools/testing/selftests/bpf/test_verifier.c | 68 +++++++++++++++++++--
1 file changed, 62 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index db1f0f758f81..05bad54f481f 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,6 +97,9 @@ 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);
uint8_t runs;
struct {
@@ -104,6 +108,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,7 +825,7 @@ 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)
{
__u8 tmp[TEST_DATA_LEN << 2];
__u32 size_tmp = sizeof(tmp);
@@ -831,6 +838,8 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
.data_size_in = size_data,
.data_out = tmp,
.data_size_out = size_tmp,
+ .ctx_in = ctx,
+ .ctx_size_in = size_ctx,
};
if (unpriv)
@@ -956,13 +965,39 @@ 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;
if (!test->runs) {
+ if (test->data_len > 0)
+ size_data = test->data_len;
+ else
+ size_data = sizeof(test->data);
+ 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);
if (err)
run_errs++;
else
@@ -970,14 +1005,35 @@ 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);
+ 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);
if (err) {
printf("(run %d/%d) ", i + 1, test->runs);
run_errs++;
--
2.20.1
^ permalink raw reply related
* [bpf-next v2 04/10] selftests/bpf: Use bpf_prog_test_run_xattr
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, Krzesimir Nowak
In-Reply-To: <20190625194215.14927-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 779e30b96ded..db1f0f758f81 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 err;
int saved_errno;
+ 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 v2 03/10] selftests/bpf: Avoid another case of errno clobbering
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, Krzesimir Nowak, Stanislav Fomichev
In-Reply-To: <20190625194215.14927-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.
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 12589da13487..779e30b96ded 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -867,6 +867,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
int fixup_skips;
__u32 pflags;
int i, err;
+ int saved_errno;
for (i = 0; i < MAX_NR_MAPS; i++)
map_fds[i] = -1;
@@ -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 v2 02/10] selftests/bpf: Avoid a clobbering of errno
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, Krzesimir Nowak
In-Reply-To: <20190625194215.14927-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.
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 9e17bda016ef..12589da13487 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -824,15 +824,17 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
__u32 size_tmp = sizeof(tmp);
uint32_t retval;
int err;
+ int saved_errno;
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 v2 00/10] Test the 32bit narrow reads
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, 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 to struct bpf_test (patch 5).
5. Some tools headers syncing (patches 6 and 7).
6. Implement bpf_prog_test_run for perf event programs and test it
(patches 8 and 9).
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 (10):
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
tools headers: Adopt compiletime_assert from kernel sources
tools headers: sync struct bpf_perf_event_data
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
kernel/trace/bpf_trace.c | 107 +++++++++++
tools/include/linux/compiler.h | 28 +++
tools/include/uapi/linux/bpf_perf_event.h | 1 +
tools/testing/selftests/bpf/test_verifier.c | 172 ++++++++++++++++--
.../selftests/bpf/verifier/perf_event_run.c | 93 ++++++++++
.../bpf/verifier/perf_event_sample_period.c | 8 +
.../testing/selftests/bpf/verifier/var_off.c | 20 ++
7 files changed, 414 insertions(+), 15 deletions(-)
create mode 100644 tools/testing/selftests/bpf/verifier/perf_event_run.c
--
2.20.1
^ permalink raw reply
* [bpf-next v2 01/10] selftests/bpf: Print a message when tester could not run a program
From: Krzesimir Nowak @ 2019-06-25 19:42 UTC (permalink / raw)
To: netdev
Cc: Alban Crequy, Iago López Galeiras, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-kernel, bpf, Krzesimir Nowak
In-Reply-To: <20190625194215.14927-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".
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..9e17bda016ef 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("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
* Re: [PATCH] xsk: Properly terminate assignment in xskq_produce_flush_desc
From: Jonathan Lemon @ 2019-06-25 19:46 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Björn Töpel, Magnus Karlsson, David S. Miller,
Alexei Starovoitov, Daniel Borkmann, Jakub Kicinski,
Jesper Dangaard Brouer, John Fastabend, netdev, bpf, xdp-newbies,
linux-kernel, clang-built-linux, Nick Desaulniers,
Nathan Huckleberry
In-Reply-To: <20190625182352.13918-1-natechancellor@gmail.com>
On 25 Jun 2019, at 11:23, Nathan Chancellor wrote:
> Clang warns:
>
> In file included from net/xdp/xsk_queue.c:10:
> net/xdp/xsk_queue.h:292:2: warning: expression result unused
> [-Wunused-value]
> WRITE_ONCE(q->ring->producer, q->prod_tail);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> include/linux/compiler.h:284:6: note: expanded from macro 'WRITE_ONCE'
> __u.__val; \
> ~~~ ^~~~~
> 1 warning generated.
>
> The q->prod_tail assignment has a comma at the end, not a semi-colon.
> Fix that so clang no longer warns and everything works as expected.
>
> Fixes: c497176cb2e4 ("xsk: add Rx receive functions and poll support")
> Link: https://github.com/ClangBuiltLinux/linux/issues/544
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Nice find.
Acked-by: Jonathan Lemon <jonathan.lemon@gmail.com>
> ---
> net/xdp/xsk_queue.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
> index 88b9ae24658d..cba4a640d5e8 100644
> --- a/net/xdp/xsk_queue.h
> +++ b/net/xdp/xsk_queue.h
> @@ -288,7 +288,7 @@ static inline void xskq_produce_flush_desc(struct
> xsk_queue *q)
> /* Order producer and data */
> smp_wmb(); /* B, matches C */
>
> - q->prod_tail = q->prod_head,
> + q->prod_tail = q->prod_head;
> WRITE_ONCE(q->ring->producer, q->prod_tail);
> }
>
> --
> 2.22.0
^ permalink raw reply
* Re: [PATCH 00/26] Netfilter updates for net-next
From: David Miller @ 2019-06-25 19:46 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel, netdev
In-Reply-To: <20190625001233.22057-1-pablo@netfilter.org>
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Tue, 25 Jun 2019 02:12:07 +0200
> The following patches contains Netfilter updates for net-next:
...
> You can pull these changes from:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next.git
>
> This batch comes with a conflict resolution between a patch to remove
> the GPL disclaimer by SPDX tags and Jozsef Kladecsik's email update.
Pulled, thanks.
^ permalink raw reply
* Re: [PATCH net] net: make skb_dst_force return false when dst was cleared
From: Eric Dumazet @ 2019-06-25 19:47 UTC (permalink / raw)
To: Florian Westphal, netdev
In-Reply-To: <20190625192209.6250-1-fw@strlen.de>
On 6/25/19 12:22 PM, Florian Westphal wrote:
> XFRM and netfilter don't expect that skb_dst_force() can cause skb to lose
> its dst entry.
>
> I got a bug report with a skb->dst NULL dereference in netfilter
> output path. The backtrace contains nf_reinject(), so the dst
> might have been cleared when skb got queued to userspace.
>
> The xfrm part of this change was done after code inspection,
> it looks like similar crash could happen here too.
>
> One way to fix this is to add a skb_dst() check right after
> skb_dst_force() call, but I think its preferable to make the
> 'dst might get cleared' part of the function explicit.
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
> include/net/dst.h | 6 +++++-
> net/netfilter/nf_queue.c | 6 +++++-
> net/xfrm/xfrm_policy.c | 5 ++++-
> 3 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/dst.h b/include/net/dst.h
> index 12b31c602cb0..42cd53d51364 100644
> --- a/include/net/dst.h
> +++ b/include/net/dst.h
> @@ -302,8 +302,9 @@ static inline bool dst_hold_safe(struct dst_entry *dst)
> * @skb: buffer
> *
> * If dst is not yet refcounted and not destroyed, grab a ref on it.
> + * Returns false if skb had a destroyed dst.
> */
> -static inline void skb_dst_force(struct sk_buff *skb)
> +static inline bool skb_dst_force(struct sk_buff *skb)
> {
> if (skb_dst_is_noref(skb)) {
> struct dst_entry *dst = skb_dst(skb);
> @@ -313,7 +314,10 @@ static inline void skb_dst_force(struct sk_buff *skb)
> dst = NULL;
>
> skb->_skb_refdst = (unsigned long)dst;
> + return dst != NULL;
> }
> +
> + return true;
This will return true, even if skb has a NULL dst.
Say if we have two skb_dst_force() calls for some reason
on the same skb, only the first one will return false.
^ permalink raw reply
* Re: [PATCH net] ipv4: Use return value of inet_iif() for __raw_v4_lookup in the while loop
From: David Miller @ 2019-06-25 19:47 UTC (permalink / raw)
To: ssuryaextr; +Cc: netdev, dsahern
In-Reply-To: <20190625001406.6437-1-ssuryaextr@gmail.com>
From: Stephen Suryaputra <ssuryaextr@gmail.com>
Date: Mon, 24 Jun 2019 20:14:06 -0400
> In commit 19e4e768064a8 ("ipv4: Fix raw socket lookup for local
> traffic"), the dif argument to __raw_v4_lookup() is coming from the
> returned value of inet_iif() but the change was done only for the first
> lookup. Subsequent lookups in the while loop still use skb->dev->ifIndex.
>
> Signed-off-by: Stephen Suryaputra <ssuryaextr@gmail.com>
Applied and queued up for -stable.
I added the appropriate Fixes: tag, please do so next time.
^ permalink raw reply
* Re: [PATCH net-next 1/1] tc-testing: Restore original behaviour for namespaces in tdc
From: David Miller @ 2019-06-25 19:57 UTC (permalink / raw)
To: lucasb
Cc: netdev, nicolas.dichtel, jhs, xiyou.wangcong, jiri, mleitner,
vladbu, dcaratti, kernel
In-Reply-To: <1561424427-9949-1-git-send-email-lucasb@mojatatu.com>
From: Lucas Bates <lucasb@mojatatu.com>
Date: Mon, 24 Jun 2019 21:00:27 -0400
> This patch restores the original behaviour for tdc prior to the
> introduction of the plugin system, where the network namespace
> functionality was split from the main script.
>
> It introduces the concept of required plugins for testcases,
> and will automatically load any plugin that isn't already
> enabled when said plugin is required by even one testcase.
>
> Additionally, the -n option for the nsPlugin is deprecated
> so the default action is to make use of the namespaces.
> Instead, we introduce -N to not use them, but still create
> the veth pair.
>
> buildebpfPlugin's -B option is also deprecated.
>
> If a test cases requires the features of a specific plugin
> in order to pass, it should instead include a new key/value
> pair describing plugin interactions:
>
> "plugins": {
> "requires": "buildebpfPlugin"
> },
>
> A test case can have more than one required plugin: a list
> can be inserted as the value for 'requires'.
>
> Signed-off-by: Lucas Bates <lucasb@mojatatu.com>
Applied.
^ permalink raw reply
* Re: [PATCH net] ipv4: Use return value of inet_iif() for __raw_v4_lookup in the while loop
From: Stefano Brivio @ 2019-06-25 19:59 UTC (permalink / raw)
To: David Miller; +Cc: ssuryaextr, netdev, dsahern
In-Reply-To: <20190625.124738.1945131933038317898.davem@davemloft.net>
On Tue, 25 Jun 2019 12:47:38 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:
> From: Stephen Suryaputra <ssuryaextr@gmail.com>
> Date: Mon, 24 Jun 2019 20:14:06 -0400
>
> > In commit 19e4e768064a8 ("ipv4: Fix raw socket lookup for local
> > traffic"), the dif argument to __raw_v4_lookup() is coming from the
> > returned value of inet_iif() but the change was done only for the first
> > lookup. Subsequent lookups in the while loop still use skb->dev->ifIndex.
> >
> > Signed-off-by: Stephen Suryaputra <ssuryaextr@gmail.com>
>
> Applied and queued up for -stable.
>
> I added the appropriate Fixes: tag, please do so next time.
I was about to point that out, but then I noticed that this
doesn't actually fix 19e4e768064a8 ("ipv4: Fix raw socket lookup for
local traffic"), it's just related as it fixes the same issue in
another (very likely) path in the same function.
I think this should have been:
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
I *guess* for -stable purposes the effect is the same.
--
Stefano
^ permalink raw reply
* Re: [PATCH 2/2] net/ipv6: Fix misuse of proc_dointvec "skip_notify_on_dev_down"
From: David Miller @ 2019-06-25 19:59 UTC (permalink / raw)
To: devel; +Cc: gregkh, jslaby, kuznet, yoshfuji, linux-kernel, netdev
In-Reply-To: <20190625030801.24538-2-devel@etsukata.com>
From: Eiichi Tsukata <devel@etsukata.com>
Date: Tue, 25 Jun 2019 12:08:01 +0900
> /proc/sys/net/ipv6/route/skip_notify_on_dev_down assumes given value to be
> 0 or 1. Use proc_dointvec_minmax instead of proc_dointvec.
>
> Fixes: 7c6bb7d2faaf ("net/ipv6: Add knob to skip DELROUTE message ondevice down")
> Signed-off-by: Eiichi Tsukata <devel@etsukata.com>
Applied.
^ permalink raw reply
* Re: [PATCH net] net: make skb_dst_force return false when dst was cleared
From: Florian Westphal @ 2019-06-25 19:59 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Florian Westphal, netdev
In-Reply-To: <8483d4dc-1ef6-20b5-735f-8d78da579a28@gmail.com>
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > -static inline void skb_dst_force(struct sk_buff *skb)
> > +static inline bool skb_dst_force(struct sk_buff *skb)
> > {
> > if (skb_dst_is_noref(skb)) {
> > struct dst_entry *dst = skb_dst(skb);
> > @@ -313,7 +314,10 @@ static inline void skb_dst_force(struct sk_buff *skb)
> > dst = NULL;
> >
> > skb->_skb_refdst = (unsigned long)dst;
> > + return dst != NULL;
> > }
> > +
> > + return true;
>
> This will return true, even if skb has a NULL dst.
Yes, that was intentional -- it should return false to
let caller know that no reference could be obtained and
that the dst was invalidated as a result.
> Say if we have two skb_dst_force() calls for some reason
> on the same skb, only the first one will return false.
What would you suggest instead?
Alternative is something like
if (skb_dst(skb)) {
skb_dst_force(skb);
if (!skb_dst(skb)) {
kfree_skb(skb);
goto err;
}
}
... i find this a bit ugly.
^ permalink raw reply
* Re: hard-coded limit on unresolved multicast route cache in ipv4/ipmr.c causes slow, unreliable creation of multicast routes on busy networks
From: David Miller @ 2019-06-25 20:03 UTC (permalink / raw)
To: liuhangbin; +Cc: sukumarg1973, karn, kuznet, yoshfuji, netdev, linux-kernel
In-Reply-To: <20190625061507.GG18865@dhcp-12-139.nay.redhat.com>
From: Hangbin Liu <liuhangbin@gmail.com>
Date: Tue, 25 Jun 2019 14:15:07 +0800
> On Tue, Dec 18, 2018 at 09:55:45PM -0800, David Miller wrote:
>> From: Sukumar Gopalakrishnan <sukumarg1973@gmail.com>
>> Date: Wed, 19 Dec 2018 10:57:02 +0530
>>
>> > Hi David,
>> >
>> > There are two patch for this issue:
>> > 1) Your changes which removes cache_resolve_queue_len
>> > 2) Hangbin's changes which make cache_resolve_queue_len configurable.
>> >
>> > Which one will be chosen for this issue ?
>>
>> I do plan to look into this, sorry for taking so long.
>>
>> Right now I am overwhelmed preparing for the next merge window and
>> synchronizing with other developers for that.
>>
>> Please be patient.
>
> Hi David,
>
> Any progress for this issue?
I have absolutely no context from a discussion that happened back in Dec 2018
If it is important to you, please restart the discussion with a new mailing list
posting restating the problem from the beginning and reiterating all of the
points and arguments that have been made thus far.
^ permalink raw reply
* Re: [PATCH v4 rdma-next 2/3] RDMA/qedr: Add doorbell overflow recovery support
From: Jason Gunthorpe @ 2019-06-25 20:04 UTC (permalink / raw)
To: Michal Kalderon
Cc: ariel.elior@marvell.com, dledford@redhat.com,
linux-rdma@vger.kernel.org, davem@davemloft.net,
netdev@vger.kernel.org
In-Reply-To: <20190624102809.8793-3-michal.kalderon@marvell.com>
On Mon, Jun 24, 2019 at 01:28:08PM +0300, Michal Kalderon wrote:
> +/* Map the kernel doorbell recovery memory entry */
> +int qedr_mmap_db_rec(struct vm_area_struct *vma)
> +{
> + unsigned long len = vma->vm_end - vma->vm_start;
> +
> + return remap_pfn_range(vma, vma->vm_start,
> + vma->vm_pgoff,
> + len, vma->vm_page_prot);
> +}
> +
> int qedr_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)
> {
> struct qedr_ucontext *ucontext = get_qedr_ucontext(context);
> @@ -390,6 +446,8 @@ int qedr_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)
> unsigned long phys_addr = vma->vm_pgoff << PAGE_SHIFT;
> unsigned long len = (vma->vm_end - vma->vm_start);
> unsigned long dpi_start;
> + struct qedr_mm *mm;
> + int rc;
>
> dpi_start = dev->db_phys_addr + (ucontext->dpi * ucontext->dpi_size);
>
> @@ -405,29 +463,28 @@ int qedr_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)
> return -EINVAL;
> }
>
> - if (!qedr_search_mmap(ucontext, phys_addr, len)) {
> - DP_ERR(dev, "failed mmap, vm_pgoff=0x%lx is not authorized\n",
> + mm = qedr_remove_mmap(ucontext, phys_addr, len);
> + if (!mm) {
> + DP_ERR(dev, "failed to remove mmap, vm_pgoff=0x%lx\n",
> vma->vm_pgoff);
> return -EINVAL;
>
This is so gross, please follow the pattern other drivers use for
managing the mmap cookie
In fact I am sick of seeing drivers wrongly re-implement this, so you
now get the job to make some proper core helpers to manage mmap
cookies for drivers.
The EFA driver is probably the best example, I suggest you move that
code to a common file in ib-core and use it here instead of redoing
yet again another broken version.
siw has another copy of basically the same thing.
> +static int qedr_init_user_db_rec(struct ib_udata *udata,
> + struct qedr_dev *dev, struct qedr_userq *q,
> + bool requires_db_rec)
> +{
> + struct qedr_ucontext *uctx =
> + rdma_udata_to_drv_context(udata, struct qedr_ucontext,
> + ibucontext);
> +
> + /* Aborting for non doorbell userqueue (SRQ) or non-supporting lib */
> + if (requires_db_rec == 0 || !uctx->db_rec)
> + return 0;
> +
> + /* Allocate a page for doorbell recovery, add to mmap ) */
> + q->db_rec_data = (void *)get_zeroed_page(GFP_KERNEL);
Pages obtained by get_zeroed_page shuld not be inserted by
remap_pfn_range, those cases need to use vm_insert_page instead.
> struct qedr_alloc_ucontext_resp {
> __aligned_u64 db_pa;
> @@ -74,6 +83,7 @@ struct qedr_create_cq_uresp {
> __u32 db_offset;
> __u16 icid;
> __u16 reserved;
> + __u64 db_rec_addr;
> };
All uapi u64s need to be __aligned_u64 in this file.
> +/* doorbell recovery entry allocated and populated by userspace doorbelling
> + * entities and mapped to kernel. Kernel uses this to register doorbell
> + * information with doorbell drop recovery mechanism.
> + */
> +struct qedr_user_db_rec {
> + __aligned_u64 db_data; /* doorbell data */
> +};
like this one :\
Jason
^ permalink raw reply
* Re: [PATCH bpf-next v4 0/2] bpf: Allow bpf_skb_event_output for more prog types
From: Song Liu @ 2019-06-25 20:06 UTC (permalink / raw)
To: allanzhang
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, David S. Miller, Networking, bpf, open list
In-Reply-To: <20190625172717.158613-1-allanzhang@google.com>
On Tue, Jun 25, 2019 at 12:45 PM allanzhang <allanzhang@google.com> wrote:
>
> Software event output is only enabled by a few prog types right now (TC,
> LWT out, XDP, sockops). Many other skb based prog types need
> bpf_skb_event_output to produce software event.
>
> Added socket_filter, cg_skb, sk_skb prog types to generate sw event.
>
> allanzhang (2):
> bpf: Allow bpf_skb_event_output for a few prog types
> bpf: Add selftests for bpf_perf_event_output
I am not sure whether this is caused by delay in the mailing list or something
else. But it appears to me that you are ignoring some of the feedback. Please
pay more attention to these feedback.
Please include changes "v1, xxx, v2, xxx, .." in the cover letter, but not the
commit log itself. In other words, include that in 0/2, but not in 1/2 or 2/2.
Thanks,
Song
>
> net/core/filter.c | 6 ++
> tools/testing/selftests/bpf/test_verifier.c | 33 ++++++-
> .../selftests/bpf/verifier/event_output.c | 94 +++++++++++++++++++
> 3 files changed, 132 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/bpf/verifier/event_output.c
>
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
^ permalink raw reply
* Re: [bpf-next v2 03/10] selftests/bpf: Avoid another case of errno clobbering
From: Stanislav Fomichev @ 2019-06-25 20:08 UTC (permalink / raw)
To: Krzesimir Nowak
Cc: netdev, Alban Crequy, Iago López Galeiras,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, linux-kernel, bpf, Stanislav Fomichev
In-Reply-To: <20190625194215.14927-4-krzesimir@kinvolk.io>
On 06/25, Krzesimir Nowak wrote:
> 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.
>
> 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 12589da13487..779e30b96ded 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -867,6 +867,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> int fixup_skips;
> __u32 pflags;
> int i, err;
> + int saved_errno;
Reverse Christmas tree. Otherwise LGTM.
>
> for (i = 0; i < MAX_NR_MAPS; i++)
> map_fds[i] = -1;
> @@ -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
* Re: [PATCH bpf-next v4 2/2] bpf: Add selftests for bpf_perf_event_output
From: Song Liu @ 2019-06-25 20:11 UTC (permalink / raw)
To: allanzhang
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, David S. Miller, Networking, bpf, open list
In-Reply-To: <20190625172717.158613-3-allanzhang@google.com>
On Tue, Jun 25, 2019 at 12:45 PM allanzhang <allanzhang@google.com> wrote:
>
> Software event output is only enabled by a few prog types.
> This test is to ensure that all supported types are enbled for
> bpf_perf_event_output sucessfully.
Please fix these typos highlighted by Daniel.
enbled
sucessfully
>
> v4:
> * Reformating log message
> v3:
> * Reformating log message
> v2:
> * Reformating log message
>
> Signed-off-by: allanzhang <allanzhang@google.com>
> ---
> tools/testing/selftests/bpf/test_verifier.c | 33 ++++++-
> .../selftests/bpf/verifier/event_output.c | 94 +++++++++++++++++++
> 2 files changed, 126 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/bpf/verifier/event_output.c
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index c5514daf8865..901a188e1eea 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -50,7 +50,7 @@
> #define MAX_INSNS BPF_MAXINSNS
> #define MAX_TEST_INSNS 1000000
> #define MAX_FIXUPS 8
> -#define MAX_NR_MAPS 18
> +#define MAX_NR_MAPS 19
> #define MAX_TEST_RUNS 8
> #define POINTER_VALUE 0xcafe4all
> #define TEST_DATA_LEN 64
> @@ -84,6 +84,7 @@ struct bpf_test {
> int fixup_map_array_wo[MAX_FIXUPS];
> int fixup_map_array_small[MAX_FIXUPS];
> int fixup_sk_storage_map[MAX_FIXUPS];
> + int fixup_map_event_output[MAX_FIXUPS];
> const char *errstr;
> const char *errstr_unpriv;
> uint32_t retval, retval_unpriv, insn_processed;
> @@ -604,6 +605,28 @@ static int create_sk_storage_map(void)
> return fd;
> }
>
> +static int create_event_output_map(void)
> +{
> + struct bpf_create_map_attr attr = {
> + .name = "test_map",
> + .map_type = BPF_MAP_TYPE_PERF_EVENT_ARRAY,
> + .key_size = 4,
> + .value_size = 4,
> + .max_entries = 1,
> + };
> + int fd, btf_fd;
> +
> + btf_fd = load_btf();
> + if (btf_fd < 0)
> + return -1;
> + attr.btf_fd = btf_fd;
> + fd = bpf_create_map_xattr(&attr);
> + close(attr.btf_fd);
> + if (fd < 0)
> + printf("Failed to create event_output\n");
> + return fd;
> +}
> +
> static char bpf_vlog[UINT_MAX >> 8];
>
> static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> @@ -627,6 +650,7 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> int *fixup_map_array_wo = test->fixup_map_array_wo;
> int *fixup_map_array_small = test->fixup_map_array_small;
> int *fixup_sk_storage_map = test->fixup_sk_storage_map;
> + int *fixup_map_event_output = test->fixup_map_event_output;
>
> if (test->fill_helper) {
> test->fill_insns = calloc(MAX_TEST_INSNS, sizeof(struct bpf_insn));
> @@ -788,6 +812,13 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> fixup_sk_storage_map++;
> } while (*fixup_sk_storage_map);
> }
> + if (*fixup_map_event_output) {
> + map_fds[18] = create_event_output_map();
> + do {
> + prog[*fixup_map_event_output].imm = map_fds[18];
> + fixup_map_event_output++;
> + } while (*fixup_map_event_output);
> + }
> }
>
> static int set_admin(bool admin)
> diff --git a/tools/testing/selftests/bpf/verifier/event_output.c b/tools/testing/selftests/bpf/verifier/event_output.c
> new file mode 100644
> index 000000000000..b25eabcfaa56
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/verifier/event_output.c
> @@ -0,0 +1,94 @@
> +/* instructions used to output a skb based software event, produced
> + * from code snippet:
> +struct TMP {
> + uint64_t tmp;
> +} tt;
> +tt.tmp = 5;
> +bpf_perf_event_output(skb, &connection_tracking_event_map, 0,
> + &tt, sizeof(tt));
> +return 1;
> +
> +the bpf assembly from llvm is:
> + 0: b7 02 00 00 05 00 00 00 r2 = 5
> + 1: 7b 2a f8 ff 00 00 00 00 *(u64 *)(r10 - 8) = r2
> + 2: bf a4 00 00 00 00 00 00 r4 = r10
> + 3: 07 04 00 00 f8 ff ff ff r4 += -8
> + 4: 18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0ll
> + 6: b7 03 00 00 00 00 00 00 r3 = 0
> + 7: b7 05 00 00 08 00 00 00 r5 = 8
> + 8: 85 00 00 00 19 00 00 00 call 25
> + 9: b7 00 00 00 01 00 00 00 r0 = 1
> + 10: 95 00 00 00 00 00 00 00 exit
> +
> + The reason I put the code here instead of fill_helpers is that map fixup is
> + against the insns, instead of filled prog.
> +*/
Please prefix every line in the comment section with space_star_space: " * ".
This makes it obvious that this is a comment.
Thanks,
Song
> +
> +#define __PERF_EVENT_INSNS__ \
> + BPF_MOV64_IMM(BPF_REG_2, 5), \
> + BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_2, -8), \
> + BPF_MOV64_REG(BPF_REG_4, BPF_REG_10), \
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -8), \
> + BPF_LD_MAP_FD(BPF_REG_2, 0), \
> + BPF_MOV64_IMM(BPF_REG_3, 0), \
> + BPF_MOV64_IMM(BPF_REG_5, 8), \
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, \
> + BPF_FUNC_perf_event_output), \
> + BPF_MOV64_IMM(BPF_REG_0, 1), \
> + BPF_EXIT_INSN(),
> +{
> + "perfevent for sockops",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_SOCK_OPS,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for tc",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for lwt out",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_LWT_OUT,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for xdp",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_XDP,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for socket filter",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_SOCKET_FILTER,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for sk_skb",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_SK_SKB,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for cgroup skb",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
^ permalink raw reply
* Re: [PATCH iproute2] ip/iptoken: fix dump error when ipv6 disabled
From: Stephen Hemminger @ 2019-06-25 20:11 UTC (permalink / raw)
To: Hangbin Liu
Cc: netdev, Daniel Borkmann, Phil Sutter, David Ahern, Andrea Claudi
In-Reply-To: <20190625093550.7804-1-liuhangbin@gmail.com>
On Tue, 25 Jun 2019 17:35:50 +0800
Hangbin Liu <liuhangbin@gmail.com> wrote:
> When we disable IPv6 from the start up (ipv6.disable=1), there will be
> no IPv6 route info in the dump message. If we return -1 when
> ifi->ifi_family != AF_INET6, we will get error like
>
> $ ip token list
> Dump terminated
>
> which will make user feel confused. There is no need to return -1 if the
> dump message not match. Return 0 is enough.
>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> ip/iptoken.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/ip/iptoken.c b/ip/iptoken.c
> index f1194c3e..dfd22734 100644
> --- a/ip/iptoken.c
> +++ b/ip/iptoken.c
> @@ -59,13 +59,9 @@ static int print_token(struct nlmsghdr *n, void *arg)
> if (len < 0)
> return -1;
>
> - if (ifi->ifi_family != AF_INET6)
> - return -1;
> - if (ifi->ifi_index == 0)
> - return -1;
> - if (ifindex > 0 && ifi->ifi_index != ifindex)
> - return 0;
> - if (ifi->ifi_flags & (IFF_LOOPBACK | IFF_NOARP))
> + if (ifi->ifi_family != AF_INET6 || ifi->ifi_index == 0 ||
> + (ifindex > 0 && ifi->ifi_index != ifindex) ||
> + (ifi->ifi_flags & (IFF_LOOPBACK | IFF_NOARP)))
> return 0;
Please don't combine all the conditions, it is simpler as:
if (ifi->ifi_family != AF_INET6)
return 0;
^ permalink raw reply
* Re: [bpf-next v2 08/10] bpf: Implement bpf_prog_test_run for perf event programs
From: Stanislav Fomichev @ 2019-06-25 20:12 UTC (permalink / raw)
To: Krzesimir Nowak
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: <20190625194215.14927-9-krzesimir@kinvolk.io>
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.
> + 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?
> + 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?
> + 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.
> @@ -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
>
^ permalink raw reply
* Re: [PATCH bpf] bpf: fix BPF_ALU32 | BPF_ARSH on BE arches
From: Song Liu @ 2019-06-25 20:15 UTC (permalink / raw)
To: Jiong Wang
Cc: Alexei Starovoitov, Daniel Borkmann, yauheni.kaliuta, bpf,
Networking, oss-drivers
In-Reply-To: <1561480910-23543-1-git-send-email-jiong.wang@netronome.com>
On Tue, Jun 25, 2019 at 12:31 PM Jiong Wang <jiong.wang@netronome.com> wrote:
>
> Yauheni reported the following code do not work correctly on BE arches:
>
> ALU_ARSH_X:
> DST = (u64) (u32) ((*(s32 *) &DST) >> SRC);
> CONT;
> ALU_ARSH_K:
> DST = (u64) (u32) ((*(s32 *) &DST) >> IMM);
> CONT;
>
> and are causing failure of test_verifier test 'arsh32 on imm 2' on BE
> arches.
>
> The code is taking address and interpreting memory directly, so is not
> endianness neutral. We should instead perform standard C type casting on
> the variable. A u64 to s32 conversion will drop the high 32-bit and reserve
> the low 32-bit as signed integer, this is all we want.
>
> Fixes: 2dc6b100f928 ("bpf: interpreter support BPF_ALU | BPF_ARSH")
> Reported-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
Acked-by: Song Liu <songliubraving@fb.com>
I guess we need:
Cc: <stable@vger.kernel.org> #v5.0+
> ---
> kernel/bpf/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 080e2bb..f2148db 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -1364,10 +1364,10 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
> insn++;
> CONT;
> ALU_ARSH_X:
> - DST = (u64) (u32) ((*(s32 *) &DST) >> SRC);
> + DST = (u64) (u32) (((s32) DST) >> SRC);
> CONT;
> ALU_ARSH_K:
> - DST = (u64) (u32) ((*(s32 *) &DST) >> IMM);
> + DST = (u64) (u32) (((s32) DST) >> IMM);
> CONT;
> ALU64_ARSH_X:
> (*(s64 *) &DST) >>= SRC;
> --
> 2.7.4
>
^ 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