* Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
From: Jakub Kicinski @ 2020-11-21 20:55 UTC (permalink / raw)
To: Johannes Berg
Cc: Florian Westphal, Ido Schimmel, Aleksandr Nogikh, davem, edumazet,
andreyknvl, dvyukov, elver, linux-kernel, netdev, linux-wireless,
willemdebruijn.kernel, Aleksandr Nogikh, Willem de Bruijn
In-Reply-To: <106fc65f0459bc316e89beaf6bd71e823c4c01b7.camel@sipsolutions.net>
On Sat, 21 Nov 2020 20:30:44 +0100 Johannes Berg wrote:
> On Sat, 2020-11-21 at 10:35 -0800, Jakub Kicinski wrote:
> > On Sat, 21 Nov 2020 19:12:21 +0100 Johannes Berg wrote:
> > > > So I'm leaning towards reverting the whole thing. You can attach
> > > > kretprobes and record the information you need in BPF maps.
> > >
> > > I'm not going to object to reverting it (and perhaps redoing it better
> > > later), but I will point out that kretprobe isn't going to work, you
> > > eventually need kcov_remote_start() to be called in strategic points
> > > before processing the skb after it bounced through the system.
> > >
> > > IOW, it's not really about serving userland, it's about enabling (and
> > > later disabling) coverage collection for the bits of code it cares
> > > about, mostly because collecting it for _everything_ is going to be too
> > > slow and will mess up the data since for coverage guided fuzzing you
> > > really need the reported coverage data to be only about the injected
> > > fuzz data...
> >
> > All you need is make kcov_remote_start_common() be BPF-able, like
> > the LSM hooks are now, right? And then BPF can return whatever handle
> > it pleases.
>
> Not sure I understand. Are you saying something should call
> "kcov_remote_start_common()" with, say, the SKB, and leave it to a mass
> of bpf hooks to figure out where the SKB got cloned or copied or
> whatnot, track that in a map, and then ... no, wait, I don't really see
> what you mean, sorry.
>
> IIUC, fundamentally, you have this:
>
> - at the beginning, a task is tagged with "please collect coverage
> data for this handle"
Write the tag into task local storage, or map indexed by PID.
> - this task creates an SKB, etc, and all of the code that this task
> executes is captured and the coverage data is reported
kprobe the right places to record the skb -> handle mapping.
> - However, the SKB traverses lots of things, gets copied, cloned, or
> whatnot, and eventually leaves the annotated task, say for further
> processing in softirq context or elsewhere.
Which is fine.
> Now since the whole point is to see what chaos this SKB created from
> beginning (allocation) to end (free), since it was filled with fuzzed
> data, you now have to figure out where to pick back up when the SKB is
> processed further.
>
> This is what the infrastructure was meant to solve. But note that the
> SKB might be further cloned etc, so in order to track it you'd have to
> (out-of-band) figure out all the possible places where it could
> be reallocated, any time the skb pointer could change.
Ack, you have to figure out all the places anyway, the question is
whether you put probes there or calls in the source code.
Shifting the maintenance burden but also BPF is flexibility.
> Then, when you know you've got interesting code on your hands, like in
> mac80211 that was annotated in patch 3 here, you basically say
>
> "oohhh, this SKB was annotated before, let's continue capturing
> coverage data here"
>
> (and turn it off again later by the corresponding kcov_remote_stop().
Yup, the point is you can feed a raw skb pointer (and all other
possible context you may want) to a BPF prog in kcov_remote_start()
and let BPF/BTF give you the handle it recorded in its maps.
> So the only way I could _possibly_ see how to do this would be to
>
> * capture all possible places where the skb pointer can change
> * still call something like skb_get_kcov_handle() but let it call out
> to a BPF program to query a map or something to figure out if this
> SKB has a handle attached to it
>
> > Or if you don't like BPF or what to KCOV BPF itself in the future you
> > can roll your own mechanism. The point is - this should be relatively
> > easily doable out of line...
>
> Seems pretty complicated to me though ...
It is more complicated. We can go back to an skb field if this work is
expected to yield results for mac80211. Would you mind sending a patch?
^ permalink raw reply
* Re: [PATCH] compiler_attribute: remove CONFIG_ENABLE_MUST_CHECK
From: Miguel Ojeda @ 2020-11-21 20:45 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Jason A. Donenfeld, Shuah Khan, linux-kernel, linux-kselftest,
Network Development, wireguard, clang-built-linux,
Nick Desaulniers
In-Reply-To: <20201121194339.52290-1-masahiroy@kernel.org>
On Sat, Nov 21, 2020 at 8:44 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Our goal is to always enable __must_check where appreciate, so this
> CONFIG option is no longer needed.
This would be great. It also implies we can then move it to
`compiler_attributes.h` since it does not depend on config options
anymore.
We should also rename it to `__nodiscard`, since that is the
standardized name (coming soon to C2x and in C++ for years).
Cc'ing the Clang folks too to make them aware.
Cheers,
Miguel
^ permalink raw reply
* Re: [PATCH bpf-next v2 5/5] selftests/bpf: xsk selftests - Bi-directional Sockets - SKB, DRV
From: Weqaar Janjua @ 2020-11-21 20:14 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, netdev, Daniel Borkmann, ast, Magnus Karlsson,
Björn Töpel, Weqaar Janjua, shuah, skhan,
linux-kselftest, Anders Roxell, jonathan.lemon
In-Reply-To: <86e3a9e4-a375-1281-07bf-6b04781bb02f@fb.com>
On Fri, 20 Nov 2020 at 20:45, Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 11/20/20 5:00 AM, Weqaar Janjua wrote:
> > Adds following tests:
> >
> > 1. AF_XDP SKB mode
> > d. Bi-directional Sockets
> > Configure sockets as bi-directional tx/rx sockets, sets up fill
> > and completion rings on each socket, tx/rx in both directions.
> > Only nopoll mode is used
> >
> > 2. AF_XDP DRV/Native mode
> > d. Bi-directional Sockets
> > * Only copy mode is supported because veth does not currently support
> > zero-copy mode
> >
> > Signed-off-by: Weqaar Janjua <weqaar.a.janjua@intel.com>
> > ---
> > tools/testing/selftests/bpf/Makefile | 4 +-
> > .../bpf/test_xsk_drv_bidirectional.sh | 23 ++++
> > .../selftests/bpf/test_xsk_drv_teardown.sh | 3 -
> > .../bpf/test_xsk_skb_bidirectional.sh | 20 ++++
> > tools/testing/selftests/bpf/xdpxceiver.c | 100 +++++++++++++-----
> > tools/testing/selftests/bpf/xdpxceiver.h | 4 +
> > 6 files changed, 126 insertions(+), 28 deletions(-)
> > create mode 100755 tools/testing/selftests/bpf/test_xsk_drv_bidirectional.sh
> > create mode 100755 tools/testing/selftests/bpf/test_xsk_skb_bidirectional.sh
> >
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 515b29d321d7..258bd72812e0 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -78,7 +78,9 @@ TEST_PROGS := test_kmod.sh \
> > test_xsk_drv_nopoll.sh \
> > test_xsk_drv_poll.sh \
> > test_xsk_skb_teardown.sh \
> > - test_xsk_drv_teardown.sh
> > + test_xsk_drv_teardown.sh \
> > + test_xsk_skb_bidirectional.sh \
> > + test_xsk_drv_bidirectional.sh
> >
> > TEST_PROGS_EXTENDED := with_addr.sh \
> > with_tunnels.sh \
> > diff --git a/tools/testing/selftests/bpf/test_xsk_drv_bidirectional.sh b/tools/testing/selftests/bpf/test_xsk_drv_bidirectional.sh
> > new file mode 100755
> > index 000000000000..d3a7e2934d83
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/test_xsk_drv_bidirectional.sh
> > @@ -0,0 +1,23 @@
> > +#!/bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright(c) 2020 Intel Corporation.
> > +
> > +# See test_xsk_prerequisites.sh for detailed information on tests
> > +
> > +. xsk_prereqs.sh
> > +. xsk_env.sh
> > +
> > +TEST_NAME="DRV BIDIRECTIONAL SOCKETS"
> > +
> > +vethXDPnative ${VETH0} ${VETH1} ${NS1}
> > +
> > +params=("-N" "-B")
> > +execxdpxceiver params
> > +
> > +retval=$?
> > +test_status $retval "${TEST_NAME}"
> > +
> > +# Must be called in the last test to execute
> > +cleanup_exit ${VETH0} ${VETH1} ${NS1}
>
> This also makes hard to run tests as users will not know this unless
> they are familiar with the details of the tests.
>
> How about you have another scripts test_xsk.sh which includes all these
> individual tests and pull the above cleanup_exit into test_xsk.sh?
> User just need to run test_xsk.sh will be able to run all tests you
> implemented here.
>
This works, test_xsk_* >> test_xsk.sh, will ship out as v3.
> > +
> > +test_exit $retval 0
> > diff --git a/tools/testing/selftests/bpf/test_xsk_drv_teardown.sh b/tools/testing/selftests/bpf/test_xsk_drv_teardown.sh
> [...]
^ permalink raw reply
* [PATCH] compiler_attribute: remove CONFIG_ENABLE_MUST_CHECK
From: Masahiro Yamada @ 2020-11-21 19:43 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Masahiro Yamada, Jason A. Donenfeld, Shuah Khan, linux-kernel,
linux-kselftest, netdev, wireguard
Revert commit cebc04ba9aeb ("add CONFIG_ENABLE_MUST_CHECK").
A lot of warn_unused_result warnings existed in 2006, but until now
they have been fixed thanks to people doing allmodconfig tests.
Our goal is to always enable __must_check where appreciate, so this
CONFIG option is no longer needed.
I see a lot of defconfig (arch/*/configs/*_defconfig) files having:
# CONFIG_ENABLE_MUST_CHECK is not set
I did not touch them for now since it would be a big churn. If arch
maintainers want to clean them up, please go ahead.
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
include/linux/compiler_types.h | 4 ----
lib/Kconfig.debug | 8 --------
tools/testing/selftests/wireguard/qemu/debug.config | 1 -
3 files changed, 13 deletions(-)
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index ac3fa37a84f9..02f6d3fbad9d 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -110,11 +110,7 @@ struct ftrace_likely_data {
unsigned long constant;
};
-#ifdef CONFIG_ENABLE_MUST_CHECK
#define __must_check __attribute__((__warn_unused_result__))
-#else
-#define __must_check
-#endif
#if defined(CC_USING_HOTPATCH)
#define notrace __attribute__((hotpatch(0, 0)))
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index c789b39ed527..cb8ef4fd0d02 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -286,14 +286,6 @@ config GDB_SCRIPTS
endif # DEBUG_INFO
-config ENABLE_MUST_CHECK
- bool "Enable __must_check logic"
- default y
- help
- Enable the __must_check logic in the kernel build. Disable this to
- suppress the "warning: ignoring return value of 'foo', declared with
- attribute warn_unused_result" messages.
-
config FRAME_WARN
int "Warn for stack frames larger than"
range 0 8192
diff --git a/tools/testing/selftests/wireguard/qemu/debug.config b/tools/testing/selftests/wireguard/qemu/debug.config
index b50c2085c1ac..fe07d97df9fa 100644
--- a/tools/testing/selftests/wireguard/qemu/debug.config
+++ b/tools/testing/selftests/wireguard/qemu/debug.config
@@ -1,5 +1,4 @@
CONFIG_LOCALVERSION="-debug"
-CONFIG_ENABLE_MUST_CHECK=y
CONFIG_FRAME_POINTER=y
CONFIG_STACK_VALIDATION=y
CONFIG_DEBUG_KERNEL=y
--
2.25.1
^ permalink raw reply related
* Re: [PATCH 072/141] can: peak_usb: Fix fall-through warnings for Clang
From: Joe Perches @ 2020-11-21 19:50 UTC (permalink / raw)
To: Marc Kleine-Budde, Gustavo A. R. Silva, Wolfgang Grandegger,
David S. Miller, Jakub Kicinski
Cc: linux-can, netdev, linux-kernel, linux-hardening
In-Reply-To: <bf3dbc5c-c34e-b3ef-abb6-0c88d8a90332@pengutronix.de>
On Sat, 2020-11-21 at 14:17 +0100, Marc Kleine-Budde wrote:
> On 11/20/20 7:34 PM, Gustavo A. R. Silva wrote:
> > In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning
> > by explicitly adding a break statement instead of letting the code fall
> > through to the next case.
> >
> > Link: https://github.com/KSPP/linux/issues/115
> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
[]
> > diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
[]
> > @@ -299,6 +299,8 @@ static void peak_usb_write_bulk_callback(struct urb *urb)
> > if (net_ratelimit())
> > netdev_err(netdev, "Tx urb aborted (%d)\n",
> > urb->status);
> > + break;
> > +
> > case -EPROTO:
> > case -ENOENT:
> > case -ECONNRESET:
> >
>
> What about moving the default to the end if the case, which is more common anyways:
>
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
[]
> @@ -295,16 +295,16 @@ static void peak_usb_write_bulk_callback(struct urb *urb)
> netif_trans_update(netdev);
> break;
>
>
> - default:
> - if (net_ratelimit())
> - netdev_err(netdev, "Tx urb aborted (%d)\n",
> - urb->status);
> case -EPROTO:
> case -ENOENT:
> case -ECONNRESET:
> case -ESHUTDOWN:
> -
> break;
> +
> + default:
> + if (net_ratelimit())
> + netdev_err(netdev, "Tx urb aborted (%d)\n",
> + urb->status);
That's fine and is more generally used style but this
default: case should IMO also end with a break;
+ break;
> }
^ permalink raw reply
* Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
From: Johannes Berg @ 2020-11-21 19:30 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Florian Westphal, Ido Schimmel, Aleksandr Nogikh, davem, edumazet,
andreyknvl, dvyukov, elver, linux-kernel, netdev, linux-wireless,
willemdebruijn.kernel, Aleksandr Nogikh, Willem de Bruijn
In-Reply-To: <20201121103529.4b4acbff@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On Sat, 2020-11-21 at 10:35 -0800, Jakub Kicinski wrote:
> On Sat, 21 Nov 2020 19:12:21 +0100 Johannes Berg wrote:
> > > So I'm leaning towards reverting the whole thing. You can attach
> > > kretprobes and record the information you need in BPF maps.
> >
> > I'm not going to object to reverting it (and perhaps redoing it better
> > later), but I will point out that kretprobe isn't going to work, you
> > eventually need kcov_remote_start() to be called in strategic points
> > before processing the skb after it bounced through the system.
> >
> > IOW, it's not really about serving userland, it's about enabling (and
> > later disabling) coverage collection for the bits of code it cares
> > about, mostly because collecting it for _everything_ is going to be too
> > slow and will mess up the data since for coverage guided fuzzing you
> > really need the reported coverage data to be only about the injected
> > fuzz data...
>
> All you need is make kcov_remote_start_common() be BPF-able, like
> the LSM hooks are now, right? And then BPF can return whatever handle
> it pleases.
Not sure I understand. Are you saying something should call
"kcov_remote_start_common()" with, say, the SKB, and leave it to a mass
of bpf hooks to figure out where the SKB got cloned or copied or
whatnot, track that in a map, and then ... no, wait, I don't really see
what you mean, sorry.
IIUC, fundamentally, you have this:
- at the beginning, a task is tagged with "please collect coverage
data for this handle"
- this task creates an SKB, etc, and all of the code that this task
executes is captured and the coverage data is reported
- However, the SKB traverses lots of things, gets copied, cloned, or
whatnot, and eventually leaves the annotated task, say for further
processing in softirq context or elsewhere.
Now since the whole point is to see what chaos this SKB created from
beginning (allocation) to end (free), since it was filled with fuzzed
data, you now have to figure out where to pick back up when the SKB is
processed further.
This is what the infrastructure was meant to solve. But note that the
SKB might be further cloned etc, so in order to track it you'd have to
(out-of-band) figure out all the possible places where it could
be reallocated, any time the skb pointer could change.
Then, when you know you've got interesting code on your hands, like in
mac80211 that was annotated in patch 3 here, you basically say
"oohhh, this SKB was annotated before, let's continue capturing
coverage data here"
(and turn it off again later by the corresponding kcov_remote_stop().
So the only way I could _possibly_ see how to do this would be to
* capture all possible places where the skb pointer can change
* still call something like skb_get_kcov_handle() but let it call out
to a BPF program to query a map or something to figure out if this
SKB has a handle attached to it
> Or if you don't like BPF or what to KCOV BPF itself in the future you
> can roll your own mechanism. The point is - this should be relatively
> easily doable out of line...
Seems pretty complicated to me though ...
johannes
^ permalink raw reply
* Re: [PATCH net-next,v3 0/9] netfilter: flowtable bridge and vlan enhancements
From: Jakub Kicinski @ 2020-11-21 19:23 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Tobias Waldekranz, netfilter-devel, davem, netdev, razor, jeremy
In-Reply-To: <20201121185621.GA23017@salvia>
On Sat, 21 Nov 2020 19:56:21 +0100 Pablo Neira Ayuso wrote:
> > Please gather some review tags from senior netdev developers. I don't
> > feel confident enough to apply this as 100% my own decision.
>
> Fair enough.
>
> This requirement for very specific Netfilter infrastructure which does
> not affect any other Networking subsystem sounds new to me.
You mean me asking for reviews from other senior folks when I don't
feel good about some code? I've asked others the same thing in the
past, e.g. Paolo for his RPS thing.
> What senior developers specifically you would like I should poke to
> get an acknowledgement on this to get this accepted of your
> preference?
I don't want to make a list. Maybe netconf attendees are a safe bet?
^ permalink raw reply
* Re: [PATCH nf-next v3 3/3] netfilter: Introduce egress hook
From: Pablo Neira Ayuso @ 2020-11-21 18:59 UTC (permalink / raw)
To: Lukas Wunner
Cc: Daniel Borkmann, Laura García Liébana, John Fastabend,
Jozsef Kadlecsik, Florian Westphal,
Netfilter Development Mailing list, coreteam, netdev,
Alexei Starovoitov, Eric Dumazet, Thomas Graf, David Miller
In-Reply-To: <20201011082657.GB15225@wunner.de>
Hi Lukas,
On Sun, Oct 11, 2020 at 10:26:57AM +0200, Lukas Wunner wrote:
> On Tue, Sep 15, 2020 at 12:02:03AM +0200, Daniel Borkmann wrote:
> > today it is possible and
> > perfectly fine to e.g. redirect to a host-facing veth from tc ingress which
> > then goes into container. Only traffic that goes up the host stack is seen
> > by nf ingress hook in that case. Likewise, reply traffic can be redirected
> > from host-facing veth to phys dev for xmit w/o any netfilter interference.
> > This means netfilter in host ns really only sees traffic to/from host as
> > intended. This is fine today, however, if 3rd party entities (e.g. distro
> > side) start pushing down rules on the two nf hooks, then these use cases will
> > break on the egress one due to this asymmetric layering violation. Hence my
> > ask that this needs to be configurable from a control plane perspective so
> > that both use cases can live next to each other w/o breakage. Most trivial
> > one I can think of is (aside from the fact to refactor the hooks and improve
> > their performance) a flag e.g. for skb that can be set from tc/BPF layer to
> > bypass the nf hooks. Basically a flexible opt-in so that existing use-cases
> > can be retained w/o breakage.
>
> I argue that being able to filter traffic coming out of the container
> is desirable because why should the host trust the software running
> in the container to never send malicious packets.
>
> As for the flag you're asking for, it already exists in the form of
> skb->mark. Just let tc set the mark when the packet exits the container
> and add a netfilter rule to accept packets carrying that mark. Or do
> not set any netfilter egress rules at all to disable the egress
> filtering and avoid the performance impact it would imply.
Would you follow up on this? I'd really appreciate if you do.
We're lately discussing more and more usecases in the NFWS meetings
where the egress can get really useful.
Thank you.
^ permalink raw reply
* Re: [PATCH net-next,v3 0/9] netfilter: flowtable bridge and vlan enhancements
From: Pablo Neira Ayuso @ 2020-11-21 18:56 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Tobias Waldekranz, netfilter-devel, davem, netdev, razor, jeremy
In-Reply-To: <20201121101551.3264c5fd@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On Sat, Nov 21, 2020 at 10:15:51AM -0800, Jakub Kicinski wrote:
> On Sat, 21 Nov 2020 13:31:38 +0100 Pablo Neira Ayuso wrote:
> > On Mon, Nov 16, 2020 at 11:56:58PM +0100, Pablo Neira Ayuso wrote:
> > > On Mon, Nov 16, 2020 at 02:45:21PM -0800, Jakub Kicinski wrote:
> > > > On Mon, 16 Nov 2020 23:36:15 +0100 Pablo Neira Ayuso wrote:
[...]
> > I have been discussing the topology update by tracking fdb updates
> > with the bridge maintainer, I'll be exploring extensions to the
> > existing fdb_notify() infrastructure to deal with this scenario you
> > describe. On my side this topology update scenario is not a priority
> > to be supported in this patchset, but it's feasible to support it
> > later on.
>
> My concern is that invalidation is _the_ hard part of creating caches.
> And I feel like merging this as is would be setting our standards pretty
> low.
Interesting, let's summarize a bit to make sure we're on the same
page:
- This "cache" is optional, you enable it on demand through ruleset.
- This "cache" is configurable, you can specify through ruleset policy
what policies get into the cache and _when_ they are placed in the
cache.
- This is not affecting any existing default configuration, neither
Linux networking not even classic path Netfilter configurations,
it's a rather new thing.
- This is showing performance improvement of ~50% with a very simple
testbed. With pktgen, back few years ago I was reaching x2.5
performance boost in software in a pktgen testbed.
- This is adding minimal changes to netdev_ops, just a single
callback.
For the live VM migration you describe, connections might time out,
but there are many use-cases where this is still valid, some of them
has been described already here.
> Please gather some review tags from senior netdev developers. I don't
> feel confident enough to apply this as 100% my own decision.
Fair enough.
This requirement for very specific Netfilter infrastructure which does
not affect any other Networking subsystem sounds new to me.
What senior developers specifically you would like I should poke to
get an acknowledgement on this to get this accepted of your
preference?
Thank you.
^ permalink raw reply
* Re: [PATCH net-next] compat: always include linux/compat.h from net/compat.h
From: Jakub Kicinski @ 2020-11-21 18:40 UTC (permalink / raw)
To: davem; +Cc: netdev, hch, arnd
In-Reply-To: <20201121175224.1465831-1-kuba@kernel.org>
On Sat, 21 Nov 2020 09:52:24 -0800 Jakub Kicinski wrote:
> In file included from net/ipv4/netfilter/arp_tables.c:26:
> include/net/compat.h:57:23: error: conflicting types for ‘uintptr_t’
> #define compat_uptr_t uintptr_t
> ^~~~~~~~~
> include/asm-generic/compat.h:22:13: note: in expansion of macro ‘compat_uptr_t’
> typedef u32 compat_uptr_t;
> ^~~~~~~~~~~~~
> In file included from include/linux/limits.h:6,
> from include/linux/kernel.h:7,
> from net/ipv4/netfilter/arp_tables.c:14:
> include/linux/types.h:37:24: note: previous declaration of ‘uintptr_t’ was here
> typedef unsigned long uintptr_t;
> ^~~~~~~~~
Ah, damn it, I obviously copied the wrong error into the commit
message. This is the correct one (after removing include of ethtool.h
from netdevice.h):
In file included from ../net/ipv4/netfilter/arp_tables.c:26:
include/net/compat.h:60:40: error: unknown type name ‘compat_uptr_t’; did you mean ‘compat_ptr_ioctl’?
struct sockaddr __user **save_addr, compat_uptr_t *ptr,
^~~~~~~~~~~~~
compat_ptr_ioctl
include/net/compat.h:61:4: error: unknown type name ‘compat_size_t’; did you mean ‘compat_sigset_t’?
compat_size_t *len);
^~~~~~~~~~~~~
compat_sigset_t
^ permalink raw reply
* Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
From: Jakub Kicinski @ 2020-11-21 18:35 UTC (permalink / raw)
To: Johannes Berg
Cc: Florian Westphal, Ido Schimmel, Aleksandr Nogikh, davem, edumazet,
andreyknvl, dvyukov, elver, linux-kernel, netdev, linux-wireless,
willemdebruijn.kernel, Aleksandr Nogikh, Willem de Bruijn
In-Reply-To: <bcfb0fe1b207d2f4bb52f0d1ef51207f9b5587de.camel@sipsolutions.net>
On Sat, 21 Nov 2020 19:12:21 +0100 Johannes Berg wrote:
> > So I'm leaning towards reverting the whole thing. You can attach
> > kretprobes and record the information you need in BPF maps.
>
> I'm not going to object to reverting it (and perhaps redoing it better
> later), but I will point out that kretprobe isn't going to work, you
> eventually need kcov_remote_start() to be called in strategic points
> before processing the skb after it bounced through the system.
>
> IOW, it's not really about serving userland, it's about enabling (and
> later disabling) coverage collection for the bits of code it cares
> about, mostly because collecting it for _everything_ is going to be too
> slow and will mess up the data since for coverage guided fuzzing you
> really need the reported coverage data to be only about the injected
> fuzz data...
All you need is make kcov_remote_start_common() be BPF-able, like
the LSM hooks are now, right? And then BPF can return whatever handle
it pleases.
Or if you don't like BPF or what to KCOV BPF itself in the future you
can roll your own mechanism. The point is - this should be relatively
easily doable out of line...
^ permalink raw reply
* Re: [PATCH net-next,v3 0/9] netfilter: flowtable bridge and vlan enhancements
From: Jakub Kicinski @ 2020-11-21 18:15 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Tobias Waldekranz, netfilter-devel, davem, netdev, razor, jeremy
In-Reply-To: <20201121123138.GA21560@salvia>
On Sat, 21 Nov 2020 13:31:38 +0100 Pablo Neira Ayuso wrote:
> On Mon, Nov 16, 2020 at 11:56:58PM +0100, Pablo Neira Ayuso wrote:
> > On Mon, Nov 16, 2020 at 02:45:21PM -0800, Jakub Kicinski wrote:
> > > On Mon, 16 Nov 2020 23:36:15 +0100 Pablo Neira Ayuso wrote:
> > > > > Are you saying A -> B traffic won't match so it will update the cache,
> > > > > since conntrack flows are bi-directional?
> > > >
> > > > Yes, Traffic for A -> B won't match the flowtable entry, this will
> > > > update the cache.
> > >
> > > That's assuming there will be A -> B traffic without B sending a
> > > request which reaches A, first.
> >
> > B might send packets to A but this will not get anywhere. Assuming
> > TCP, this will trigger retransmissions so B -> A will kick in to
> > refresh the entry.
> >
> > Is this scenario that you describe a showstopper?
Sorry I got distracted.
> I have been discussing the topology update by tracking fdb updates
> with the bridge maintainer, I'll be exploring extensions to the
> existing fdb_notify() infrastructure to deal with this scenario you
> describe. On my side this topology update scenario is not a priority
> to be supported in this patchset, but it's feasible to support it
> later on.
My concern is that invalidation is _the_ hard part of creating caches.
And I feel like merging this as is would be setting our standards pretty
low.
Please gather some review tags from senior netdev developers. I don't
feel confident enough to apply this as 100% my own decision.
^ permalink raw reply
* Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
From: Johannes Berg @ 2020-11-21 18:12 UTC (permalink / raw)
To: Jakub Kicinski, Florian Westphal
Cc: Ido Schimmel, Aleksandr Nogikh, davem, edumazet, andreyknvl,
dvyukov, elver, linux-kernel, netdev, linux-wireless,
willemdebruijn.kernel, Aleksandr Nogikh, Willem de Bruijn
In-Reply-To: <20201121100636.26aaaf8a@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On Sat, 2020-11-21 at 10:06 -0800, Jakub Kicinski wrote:
> On Sat, 21 Nov 2020 17:52:27 +0100 Florian Westphal wrote:
> > Ido Schimmel <idosch@idosch.org> wrote:
> > > Other suggestions?
> >
> > Aleksandr, why was this made into an skb extension in the first place?
> >
> > AFAIU this feature is usually always disabled at build time.
> > For debug builds (test farm /debug kernel etc) its always needed.
> >
> > If thats the case this u64 should be an sk_buff member, not an
> > extension.
>
> Yeah, in hindsight I should have looked at how it's used. Not a great
> fit for extensions. We can go back, but...
>
> In general I'm not very happy at how this is going. First of all just
> setting the handle in a couple of allocs seems to not be enough, skbs
> get cloned, reused etc. There were also build problems caused by this
> patch and Aleksandr & co where nowhere to be found. Now we find out
> this causes leaks, how was that not caught by the syzbot it's supposed
> to serve?!
Heh.
> So I'm leaning towards reverting the whole thing. You can attach
> kretprobes and record the information you need in BPF maps.
I'm not going to object to reverting it (and perhaps redoing it better
later), but I will point out that kretprobe isn't going to work, you
eventually need kcov_remote_start() to be called in strategic points
before processing the skb after it bounced through the system.
IOW, it's not really about serving userland, it's about enabling (and
later disabling) coverage collection for the bits of code it cares
about, mostly because collecting it for _everything_ is going to be too
slow and will mess up the data since for coverage guided fuzzing you
really need the reported coverage data to be only about the injected
fuzz data...
johannes
^ permalink raw reply
* Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
From: Jakub Kicinski @ 2020-11-21 18:06 UTC (permalink / raw)
To: Florian Westphal
Cc: Ido Schimmel, Aleksandr Nogikh, davem, johannes, edumazet,
andreyknvl, dvyukov, elver, linux-kernel, netdev, linux-wireless,
willemdebruijn.kernel, Aleksandr Nogikh, Willem de Bruijn
In-Reply-To: <20201121165227.GT15137@breakpoint.cc>
On Sat, 21 Nov 2020 17:52:27 +0100 Florian Westphal wrote:
> Ido Schimmel <idosch@idosch.org> wrote:
> > Other suggestions?
>
> Aleksandr, why was this made into an skb extension in the first place?
>
> AFAIU this feature is usually always disabled at build time.
> For debug builds (test farm /debug kernel etc) its always needed.
>
> If thats the case this u64 should be an sk_buff member, not an
> extension.
Yeah, in hindsight I should have looked at how it's used. Not a great
fit for extensions. We can go back, but...
In general I'm not very happy at how this is going. First of all just
setting the handle in a couple of allocs seems to not be enough, skbs
get cloned, reused etc. There were also build problems caused by this
patch and Aleksandr & co where nowhere to be found. Now we find out
this causes leaks, how was that not caught by the syzbot it's supposed
to serve?!
So I'm leaning towards reverting the whole thing. You can attach
kretprobes and record the information you need in BPF maps.
^ permalink raw reply
* Re: [RFC] MAINTAINERS tag for cleanup robot
From: Joe Perches @ 2020-11-21 18:02 UTC (permalink / raw)
To: James Bottomley, trix, clang-built-linux
Cc: linux-hyperv, linux-kernel, xen-devel, tboot-devel, kvm,
linux-crypto, linux-acpi, devel, amd-gfx, dri-devel, intel-gfx,
netdev, linux-media, MPT-FusionLinux.pdl, linux-scsi,
linux-wireless, ibm-acpi-devel, platform-driver-x86, linux-usb,
linux-omap, linux-fbdev, ecryptfs, linux-fsdevel, cluster-devel,
linux-mtd, keyrings, netfilter-devel, coreteam, alsa-devel, bpf,
linux-bluetooth, linux-nfs, patches
In-Reply-To: <5843ef910b0e86c00d9c0143dec20f93823b016b.camel@HansenPartnership.com>
On Sat, 2020-11-21 at 09:18 -0800, James Bottomley wrote:
> On Sat, 2020-11-21 at 08:50 -0800, trix@redhat.com wrote:
> > A difficult part of automating commits is composing the subsystem
> > preamble in the commit log. For the ongoing effort of a fixer
> > producing one or two fixes a release the use of 'treewide:' does
> > not seem appropriate.
> >
> > It would be better if the normal prefix was used. Unfortunately
> > normal is not consistent across the tree.
> >
> > D: Commit subsystem prefix
> >
> > ex/ for FPGA DFL DRIVERS
> >
> > D: fpga: dfl:
>
> I've got to bet this is going to cause more issues than it solves.
> SCSI uses scsi: <driver>: for drivers but not every driver has a
> MAINTAINERS entry. We use either scsi: or scsi: core: for mid layer
> things, but we're not consistent. Block uses blk-<something>: for all
> of it's stuff but almost no <somtehing>s have a MAINTAINERS entry. So
> the next thing you're going to cause is an explosion of suggested
> MAINTAINERs entries.
As well as some changes require simultaneous changes across
multiple subsystems.
> Has anyone actually complained about treewide:?
It depends on what you mean by treewide:
If a treewide: patch is applied by some "higher level" maintainer,
then generally, no.
If the treewide patch is also cc'd to many individual maintainers,
then yes, many many times.
Mostly because patches cause what is in their view churn or that
changes are not specific to their subsystem grounds.
The treewide patch is sometimes dropped, sometimes broken up and
generally not completely applied.
What would be useful in many cases like this is for a pre and post
application of the treewide patch to be compiled and the object
code verified for lack of any logic change.
Unfortunately, gcc does not guarantee deterministic compilation so
this isn't feasible with at least gcc. Does clang guarantee this?
I'm not sure it's possible:
https://blog.llvm.org/2019/11/deterministic-builds-with-clang-and-lld.html
^ permalink raw reply
* [PATCH net-next] compat: always include linux/compat.h from net/compat.h
From: Jakub Kicinski @ 2020-11-21 17:52 UTC (permalink / raw)
To: davem; +Cc: netdev, hch, arnd, Jakub Kicinski
We're about to do some reshuffling in networking headers and make
some of the file lose the implicit includes. This results in:
In file included from net/ipv4/netfilter/arp_tables.c:26:
include/net/compat.h:57:23: error: conflicting types for ‘uintptr_t’
#define compat_uptr_t uintptr_t
^~~~~~~~~
include/asm-generic/compat.h:22:13: note: in expansion of macro ‘compat_uptr_t’
typedef u32 compat_uptr_t;
^~~~~~~~~~~~~
In file included from include/linux/limits.h:6,
from include/linux/kernel.h:7,
from net/ipv4/netfilter/arp_tables.c:14:
include/linux/types.h:37:24: note: previous declaration of ‘uintptr_t’ was here
typedef unsigned long uintptr_t;
^~~~~~~~~
Currently net/compat.h depends on linux/compat.h being included
first. After the upcoming changes this would break the 32bit build.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
Not sure who officially maintains this. Arnd, Christoph any objections?
include/net/compat.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/net/compat.h b/include/net/compat.h
index 745db0d605b6..08a089bbaecc 100644
--- a/include/net/compat.h
+++ b/include/net/compat.h
@@ -5,10 +5,10 @@
struct sock;
-#if defined(CONFIG_COMPAT)
-
#include <linux/compat.h>
+#if defined(CONFIG_COMPAT)
+
struct compat_msghdr {
compat_uptr_t msg_name; /* void * */
compat_int_t msg_namelen;
--
2.24.1
^ permalink raw reply related
* Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
From: Johannes Berg @ 2020-11-21 17:39 UTC (permalink / raw)
To: Florian Westphal, Ido Schimmel
Cc: Aleksandr Nogikh, davem, kuba, edumazet, andreyknvl, dvyukov,
elver, linux-kernel, netdev, linux-wireless,
willemdebruijn.kernel, Aleksandr Nogikh, Willem de Bruijn
In-Reply-To: <20201121165227.GT15137@breakpoint.cc>
On Sat, 2020-11-21 at 17:52 +0100, Florian Westphal wrote:
>
> Aleksandr, why was this made into an skb extension in the first place?
>
> AFAIU this feature is usually always disabled at build time.
> For debug builds (test farm /debug kernel etc) its always needed.
>
> If thats the case this u64 should be an sk_buff member, not an
> extension.
Because it was requested :-)
https://lore.kernel.org/netdev/20201009161558.57792e1a@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/
johannes
^ permalink raw reply
* Re: [RFC] MAINTAINERS tag for cleanup robot
From: James Bottomley @ 2020-11-21 17:18 UTC (permalink / raw)
To: trix, joe, clang-built-linux
Cc: linux-hyperv, linux-kernel, xen-devel, tboot-devel, kvm,
linux-crypto, linux-acpi, devel, amd-gfx, dri-devel, intel-gfx,
netdev, linux-media, MPT-FusionLinux.pdl, linux-scsi,
linux-wireless, ibm-acpi-devel, platform-driver-x86, linux-usb,
linux-omap, linux-fbdev, ecryptfs, linux-fsdevel, cluster-devel,
linux-mtd, keyrings, netfilter-devel, coreteam, alsa-devel, bpf,
linux-bluetooth, linux-nfs, patches
In-Reply-To: <20201121165058.1644182-1-trix@redhat.com>
On Sat, 2020-11-21 at 08:50 -0800, trix@redhat.com wrote:
> A difficult part of automating commits is composing the subsystem
> preamble in the commit log. For the ongoing effort of a fixer
> producing
> one or two fixes a release the use of 'treewide:' does not seem
> appropriate.
>
> It would be better if the normal prefix was used. Unfortunately
> normal is
> not consistent across the tree.
>
>
> D: Commit subsystem prefix
>
> ex/ for FPGA DFL DRIVERS
>
> D: fpga: dfl:
>
I've got to bet this is going to cause more issues than it solves.
SCSI uses scsi: <driver>: for drivers but not every driver has a
MAINTAINERS entry. We use either scsi: or scsi: core: for mid layer
things, but we're not consistent. Block uses blk-<something>: for all
of it's stuff but almost no <somtehing>s have a MAINTAINERS entry. So
the next thing you're going to cause is an explosion of suggested
MAINTAINERs entries.
Has anyone actually complained about treewide:?
James
^ permalink raw reply
* Re: [RFC] MAINTAINERS tag for cleanup robot
From: Joe Perches @ 2020-11-21 17:10 UTC (permalink / raw)
To: trix, clang-built-linux
Cc: linux-hyperv, linux-kernel, xen-devel, tboot-devel, kvm,
linux-crypto, linux-acpi, devel, amd-gfx, dri-devel, intel-gfx,
netdev, linux-media, MPT-FusionLinux.pdl, linux-scsi,
linux-wireless, ibm-acpi-devel, platform-driver-x86, linux-usb,
linux-omap, linux-fbdev, ecryptfs, linux-fsdevel, cluster-devel,
linux-mtd, keyrings, netfilter-devel, coreteam, alsa-devel, bpf,
linux-bluetooth, linux-nfs, patches
In-Reply-To: <20201121165058.1644182-1-trix@redhat.com>
On Sat, 2020-11-21 at 08:50 -0800, trix@redhat.com wrote:
> A difficult part of automating commits is composing the subsystem
> preamble in the commit log. For the ongoing effort of a fixer producing
> one or two fixes a release the use of 'treewide:' does not seem appropriate.
>
> It would be better if the normal prefix was used. Unfortunately normal is
> not consistent across the tree.
>
> So I am looking for comments for adding a new tag to the MAINTAINERS file
>
> D: Commit subsystem prefix
>
> ex/ for FPGA DFL DRIVERS
>
> D: fpga: dfl:
I'm all for it. Good luck with the effort. It's not completely trivial.
From a decade ago:
https://lore.kernel.org/lkml/1289919077.28741.50.camel@Joe-Laptop/
(and that thread started with extra semicolon patches too)
> Continuing with cleaning up clang's -Wextra-semi-stmt
> diff --git a/Makefile b/Makefile
[]
> @@ -1567,20 +1567,21 @@ help:
> echo ''
> @echo 'Static analysers:'
> @echo ' checkstack - Generate a list of stack hogs'
> @echo ' versioncheck - Sanity check on version.h usage'
> @echo ' includecheck - Check for duplicate included header files'
> @echo ' export_report - List the usages of all exported symbols'
> @echo ' headerdep - Detect inclusion cycles in headers'
> @echo ' coccicheck - Check with Coccinelle'
> @echo ' clang-analyzer - Check with clang static analyzer'
> @echo ' clang-tidy - Check with clang-tidy'
> + @echo ' clang-tidy-fix - Check and fix with clang-tidy'
A pity the ordering of the code below isn't the same as the above.
> -PHONY += clang-tidy clang-analyzer
> +PHONY += clang-tidy-fix clang-tidy clang-analyzer
[]
> -clang-tidy clang-analyzer: $(extmod-prefix)compile_commands.json
> +clang-tidy-fix clang-tidy clang-analyzer: $(extmod-prefix)compile_commands.json
> $(call cmd,clang_tools)
> else
> -clang-tidy clang-analyzer:
> +clang-tidy-fix clang-tidy clang-analyzer:
[]
> diff --git a/scripts/clang-tools/run-clang-tools.py b/scripts/clang-tools/run-clang-tools.py
[]
> @@ -22,43 +22,57 @@ def parse_arguments():
[]
> parser.add_argument("type",
> - choices=["clang-tidy", "clang-analyzer"],
> + choices=["clang-tidy-fix", "clang-tidy", "clang-analyzer"],
etc...
^ permalink raw reply
* Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
From: Florian Westphal @ 2020-11-21 16:52 UTC (permalink / raw)
To: Ido Schimmel
Cc: Aleksandr Nogikh, fw, davem, kuba, johannes, edumazet, andreyknvl,
dvyukov, elver, linux-kernel, netdev, linux-wireless,
willemdebruijn.kernel, Aleksandr Nogikh, Willem de Bruijn
In-Reply-To: <20201121160941.GA485907@shredder.lan>
Ido Schimmel <idosch@idosch.org> wrote:
> On Thu, Oct 29, 2020 at 05:36:19PM +0000, Aleksandr Nogikh wrote:
> > From: Aleksandr Nogikh <nogikh@google.com>
> >
> > Remote KCOV coverage collection enables coverage-guided fuzzing of the
> > code that is not reachable during normal system call execution. It is
> > especially helpful for fuzzing networking subsystems, where it is
> > common to perform packet handling in separate work queues even for the
> > packets that originated directly from the user space.
> >
> > Enable coverage-guided frame injection by adding kcov remote handle to
> > skb extensions. Default initialization in __alloc_skb and
> > __build_skb_around ensures that no socket buffer that was generated
> > during a system call will be missed.
> >
> > Code that is of interest and that performs packet processing should be
> > annotated with kcov_remote_start()/kcov_remote_stop().
> >
> > An alternative approach is to determine kcov_handle solely on the
> > basis of the device/interface that received the specific socket
> > buffer. However, in this case it would be impossible to distinguish
> > between packets that originated during normal background network
> > processes or were intentionally injected from the user space.
> >
> > Signed-off-by: Aleksandr Nogikh <nogikh@google.com>
> > Acked-by: Willem de Bruijn <willemb@google.com>
>
> [...]
>
> > @@ -249,6 +249,9 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
> >
> > fclones->skb2.fclone = SKB_FCLONE_CLONE;
> > }
> > +
> > + skb_set_kcov_handle(skb, kcov_common_handle());
>
> Hi,
>
> This causes skb extensions to be allocated for the allocated skb, but
> there are instances that blindly overwrite 'skb->extensions' by invoking
> skb_copy_header() after __alloc_skb(). For example, skb_copy(),
> __pskb_copy_fclone() and skb_copy_expand(). This results in the skb
> extensions being leaked [1].
[..]
> Other suggestions?
Aleksandr, why was this made into an skb extension in the first place?
AFAIU this feature is usually always disabled at build time.
For debug builds (test farm /debug kernel etc) its always needed.
If thats the case this u64 should be an sk_buff member, not an
extension.
^ permalink raw reply
* [RFC] MAINTAINERS tag for cleanup robot
From: trix @ 2020-11-21 16:50 UTC (permalink / raw)
To: trix, joe, clang-built-linux
Cc: linux-hyperv, linux-kernel, xen-devel, tboot-devel, kvm,
linux-crypto, linux-acpi, devel, amd-gfx, dri-devel, intel-gfx,
netdev, linux-media, MPT-FusionLinux.pdl, linux-scsi,
linux-wireless, ibm-acpi-devel, platform-driver-x86, linux-usb,
linux-omap, linux-fbdev, ecryptfs, linux-fsdevel, cluster-devel,
linux-mtd, keyrings, netfilter-devel, coreteam, alsa-devel, bpf,
linux-bluetooth, linux-nfs, patches
A difficult part of automating commits is composing the subsystem
preamble in the commit log. For the ongoing effort of a fixer producing
one or two fixes a release the use of 'treewide:' does not seem appropriate.
It would be better if the normal prefix was used. Unfortunately normal is
not consistent across the tree.
So I am looking for comments for adding a new tag to the MAINTAINERS file
D: Commit subsystem prefix
ex/ for FPGA DFL DRIVERS
D: fpga: dfl:
Continuing with cleaning up clang's -Wextra-semi-stmt
A significant number of warnings are caused by function like macros with
a trailing semicolon. For example.
#define FOO(a) a++; <-- extra, unneeded semicolon
void bar() {
int v = 0;
FOO(a);
}
Clang will warn at the FOO(a); expansion location. Instead of removing
the semicolon there, the fixer removes semicolon from the macro
definition. After the fixer, the code will be:
#define FOO(a) a++
void bar() {
int v = 0;
FOO(a);
}
The fixer review is
https://reviews.llvm.org/D91789
A run over allyesconfig for x86_64 finds 62 issues, 5 are false positives.
The false positives are caused by macros passed to other macros and by
some macro expansions that did not have an extra semicolon.
This cleans up about 1,000 of the current 10,000 -Wextra-semi-stmt
warnings in linux-next.
An update to [RFC] clang tooling cleanup
This change adds the clang-tidy-fix as a top level target and
uses it to do the cleaning. The next iteration will do a loop of
cleaners. This will mean breaking clang-tidy-fix out into its own
processing function 'run_fixers'.
Makefile: Add toplevel target clang-tidy-fix to makefile
Calls clang-tidy with -fix option for a set of checkers that
programatically fixes the kernel source in place, treewide.
Signed-off-by: Tom Rix <trix@redhat.com>
---
Makefile | 7 ++++---
scripts/clang-tools/run-clang-tools.py | 20 +++++++++++++++++---
2 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/Makefile b/Makefile
index 47a8add4dd28..57756dbb767b 100644
--- a/Makefile
+++ b/Makefile
@@ -1567,20 +1567,21 @@ help:
echo ''
@echo 'Static analysers:'
@echo ' checkstack - Generate a list of stack hogs'
@echo ' versioncheck - Sanity check on version.h usage'
@echo ' includecheck - Check for duplicate included header files'
@echo ' export_report - List the usages of all exported symbols'
@echo ' headerdep - Detect inclusion cycles in headers'
@echo ' coccicheck - Check with Coccinelle'
@echo ' clang-analyzer - Check with clang static analyzer'
@echo ' clang-tidy - Check with clang-tidy'
+ @echo ' clang-tidy-fix - Check and fix with clang-tidy'
@echo ''
@echo 'Tools:'
@echo ' nsdeps - Generate missing symbol namespace dependencies'
@echo ''
@echo 'Kernel selftest:'
@echo ' kselftest - Build and run kernel selftest'
@echo ' Build, install, and boot kernel before'
@echo ' running kselftest on it'
@echo ' Run as root for full coverage'
@echo ' kselftest-all - Build kernel selftest'
@@ -1842,30 +1843,30 @@ nsdeps: modules
quiet_cmd_gen_compile_commands = GEN $@
cmd_gen_compile_commands = $(PYTHON3) $< -a $(AR) -o $@ $(filter-out $<, $(real-prereqs))
$(extmod-prefix)compile_commands.json: scripts/clang-tools/gen_compile_commands.py \
$(if $(KBUILD_EXTMOD),,$(KBUILD_VMLINUX_OBJS) $(KBUILD_VMLINUX_LIBS)) \
$(if $(CONFIG_MODULES), $(MODORDER)) FORCE
$(call if_changed,gen_compile_commands)
targets += $(extmod-prefix)compile_commands.json
-PHONY += clang-tidy clang-analyzer
+PHONY += clang-tidy-fix clang-tidy clang-analyzer
ifdef CONFIG_CC_IS_CLANG
quiet_cmd_clang_tools = CHECK $<
cmd_clang_tools = $(PYTHON3) $(srctree)/scripts/clang-tools/run-clang-tools.py $@ $<
-clang-tidy clang-analyzer: $(extmod-prefix)compile_commands.json
+clang-tidy-fix clang-tidy clang-analyzer: $(extmod-prefix)compile_commands.json
$(call cmd,clang_tools)
else
-clang-tidy clang-analyzer:
+clang-tidy-fix clang-tidy clang-analyzer:
@echo "$@ requires CC=clang" >&2
@false
endif
# Scripts to check various things for consistency
# ---------------------------------------------------------------------------
PHONY += includecheck versioncheck coccicheck export_report
includecheck:
diff --git a/scripts/clang-tools/run-clang-tools.py b/scripts/clang-tools/run-clang-tools.py
index fa7655c7cec0..c177ca822c56 100755
--- a/scripts/clang-tools/run-clang-tools.py
+++ b/scripts/clang-tools/run-clang-tools.py
@@ -22,43 +22,57 @@ def parse_arguments():
Returns:
args: Dict of parsed args
Has keys: [path, type]
"""
usage = """Run clang-tidy or the clang static-analyzer on a
compilation database."""
parser = argparse.ArgumentParser(description=usage)
type_help = "Type of analysis to be performed"
parser.add_argument("type",
- choices=["clang-tidy", "clang-analyzer"],
+ choices=["clang-tidy-fix", "clang-tidy", "clang-analyzer"],
help=type_help)
path_help = "Path to the compilation database to parse"
parser.add_argument("path", type=str, help=path_help)
return parser.parse_args()
def init(l, a):
global lock
global args
lock = l
args = a
def run_analysis(entry):
# Disable all checks, then re-enable the ones we want
checks = "-checks=-*,"
- if args.type == "clang-tidy":
+ fix = ""
+ header_filter = ""
+ if args.type == "clang-tidy-fix":
+ checks += "linuxkernel-macro-trailing-semi"
+ #
+ # Fix this
+ # #define M(a) a++; <-- clang-tidy fixes the problem here
+ # int f() {
+ # int v = 0;
+ # M(v); <-- clang reports problem here
+ # return v;
+ # }
+ fix += "-fix"
+ header_filter += "-header-filter=.*"
+ elif args.type == "clang-tidy":
checks += "linuxkernel-*"
else:
checks += "clang-analyzer-*"
- p = subprocess.run(["clang-tidy", "-p", args.path, checks, entry["file"]],
+ p = subprocess.run(["clang-tidy", "-p", args.path, checks, header_filter, fix, entry["file"]],
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
cwd=entry["directory"])
with lock:
sys.stderr.buffer.write(p.stdout)
def main():
args = parse_arguments()
--
2.18.4
^ permalink raw reply related
* Re: [PATCH v5 2/3] net: add kcov handle to skb extensions
From: Ido Schimmel @ 2020-11-21 16:09 UTC (permalink / raw)
To: Aleksandr Nogikh, fw
Cc: davem, kuba, johannes, edumazet, andreyknvl, dvyukov, elver,
linux-kernel, netdev, linux-wireless, willemdebruijn.kernel,
Aleksandr Nogikh, Willem de Bruijn
In-Reply-To: <20201029173620.2121359-3-aleksandrnogikh@gmail.com>
+ Florian
On Thu, Oct 29, 2020 at 05:36:19PM +0000, Aleksandr Nogikh wrote:
> From: Aleksandr Nogikh <nogikh@google.com>
>
> Remote KCOV coverage collection enables coverage-guided fuzzing of the
> code that is not reachable during normal system call execution. It is
> especially helpful for fuzzing networking subsystems, where it is
> common to perform packet handling in separate work queues even for the
> packets that originated directly from the user space.
>
> Enable coverage-guided frame injection by adding kcov remote handle to
> skb extensions. Default initialization in __alloc_skb and
> __build_skb_around ensures that no socket buffer that was generated
> during a system call will be missed.
>
> Code that is of interest and that performs packet processing should be
> annotated with kcov_remote_start()/kcov_remote_stop().
>
> An alternative approach is to determine kcov_handle solely on the
> basis of the device/interface that received the specific socket
> buffer. However, in this case it would be impossible to distinguish
> between packets that originated during normal background network
> processes or were intentionally injected from the user space.
>
> Signed-off-by: Aleksandr Nogikh <nogikh@google.com>
> Acked-by: Willem de Bruijn <willemb@google.com>
[...]
> @@ -249,6 +249,9 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
>
> fclones->skb2.fclone = SKB_FCLONE_CLONE;
> }
> +
> + skb_set_kcov_handle(skb, kcov_common_handle());
Hi,
This causes skb extensions to be allocated for the allocated skb, but
there are instances that blindly overwrite 'skb->extensions' by invoking
skb_copy_header() after __alloc_skb(). For example, skb_copy(),
__pskb_copy_fclone() and skb_copy_expand(). This results in the skb
extensions being leaked [1].
One possible solution is to try to patch all these instances with
skb_ext_put() before skb_copy_header().
Another possible solution is to convert skb_copy_header() to use
skb_ext_copy() instead of __skb_ext_copy(). It will first drop the
reference on the skb extensions of the new skb, but it assumes that
'skb->active_extensions' is valid. This is not the case in the
skb_clone() path so we should probably zero this field in __skb_clone().
Other suggestions?
Thanks
[1]
BUG: memory leak
unreferenced object 0xffff888027f9a490 (size 16):
comm "syz-executor.0", pid 1155, jiffies 4295996826 (age 66.927s)
hex dump (first 16 bytes):
01 00 00 00 01 02 6b 6b 01 00 00 00 00 00 00 00 ......kk........
backtrace:
[<0000000005a5f2c4>] kmemleak_alloc_recursive include/linux/kmemleak.h:43 [inline]
[<0000000005a5f2c4>] slab_post_alloc_hook mm/slab.h:528 [inline]
[<0000000005a5f2c4>] slab_alloc_node mm/slub.c:2891 [inline]
[<0000000005a5f2c4>] slab_alloc mm/slub.c:2899 [inline]
[<0000000005a5f2c4>] kmem_cache_alloc+0x173/0x800 mm/slub.c:2904
[<00000000c5e43ea9>] __skb_ext_alloc+0x22/0x90 net/core/skbuff.c:6173
[<000000000de35e81>] skb_ext_add+0x230/0x4a0 net/core/skbuff.c:6268
[<000000003b7efba4>] skb_set_kcov_handle include/linux/skbuff.h:4622 [inline]
[<000000003b7efba4>] skb_set_kcov_handle include/linux/skbuff.h:4612 [inline]
[<000000003b7efba4>] __alloc_skb+0x47f/0x6a0 net/core/skbuff.c:253
[<000000007f789b23>] skb_copy+0x151/0x310 net/core/skbuff.c:1512
[<000000001ce26864>] mlxsw_emad_transmit+0x4e/0x620 drivers/net/ethernet/mellanox/mlxsw/core.c:585
[<000000005c732123>] mlxsw_emad_reg_access drivers/net/ethernet/mellanox/mlxsw/core.c:829 [inline]
[<000000005c732123>] mlxsw_core_reg_access_emad+0xda8/0x1770 drivers/net/ethernet/mellanox/mlxsw/core.c:2408
[<00000000c07840b3>] mlxsw_core_reg_access+0x101/0x7f0 drivers/net/ethernet/mellanox/mlxsw/core.c:2583
[<000000007c47f30f>] mlxsw_reg_write+0x30/0x40 drivers/net/ethernet/mellanox/mlxsw/core.c:2603
[<00000000675e3fc7>] mlxsw_sp_port_admin_status_set+0x8a7/0x980 drivers/net/ethernet/mellanox/mlxsw/spectrum.c:300
[<00000000fefe35a4>] mlxsw_sp_port_stop+0x63/0x70 drivers/net/ethernet/mellanox/mlxsw/spectrum.c:537
[<00000000c41390e8>] __dev_close_many+0x1c7/0x300 net/core/dev.c:1607
[<00000000628c5987>] __dev_close net/core/dev.c:1619 [inline]
[<00000000628c5987>] __dev_change_flags+0x2b9/0x710 net/core/dev.c:8421
[<000000008cc810c6>] dev_change_flags+0x97/0x170 net/core/dev.c:8494
[<0000000053274a78>] do_setlink+0xa5b/0x3b80 net/core/rtnetlink.c:2706
[<00000000e4085785>] rtnl_group_changelink net/core/rtnetlink.c:3225 [inline]
[<00000000e4085785>] __rtnl_newlink+0xe06/0x17d0 net/core/rtnetlink.c:3379
^ permalink raw reply
* [PATCH net-next] net: sched: alias action flags with TCA_ACT_ prefix
From: Vlad Buslov @ 2020-11-21 16:09 UTC (permalink / raw)
To: netdev, davem, kuba; +Cc: jhs, xiyou.wangcong, jiri, Vlad Buslov
Currently both filter and action flags use same "TCA_" prefix which makes
them hard to distinguish to code and confusing for users. Create aliases
for existing action flags constants with "TCA_ACT_" prefix.
Signed-off-by: Vlad Buslov <vlad@buslov.dev>
---
include/uapi/linux/rtnetlink.h | 11 +++++++----
net/sched/act_api.c | 10 +++++-----
2 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 2ffbef5da6c1..91db081e5360 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -768,16 +768,19 @@ enum {
#define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
/* tcamsg flags stored in attribute TCA_ROOT_FLAGS
*
- * TCA_FLAG_LARGE_DUMP_ON user->kernel to request for larger than TCA_ACT_MAX_PRIO
- * actions in a dump. All dump responses will contain the number of actions
- * being dumped stored in for user app's consumption in TCA_ROOT_COUNT
+ * TCA_ACT_FLAG_LARGE_DUMP_ON user->kernel to request for larger than
+ * TCA_ACT_MAX_PRIO actions in a dump. All dump responses will contain the
+ * number of actions being dumped stored in for user app's consumption in
+ * TCA_ROOT_COUNT
*
- * TCA_FLAG_TERSE_DUMP user->kernel to request terse (brief) dump that only
+ * TCA_ACT_FLAG_TERSE_DUMP user->kernel to request terse (brief) dump that only
* includes essential action info (kind, index, etc.)
*
*/
#define TCA_FLAG_LARGE_DUMP_ON (1 << 0)
+#define TCA_ACT_FLAG_LARGE_DUMP_ON TCA_FLAG_LARGE_DUMP_ON
#define TCA_FLAG_TERSE_DUMP (1 << 1)
+#define TCA_ACT_FLAG_TERSE_DUMP TCA_FLAG_TERSE_DUMP
/* New extended info filters for IFLA_EXT_MASK */
#define RTEXT_FILTER_VF (1 << 0)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index fc23f46a315c..99db1c77426b 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -278,7 +278,7 @@ static int tcf_dump_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
index--;
goto nla_put_failure;
}
- err = (act_flags & TCA_FLAG_TERSE_DUMP) ?
+ err = (act_flags & TCA_ACT_FLAG_TERSE_DUMP) ?
tcf_action_dump_terse(skb, p, true) :
tcf_action_dump_1(skb, p, 0, 0);
if (err < 0) {
@@ -288,7 +288,7 @@ static int tcf_dump_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
}
nla_nest_end(skb, nest);
n_i++;
- if (!(act_flags & TCA_FLAG_LARGE_DUMP_ON) &&
+ if (!(act_flags & TCA_ACT_FLAG_LARGE_DUMP_ON) &&
n_i >= TCA_ACT_MAX_PRIO)
goto done;
}
@@ -298,7 +298,7 @@ static int tcf_dump_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
mutex_unlock(&idrinfo->lock);
if (n_i) {
- if (act_flags & TCA_FLAG_LARGE_DUMP_ON)
+ if (act_flags & TCA_ACT_FLAG_LARGE_DUMP_ON)
cb->args[1] = n_i;
}
return n_i;
@@ -1473,8 +1473,8 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
}
static const struct nla_policy tcaa_policy[TCA_ROOT_MAX + 1] = {
- [TCA_ROOT_FLAGS] = NLA_POLICY_BITFIELD32(TCA_FLAG_LARGE_DUMP_ON |
- TCA_FLAG_TERSE_DUMP),
+ [TCA_ROOT_FLAGS] = NLA_POLICY_BITFIELD32(TCA_ACT_FLAG_LARGE_DUMP_ON |
+ TCA_ACT_FLAG_TERSE_DUMP),
[TCA_ROOT_TIME_DELTA] = { .type = NLA_U32 },
};
--
2.29.1
^ permalink raw reply related
* Re: [PATCH net-next 2/2] net: dsa: hellcreek: Don't print error message on defer
From: Florian Fainelli @ 2020-11-21 15:01 UTC (permalink / raw)
To: Kurt Kanzenbach, Andrew Lunn, Vivien Didelot, Vladimir Oltean
Cc: David S. Miller, Jakub Kicinski, netdev
In-Reply-To: <20201121114455.22422-3-kurt@linutronix.de>
On 11/21/2020 3:44 AM, Kurt Kanzenbach wrote:
> When DSA is not loaded when the driver is probed an error message is
> printed. But, that's not really an error, just a defer. Use dev_err_probe()
> instead.
>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH net-next 1/2] net: dsa: tag_hellcreek: Cleanup includes
From: Florian Fainelli @ 2020-11-21 15:00 UTC (permalink / raw)
To: Kurt Kanzenbach, Andrew Lunn, Vivien Didelot, Vladimir Oltean
Cc: David S. Miller, Jakub Kicinski, netdev
In-Reply-To: <20201121114455.22422-2-kurt@linutronix.de>
On 11/21/2020 3:44 AM, Kurt Kanzenbach wrote:
> Remove unused and add needed includes. No functional change.
>
> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ 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