* Re: selftests/bpf doesn't compile
From: Daniel Borkmann @ 2017-09-15 18:48 UTC (permalink / raw)
To: Alexei Starovoitov, Edward Cree
Cc: Shuah Khan, Thomas Meyer, linux-kernel, linux-kselftest,
Shuah Khan, Networking
In-Reply-To: <59BC1AAC.3040401@iogearbox.net>
On 09/15/2017 08:23 PM, Daniel Borkmann wrote:
> On 09/15/2017 08:07 PM, Alexei Starovoitov wrote:
>> On Fri, Sep 15, 2017 at 05:58:40PM +0100, Edward Cree wrote:
>>> On 15/09/17 17:02, Alexei Starovoitov wrote:
>>>> On Thu, Sep 14, 2017 at 09:33:48AM -0600, Shuah Khan wrote:
>>>>> Is bpf test intended to be run in kselftest run? The clang dependency might
>>>>> not be met on majority of the systems. Is this a hard dependency??
>>>> It is a hard dependency and clang should be present on majority of the systems.
>>> I think this is the wrong approach. Making kselftest hard-require clang doesn't
>>> mean that the bpf tests will be run more often, it means that the rest of the
>>> kselftests will be run less often. clang is quite big (when I tried to install
>>> it on one of my test servers, I didn't have enough disk space & had to go on a
>>> clear-out of unused packages), and most people aren't interested in the bpf
>>> subsystem specifically; they would rather be able to skip those tests.
>>> I feel that as long as they know they are skipping some tests (so e.g. they
>>> won't consider it a sufficient test of a kselftest refactor), that's fine.
>>> It's not even as though all of the bpf tests require clang; the (smaller) tests
>>> written directly in raw eBPF instructions could still be run on such a system.
>>> So I think we should attempt to run as much as possible but accept that clang
>>> may not be available and have an option to skip some tests in that case.
>>
>> imo the value of selftests/bpf is twofold:
>> 1. it helps bpf developers avoid regressions
>> 2. as part of continuous integration it helps to catch bpf regressions
>> that were somehow caused by changes in other parts of the kernel
>>
>> If a developer didn't bother to satisfy all bpf tests dependencies
>> (which includes clang) and ran all tests before sending a patch,
>> I don't want to see such patches. It just wastes maintainers time
>> to review code and spot bugs that could have been caught by tests.
>> Collectively we invested years of work into these tests and
>> developers better take advantage of it by running all.
>
> +1
>
>> If a CI server didn't satisfy all bpf test dependencies,
>> I don't want such CI setup to be running and reporting results,
>> since it will give false sense of test coverage.
>> Test failures due to missing dependencies are hard failures.
>> We cannot skip them.
>
> +1
Btw, on that note, the folks from zero-day bot run the BPF kselftests
for a while now just fine and they do run them together with clang,
so they have the full, proper coverage how it should be. It's not
how it used to be in the early days, you can just go and install
llvm/clang package on all the major distros today and you get the
bpf target by default enabled already.
>> I'd like generic XDP tests to be added to selftests/bpf which
>> would mean that the latest iproute2 will become a hard dependency
>> and bpf developers and CI host owners would need to upgrade
>> their iproute2.
>> The tests either pass or fail. Skipping them due to missing
>> dependencies is the same as fail and in that sense I don't want
>> to change selftests/bpf/Makefile to make it skip clang.
>
> I fully agree that for the BPF selftests it is very desirable
> to not only test the verifier with couple of BPF insn snippets,
> but to actually load and run programs that more closely resemble
> real world programs. For more complex interactions these snippets
> are just limited, think of tail calls, testing perf event output
> helper, etc, which would all require to write these tests with
> restricted C when we add them (unless we want to make writing
> these tests a real pain ;) in which case no-one will bother to
> write tests at all for them). Mid to long term I would definitely
> like to see more programs in BPF selftests (e.g. moved over from
> samples/bpf/) to increase the test coverage.
^ permalink raw reply
* Re: [PATCH net] packet: hold bind lock when rebinding to fanout hook
From: Cong Wang @ 2017-09-15 18:33 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Linux Kernel Network Developers, David Miller, nixiaoming
In-Reply-To: <20170914211441.67326-1-willemb@google.com>
On Thu, Sep 14, 2017 at 2:14 PM, Willem de Bruijn <willemb@google.com> wrote:
> Packet socket bind operations must hold the po->bind_lock. This keeps
> po->running consistent with whether the socket is actually on a ptype
> list to receive packets.
>
> fanout_add unbinds a socket and its packet_rcv/tpacket_rcv call, then
> binds the fanout object to receive through packet_rcv_fanout.
>
> Make it hold the po->bind_lock when testing po->running and rebinding.
> Else, it can race with other rebind operations, such as that in
> packet_set_ring from packet_rcv to tpacket_rcv. Concurrent updates
> can result in a socket being added to a fanout group twice, causing
> use-after-free KASAN bug reports, among others.
>
> Reported independently by both trinity and syzkaller.
> Verified that the syzkaller reproducer passes after this patch.
>
> Reported-by: nixioaming <nixiaoming@huawei.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
> net/packet/af_packet.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index c26172995511..d288f52c53f7 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1684,10 +1684,6 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
>
> mutex_lock(&fanout_mutex);
>
> - err = -EINVAL;
> - if (!po->running)
> - goto out;
> -
> err = -EALREADY;
> if (po->fanout)
> goto out;
> @@ -1749,7 +1745,10 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
> list_add(&match->list, &fanout_list);
> }
> err = -EINVAL;
> - if (match->type == type &&
> +
> + spin_lock(&po->bind_lock);
> + if (po->running &&
As you move the po->running check later after setting po->rollover, I wonder
if po->rollover possibly depends on po>running on other path?
> + match->type == type &&
> match->prot_hook.type == po->prot_hook.type &&
> match->prot_hook.dev == po->prot_hook.dev) {
> err = -ENOSPC;
> @@ -1761,6 +1760,13 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)
> err = 0;
> }
> }
> + spin_unlock(&po->bind_lock);
> +
> + if (err && !refcount_read(&match->sk_ref)) {
> + list_del(&match->list);
> + kfree(match);
> + }
This looks correct but still seems odd, it smells you don't use refcnt in an
expected way.
^ permalink raw reply
* Re: selftests/bpf doesn't compile
From: Daniel Borkmann @ 2017-09-15 18:23 UTC (permalink / raw)
To: Alexei Starovoitov, Edward Cree
Cc: Shuah Khan, Thomas Meyer, linux-kernel, linux-kselftest,
Shuah Khan, Networking
In-Reply-To: <20170915180751.rnmvttf4lnz26qit@ast-mbp>
On 09/15/2017 08:07 PM, Alexei Starovoitov wrote:
> On Fri, Sep 15, 2017 at 05:58:40PM +0100, Edward Cree wrote:
>> On 15/09/17 17:02, Alexei Starovoitov wrote:
>>> On Thu, Sep 14, 2017 at 09:33:48AM -0600, Shuah Khan wrote:
>>>> Is bpf test intended to be run in kselftest run? The clang dependency might
>>>> not be met on majority of the systems. Is this a hard dependency??
>>> It is a hard dependency and clang should be present on majority of the systems.
>> I think this is the wrong approach. Making kselftest hard-require clang doesn't
>> mean that the bpf tests will be run more often, it means that the rest of the
>> kselftests will be run less often. clang is quite big (when I tried to install
>> it on one of my test servers, I didn't have enough disk space & had to go on a
>> clear-out of unused packages), and most people aren't interested in the bpf
>> subsystem specifically; they would rather be able to skip those tests.
>> I feel that as long as they know they are skipping some tests (so e.g. they
>> won't consider it a sufficient test of a kselftest refactor), that's fine.
>> It's not even as though all of the bpf tests require clang; the (smaller) tests
>> written directly in raw eBPF instructions could still be run on such a system.
>> So I think we should attempt to run as much as possible but accept that clang
>> may not be available and have an option to skip some tests in that case.
>
> imo the value of selftests/bpf is twofold:
> 1. it helps bpf developers avoid regressions
> 2. as part of continuous integration it helps to catch bpf regressions
> that were somehow caused by changes in other parts of the kernel
>
> If a developer didn't bother to satisfy all bpf tests dependencies
> (which includes clang) and ran all tests before sending a patch,
> I don't want to see such patches. It just wastes maintainers time
> to review code and spot bugs that could have been caught by tests.
> Collectively we invested years of work into these tests and
> developers better take advantage of it by running all.
+1
> If a CI server didn't satisfy all bpf test dependencies,
> I don't want such CI setup to be running and reporting results,
> since it will give false sense of test coverage.
> Test failures due to missing dependencies are hard failures.
> We cannot skip them.
+1
> I'd like generic XDP tests to be added to selftests/bpf which
> would mean that the latest iproute2 will become a hard dependency
> and bpf developers and CI host owners would need to upgrade
> their iproute2.
> The tests either pass or fail. Skipping them due to missing
> dependencies is the same as fail and in that sense I don't want
> to change selftests/bpf/Makefile to make it skip clang.
I fully agree that for the BPF selftests it is very desirable
to not only test the verifier with couple of BPF insn snippets,
but to actually load and run programs that more closely resemble
real world programs. For more complex interactions these snippets
are just limited, think of tail calls, testing perf event output
helper, etc, which would all require to write these tests with
restricted C when we add them (unless we want to make writing
these tests a real pain ;) in which case no-one will bother to
write tests at all for them). Mid to long term I would definitely
like to see more programs in BPF selftests (e.g. moved over from
samples/bpf/) to increase the test coverage.
^ permalink raw reply
* Re: selftests/bpf doesn't compile
From: Alexei Starovoitov @ 2017-09-15 18:14 UTC (permalink / raw)
To: Shuah Khan
Cc: Shuah Khan, Daniel Borkmann, Thomas Meyer, linux-kernel,
linux-kselftest, Networking
In-Reply-To: <e07dabee-6c9c-7842-65b0-990a2ed1a2e9@osg.samsung.com>
On Fri, Sep 15, 2017 at 11:00:31AM -0600, Shuah Khan wrote:
> >> I could add a special target for bpf TARGET_BPF perhaps and exclude it from
> >> the run_test>
> > I'm not sure what was the motivation to exclude hotplug from default testing,
>
> These are considered a bit more disruptive and were excluded a while
> back. These take cpus and memory on and off-line. Also require
> root access. So even if they are included in the regular run, these
> won't run.
most of bpf tests require root access as well.
> The first failure due to clang dependency is not a problem. The second one
> in the case of "make kselftest" is the one that requires some work when bpf
> make is run from the main Makefile. A lots of users run tests using the
> kselftest target from the mail Makefile. hence I would like to get this
> working, so it would be easier to run this test on test servers.
'make kselftest' doesn't work for me at all, since I suspect it
assumes in-source build. I always use KBUILD_OUTPUT,
since I build multiple archs with different configs out of the same
source tree, so there is a bigger problem here.
$ make kselftest
make[1]: Entering directory `/data/users/ast/net-next/bld_x64'
make: Entering an unknown directory
make: *** tools/testing/selftests: No such file or directory. Stop.
make: Leaving an unknown directory
make[1]: *** [kselftest] Error 2
make[1]: Leaving directory `/data/users/ast/net-next/bld_x64'
make: *** [sub-make] Error 2
^ permalink raw reply
* Re: [PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook
From: Cong Wang @ 2017-09-15 18:09 UTC (permalink / raw)
To: Willem de Bruijn
Cc: nixiaoming, David Miller, Eric Dumazet, waltje, gw4pts,
Andrey Konovalov, Tobias Klauser, Philip Pettersson,
Alexander Potapenko, Network Development, LKML, dede.wu
In-Reply-To: <CAF=yD-LZMjB5u0JRX36fYgV1HR6WqyJOGKopk5CLR=0rFBEV7g@mail.gmail.com>
On Fri, Sep 15, 2017 at 10:46 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> In case of failure we also need to unlink and free match. I
> sent the following:
>
> http://patchwork.ozlabs.org/patch/813945/
Ah, will take a look.
^ permalink raw reply
* Re: selftests/bpf doesn't compile
From: Alexei Starovoitov @ 2017-09-15 18:07 UTC (permalink / raw)
To: Edward Cree
Cc: Shuah Khan, Daniel Borkmann, Thomas Meyer, linux-kernel,
linux-kselftest, Shuah Khan, Networking
In-Reply-To: <701006c0-f80a-6bd7-fb5e-5bca1dc33a31@solarflare.com>
On Fri, Sep 15, 2017 at 05:58:40PM +0100, Edward Cree wrote:
> On 15/09/17 17:02, Alexei Starovoitov wrote:
> > On Thu, Sep 14, 2017 at 09:33:48AM -0600, Shuah Khan wrote:
> >> Is bpf test intended to be run in kselftest run? The clang dependency might
> >> not be met on majority of the systems. Is this a hard dependency??
> > It is a hard dependency and clang should be present on majority of the systems.
> I think this is the wrong approach. Making kselftest hard-require clang doesn't
> mean that the bpf tests will be run more often, it means that the rest of the
> kselftests will be run less often. clang is quite big (when I tried to install
> it on one of my test servers, I didn't have enough disk space & had to go on a
> clear-out of unused packages), and most people aren't interested in the bpf
> subsystem specifically; they would rather be able to skip those tests.
> I feel that as long as they know they are skipping some tests (so e.g. they
> won't consider it a sufficient test of a kselftest refactor), that's fine.
> It's not even as though all of the bpf tests require clang; the (smaller) tests
> written directly in raw eBPF instructions could still be run on such a system.
> So I think we should attempt to run as much as possible but accept that clang
> may not be available and have an option to skip some tests in that case.
imo the value of selftests/bpf is twofold:
1. it helps bpf developers avoid regressions
2. as part of continuous integration it helps to catch bpf regressions
that were somehow caused by changes in other parts of the kernel
If a developer didn't bother to satisfy all bpf tests dependencies
(which includes clang) and ran all tests before sending a patch,
I don't want to see such patches. It just wastes maintainers time
to review code and spot bugs that could have been caught by tests.
Collectively we invested years of work into these tests and
developers better take advantage of it by running all.
If a CI server didn't satisfy all bpf test dependencies,
I don't want such CI setup to be running and reporting results,
since it will give false sense of test coverage.
Test failures due to missing dependencies are hard failures.
We cannot skip them.
I'd like generic XDP tests to be added to selftests/bpf which
would mean that the latest iproute2 will become a hard dependency
and bpf developers and CI host owners would need to upgrade
their iproute2.
The tests either pass or fail. Skipping them due to missing
dependencies is the same as fail and in that sense I don't want
to change selftests/bpf/Makefile to make it skip clang.
^ permalink raw reply
* Re: [PATCH net] bpf/verifier: reject BPF_ALU64|BPF_END
From: Daniel Borkmann @ 2017-09-15 18:01 UTC (permalink / raw)
To: Edward Cree, David Miller; +Cc: netdev
In-Reply-To: <8c3d5513-6171-3e68-56df-1efd0e87f071@solarflare.com>
On 09/15/2017 03:37 PM, Edward Cree wrote:
> Neither ___bpf_prog_run nor the JITs accept it.
> Also adds a new test case.
>
> Fixes: 17a5267067f3 ("bpf: verifier (add verifier core)")
> Signed-off-by: Edward Cree <ecree@solarflare.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
^ permalink raw reply
* Re: 319554f284dd ("inet: don't use sk_v6_rcv_saddr directly") causes bind port regression
From: Josef Bacik @ 2017-09-15 17:51 UTC (permalink / raw)
To: Cole Robinson, Laura Abbott, David S. Miller, Alexey Kuznetsov,
Hideaki YOSHIFUJI
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <813386f2-81ba-db8c-e86d-f36cd1b89537@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 530 bytes --]
Finally got access to a box to run this down myself. This patch on top of the other patches fixes the problem for me, could you verify it works for you? Thanks,
Josef
On 9/13/17, 3:49 PM, "Cole Robinson" <crobinso@redhat.com> wrote:
On 09/13/2017 03:44 PM, Josef Bacik wrote:
> Alright thanks, this should fix it.
>
Still no luck with all three patches applied to fedora 4.12.8-300 RPM. Pretty
sure I didn't mess up the testing but since I rarely do kernel builds it's not
impossible...
Thanks,
Cole
[-- Attachment #2: 0001-net-call-sk_reuseport_match-if-we-are-a-reusesock.patch --]
[-- Type: application/octet-stream, Size: 1223 bytes --]
From b70e7a78af2c4c090ca816d9f127a2f1e5866fb8 Mon Sep 17 00:00:00 2001
From: Josef Bacik <josef@toxicpanda.com>
Date: Fri, 15 Sep 2017 10:48:17 -0700
Subject: [PATCH] net: call sk_reuseport_match if we are a reusesock
When adding the sk_reuseport_match helper I screwed up and made it so we
unconditionally succeeded if our socket was a reuseport socket and the
tb was marked reuseable. Fix this by actually calling
sk_reuseport_match() to make sure our new socket matches the tb
reuseport settings.
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
net/ipv4/inet_connection_sock.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index fe9cf4862de2..0e30e504c7d4 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -316,8 +316,7 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum)
if (sk->sk_reuse == SK_FORCE_REUSE)
goto success;
- if ((tb->fastreuse > 0 && reuse) ||
- sk_reuseport_match(tb, sk))
+ if (tb->fastreuse > 0 && reuse && sk_reuseport_match(tb, sk))
goto success;
if (inet_csk_bind_conflict(sk, tb, true, true))
goto fail_unlock;
--
2.13.5
^ permalink raw reply related
* Re: [PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook
From: Willem de Bruijn @ 2017-09-15 17:46 UTC (permalink / raw)
To: Cong Wang
Cc: nixiaoming, David Miller, Eric Dumazet, waltje, gw4pts,
Andrey Konovalov, Tobias Klauser, Philip Pettersson,
Alexander Potapenko, Network Development, LKML, dede.wu
In-Reply-To: <CAM_iQpWAeB+8bm9u+eHS3k_h5PSbnztXBvzTC=f0-tohdiSgzQ@mail.gmail.com>
On Fri, Sep 15, 2017 at 1:41 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Sep 14, 2017 at 7:35 AM, Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
>> On Thu, Sep 14, 2017 at 10:07 AM, nixiaoming <nixiaoming@huawei.com> wrote:
>>> From: l00219569 <lisimin@huawei.com>
>>>
>>> If fanout_add is preempted after running po-> fanout = match
>>> and before running __fanout_link,
>>> it will cause BUG_ON when __unregister_prot_hook call __fanout_unlink
>>>
>>> so, we need add mutex_lock(&fanout_mutex) to __unregister_prot_hook
>>
>> The packet socket code has no shortage of locks, so there are many
>> ways to avoid the race condition between fanout_add and packet_set_ring.
>>
>> Another option would be to lock the socket when calling fanout_add:
>>
>> - return fanout_add(sk, val & 0xffff, val >> 16);
>> + lock_sock(sk);
>> + ret = fanout_add(sk, val & 0xffff, val >> 16);
>> + release_sock(sk);
>> + return ret;
>>
>
> I don't think this is an option, because __unregister_prot_hook()
> can be called without lock_sock(), for example in packet_notifier().
>
>
>> But, for consistency, and to be able to continue to make sense of the
>> locking policy, we should use the most appropriate lock. This
>> is po->bind_lock, as it ensures atomicity between testing whether
>> a protocol hook is active through po->running and the actual existence
>> of that hook on the protocol hook list.
>
> Yeah, register_prot_hook() and unregister_prot_hook() already assume
> bind_lock.
>
> [...]
>
>>> out:
>>> mutex_unlock(&fanout_mutex);
>>> + spin_unlock(&po->bind_lock);
>>
>> This function can call kzalloc with GFP_KERNEL, which may sleep. It is
>> not correct to sleep while holding a spinlock. Which is why I take the lock
>> later and test po->running again.
>
>
> Right, no need to mention the mutex_unlock() before the spin_unlock()
> is clearly wrong.
>
>
>>
>> I will clean up that patch and send it for review.
>
> How about the following patch?
>
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index c26172995511..f5c696a548ed 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1754,10 +1754,14 @@ static int fanout_add(struct sock *sk, u16 id,
> u16 type_flags)
> match->prot_hook.dev == po->prot_hook.dev) {
> err = -ENOSPC;
> if (refcount_read(&match->sk_ref) < PACKET_FANOUT_MAX) {
> + spin_lock(&po->bind_lock);
> __dev_remove_pack(&po->prot_hook);
> - po->fanout = match;
> - refcount_set(&match->sk_ref,
> refcount_read(&match->sk_ref) + 1);
> - __fanout_link(sk, po);
> + if (po->running) {
> + refcount_set(&match->sk_ref,
> refcount_read(&match->sk_ref) + 1);
> + po->fanout = match;
> + __fanout_link(sk, po);
> + }
> + spin_unlock(&po->bind_lock);
> err = 0;
> }
> }
In case of failure we also need to unlink and free match. I
sent the following:
http://patchwork.ozlabs.org/patch/813945/
^ permalink raw reply
* Re: selftests/bpf doesn't compile
From: Shuah Khan @ 2017-09-15 17:44 UTC (permalink / raw)
To: Alexei Starovoitov, Shuah Khan
Cc: Daniel Borkmann, Thomas Meyer, linux-kernel, linux-kselftest,
Networking, Shuah Khan
In-Reply-To: <e07dabee-6c9c-7842-65b0-990a2ed1a2e9@osg.samsung.com>
On 09/15/2017 11:00 AM, Shuah Khan wrote:
> On 09/15/2017 10:02 AM, Alexei Starovoitov wrote:
>> On Thu, Sep 14, 2017 at 09:33:48AM -0600, Shuah Khan wrote:
>>> Hi Alexei and Daniel,
>>>
>>> bpf test depends on clang and fails to compile when
>>>
>>> ------------------------------------------------------
>>> make -C tools/testing/selftests/bpf run_tests
>>>
>>>
>>> make: clang: Command not found
>>> Makefile:39: recipe for target '.linux-kselftest/tools/testing/selftests/bpf/test_pkt_access.o' failed
>>> make: *** [./linux-kselftest/tools/testing/selftests/bpf/test_pkt_access.o] Error 127
>>> make: Leaving directory '.linux-kselftest/tools/testing/selftests/bpf'
>>>
>>> With "make TARGETS=bpf kselftest" it fails earlier:
>>>
>>>
>>> make[3]: Entering directory './linux-kselftest/tools/lib/bpf'
>>> Makefile:40: tools/scripts/Makefile.arch: No such file or directory
>>> Makefile:84: tools/build/Makefile.feature: No such file or directory
>>> Makefile:143: tools/build/Makefile.include: No such file or directory
>>> make[3]: *** No rule to make target 'tools/build/Makefile.include'. Stop.
>>> make[3]: Leaving directory '.linux-kselftest/tools/lib/bpf'
>>> Makefile:34: recipe for target './linux-kselftest/tools/testing/selftests/bpf/libbpf.a' failed
>>> make[2]: *** [./linux-kselftest/tools/testing/selftests/bpf/libbpf.a] Error 2
>>> make[2]: Leaving directory './linux-kselftest/tools/testing/selftests/bpf'
>>> Makefile:69: recipe for target 'all' failed
>>> make[1]: *** [all] Error 2
>>> Makefile:1190: recipe for target 'kselftest' failed
>>> make: *** [kselftest] Error 2
>>>
>>> --------------------------------------------------------------
>>>
>>> Is bpf test intended to be run in kselftest run? The clang dependency might
>>> not be met on majority of the systems. Is this a hard dependency??
>>
>> It is a hard dependency and clang should be present on majority of the systems.
>> More details are in samples/bpf/README.rst
>> which was written long ago. Nowadays apt-get/yum will install clang
>> with bpf support builtin.
>
> Thanks for the clarification.
>
>>
>>> Would it make sense to create a special target for bpf test? We do have a few
>>> tests that do that now.
>>>
>>> TARGETS_HOTPLUG = cpu-hotplug
>>> TARGETS_HOTPLUG += memory-hotplug
>>>
>>> I could add a special target for bpf TARGET_BPF perhaps and exclude it from
>>> the run_test>
>> I'm not sure what was the motivation to exclude hotplug from default testing,
>
> These are considered a bit more disruptive and were excluded a while
> back. These take cpus and memory on and off-line. Also require
> root access. So even if they are included in the regular run, these
> won't run.
>
>> since I think it diminishes the value of selftests overall.
>
> I agree. We do have some timer tests that are destructive/stress that
> et run using a special target. It is the idea that if somebody wants
> to test all them, there is a way to do that.
>
> In any case, I didn't think bpf falls into this category of tests that
> belong in the destructive category. I am looking to understand the failures
> and your take on those.
>
>> Not running all tests all the time risks breaking them
> It is balance of providing a choice to users if they don't want to
> run destructive tests. For example, suspend test which requires root
> access. So the idea is for users to run these by choice as opposed
> to running them in the normal run and cause disruption.
>
>> selftest makefile refactoring broke selftests/bpf in the past,
>
> Yeah. We have had some fallout from the KBUILD_OUTPUT work that didn't
> take all the use-cases into account and tests that require custom
> builds such as the bpf tests. Using common build infrastructure doesn't
> work for all tests.
>
> Looks like there are other patches that went in later with lcap work.
>
>> so I strongly suggest to install clang and make sure the tests are passing
>> on the test servers
>
> You will have to request kernelci, stable, and It is a choice to be made by
> kernelci/zero-day folks.
>
> otherwise we'd need to move selftests/bpf out of
>> selftests to avoid further headaches for us when selftests infra keeps
>> changing.
>>
>
> Let's not go to extreme options. :) I am merely looking for more information
> and trying to understand the dependencies for this test.
>
> Let's look for a constructive option to fix the build failures I am seeing.
>
> The first failure due to clang dependency is not a problem.
Let me clarify that. People interested in running bpf test will have to
install clang. In that sense installing clang will solve that issue.
The hard dependency on clang does make it difficult for developers to
ensure they didn't break bpf when they make changes to the kselftest
common infrastructure.
The second one
> in the case of "make kselftest" is the one that requires some work when bpf
> make is run from the main Makefile. A lots of users run tests using the
> kselftest target from the mail Makefile. hence I would like to get this
> working, so it would be easier to run this test on test servers.
>
Even if this is fixed, unless users choose to install clang, bpf will always
fail run without clang. So clang dependency is an issue for bpf test coverage
in general. But that is your choice as to whether you want to increase the
scope of regression test coverage for bpf or not.
thanks,
-- Shuah
^ permalink raw reply
* Re: [PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook
From: Cong Wang @ 2017-09-15 17:41 UTC (permalink / raw)
To: Willem de Bruijn
Cc: nixiaoming, David Miller, Eric Dumazet, waltje, gw4pts,
Andrey Konovalov, Tobias Klauser, Philip Pettersson,
Alexander Potapenko, Network Development, LKML, dede.wu
In-Reply-To: <CAF=yD-Jt5boxgQne4s1Fzy0y=Lq7RO4fjF6BfOCoh1bgaSECHQ@mail.gmail.com>
On Thu, Sep 14, 2017 at 7:35 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Thu, Sep 14, 2017 at 10:07 AM, nixiaoming <nixiaoming@huawei.com> wrote:
>> From: l00219569 <lisimin@huawei.com>
>>
>> If fanout_add is preempted after running po-> fanout = match
>> and before running __fanout_link,
>> it will cause BUG_ON when __unregister_prot_hook call __fanout_unlink
>>
>> so, we need add mutex_lock(&fanout_mutex) to __unregister_prot_hook
>
> The packet socket code has no shortage of locks, so there are many
> ways to avoid the race condition between fanout_add and packet_set_ring.
>
> Another option would be to lock the socket when calling fanout_add:
>
> - return fanout_add(sk, val & 0xffff, val >> 16);
> + lock_sock(sk);
> + ret = fanout_add(sk, val & 0xffff, val >> 16);
> + release_sock(sk);
> + return ret;
>
I don't think this is an option, because __unregister_prot_hook()
can be called without lock_sock(), for example in packet_notifier().
> But, for consistency, and to be able to continue to make sense of the
> locking policy, we should use the most appropriate lock. This
> is po->bind_lock, as it ensures atomicity between testing whether
> a protocol hook is active through po->running and the actual existence
> of that hook on the protocol hook list.
Yeah, register_prot_hook() and unregister_prot_hook() already assume
bind_lock.
[...]
>> out:
>> mutex_unlock(&fanout_mutex);
>> + spin_unlock(&po->bind_lock);
>
> This function can call kzalloc with GFP_KERNEL, which may sleep. It is
> not correct to sleep while holding a spinlock. Which is why I take the lock
> later and test po->running again.
Right, no need to mention the mutex_unlock() before the spin_unlock()
is clearly wrong.
>
> I will clean up that patch and send it for review.
How about the following patch?
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index c26172995511..f5c696a548ed 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1754,10 +1754,14 @@ static int fanout_add(struct sock *sk, u16 id,
u16 type_flags)
match->prot_hook.dev == po->prot_hook.dev) {
err = -ENOSPC;
if (refcount_read(&match->sk_ref) < PACKET_FANOUT_MAX) {
+ spin_lock(&po->bind_lock);
__dev_remove_pack(&po->prot_hook);
- po->fanout = match;
- refcount_set(&match->sk_ref,
refcount_read(&match->sk_ref) + 1);
- __fanout_link(sk, po);
+ if (po->running) {
+ refcount_set(&match->sk_ref,
refcount_read(&match->sk_ref) + 1);
+ po->fanout = match;
+ __fanout_link(sk, po);
+ }
+ spin_unlock(&po->bind_lock);
err = 0;
}
}
^ permalink raw reply related
* Re: scheduling while atomic from vmci_transport_recv_stream_cb in 3.16 kernels
From: Ben Hutchings @ 2017-09-15 17:12 UTC (permalink / raw)
To: Michal Hocko, Jorgen S. Hansen
Cc: Aditya Sarwade, Thomas Hellstrom, LKML, netdev@vger.kernel.org,
Masik Petr, Sasha Levin, Stable tree
In-Reply-To: <20170914085959.nkiefbzupwmknncy@dhcp22.suse.cz>
[-- Attachment #1: Type: text/plain, Size: 509 bytes --]
On Thu, 2017-09-14 at 10:59 +0200, Michal Hocko wrote:
> On Wed 13-09-17 18:58:13, Jorgen S. Hansen wrote:
> [...]
> > The patch series look good to me.
>
> Thanks for double checking. Ben, could you merge this to 3.16 stable
> branch, please?
I have a long list of requests to work through, but I will get to this
eventually.
Ben.
--
Ben Hutchings
Kids! Bringing about Armageddon can be dangerous. Do not attempt it in
your own home. - Terry Pratchett and Neil Gaiman, `Good Omens'
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* (unknown),
From: noreply @ 2017-09-15 17:01 UTC (permalink / raw)
To: netdev
[-- Attachment #1: EMAIL_75480323541895_netdev.doc --]
[-- Type: application/msword, Size: 76645 bytes --]
^ permalink raw reply
* Re: selftests/bpf doesn't compile
From: Shuah Khan @ 2017-09-15 17:00 UTC (permalink / raw)
To: Alexei Starovoitov, Shuah Khan
Cc: Daniel Borkmann, Thomas Meyer, linux-kernel, linux-kselftest,
Networking, Shuah Khan
In-Reply-To: <20170915160253.q3x5j7hfkxxh2g6w@ast-mbp>
On 09/15/2017 10:02 AM, Alexei Starovoitov wrote:
> On Thu, Sep 14, 2017 at 09:33:48AM -0600, Shuah Khan wrote:
>> Hi Alexei and Daniel,
>>
>> bpf test depends on clang and fails to compile when
>>
>> ------------------------------------------------------
>> make -C tools/testing/selftests/bpf run_tests
>>
>>
>> make: clang: Command not found
>> Makefile:39: recipe for target '.linux-kselftest/tools/testing/selftests/bpf/test_pkt_access.o' failed
>> make: *** [./linux-kselftest/tools/testing/selftests/bpf/test_pkt_access.o] Error 127
>> make: Leaving directory '.linux-kselftest/tools/testing/selftests/bpf'
>>
>> With "make TARGETS=bpf kselftest" it fails earlier:
>>
>>
>> make[3]: Entering directory './linux-kselftest/tools/lib/bpf'
>> Makefile:40: tools/scripts/Makefile.arch: No such file or directory
>> Makefile:84: tools/build/Makefile.feature: No such file or directory
>> Makefile:143: tools/build/Makefile.include: No such file or directory
>> make[3]: *** No rule to make target 'tools/build/Makefile.include'. Stop.
>> make[3]: Leaving directory '.linux-kselftest/tools/lib/bpf'
>> Makefile:34: recipe for target './linux-kselftest/tools/testing/selftests/bpf/libbpf.a' failed
>> make[2]: *** [./linux-kselftest/tools/testing/selftests/bpf/libbpf.a] Error 2
>> make[2]: Leaving directory './linux-kselftest/tools/testing/selftests/bpf'
>> Makefile:69: recipe for target 'all' failed
>> make[1]: *** [all] Error 2
>> Makefile:1190: recipe for target 'kselftest' failed
>> make: *** [kselftest] Error 2
>>
>> --------------------------------------------------------------
>>
>> Is bpf test intended to be run in kselftest run? The clang dependency might
>> not be met on majority of the systems. Is this a hard dependency??
>
> It is a hard dependency and clang should be present on majority of the systems.
> More details are in samples/bpf/README.rst
> which was written long ago. Nowadays apt-get/yum will install clang
> with bpf support builtin.
Thanks for the clarification.
>
>> Would it make sense to create a special target for bpf test? We do have a few
>> tests that do that now.
>>
>> TARGETS_HOTPLUG = cpu-hotplug
>> TARGETS_HOTPLUG += memory-hotplug
>>
>> I could add a special target for bpf TARGET_BPF perhaps and exclude it from
>> the run_test>
> I'm not sure what was the motivation to exclude hotplug from default testing,
These are considered a bit more disruptive and were excluded a while
back. These take cpus and memory on and off-line. Also require
root access. So even if they are included in the regular run, these
won't run.
> since I think it diminishes the value of selftests overall.
I agree. We do have some timer tests that are destructive/stress that
et run using a special target. It is the idea that if somebody wants
to test all them, there is a way to do that.
In any case, I didn't think bpf falls into this category of tests that
belong in the destructive category. I am looking to understand the failures
and your take on those.
> Not running all tests all the time risks breaking them
It is balance of providing a choice to users if they don't want to
run destructive tests. For example, suspend test which requires root
access. So the idea is for users to run these by choice as opposed
to running them in the normal run and cause disruption.
> selftest makefile refactoring broke selftests/bpf in the past,
Yeah. We have had some fallout from the KBUILD_OUTPUT work that didn't
take all the use-cases into account and tests that require custom
builds such as the bpf tests. Using common build infrastructure doesn't
work for all tests.
Looks like there are other patches that went in later with lcap work.
> so I strongly suggest to install clang and make sure the tests are passing
> on the test servers
You will have to request kernelci, stable, and It is a choice to be made by
kernelci/zero-day folks.
otherwise we'd need to move selftests/bpf out of
> selftests to avoid further headaches for us when selftests infra keeps
> changing.
>
Let's not go to extreme options. :) I am merely looking for more information
and trying to understand the dependencies for this test.
Let's look for a constructive option to fix the build failures I am seeing.
The first failure due to clang dependency is not a problem. The second one
in the case of "make kselftest" is the one that requires some work when bpf
make is run from the main Makefile. A lots of users run tests using the
kselftest target from the mail Makefile. hence I would like to get this
working, so it would be easier to run this test on test servers.
thanks,
-- Shuah
^ permalink raw reply
* Re: selftests/bpf doesn't compile
From: Edward Cree @ 2017-09-15 16:58 UTC (permalink / raw)
To: Alexei Starovoitov, Shuah Khan
Cc: Daniel Borkmann, Thomas Meyer, linux-kernel, linux-kselftest,
Shuah Khan, Networking
In-Reply-To: <20170915160253.q3x5j7hfkxxh2g6w@ast-mbp>
On 15/09/17 17:02, Alexei Starovoitov wrote:
> On Thu, Sep 14, 2017 at 09:33:48AM -0600, Shuah Khan wrote:
>> Is bpf test intended to be run in kselftest run? The clang dependency might
>> not be met on majority of the systems. Is this a hard dependency??
> It is a hard dependency and clang should be present on majority of the systems.
I think this is the wrong approach. Making kselftest hard-require clang doesn't
mean that the bpf tests will be run more often, it means that the rest of the
kselftests will be run less often. clang is quite big (when I tried to install
it on one of my test servers, I didn't have enough disk space & had to go on a
clear-out of unused packages), and most people aren't interested in the bpf
subsystem specifically; they would rather be able to skip those tests.
I feel that as long as they know they are skipping some tests (so e.g. they
won't consider it a sufficient test of a kselftest refactor), that's fine.
It's not even as though all of the bpf tests require clang; the (smaller) tests
written directly in raw eBPF instructions could still be run on such a system.
So I think we should attempt to run as much as possible but accept that clang
may not be available and have an option to skip some tests in that case.
-Ed
^ permalink raw reply
* Re: selftests/bpf doesn't compile
From: Alexei Starovoitov @ 2017-09-15 16:02 UTC (permalink / raw)
To: Shuah Khan
Cc: Daniel Borkmann, Thomas Meyer, linux-kernel, linux-kselftest,
Shuah Khan, Networking
In-Reply-To: <295553a4-aadc-e5d1-229e-22d1966bc9f5@kernel.org>
On Thu, Sep 14, 2017 at 09:33:48AM -0600, Shuah Khan wrote:
> Hi Alexei and Daniel,
>
> bpf test depends on clang and fails to compile when
>
> ------------------------------------------------------
> make -C tools/testing/selftests/bpf run_tests
>
>
> make: clang: Command not found
> Makefile:39: recipe for target '.linux-kselftest/tools/testing/selftests/bpf/test_pkt_access.o' failed
> make: *** [./linux-kselftest/tools/testing/selftests/bpf/test_pkt_access.o] Error 127
> make: Leaving directory '.linux-kselftest/tools/testing/selftests/bpf'
>
> With "make TARGETS=bpf kselftest" it fails earlier:
>
>
> make[3]: Entering directory './linux-kselftest/tools/lib/bpf'
> Makefile:40: tools/scripts/Makefile.arch: No such file or directory
> Makefile:84: tools/build/Makefile.feature: No such file or directory
> Makefile:143: tools/build/Makefile.include: No such file or directory
> make[3]: *** No rule to make target 'tools/build/Makefile.include'. Stop.
> make[3]: Leaving directory '.linux-kselftest/tools/lib/bpf'
> Makefile:34: recipe for target './linux-kselftest/tools/testing/selftests/bpf/libbpf.a' failed
> make[2]: *** [./linux-kselftest/tools/testing/selftests/bpf/libbpf.a] Error 2
> make[2]: Leaving directory './linux-kselftest/tools/testing/selftests/bpf'
> Makefile:69: recipe for target 'all' failed
> make[1]: *** [all] Error 2
> Makefile:1190: recipe for target 'kselftest' failed
> make: *** [kselftest] Error 2
>
> --------------------------------------------------------------
>
> Is bpf test intended to be run in kselftest run? The clang dependency might
> not be met on majority of the systems. Is this a hard dependency??
It is a hard dependency and clang should be present on majority of the systems.
More details are in samples/bpf/README.rst
which was written long ago. Nowadays apt-get/yum will install clang
with bpf support builtin.
> Would it make sense to create a special target for bpf test? We do have a few
> tests that do that now.
>
> TARGETS_HOTPLUG = cpu-hotplug
> TARGETS_HOTPLUG += memory-hotplug
>
> I could add a special target for bpf TARGET_BPF perhaps and exclude it from
> the run_tests.
I'm not sure what was the motivation to exclude hotplug from default testing,
since I think it diminishes the value of selftests overall.
Not running all tests all the time risks breaking them.
selftest makefile refactoring broke selftests/bpf in the past,
so I strongly suggest to install clang and make sure the tests are passing
on the test servers otherwise we'd need to move selftests/bpf out of
selftests to avoid further headaches for us when selftests infra keeps
changing.
^ permalink raw reply
* Re: [PATCH net] bpf/verifier: reject BPF_ALU64|BPF_END
From: Alexei Starovoitov @ 2017-09-15 15:50 UTC (permalink / raw)
To: Edward Cree; +Cc: David Miller, netdev, Daniel Borkmann
In-Reply-To: <8c3d5513-6171-3e68-56df-1efd0e87f071@solarflare.com>
On Fri, Sep 15, 2017 at 02:37:38PM +0100, Edward Cree wrote:
> Neither ___bpf_prog_run nor the JITs accept it.
> Also adds a new test case.
>
> Fixes: 17a5267067f3 ("bpf: verifier (add verifier core)")
> Signed-off-by: Edward Cree <ecree@solarflare.com>
Good catch. Thanks!
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: RTL8192EE PCIe Wireless Network Adapter crashed with linux-4.13
From: Larry Finger @ 2017-09-15 15:19 UTC (permalink / raw)
To: Zwindl
Cc: linux-wireless@vger.kernel.org, chaoming_li@realsil.com.cn,
kvalo@codeaurora.org, pkshih@realtek.com, johannes.berg@intel.com,
gregkh@linuxfoundation.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <jB0WJNG17mSfKo9m7T26r-b2Gr7mDL4lJ-1cWxeCJ6UCsA4TsxXz4znCLhYY5EASPQciwp1rf_xtwvDR-xzwD-OTApTUFoaVmIvahleDcJk=@protonmail.com>
On 09/15/2017 05:10 AM, Zwindl wrote:
>
>> -------- Original Message --------
>> Subject: Re: RTL8192EE PCIe Wireless Network Adapter crashed with linux-4.13
>> Local Time: 14 September 2017 6:05 PM
>> UTC Time: 14 September 2017 18:05
>> From: Larry.Finger@lwfinger.net
>> To: Zwindl <zwindl@protonmail.com>, linux-wireless@vger.kernel.org
>> <linux-wireless@vger.kernel.org>
>> chaoming_li@realsil.com.cn <chaoming_li@realsil.com.cn>, kvalo@codeaurora.org
>> <kvalo@codeaurora.org>, pkshih@realtek.com <pkshih@realtek.com>,
>> johannes.berg@intel.com <johannes.berg@intel.com>, gregkh@linuxfoundation.org
>> <gregkh@linuxfoundation.org>, netdev@vger.kernel.org <netdev@vger.kernel.org>,
>> linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
>>
>> On 09/14/2017 08:30 AM, Zwindl wrote:
>> > Dear developers:
>> > I"m using Arch Linux with testing enabled, the current kernel version and
>> > details are
>> > `Linux zwindl 4.13.2-1-ARCH #1 SMP PREEMPT Thu Sep 14 02:57:34 UTC 2017 x86_64
>> > GNU/Linux`.
>> > The wireless card can"t work properly from the kernel 4.13. Here"s the log(in
>> > attachment) when NetworkManager trying to connect my wifi which is named as
>> > "TP", my mac addr hided as xx:xx:xx:xx:xx.
>> > What should I provide to help to debug?
>> > ZWindL.
>>
>> The BUG-ON arises in __intel_map_single() due to dir (for direction of DMA)
>> equal to DMA_NONE (3). When rtl8192ee calls pci_map_single(), it uses
>> PCI_DMA_TODEVICE (1). I followed the calling sequence through the entire chain,
>> and none of the routines made any changes to "dir", other that changing the type
>> from int to enum dma_data_direction. That would not have changed a 1 to a 3.
>>
>> I built a 4.13.2 system. The problem does not happen here. At this point, the
>> system has been up for about two hours. I did discover a small memory leak
>> associated with firmware loading, but that should not have caused the problem.
>> Nonetheless, I will be sending a patch to fix that problem.
>>
>> I will continue testing, although I doubt that the problem will happen here.
>>
>> How long had your system been up when the problem occurred? Your dmesg fragment
>> did not show any times. What kernels have you tried besides 4.13.2?
>>
>> Larry
> Oh, sorry, the original log is from `journalctl`.
> Here's the `dmesg` prints(error.txt). I can't determine which part is related,
> so I paste all of it. I've tried 4.12.X(no issue), 4.13.1(issue), 4.13.2(issue).
> ZWindL
The output of dmesg is a lot more instructive than that of journalctl. I now
know exactly the location that triggered the WARNING. I still do not understand
it. In fact, it is likely a regression in kernel 4.13 that does not affect my
Toshiba laptop, nor a Lenovo machine I have, but does affect your Lenovo laptop.
Is it possible for you to install the mainline source from vger.kernel.org using
git and bisect the issue? It will take quite a bit of time, but it is likely the
only way to find the offending change. If you are willing to try this, I will
send you reasonably complete instructions.
By the way, it is usually better to load the dmesg output into a pastebin site
and post the link. Sending the entire file to a list makes a lot of people
receive a lot of data for which they have no interest.
Larry
^ permalink raw reply
* Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface
From: Andrew Lunn @ 2017-09-15 15:19 UTC (permalink / raw)
To: Jiri Pirko
Cc: Alexander Duyck, Maxim Uvarov, Vivien Didelot, netdev, kernel,
Florian Fainelli, Egil Hjelmeland, John Crispin, Woojung Huh,
Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <20170915142617.GA2060@nanopsycho.orion>
> > Reg cpu lan0 lan1 lan2 lan3 lan4 lan5 global0 global1
> >-----------------------------------------------------------------------------
> > 00: 4e07 4d04 4d04 4d04 4d04 4d04 4d04 0000 00000
> > 01: 403e 003d 003d 003d 003d 003d 003d 0000 00000
> > 02: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 03: 3521 3521 3521 3521 3521 3521 3521 0000 00000
> > 04: 0533 373f 373f 373f 373f 373f 373f 0000 00000
> > 05: 8000 0000 0000 0000 0000 0000 0000 0000 00000
> > 06: 005f 003f 003f 003f 003f 003f 003f 0000 00000
> > 07: 002a 002a 002a 002a 002a 002a 002a 0000 00000
> > 08: 2080 2080 2080 2080 2080 2080 2080 0000 00000
> > 09: 0001 0001 0001 0001 0001 0001 0001 0000 00000
> > 0a: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 0b: 0020 0000 0000 0000 0000 0000 0000 0000 00000
> > 0c: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 0d: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 0e: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 0f: 9100 dada dada dada dada dada dada 0000 00000
> > 10: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 11: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 12: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 13: 0000 00d8 00d8 00d8 00d8 00d8 00d8 0000 00000
> > 14: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 15: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 16: 0022 0000 0000 0000 0000 0000 0000 0000 00000
> > 17: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 18: 3210 2210 2210 2210 2210 2210 2210 0000 00000
> > 19: 7654 7654 7654 7654 7654 7654 7654 0000 00000
> > 1a: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 1b: 8000 8000 8000 8000 8000 8000 8000 0000 00000
> > 1c: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 1d: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 1e: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 1f: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> >
>
> Is this a reg dump per-port?
No. Look at the global0 and global1 columns. These are not port
registers, but switch global registers. The other columns are per
port. We could have a two column table per port for these
registers. However for debugging, it is useful to see the port
registers side-by-side. The difference between ports often gives you
clues as to what is wrong.
The global registers however are scoped to a switch, not a port.
> you can have reg array for each. How the values can change? Is this
> change result of driver<->hw communication? If yes, you might consider
> using devlink hwmsg trace to expose the communication to userspace.
The reason a value changes varies per bit. Some are status, set by the
switch. Some are configuration, set by the driver. They are all mixed
up together. And some registers are actually just multiplexors onto
other tables of registers. You set an opcode in the lower nibble,
address in the middle, set the top bit and busy loop waiting for it to
clear. You can then read a result out of the register, or a near by
register. So the register dump gives you an idea what the last access
to such a table was.
> I would rather focus on what exactly you need to expose to userspace,
> then we can figure out how to do it. Generic multipurpose arrays should
> be considered as last-resort solution in my opinion.
So go look at Vivien's patches. That is what we want to expose. For a
start. Viviens patches are at the DSA level. Once we get something
accepted at that level, i can imaging drivers want to augment it with
driver specific tables. But we can handle that later.
Andrew
^ permalink raw reply
* Re: [PATCH net] l2tp: fix race condition in l2tp_tunnel_delete
From: Sabrina Dubroca @ 2017-09-15 14:55 UTC (permalink / raw)
To: Tom Parkin; +Cc: netdev, Guillaume Nault, Xin Long
In-Reply-To: <20170915094259.GA3209@jackdaw>
2017-09-15, 10:42:59 +0100, Tom Parkin wrote:
> On Fri, Sep 15, 2017 at 11:08:07AM +0200, Sabrina Dubroca wrote:
> > The tunnel is currently removed from the list during destruction. This
> > can lead to a double-free of the struct sock if we try to delete the tunnel
> > twice fast enough.
> >
> > The first delete operation does a lookup (l2tp_tunnel_get), finds the
> > tunnel, calls l2tp_tunnel_delete, which queues it for deletion by
> > l2tp_tunnel_del_work.
> >
> > The second delete operation also finds the tunnel and calls
> > l2tp_tunnel_delete. If the workqueue has already fired and started
> > running l2tp_tunnel_del_work, then l2tp_tunnel_delete will queue the
> > same tunnel a second time, and try to free the socket again.
> >
> > Add a dead flag and remove tunnel from its list earlier. Then we can
> > remove the check of queue_work's result that was meant to prevent that
> > race but doesn't.
>
> How do we avoid leaving stale information on the tunnel list for
> use-cases which don't delete tunnels using netlink? For example the
> L2TPv2 ppp/socket API depends on sk_destruct to clean up the kernel
> context on socket destruction. Similarly, userspace may just close
> the tunnel socket without first making netlink calls to delete the
> tunnel.
>
> By moving the tunnel list removal from l2tp_tunnel_destruct to
> l2tp_tunnel_delete I can't see how codepaths which don't involve
> l2tp_tunnel_delete don't end up with a corrupted tunnel list.
Ok, thanks for pointing that out. We could go with just the ->dead
flag then. I'm not sure whether we need to set it in
l2tp_tunnel_destruct as well.
-------- 8< --------
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index ee485df73ccd..e74596418169 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1685,14 +1685,12 @@ EXPORT_SYMBOL_GPL(l2tp_tunnel_create);
/* This function is used by the netlink TUNNEL_DELETE command.
*/
-int l2tp_tunnel_delete(struct l2tp_tunnel *tunnel)
+void l2tp_tunnel_delete(struct l2tp_tunnel *tunnel)
{
- l2tp_tunnel_inc_refcount(tunnel);
- if (false == queue_work(l2tp_wq, &tunnel->del_work)) {
- l2tp_tunnel_dec_refcount(tunnel);
- return 1;
+ if (!test_and_set_bit(1, &tunnel->dead)) {
+ l2tp_tunnel_inc_refcount(tunnel);
+ queue_work(l2tp_wq, &tunnel->del_work);
}
- return 0;
}
EXPORT_SYMBOL_GPL(l2tp_tunnel_delete);
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index a305e0c5925a..deda869504d0 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -160,6 +160,9 @@ struct l2tp_tunnel_cfg {
struct l2tp_tunnel {
int magic; /* Should be L2TP_TUNNEL_MAGIC */
+
+ unsigned long dead;
+
struct rcu_head rcu;
rwlock_t hlist_lock; /* protect session_hlist */
bool acpt_newsess; /* Indicates whether this
@@ -254,7 +257,7 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id,
u32 peer_tunnel_id, struct l2tp_tunnel_cfg *cfg,
struct l2tp_tunnel **tunnelp);
void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel);
-int l2tp_tunnel_delete(struct l2tp_tunnel *tunnel);
+void l2tp_tunnel_delete(struct l2tp_tunnel *tunnel);
struct l2tp_session *l2tp_session_create(int priv_size,
struct l2tp_tunnel *tunnel,
u32 session_id, u32 peer_session_id,
--
Sabrina
^ permalink raw reply related
* Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface
From: Jiri Pirko @ 2017-09-15 14:26 UTC (permalink / raw)
To: Andrew Lunn
Cc: Alexander Duyck, Maxim Uvarov, Vivien Didelot, netdev, kernel,
Florian Fainelli, Egil Hjelmeland, John Crispin, Woojung Huh,
Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <20170915140839.GD3084@lunn.ch>
Fri, Sep 15, 2017 at 04:08:39PM CEST, andrew@lunn.ch wrote:
>> Could you put together your requirements so we can work it out to extend
>> devlink to support them?
>
>As i've said multiple times, generic two dimensional tables. Examples
>could look like:
>
>Stats cpu lan0 lan1 lan2 lan3 lan4 dsa
>---------------------------------------------------------------------
>tx_packets: 2 0 0 0 0 0 0
>tx_bytes: 1180 6666 0 0 0 0 0
>rx_packets: 0 0 0 0 0 0 0
>rx_bytes: 0 1180 0 0 0 0 0
>in_good_octets: 6666 1188 0 0 0 0 0
>in_bad_octets: 0 0 0 0 0 0 0
>in_unicast: 0 0 0 0 0 0 0
>in_broadcasts: 0 0 0 0 0 0 0
>in_multicasts: 89 0 0 0 0 0 0
>in_pause: 0 0 0 0 0 0 0
>in_undersize: 0 0 0 0 0 0 0
>in_fragments: 0 0 0 0 0 0 0
>in_oversize: 0 0 0 0 0 0 0
>in_jabber: 0 0 0 0 0 0 0
>in_rx_error: 0 0 0 0 0 0 0
>in_fcs_error: 0 0 0 0 0 0 0
>out_octets: 1188 6666 0 0 0 0 0
>out_unicast: 0 0 0 0 0 0 0
>out_broadcasts: 2 2 0 0 0 0 0
>out_multicasts: 0 89 0 0 0 0 0
>out_pause: 0 0 0 0 0 0 0
>excessive: 0 0 0 0 0 0 0
>collisions: 0 0 0 0 0 0 0
>deferred: 0 0 0 0 0 0 0
>single: 0 0 0 0 0 0 0
>multiple: 0 0 0 0 0 0 0
>out_fcs_error: 0 0 0 0 0 0 0
>late: 0 0 0 0 0 0 0
>hist_64bytes: 0 0 0 0 0 0 0
>hist_65_127bytes: 0 0 0 0 0 0 0
>hist_128_255bytes: 0 0 0 0 0 0 0
>hist_256_511bytes: 0 0 0 0 0 0 0
>hist_512_1023bytes: 0 0 0 0 0 0 0
>hist_1024_max_bytes: 0 0 0 0 0 0 0
>sw_in_discards: 0 0 0 0 0 0 0
>sw_in_filtered: 0 0 0 0 0 0 0
>sw_out_filtered: 89 89 0 0 0 0 0
I believe we discussed this already. You can have devlink_port instance
for each port, then you just have stats-per-devlink_port. Looks quite
easy to implement actually.
Would be mistake to have this as just 2 dim array.
>
>
> Reg cpu lan0 lan1 lan2 lan3 lan4 lan5 global0 global1
>-----------------------------------------------------------------------------
> 00: 4e07 4d04 4d04 4d04 4d04 4d04 4d04 0000 00000
> 01: 403e 003d 003d 003d 003d 003d 003d 0000 00000
> 02: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 03: 3521 3521 3521 3521 3521 3521 3521 0000 00000
> 04: 0533 373f 373f 373f 373f 373f 373f 0000 00000
> 05: 8000 0000 0000 0000 0000 0000 0000 0000 00000
> 06: 005f 003f 003f 003f 003f 003f 003f 0000 00000
> 07: 002a 002a 002a 002a 002a 002a 002a 0000 00000
> 08: 2080 2080 2080 2080 2080 2080 2080 0000 00000
> 09: 0001 0001 0001 0001 0001 0001 0001 0000 00000
> 0a: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 0b: 0020 0000 0000 0000 0000 0000 0000 0000 00000
> 0c: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 0d: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 0e: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 0f: 9100 dada dada dada dada dada dada 0000 00000
> 10: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 11: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 12: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 13: 0000 00d8 00d8 00d8 00d8 00d8 00d8 0000 00000
> 14: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 15: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 16: 0022 0000 0000 0000 0000 0000 0000 0000 00000
> 17: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 18: 3210 2210 2210 2210 2210 2210 2210 0000 00000
> 19: 7654 7654 7654 7654 7654 7654 7654 0000 00000
> 1a: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 1b: 8000 8000 8000 8000 8000 8000 8000 0000 00000
> 1c: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 1d: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 1e: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 1f: 0000 0000 0000 0000 0000 0000 0000 0000 00000
>
Is this a reg dump per-port? Also, with per-port devlink_port instance,
you can have reg array for each. How the values can change? Is this
change result of driver<->hw communication? If yes, you might consider
using devlink hwmsg trace to expose the communication to userspace.
>So a table would have an optional header row. Then a number of data
>rows. The number of columns is the same for each row. The number of
>columns is determined at run time, but is known at the beginning of
>enumerating the table. The number of data rows is not known until the
>last one is enumerated.
I would rather focus on what exactly you need to expose to userspace,
then we can figure out how to do it. Generic multipurpose arrays should
be considered as last-resort solution in my opinion.
>
>Each cell in the row is typed. Can be a string, bool, u8, u16, u32, u64,
>ifindex, MAC address, IP address, devlink port, ....
>
>Each cell in a row can have a different type. The types of the header
>row cells can be different to the types of the data cells. All data
>rows have the same type information. So you can enumerate the types
>once, and use them for the whole table.
>
>The userspace tool should make its best effort to print the table,
>using the type info. It might need hits from the command line, like -x
>to print in hex not decimal. The second example shows this. Registers
>make more sense in hex, where as statistics make more sense in
>decimal. But let the user choose, so keeping the kAPI simple. This is
>intended as a debug tool. The output does not need to be highly
>polished. But it needs to be a lot more readable than the current
>dpipe output which prints a table as a list, not a table.
>
>The tables can be per port, or per switch. Remember that DSA allows a
>cluster of switches within one DSA instance.
>
>Vivien, Florian, did i miss anything?
>
> Andrew
^ permalink raw reply
* Re: [PATCH] net: phy: Fix mask value write on gmii2rgmii converter speed register.
From: Andrew Lunn @ 2017-09-15 14:10 UTC (permalink / raw)
To: Michal Simek
Cc: Fahad Kunnathadi, f.fainelli, netdev, linux-kernel,
soren.brinkmann, linux-arm-kernel
In-Reply-To: <5f4bd003-d251-5a14-497a-9cc72dd0f160@xilinx.com>
> Can you please point me to that email?
I assume you can search the email lists just as well as i can.
Andrew
^ permalink raw reply
* Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface
From: Andrew Lunn @ 2017-09-15 14:08 UTC (permalink / raw)
To: Jiri Pirko
Cc: Alexander Duyck, Maxim Uvarov, Vivien Didelot, netdev, kernel,
Florian Fainelli, Egil Hjelmeland, John Crispin, Woojung Huh,
Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <20170915055107.GA1927@nanopsycho.orion>
> Could you put together your requirements so we can work it out to extend
> devlink to support them?
As i've said multiple times, generic two dimensional tables. Examples
could look like:
Stats cpu lan0 lan1 lan2 lan3 lan4 dsa
---------------------------------------------------------------------
tx_packets: 2 0 0 0 0 0 0
tx_bytes: 1180 6666 0 0 0 0 0
rx_packets: 0 0 0 0 0 0 0
rx_bytes: 0 1180 0 0 0 0 0
in_good_octets: 6666 1188 0 0 0 0 0
in_bad_octets: 0 0 0 0 0 0 0
in_unicast: 0 0 0 0 0 0 0
in_broadcasts: 0 0 0 0 0 0 0
in_multicasts: 89 0 0 0 0 0 0
in_pause: 0 0 0 0 0 0 0
in_undersize: 0 0 0 0 0 0 0
in_fragments: 0 0 0 0 0 0 0
in_oversize: 0 0 0 0 0 0 0
in_jabber: 0 0 0 0 0 0 0
in_rx_error: 0 0 0 0 0 0 0
in_fcs_error: 0 0 0 0 0 0 0
out_octets: 1188 6666 0 0 0 0 0
out_unicast: 0 0 0 0 0 0 0
out_broadcasts: 2 2 0 0 0 0 0
out_multicasts: 0 89 0 0 0 0 0
out_pause: 0 0 0 0 0 0 0
excessive: 0 0 0 0 0 0 0
collisions: 0 0 0 0 0 0 0
deferred: 0 0 0 0 0 0 0
single: 0 0 0 0 0 0 0
multiple: 0 0 0 0 0 0 0
out_fcs_error: 0 0 0 0 0 0 0
late: 0 0 0 0 0 0 0
hist_64bytes: 0 0 0 0 0 0 0
hist_65_127bytes: 0 0 0 0 0 0 0
hist_128_255bytes: 0 0 0 0 0 0 0
hist_256_511bytes: 0 0 0 0 0 0 0
hist_512_1023bytes: 0 0 0 0 0 0 0
hist_1024_max_bytes: 0 0 0 0 0 0 0
sw_in_discards: 0 0 0 0 0 0 0
sw_in_filtered: 0 0 0 0 0 0 0
sw_out_filtered: 89 89 0 0 0 0 0
Reg cpu lan0 lan1 lan2 lan3 lan4 lan5 global0 global1
-----------------------------------------------------------------------------
00: 4e07 4d04 4d04 4d04 4d04 4d04 4d04 0000 00000
01: 403e 003d 003d 003d 003d 003d 003d 0000 00000
02: 0000 0000 0000 0000 0000 0000 0000 0000 00000
03: 3521 3521 3521 3521 3521 3521 3521 0000 00000
04: 0533 373f 373f 373f 373f 373f 373f 0000 00000
05: 8000 0000 0000 0000 0000 0000 0000 0000 00000
06: 005f 003f 003f 003f 003f 003f 003f 0000 00000
07: 002a 002a 002a 002a 002a 002a 002a 0000 00000
08: 2080 2080 2080 2080 2080 2080 2080 0000 00000
09: 0001 0001 0001 0001 0001 0001 0001 0000 00000
0a: 0000 0000 0000 0000 0000 0000 0000 0000 00000
0b: 0020 0000 0000 0000 0000 0000 0000 0000 00000
0c: 0000 0000 0000 0000 0000 0000 0000 0000 00000
0d: 0000 0000 0000 0000 0000 0000 0000 0000 00000
0e: 0000 0000 0000 0000 0000 0000 0000 0000 00000
0f: 9100 dada dada dada dada dada dada 0000 00000
10: 0000 0000 0000 0000 0000 0000 0000 0000 00000
11: 0000 0000 0000 0000 0000 0000 0000 0000 00000
12: 0000 0000 0000 0000 0000 0000 0000 0000 00000
13: 0000 00d8 00d8 00d8 00d8 00d8 00d8 0000 00000
14: 0000 0000 0000 0000 0000 0000 0000 0000 00000
15: 0000 0000 0000 0000 0000 0000 0000 0000 00000
16: 0022 0000 0000 0000 0000 0000 0000 0000 00000
17: 0000 0000 0000 0000 0000 0000 0000 0000 00000
18: 3210 2210 2210 2210 2210 2210 2210 0000 00000
19: 7654 7654 7654 7654 7654 7654 7654 0000 00000
1a: 0000 0000 0000 0000 0000 0000 0000 0000 00000
1b: 8000 8000 8000 8000 8000 8000 8000 0000 00000
1c: 0000 0000 0000 0000 0000 0000 0000 0000 00000
1d: 0000 0000 0000 0000 0000 0000 0000 0000 00000
1e: 0000 0000 0000 0000 0000 0000 0000 0000 00000
1f: 0000 0000 0000 0000 0000 0000 0000 0000 00000
So a table would have an optional header row. Then a number of data
rows. The number of columns is the same for each row. The number of
columns is determined at run time, but is known at the beginning of
enumerating the table. The number of data rows is not known until the
last one is enumerated.
Each cell in the row is typed. Can be a string, bool, u8, u16, u32, u64,
ifindex, MAC address, IP address, devlink port, ....
Each cell in a row can have a different type. The types of the header
row cells can be different to the types of the data cells. All data
rows have the same type information. So you can enumerate the types
once, and use them for the whole table.
The userspace tool should make its best effort to print the table,
using the type info. It might need hits from the command line, like -x
to print in hex not decimal. The second example shows this. Registers
make more sense in hex, where as statistics make more sense in
decimal. But let the user choose, so keeping the kAPI simple. This is
intended as a debug tool. The output does not need to be highly
polished. But it needs to be a lot more readable than the current
dpipe output which prints a table as a list, not a table.
The tables can be per port, or per switch. Remember that DSA allows a
cluster of switches within one DSA instance.
Vivien, Florian, did i miss anything?
Andrew
^ permalink raw reply
* Re: [PATCH net] packet: hold bind lock when rebinding to fanout hook
From: Willem de Bruijn @ 2017-09-15 14:07 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, David Miller, nixiaoming
In-Reply-To: <20170914211441.67326-1-willemb@google.com>
On Thu, Sep 14, 2017 at 5:14 PM, Willem de Bruijn <willemb@google.com> wrote:
> Packet socket bind operations must hold the po->bind_lock. This keeps
> po->running consistent with whether the socket is actually on a ptype
> list to receive packets.
>
> fanout_add unbinds a socket and its packet_rcv/tpacket_rcv call, then
> binds the fanout object to receive through packet_rcv_fanout.
>
> Make it hold the po->bind_lock when testing po->running and rebinding.
> Else, it can race with other rebind operations, such as that in
> packet_set_ring from packet_rcv to tpacket_rcv. Concurrent updates
> can result in a socket being added to a fanout group twice, causing
> use-after-free KASAN bug reports, among others.
>
> Reported independently by both trinity and syzkaller.
> Verified that the syzkaller reproducer passes after this patch.
>
I forgot to add the Fixes tag, sorry.
Fixes: dc99f600698d ("packet: Add fanout support.")
> Reported-by: nixioaming <nixiaoming@huawei.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
^ permalink raw reply
* Re: [REGRESSION] Warning in tcp_fastretrans_alert() of net/ipv4/tcp_input.c
From: Neal Cardwell @ 2017-09-15 14:03 UTC (permalink / raw)
To: Oleksandr Natalenko
Cc: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, Netdev,
Yuchung Cheng
In-Reply-To: <1760119.uO98D2ft6f@natalenko.name>
On Fri, Sep 15, 2017 at 1:03 AM, Oleksandr Natalenko
<oleksandr@natalenko.name> wrote:
> Hi.
>
> I've applied your test patch but it doesn't fix the issue for me since the
> warning is still there.
>
> Were you able to reproduce it?
Hi,
Thanks for testing that. That is a very useful data point.
I was able to cook up a packetdrill test that could put the connection
in CA_Disorder with retransmitted packets out, but not in CA_Open. So
we do not yet have a test case to reproduce this.
We do not see this warning on our fleet at Google. One significant
difference I see between our environment and yours is that it seems
you run with FACK enabled:
net.ipv4.tcp_fack = 1
Note that FACK was disabled by default (since it was replaced by RACK)
between kernel v4.10 and v4.11. And this is exactly the time when this
bug started manifesting itself for you and some others, but not our
fleet. So my new working hypothesis would be that this warning is due
to a behavior that only shows up in kernels >=4.11 when FACK is
enabled.
Would you be able to disable FACK ("sysctl net.ipv4.tcp_fack=0" at
boot, or net.ipv4.tcp_fack=0 in /etc/sysctl.conf, or equivalent),
reboot, and test the kernel for a few days to see if the warning still
pops up?
thanks,
neal
[ps: apologies for the previous, mis-formatted post...]
^ 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