* 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 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] 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
* [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
* [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 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 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 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 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 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 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 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 10/10] selftests/bpf: Test correctness of narrow 32bit read on 64bit field
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>
Test the correctness of the 32bit narrow reads by reading both halves
of the 64 bit field and doing a binary or on them to see if we get the
original value.
It succeeds as it should, but with the commit e2f7fc0ac695 ("bpf: fix
undefined behavior in narrow load handling") reverted, the test fails
with a following message:
> $ sudo ./test_verifier
> ...
> #967/p 32bit loads of a 64bit field (both least and most significant words) FAIL retval -1985229329 != 0
> verification time 17 usec
> stack depth 0
> processed 8 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
> ...
> Summary: 1519 PASSED, 0 SKIPPED, 1 FAILED
Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
---
tools/testing/selftests/bpf/test_verifier.c | 19 ++++++++++++++++++
.../testing/selftests/bpf/verifier/var_off.c | 20 +++++++++++++++++++
2 files changed, 39 insertions(+)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 6fa962014b64..444c1ea1e326 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -24,6 +24,7 @@
#include <sys/capability.h>
+#include <linux/compiler.h>
#include <linux/unistd.h>
#include <linux/filter.h>
#include <linux/bpf_perf_event.h>
@@ -341,6 +342,24 @@ static void bpf_fill_perf_event_test_run_check(struct bpf_test *self)
self->fill_insns = NULL;
}
+static void bpf_fill_32bit_loads(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 = {
+ .sample_period = 0x0123456789abcdef,
+ };
+
+ memcpy(self->ctx, &ctx, sizeof(ctx));
+ 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/var_off.c b/tools/testing/selftests/bpf/verifier/var_off.c
index 8504ac937809..14d222f37081 100644
--- a/tools/testing/selftests/bpf/verifier/var_off.c
+++ b/tools/testing/selftests/bpf/verifier/var_off.c
@@ -246,3 +246,23 @@
.result = ACCEPT,
.prog_type = BPF_PROG_TYPE_LWT_IN,
},
+{
+ "32bit loads of a 64bit field (both least and most significant words)",
+ .insns = {
+ BPF_LDX_MEM(BPF_W, BPF_REG_4, BPF_REG_1, offsetof(struct bpf_perf_event_data, sample_period)),
+ BPF_LDX_MEM(BPF_W, BPF_REG_5, BPF_REG_1, offsetof(struct bpf_perf_event_data, sample_period) + 4),
+ BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1, offsetof(struct bpf_perf_event_data, sample_period)),
+ BPF_ALU64_IMM(BPF_LSH, BPF_REG_5, 32),
+ BPF_ALU64_REG(BPF_OR, BPF_REG_4, BPF_REG_5),
+ BPF_ALU64_REG(BPF_XOR, BPF_REG_4, BPF_REG_6),
+ BPF_MOV64_REG(BPF_REG_0, BPF_REG_4),
+ BPF_EXIT_INSN(),
+ },
+ .result = ACCEPT,
+ .prog_type = BPF_PROG_TYPE_PERF_EVENT,
+ .ctx = { 0, },
+ .ctx_len = sizeof(struct bpf_perf_event_data),
+ .data = { 0, },
+ .data_len = sizeof(struct bpf_perf_event_value),
+ .fill_helper = bpf_fill_32bit_loads,
+},
--
2.20.1
^ permalink raw reply related
* [bpf-next v2 06/10] tools headers: Adopt compiletime_assert from kernel sources
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 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
* Re: [PATCH net] net/sched: flower: fix infinite loop in fl_walk()
From: Cong Wang @ 2019-06-25 19:29 UTC (permalink / raw)
To: Davide Caratti
Cc: Vlad Buslov, David S. Miller, Linux Kernel Network Developers,
Lucas Bates
In-Reply-To: <CAM_iQpW_-e+duPqKVXSDn7fp3WOKfs+RgVkFkfeQJQUTP_0x1Q@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 601 bytes --]
On Tue, Jun 25, 2019 at 11:07 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On one hand, its callers should not need to worry about details
> like overflow. On the other hand, in fact it does exactly what its
> callers tell it to do, the problematic part is actually the
> incremented id. On 64bit, it is fairly easy, we can just simply
> know 'long' is longer than 32bit and leverage this to detect overflow,
> but on 32bit this clearly doesn't work.
>
> Let me think about it.
Davide, do you mind to try the attached patch?
It should handle this overflow case more gracefully, I hope.
Thanks.
[-- Attachment #2: idr_get_next_ul.diff --]
[-- Type: application/octet-stream, Size: 2903 bytes --]
commit 685934f9eed9b50a46d33a9ec9671800397d20cc
Author: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue Jun 25 12:23:18 2019 -0700
idr: fix idr_get_next_ul() usage
Reported-by: Li Shuang <shuali@redhat.com>
Cc: Davide Caratti <dcaratti@redhat.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c
index c6c28f56aa29..c73f80bddde4 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c
@@ -105,10 +105,11 @@ static struct list_head *mlx5_fc_counters_lookup_next(struct mlx5_core_dev *dev,
rcu_read_lock();
/* skip counters that are in idr, but not yet in counters list */
- while ((counter = idr_get_next_ul(&fc_stats->counters_idr,
- &next_id)) != NULL &&
- list_empty(&counter->list))
- next_id++;
+ idr_for_each_entry_continue_ul(&fc_stats->counters_idr,
+ counter, next_id) {
+ if (list_empty(&counter->list))
+ continue;
+ }
rcu_read_unlock();
return counter ? &counter->list : &fc_stats->counters;
diff --git a/include/linux/idr.h b/include/linux/idr.h
index ee7abae143d3..af7a67e65c1c 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -198,7 +198,21 @@ static inline void idr_preload_end(void)
* is convenient for a "not found" value.
*/
#define idr_for_each_entry_ul(idr, entry, id) \
- for (id = 0; ((entry) = idr_get_next_ul(idr, &(id))) != NULL; ++id)
+ for (id = 0; \
+ (u32)id + 1 > id && ((entry) = idr_get_next_ul(idr, &(id))) != NULL; \
+ ++id)
+
+/**
+ * idr_for_each_entry_continue_ul() - Continue iteration over an IDR's elements of a given type
+ * @idr: IDR handle.
+ * @entry: The type * to use as a cursor.
+ * @id: Entry ID.
+ *
+ * Continue to iterate over entries, continuing after the current position.
+ */
+#define idr_for_each_entry_continue_ul(idr, entry, id) \
+ for (; (u32)id + 1 > id && ((entry) = idr_get_next_ul(idr, &(id))) != NULL; \
+ ++id)
/**
* idr_for_each_entry_continue() - Continue iteration over an IDR's elements of a given type
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index eedd5786c084..06338dadd5e4 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -528,17 +528,18 @@ static struct cls_fl_filter *fl_get_next_filter(struct tcf_proto *tp,
unsigned long *handle)
{
struct cls_fl_head *head = fl_head_dereference(tp);
+ unsigned long id = *handle;
struct cls_fl_filter *f;
rcu_read_lock();
- while ((f = idr_get_next_ul(&head->handle_idr, handle))) {
+ idr_for_each_entry_continue_ul(&head->handle_idr, f, id) {
/* don't return filters that are being deleted */
if (refcount_inc_not_zero(&f->refcnt))
break;
- ++(*handle);
}
rcu_read_unlock();
+ *handle = id;
return f;
}
^ permalink raw reply related
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: Daniel Santos @ 2019-06-25 19:27 UTC (permalink / raw)
To: Andrew Lunn
Cc: Russell King - ARM Linux admin, René van Dorst, sean.wang,
f.fainelli, davem, matthias.bgg, vivien.didelot, frank-w, netdev,
linux-mediatek, linux-mips
In-Reply-To: <20190625190246.GA27733@lunn.ch>
On 6/25/19 2:02 PM, Andrew Lunn wrote:
>> But will there still be a mechanism to ignore link partner's advertising
>> and force these parameters?
> >From man 1 ethtool:
>
> -a --show-pause
> Queries the specified Ethernet device for pause parameter information.
>
> -A --pause
> Changes the pause parameters of the specified Ethernet device.
>
> autoneg on|off
> Specifies whether pause autonegotiation should be enabled.
>
> rx on|off
> Specifies whether RX pause should be enabled.
>
> tx on|off
> Specifies whether TX pause should be enabled.
>
> You need to check the driver to see if it actually implements this
> ethtool call, but that is how it should be configured.
>
> Andrew
>
Thank you Andrew,
So in this context, my question is the difference between "enabling" and
"forcing". Here's that register for the mt7620 (which has an mt7530 on
its die): https://imgur.com/a/pTk0668 I believe this is also what René
is seeking clarity on?
Thanks,
Daniel
^ permalink raw reply
* [PATCH net] net: make skb_dst_force return false when dst was cleared
From: Florian Westphal @ 2019-06-25 19:22 UTC (permalink / raw)
To: netdev; +Cc: eric.dumazet, Florian Westphal
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;
}
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index b5b2be55ca82..dc8628a919a5 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -190,6 +190,11 @@ static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
goto err;
}
+ if (!skb_dst_force(skb)) {
+ status = -ENETDOWN;
+ goto err;
+ }
+
*entry = (struct nf_queue_entry) {
.skb = skb,
.state = *state,
@@ -198,7 +203,6 @@ static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
};
nf_queue_entry_get_refs(entry);
- skb_dst_force(skb);
switch (entry->state.pf) {
case AF_INET:
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index b1694d5d15d3..5c66c18d5ff5 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2842,7 +2842,10 @@ static int xdst_queue_output(struct net *net, struct sock *sk, struct sk_buff *s
return -EAGAIN;
}
- skb_dst_force(skb);
+ if (!skb_dst_force(skb)) {
+ kfree_skb(skb);
+ return -ENETDOWN;
+ }
spin_lock_bh(&pq->hold_queue.lock);
--
2.21.0
^ permalink raw reply related
* Re: [PATCH RFC net-next 4/5] dt-bindings: net: dsa: mt7530: Add mediatek,ephy-handle to isolate ext. phy
From: Florian Fainelli @ 2019-06-25 19:16 UTC (permalink / raw)
To: René van Dorst
Cc: sean.wang, linux, davem, matthias.bgg, andrew, vivien.didelot,
frank-w, netdev, linux-mediatek, linux-mips
In-Reply-To: <20190625093042.Horde._8BNPFSzW6B9-CI8P6akHTh@www.vdorst.com>
On 6/25/19 2:30 AM, René van Dorst wrote:
> Quoting Florian Fainelli <f.fainelli@gmail.com>:
>
> Hi Florian
>
>> On 6/24/19 7:52 AM, René van Dorst wrote:
>>> On some platforum the external phy can only interface to the port 5
>>> of the
>>> switch because the RGMII TX and RX lines are swapped. But it still
>>> can be
>>> useful to use the internal phy of the switch to act as a WAN port which
>>> connectes to the 2nd GMAC. This gives WAN port dedicated bandwidth to
>>> the SOC. This increases the LAN and WAN routing.
>>>
>>> By adding the optional property mediatek,ephy-handle, the external phy
>>> is put in isolation mode when internal phy is connected to 2nd GMAC via
>>> phy-handle property.
>>
>> Most platforms we have seen so far implement this logic with a mdio-mux,
>> can you see if that is possible here? stmmac has plenty of examples like
>> those.
>
> May I don't understand it correctly, but all the devices are on the same
> MDIO
> bus.
> I tried to make a ASCII diagram to make it a bit more clear.
Based on your diagram and your explanation, then I do not really see a
need for that property you can scan all of the switch port's properties
and determined which configuration is applied and perform the PHY
isolation as necessary. It looks like you are using this property as a
way to simplify your configuration logic, that is not quite what Device
Tree is meant for.
--
Florian
^ permalink raw reply
* Re: [PATCH v4 3/7] lib/hexdump.c: Optionally suppress lines of repeated bytes
From: kbuild test robot @ 2019-06-25 19:13 UTC (permalink / raw)
To: Alastair D'Silva
Cc: kbuild-all, alastair, linux-fbdev, Stanislaw Gruszka, Petr Mladek,
David Airlie, Joonas Lahtinen, dri-devel, devel, linux-scsi,
Jassi Brar, ath10k, intel-gfx, Dan Carpenter, Jose Abreu,
Tom Lendacky, James E.J. Bottomley, Jani Nikula, linux-fsdevel,
Steven Rostedt, Rodrigo Vivi, Benson Leung, Kalle Valo,
Karsten Keil, Martin K. Petersen, Greg Kroah-Hartman,
linux-wireless, linux-kernel, Sergey Senozhatsky, David Laight,
Daniel Vetter, netdev, Enric Balletbo i Serra, Andrew Morton,
David S. Miller, Alexander Viro
In-Reply-To: <20190625031726.12173-4-alastair@au1.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 1676 bytes --]
Hi Alastair,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.2-rc6 next-20190625]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Alastair-D-Silva/Hexdump-Enhancements/20190625-224046
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=xtensa
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
lib/hexdump.c: In function 'announce_skipped':
>> lib/hexdump.c:243:28: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t {aka unsigned int}' [-Wformat=]
printk("%s%s ** Skipped %lu bytes of value 0x%x **\n",
~~^
%u
vim +243 lib/hexdump.c
236
237 static void announce_skipped(const char *level, const char *prefix_str,
238 u8 val, size_t count)
239 {
240 if (count == 0)
241 return;
242
> 243 printk("%s%s ** Skipped %lu bytes of value 0x%x **\n",
244 level, prefix_str, count, val);
245 }
246
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 58637 bytes --]
^ permalink raw reply
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: Andrew Lunn @ 2019-06-25 19:02 UTC (permalink / raw)
To: Daniel Santos
Cc: Russell King - ARM Linux admin, René van Dorst, sean.wang,
f.fainelli, davem, matthias.bgg, vivien.didelot, frank-w, netdev,
linux-mediatek, linux-mips
In-Reply-To: <1ad9f9a5-8f39-40bd-94bb-6b700f30c4ba@pobox.com>
> But will there still be a mechanism to ignore link partner's advertising
> and force these parameters?
From man 1 ethtool:
-a --show-pause
Queries the specified Ethernet device for pause parameter information.
-A --pause
Changes the pause parameters of the specified Ethernet device.
autoneg on|off
Specifies whether pause autonegotiation should be enabled.
rx on|off
Specifies whether RX pause should be enabled.
tx on|off
Specifies whether TX pause should be enabled.
You need to check the driver to see if it actually implements this
ethtool call, but that is how it should be configured.
Andrew
^ permalink raw reply
* Re: [PATCH iproute2 2/2] devlink: fix libc and kernel headers collision
From: Stephen Hemminger @ 2019-06-25 18:59 UTC (permalink / raw)
To: Baruch Siach; +Cc: Jiri Pirko, netdev, Aya Levin, Moshe Shemesh
In-Reply-To: <7f81026b803edcaeaca05994298118271a2f287d.1561463345.git.baruch@tkos.co.il>
On Tue, 25 Jun 2019 14:49:05 +0300
Baruch Siach <baruch@tkos.co.il> wrote:
> +/* Suppress linux/sysinfo.h to avoid
> + * collision of struct sysinfo definition
> + * with musl libc headers
> + */
You only need a small comment for this.
#define _LINUX_SYSINFO_H /* avoid collision with musl header */
^ permalink raw reply
* Re: [PATCH iproute2 1/2] devlink: fix format string warning for 32bit targets
From: Stephen Hemminger @ 2019-06-25 18:58 UTC (permalink / raw)
To: Baruch Siach; +Cc: Jiri Pirko, netdev, Aya Levin, Moshe Shemesh
In-Reply-To: <016aabe2639668b4710b73157ea39e8f97f7d726.1561463345.git.baruch@tkos.co.il>
On Tue, 25 Jun 2019 14:49:04 +0300
Baruch Siach <baruch@tkos.co.il> wrote:
> diff --git a/devlink/devlink.c b/devlink/devlink.c
> index 436935f88bda..b400fab17578 100644
> --- a/devlink/devlink.c
> +++ b/devlink/devlink.c
> @@ -1726,9 +1726,9 @@ static void pr_out_u64(struct dl *dl, const char *name, uint64_t val)
> jsonw_u64_field(dl->jw, name, val);
> } else {
> if (g_indent_newline)
> - pr_out("%s %lu", name, val);
> + pr_out("%s %llu", name, val);
> else
> - pr_out(" %s %lu", name, val);
> + pr_out(" %s %llu", name, val);
But on 64 bit target %llu expects unsigned long long which is 128bit.
The better way to fix this is to use:
#include <inttypes.h>
And the use the macro PRIu64
pr_out(" %s %"PRIu64, name, val);
^ permalink raw reply
* Re: [PATCH V3 04/15] ARM: exynos: cleanup cppcheck shifting error
From: Krzysztof Kozlowski @ 2019-06-25 18:49 UTC (permalink / raw)
To: Phong Tran
Cc: acme, alexander.shishkin, alexander.sverdlin, allison, andrew,
ast, bgolaszewski, bpf, daniel, daniel, dmg, festevam, gerg,
gregkh, gregory.clement, haojian.zhuang, hsweeten,
illusionist.neo, info, jason, jolsa, kafai, kernel, kgene,
kstewart, linux-arm-kernel, linux-imx, linux-kernel, linux-omap,
linux-samsung-soc, linux, liviu.dudau, lkundrak,
lorenzo.pieralisi, mark.rutland, mingo, namhyung, netdev, nsekhar,
peterz, robert.jarzmik, s.hauer, sebastian.hesselbarth, shawnguo,
songliubraving, sudeep.holla, swinslow, tglx, tony, will, yhs
In-Reply-To: <20190625040356.27473-5-tranmanphong@gmail.com>
On Tue, Jun 25, 2019 at 11:03:45AM +0700, Phong Tran wrote:
> There is error from cppcheck tool
> "Shifting signed 32-bit value by 31 bits is undefined behaviour errors"
> change to use BIT() marco for improvement.
>
> Signed-off-by: Phong Tran <tranmanphong@gmail.com>
> ---
> arch/arm/mach-exynos/suspend.c | 2 +-
Thanks, applied with slightly different commit message. As Peter
pointed, there is no error because of GCC. Usually we expect a reply to
comments on LKML... and also you could take his hints and use them to
improve the commit msg to properly describe what is the problem.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 00/11] XDP unaligned chunk placement support
From: Jonathan Lemon @ 2019-06-25 18:44 UTC (permalink / raw)
To: Kevin Laatz
Cc: netdev, ast, daniel, bjorn.topel, magnus.karlsson, bpf,
intel-wired-lan, bruce.richardson, ciara.loftus
In-Reply-To: <20190620083924.1996-1-kevin.laatz@intel.com>
On 20 Jun 2019, at 1:39, Kevin Laatz wrote:
> This patchset adds the ability to use unaligned chunks in the XDP
> umem.
>
> Currently, all chunk addresses passed to the umem are masked to be
> chunk
> size aligned (default is 2k, max is PAGE_SIZE). This limits where we
> can
> place chunks within the umem as well as limiting the packet sizes that
> are
> supported.
>
> The changes in this patchset removes these restrictions, allowing XDP
> to be
> more flexible in where it can place a chunk within a umem. By relaxing
> where
> the chunks can be placed, it allows us to use an arbitrary buffer size
> and
> place that wherever we have a free address in the umem. These changes
> add the
> ability to support jumboframes and make it easy to integrate with
> other
> existing frameworks that have their own memory management systems,
> such as
> DPDK.
I'm a little unclear on how this should work, and have a few issues
here:
1) There isn't any support for the user defined umem->headroom
2) When queuing RX buffers, the handle (aka umem offset) is used,
which
points to the start of the buffer area. When the buffer appears in
the completion queue, handle points to the start of the received
data,
which might be different from the buffer start address.
Normally, this RX address is just put back in the fill queue, and
the
mask is used to find the buffer start address again. This no
longer
works, so my question is, how is the buffer start address
recomputed
from the actual data payload address?
Same with TX - if the TX payload isn't aligned in with the start of
the buffer, what happens?
3) This appears limited to crossing a single page boundary, but there
is no constraint check on chunk_size.
--
Jonathan
>
> Structure of the patchset:
> Patch 1:
> - Remove unnecessary masking and headroom addition during zero-copy
> Rx
> buffer recycling in i40e. This change is required in order for the
> buffer recycling to work in the unaligned chunk mode.
>
> Patch 2:
> - Remove unnecessary masking and headroom addition during
> zero-copy Rx buffer recycling in ixgbe. This change is required in
> order for the buffer recycling to work in the unaligned chunk
> mode.
>
> Patch 3:
> - Adds an offset parameter to zero_copy_allocator. This change will
> enable us to calculate the original handle in zca_free. This will
> be
> required for unaligned chunk mode since we can't easily mask back
> to
> the original handle.
>
> Patch 4:
> - Adds the offset parameter to i40e_zca_free. This change is needed
> for
> calculating the handle since we can't easily mask back to the
> original
> handle like we can in the aligned case.
>
> Patch 5:
> - Adds the offset parameter to ixgbe_zca_free. This change is needed
> for
> calculating the handle since we can't easily mask back to the
> original
> handle like we can in the aligned case.
>
>
> Patch 6:
> - Add infrastructure for unaligned chunks. Since we are dealing
> with unaligned chunks that could potentially cross a physical page
> boundary, we add checks to keep track of that information. We can
> later use this information to correctly handle buffers that are
> placed at an address where they cross a page boundary.
>
> Patch 7:
> - Add flags for umem configuration to libbpf
>
> Patch 8:
> - Modify xdpsock application to add a command line option for
> unaligned chunks
>
> Patch 9:
> - Addition of command line argument to pass in a desired buffer size
> and buffer recycling for unaligned mode. Passing in a buffer size
> will
> allow the application to use unaligned chunks with the unaligned
> chunk
> mode. Since we are now using unaligned chunks, we need to recycle
> our
> buffers in a slightly different way.
>
> Patch 10:
> - Adds hugepage support to the xdpsock application
>
> Patch 11:
> - Documentation update to include the unaligned chunk scenario. We
> need
> to explicitly state that the incoming addresses are only masked in
> the
> aligned chunk mode and not the unaligned chunk mode.
>
> Kevin Laatz (11):
> i40e: simplify Rx buffer recycle
> ixgbe: simplify Rx buffer recycle
> xdp: add offset param to zero_copy_allocator
> i40e: add offset to zca_free
> ixgbe: add offset to zca_free
> xsk: add support to allow unaligned chunk placement
> libbpf: add flags to umem config
> samples/bpf: add unaligned chunks mode support to xdpsock
> samples/bpf: add buffer recycling for unaligned chunks to xdpsock
> samples/bpf: use hugepages in xdpsock app
> doc/af_xdp: include unaligned chunk case
>
> Documentation/networking/af_xdp.rst | 10 +-
> drivers/net/ethernet/intel/i40e/i40e_xsk.c | 21 ++--
> drivers/net/ethernet/intel/i40e/i40e_xsk.h | 3 +-
> .../ethernet/intel/ixgbe/ixgbe_txrx_common.h | 3 +-
> drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 21 ++--
> include/net/xdp.h | 3 +-
> include/net/xdp_sock.h | 2 +
> include/uapi/linux/if_xdp.h | 4 +
> net/core/xdp.c | 11 ++-
> net/xdp/xdp_umem.c | 17 ++--
> net/xdp/xsk.c | 60 +++++++++--
> net/xdp/xsk_queue.h | 60 +++++++++--
> samples/bpf/xdpsock_user.c | 99
> ++++++++++++++-----
> tools/include/uapi/linux/if_xdp.h | 4 +
> tools/lib/bpf/xsk.c | 7 ++
> tools/lib/bpf/xsk.h | 2 +
> 16 files changed, 241 insertions(+), 86 deletions(-)
>
> --
> 2.17.1
^ permalink raw reply
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: Daniel Santos @ 2019-06-25 18:37 UTC (permalink / raw)
To: Russell King - ARM Linux admin, René van Dorst
Cc: sean.wang, f.fainelli, davem, matthias.bgg, andrew,
vivien.didelot, frank-w, netdev, linux-mediatek, linux-mips
In-Reply-To: <20190625121030.m5w7wi3rpezhfgyo@shell.armlinux.org.uk>
Hello,
Although I'm new to the entire Ethernet / *MII subsystem and I haven't
touched DSA yet, I've recently had to add some of this functionality to
the older OpenWRT drivers for swconfig control over the ports. René, do
you have an actual datasheet or programming guide for the mt7530? I
only have one for the mt7620.
On 6/25/19 7:10 AM, Russell King - ARM Linux admin wrote:
> mac_link_*().
>
>>>> + if (state->pause || phylink_test(state->advertising, Pause))
>>>> + mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
>>>> + if (state->pause & MLO_PAUSE_TX)
>>>> + mcr |= PMCR_TX_FC_EN;
>>>> + if (state->pause & MLO_PAUSE_RX)
>>>> + mcr |= PMCR_RX_FC_EN;
>>> This is clearly wrong - if any bit in state->pause is set, then we
>>> end up with both PMCR_TX_FC_EN | PMCR_RX_FC_EN set. If we have Pause
>>> Pause set in the advertising mask, then both are set. This doesn't
>>> seem right - are these bits setting the advertisement, or are they
>>> telling the MAC to use flow control?
>> Last one, tell the MAC to use flow control.
> So the first if() statement is incorrect, and should be removed
> entirely. You only want to enable the MAC to use flow control as a
> result of the negotiation results.
René,
iiuc, this is what's documented in table 28B-3 of the 802.3 spec on page
598. pdf of section 2 here:
http://www.ismlab.usf.edu/dcom/Ch3_802.3-2005_section2.pdf
>> On the current driver both bits are set in a forced-link situation.
>>
>> If we always forces the MAC mode I think I always set these bits and don't
>> anything with the Pause modes? Is that the right way to do it?
> So what happens if your link partner (e.g. switch) does not support
> flow control? What if your link partner floods such frames to all
> ports? You end up transmitting flow control frames, which could be
> sent to all stations on the network... seems not a good idea.
>
> Implementing stuff properly and not taking short-cuts is always a
> good idea for inter-operability.
But will there still be a mechanism to ignore link partner's advertising
and force these parameters? I've run into what appears to be quirks on
two separate NICs or their drivers, a tp-link tg-3468 (r8169) and an
Aquantia AQC107 802.3bz (atlantic) where these link partners aren't
auto-negotiating correctly after I switch the mt7530 out of
auto-negotiation mode. Of course, it could be a mistake I've made (and
should thus be discussed elsewhere), but iirc, I had to force enable
flow control and then also disable auto-negotiation on the link partner
and force the mode I wanted.
Cheers,
Daniel
^ 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