* Re: [PATCH 2/4] seccomp: add two missing ptrace ifdefines
From: shuah @ 2019-09-19 17:04 UTC (permalink / raw)
To: Kees Cook, Dmitry V. Levin
Cc: Tyler Hicks, Christian Brauner, luto, jannh, wad, ast, daniel,
kafai, songliubraving, yhs, linux-kernel, linux-kselftest, netdev,
bpf, Tycho Andersen, stable, shuah
In-Reply-To: <201909190918.443D6BC7@keescook>
On 9/19/19 10:55 AM, Kees Cook wrote:
> On Thu, Sep 19, 2019 at 01:42:51PM +0300, Dmitry V. Levin wrote:
>> On Wed, Sep 18, 2019 at 10:33:09AM -0700, Kees Cook wrote:
>>> This is actually fixed in -next already (and, yes, with the Fixes line
>>> Tyler has mentioned):
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=next&id=69b2d3c5924273a0ae968d3818210fc57a1b9d07
>>
>> Excuse me, does it mean that you expect each selftest to be self-hosted?
>> I was (and still is) under impression that selftests should be built
>> with headers installed from the tree. Is it the case, or is it not?
>
> As you know (but to give others some context) there is a long-standing
> bug in the selftest build environment that causes these problems (it
> isn't including the uAPI headers) which you'd proposed to be fixed
> recently[1]. Did that ever get sent as a "real" patch? I don't see it
> in Shuah's tree; can you send it to Shuah?
>
> But even with that fixed, since the seccomp selftest has a history of
> being built stand-alone, I've continued to take these kinds of fixes.
>
> -Kees
>
> [1] https://lore.kernel.org/lkml/20190805094719.GA1693@altlinux.org/
>
It has been sent to kselftest list yesterday. I will pull this in for
my next update.
thanks,
-- Shuah
^ permalink raw reply
* Re: dsa traffic priorization
From: Florian Fainelli @ 2019-09-19 17:12 UTC (permalink / raw)
To: Sascha Hauer; +Cc: Vladimir Oltean, netdev, Andrew Lunn, Vivien Didelot, kernel
In-Reply-To: <20190919084416.33ifxohtgkofrleb@pengutronix.de>
On 9/19/19 1:44 AM, Sascha Hauer wrote:
> Hi Florian,
>
> On Wed, Sep 18, 2019 at 10:41:58AM -0700, Florian Fainelli wrote:
>> On 9/18/19 7:36 AM, Vladimir Oltean wrote:
>>> Hi Sascha,
>>>
>>> On Wed, 18 Sep 2019 at 17:03, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>>>>
>>>> Hi All,
>>>>
>>>> We have a customer using a Marvell 88e6240 switch with Ethercat on one port and
>>>> regular network traffic on another port. The customer wants to configure two things
>>>> on the switch: First Ethercat traffic shall be priorized over other network traffic
>>>> (effectively prioritizing traffic based on port). Second the ethernet controller
>>>> in the CPU is not able to handle full bandwidth traffic, so the traffic to the CPU
>>>> port shall be rate limited.
>>>>
>>>
>>> You probably already know this, but egress shaping will not drop
>>> frames, just let them accumulate in the egress queue until something
>>> else happens (e.g. queue occupancy threshold triggers pause frames, or
>>> tail dropping is enabled, etc). Is this what you want? It sounds a bit
>>> strange to me to configure egress shaping on the CPU port of a DSA
>>> switch. That literally means you are buffering frames inside the
>>> system. What about ingress policing?
>>
>> Indeed, but I suppose that depending on the switch architecture and/or
>> nomenclature, configuring egress shaping amounts to determining ingress
>> for the ports where the frame is going to be forwarded to.
>>
>> For instance Broadcom switches rarely if at all mention ingress because
>> the frames have to originate from somewhere and be forwarded to other
>> port(s), therefore, they will egress their original port (which for all
>> practical purposes is the direct continuation of the ingress stage),
>> where shaping happens, which immediately influences the ingress shaping
>> of the destination port, which will egress the frame eventually because
>> packets have to be delivered to the final port's egress queue anyway.
>>
>>>
>>>> For reference the patch below configures the switch to their needs. Now the question
>>>> is how this can be implemented in a way suitable for mainline. It looks like the per
>>>> port priority mapping for VLAN tagged packets could be done via ip link add link ...
>>>> ingress-qos-map QOS-MAP. How the default priority would be set is unclear to me.
>>>>
>>>
>>> Technically, configuring a match-all rxnfc rule with ethtool would
>>> count as 'default priority' - I have proposed that before. Now I'm not
>>> entirely sure how intuitive it is, but I'm also interested in being
>>> able to configure this.
>>
>> That does not sound too crazy from my perspective.
>>
>>>
>>>> The other part of the problem seems to be that the CPU port has no network device
>>>> representation in Linux, so there's no interface to configure the egress limits via tc.
>>>> This has been discussed before, but it seems there hasn't been any consensous regarding how
>>>> we want to proceed?
>>
>> You have the DSA master network device which is on the other side of the
>> switch,
>
> You mean the (in my case) i.MX FEC? Everything I do on this device ends
> up in the FEC itself and not on the switch, right?
Yes, we have a way to overload specific netdevice_ops and ethtool_ops
operations in order to use the i.MX FEC network device as configuration
entry point, say eth0, but have it operate on the switch, because when
the DSA switch got attached to the DSA master, we replaced some of that
network device's operations with ones that piggy back into the switch.
See net/dsa/master.c for details.
--
Florian
^ permalink raw reply
* [PATCH net] ipv6: fix a typo in fib6_rule_lookup()
From: Eric Dumazet @ 2019-09-19 17:12 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet, Wei Wang, Yi Ren
Yi Ren reported an issue discovered by syzkaller, and bisected
to the cited commit.
Many thanks to Yi, this trivial patch does not reflect the patient
work that has been done.
Fixes: d64a1f574a29 ("ipv6: honor RT6_LOOKUP_F_DST_NOREF in rule lookup logic")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Acked-by: Wei Wang <weiwan@google.com>
Bisected-and-Reported-by: Yi Ren <c4tren@gmail.com>
---
net/ipv6/ip6_fib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 87f47bc55c5e3f5e5f522d153fad69d6d82e6332..6e2af411cd9cc4131c5d457e6d48f177fc6d4428 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -318,7 +318,7 @@ struct dst_entry *fib6_rule_lookup(struct net *net, struct flowi6 *fl6,
if (rt->dst.error == -EAGAIN) {
ip6_rt_put_flags(rt, flags);
rt = net->ipv6.ip6_null_entry;
- if (!(flags | RT6_LOOKUP_F_DST_NOREF))
+ if (!(flags & RT6_LOOKUP_F_DST_NOREF))
dst_hold(&rt->dst);
}
--
2.23.0.237.gc6a4ce50a0-goog
^ permalink raw reply related
* Re: [PATCH v1 3/3] seccomp: test SECCOMP_USER_NOTIF_FLAG_CONTINUE
From: shuah @ 2019-09-19 17:13 UTC (permalink / raw)
To: Christian Brauner, keescook, luto
Cc: jannh, wad, ast, daniel, kafai, songliubraving, yhs, linux-kernel,
linux-kselftest, netdev, bpf, Tycho Andersen, Tyler Hicks, stable,
shuah
In-Reply-To: <20190919095903.19370-4-christian.brauner@ubuntu.com>
On 9/19/19 3:59 AM, Christian Brauner wrote:
> Test whether a syscall can be performed after having been intercepted by
> the seccomp notifier. The test uses dup() and kcmp() since it allows us to
> nicely test whether the dup() syscall actually succeeded by comparing whether
> the fds refer to the same underlying struct file.
>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Will Drewry <wad@chromium.org>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Martin KaFai Lau <kafai@fb.com>
> Cc: Song Liu <songliubraving@fb.com>
> Cc: Yonghong Song <yhs@fb.com>
> Cc: Tycho Andersen <tycho@tycho.ws>
> CC: Tyler Hicks <tyhicks@canonical.com>
> Cc: stable@vger.kernel.org
> Cc: linux-kselftest@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: bpf@vger.kernel.org
> ---
> /* v1 */
> - Christian Brauner <christian.brauner@ubuntu.com>:
> - adapt to new flag name SECCOMP_USER_NOTIF_FLAG_CONTINUE
>
> /* v0 */
> Link: https://lore.kernel.org/r/20190918084833.9369-5-christian.brauner@ubuntu.com
> ---
> tools/testing/selftests/seccomp/seccomp_bpf.c | 102 ++++++++++++++++++
> 1 file changed, 102 insertions(+)
>
> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> index e996d7b7fd6e..b0966599acb5 100644
> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> @@ -44,6 +44,7 @@
> #include <sys/times.h>
> #include <sys/socket.h>
> #include <sys/ioctl.h>
> +#include <linux/kcmp.h>
>
> #include <unistd.h>
> #include <sys/syscall.h>
> @@ -167,6 +168,10 @@ struct seccomp_metadata {
>
> #define SECCOMP_RET_USER_NOTIF 0x7fc00000U
>
> +#ifndef SECCOMP_USER_NOTIF_FLAG_CONTINUE
> +#define SECCOMP_USER_NOTIF_FLAG_CONTINUE 0x00000001
> +#endif
> +
> #define SECCOMP_IOC_MAGIC '!'
> #define SECCOMP_IO(nr) _IO(SECCOMP_IOC_MAGIC, nr)
> #define SECCOMP_IOR(nr, type) _IOR(SECCOMP_IOC_MAGIC, nr, type)
> @@ -3481,6 +3486,103 @@ TEST(seccomp_get_notif_sizes)
> EXPECT_EQ(sizes.seccomp_notif_resp, sizeof(struct seccomp_notif_resp));
> }
>
> +static int filecmp(pid_t pid1, pid_t pid2, int fd1, int fd2)
> +{
> +#ifdef __NR_kcmp
> + return syscall(__NR_kcmp, pid1, pid2, KCMP_FILE, fd1, fd2);
> +#else
> + errno = ENOSYS;
> + return -1;
This should be SKIP for kselftest so this isn't counted a failure.
In this case test can't be run because of a missing dependency.
> +#endif
> +}
> +
> +TEST(user_notification_continue)
> +{
> + pid_t pid;
> + long ret;
> + int status, listener;
> + struct seccomp_notif req = {};
> + struct seccomp_notif_resp resp = {};
> + struct pollfd pollfd;
> +
> + ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
> + ASSERT_EQ(0, ret) {
> + TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
> + }
> +
> + listener = user_trap_syscall(__NR_dup, SECCOMP_FILTER_FLAG_NEW_LISTENER);
> + ASSERT_GE(listener, 0);
> +
> + pid = fork();
> + ASSERT_GE(pid, 0);
> +
> + if (pid == 0) {
> + int dup_fd, pipe_fds[2];
> + pid_t self;
> +
> + ret = pipe(pipe_fds);
> + if (ret < 0)
> + exit(EXIT_FAILURE);
> +
> + dup_fd = dup(pipe_fds[0]);
> + if (dup_fd < 0)
> + exit(EXIT_FAILURE);
> +
> + self = getpid();
> +
> + ret = filecmp(self, self, pipe_fds[0], dup_fd);
> + if (ret)
> + exit(EXIT_FAILURE);
> +
> + exit(EXIT_SUCCESS);
> + }
> +
> + pollfd.fd = listener;
> + pollfd.events = POLLIN | POLLOUT;
> +
> + EXPECT_GT(poll(&pollfd, 1, -1), 0);
> + EXPECT_EQ(pollfd.revents, POLLIN);
> +
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0);
> +
> + pollfd.fd = listener;
> + pollfd.events = POLLIN | POLLOUT;
> +
> + EXPECT_GT(poll(&pollfd, 1, -1), 0);
> + EXPECT_EQ(pollfd.revents, POLLOUT);
> +
> + EXPECT_EQ(req.data.nr, __NR_dup);
> +
> + resp.id = req.id;
> + resp.flags = SECCOMP_USER_NOTIF_FLAG_CONTINUE;
> +
> + /*
> + * Verify that setting SECCOMP_USER_NOTIF_FLAG_CONTINUE enforces other
> + * args be set to 0.
> + */
> + resp.error = 0;
> + resp.val = USER_NOTIF_MAGIC;
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), -1);
> + EXPECT_EQ(errno, EINVAL);
> +
> + resp.error = USER_NOTIF_MAGIC;
> + resp.val = 0;
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), -1);
> + EXPECT_EQ(errno, EINVAL);
> +
> + resp.error = 0;
> + resp.val = 0;
> + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0) {
> + if (errno == EINVAL)
> + XFAIL(goto skip, "Kernel does not support SECCOMP_USER_NOTIF_FLAG_CONTINUE");
> + }
> +
> +skip:
> + EXPECT_EQ(waitpid(pid, &status, 0), pid);
> + EXPECT_EQ(true, WIFEXITED(status));
> + EXPECT_EQ(0, WEXITSTATUS(status));
> +}
> +
> /*
> * TODO:
> * - add microbenchmarks
>
thanks,
-- Shuah
^ permalink raw reply
* Re: [bpf-next,v3] samples: bpf: add max_pckt_size option at xdp_adjust_tail
From: Andrii Nakryiko @ 2019-09-19 17:52 UTC (permalink / raw)
To: Daniel T. Lee; +Cc: Daniel Borkmann, Alexei Starovoitov, Networking, bpf
In-Reply-To: <CAEKGpzjf22NpMapev7OnxSmU2HpHoEcGHjX81Pw4LDvOt58NRw@mail.gmail.com>
On Thu, Sep 19, 2019 at 2:16 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> On Thu, Sep 19, 2019 at 3:00 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Wed, Sep 18, 2019 at 10:37 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> > >
> > > On Tue, Sep 17, 2019 at 1:04 PM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > On Wed, Sep 11, 2019 at 2:33 PM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> > > > >
> > > > > Currently, at xdp_adjust_tail_kern.c, MAX_PCKT_SIZE is limited
> > > > > to 600. To make this size flexible, a new map 'pcktsz' is added.
> > > > >
> > > > > By updating new packet size to this map from the userland,
> > > > > xdp_adjust_tail_kern.o will use this value as a new max_pckt_size.
> > > > >
> > > > > If no '-P <MAX_PCKT_SIZE>' option is used, the size of maximum packet
> > > > > will be 600 as a default.
> > > > >
> > > > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > > > >
> > > > > ---
> > > > > Changes in v2:
> > > > > - Change the helper to fetch map from 'bpf_map__next' to
> > > > > 'bpf_object__find_map_fd_by_name'.
> > > > >
> > > > > samples/bpf/xdp_adjust_tail_kern.c | 23 +++++++++++++++++++----
> > > > > samples/bpf/xdp_adjust_tail_user.c | 28 ++++++++++++++++++++++------
> > > > > 2 files changed, 41 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/samples/bpf/xdp_adjust_tail_kern.c b/samples/bpf/xdp_adjust_tail_kern.c
> > > > > index 411fdb21f8bc..d6d84ffe6a7a 100644
> > > > > --- a/samples/bpf/xdp_adjust_tail_kern.c
> > > > > +++ b/samples/bpf/xdp_adjust_tail_kern.c
> > > > > @@ -25,6 +25,13 @@
> > > > > #define ICMP_TOOBIG_SIZE 98
> > > > > #define ICMP_TOOBIG_PAYLOAD_SIZE 92
> > > > >
> > > > > +struct bpf_map_def SEC("maps") pcktsz = {
> > > > > + .type = BPF_MAP_TYPE_ARRAY,
> > > > > + .key_size = sizeof(__u32),
> > > > > + .value_size = sizeof(__u32),
> > > > > + .max_entries = 1,
> > > > > +};
> > > > > +
> > > >
> > > > Hey Daniel,
> > > >
> > > > This looks like an ideal use case for global variables on BPF side. I
> > > > think it's much cleaner and will make BPF side of things simpler.
> > > > Would you mind giving global data a spin instead of adding this map?
> > > >
> > >
> > > Sure thing!
> > > But, I'm not sure there is global variables for BPF?
> > > AFAIK, there aren't any support for global variables yet in BPF
> > > program (_kern.c).
> > >
> > > # when defining global variable at _kern.c
> > > libbpf: bpf: relocation: not yet supported relo for non-static
> > > global '<var>' variable found in insns[39].code 0x18
> >
> > just what it says: use static global variable (also volatile to
> > prevent compiler optimizations) :)
> >
> > static volatile __u32 pcktsz; /* this should work */
> >
>
> My apologies, but I'm not sure I'm following.
> What you are saying is, should I define global variable to _kern,c
> and access and modify this variable from _user.c?
Exactly.
>
> For example,
>
> <_kern.c>
> static volatile __u32 pcktsz = 300;
So this part is right.
>
> <_user.c>
> extern __u32 pcktsz;
> // Later in code
> pcktsz = 400;
This one is not as simple, unfortunately. From user side you need to
find an internal map (it will most probably have .bss suffix) and
update it. See selftests/bpf/prog_tests/global_data.c for how it can
be done. Eventually working with BPF global data will be much more
natural, but we don't yet have that implemented.
>
> Is this code means similar to what you've said?
> AFAIK, 'static' keyword for global variable restricts scope to file itself,
> so the 'accessing' and 'modifying' this variable from the <_user.c>
> isn't available.
>
> The reason why I've used bpf map for this 'pcktsz' option is,
> I've wanted to run this kernel xdp program (xdp_adjust_tail_kern.o)
> as it itself, not heavily controlled by user program (./xdp_adjust_tail).
>
> When this 'pcktsz' option is implemented in bpf map, user can simply
> modify 'map' to change this size. (such as bpftool prog map)
>
> But when this variable comes to global data, it can't be changed
> after the program gets loaded.
>
> I really appreciate your time and effort for the review.
> But I'm sorry that I seem to get it wrong.
I understand your confusion, but BPF global data behaves exactly like
what you explain. From BPF side it looks like a normal variable.
Performance-wise it's also faster than doing explicit map lookup. From
user space side it's still a map, though, so you can read/modify it
and generally do the same communication between BPF kernel and user
space as you are used to with maps. Check selftests, it should make it
clearer.
>
> Thanks,
> Daniel
>
> > >
> > > By the way, thanks for the review.
> > >
> > > Thanks,
> > > Daniel
> > >
> > >
> > > > > struct bpf_map_def SEC("maps") icmpcnt = {
> > > > > .type = BPF_MAP_TYPE_ARRAY,
> > > > > .key_size = sizeof(__u32),
> > > > > @@ -64,7 +71,8 @@ static __always_inline void ipv4_csum(void *data_start, int data_size,
> > > > > *csum = csum_fold_helper(*csum);
> > > > > }
> > > > >
> > > >
> > > > [...]
^ permalink raw reply
* Re: [PATCH v3 bpf-next 09/14] samples: bpf: makefile: use own flags but not host when cross compile
From: Andrii Nakryiko @ 2019-09-19 17:54 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: Alexei Starovoitov, Daniel Borkmann, Yonghong Song,
David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
john fastabend, open list, Networking, bpf, clang-built-linux,
sergei.shtylyov
In-Reply-To: <20190919141848.GA8870@khorivan>
On Thu, Sep 19, 2019 at 7:18 AM Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
> On Wed, Sep 18, 2019 at 02:29:53PM -0700, Andrii Nakryiko wrote:
> >On Wed, Sep 18, 2019 at 3:35 AM Ivan Khoronzhuk
> ><ivan.khoronzhuk@linaro.org> wrote:
> >>
> >> On Tue, Sep 17, 2019 at 04:42:07PM -0700, Andrii Nakryiko wrote:
> >> >On Mon, Sep 16, 2019 at 3:59 AM Ivan Khoronzhuk
> >> ><ivan.khoronzhuk@linaro.org> wrote:
> >> >>
> >> >> While compile natively, the hosts cflags and ldflags are equal to ones
> >> >> used from HOSTCFLAGS and HOSTLDFLAGS. When cross compiling it should
> >> >> have own, used for target arch. While verification, for arm, arm64 and
> >> >> x86_64 the following flags were used alsways:
> >> >>
> >> >> -Wall
> >> >> -O2
> >> >> -fomit-frame-pointer
> >> >> -Wmissing-prototypes
> >> >> -Wstrict-prototypes
> >> >>
> >> >> So, add them as they were verified and used before adding
> >> >> Makefile.target, but anyway limit it only for cross compile options as
> >> >> for host can be some configurations when another options can be used,
> >> >> So, for host arch samples left all as is, it allows to avoid potential
> >> >> option mistmatches for existent environments.
> >> >>
> >> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >> >> ---
> >> >> samples/bpf/Makefile | 9 +++++++++
> >> >> 1 file changed, 9 insertions(+)
> >> >>
> >> >> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> >> >> index 1579cc16a1c2..b5c87a8b8b51 100644
> >> >> --- a/samples/bpf/Makefile
> >> >> +++ b/samples/bpf/Makefile
> >> >> @@ -178,8 +178,17 @@ CLANG_EXTRA_CFLAGS := $(ARM_ARCH_SELECTOR)
> >> >> TPROGS_CFLAGS += $(ARM_ARCH_SELECTOR)
> >> >> endif
> >> >>
> >> >> +ifdef CROSS_COMPILE
> >> >> +TPROGS_CFLAGS += -Wall
> >> >> +TPROGS_CFLAGS += -O2
> >> >
> >> >Specifying one arg per line seems like overkill, put them in one line?
> >> Will combine.
> >>
> >> >
> >> >> +TPROGS_CFLAGS += -fomit-frame-pointer
> >> >
> >> >Why this one?
> >> I've explained in commit msg. The logic is to have as much as close options
> >> to have smiliar binaries. As those options are used before for hosts and kinda
> >> cross builds - better follow same way.
> >
> >I'm just asking why omit frame pointers and make it harder to do stuff
> >like profiling? What performance benefits are we seeking for in BPF
> >samples?
> >
> >>
> >> >
> >> >> +TPROGS_CFLAGS += -Wmissing-prototypes
> >> >> +TPROGS_CFLAGS += -Wstrict-prototypes
> >> >
> >> >Are these in some way special that we want them in cross-compile mode only?
> >> >
> >> >All of those flags seem useful regardless of cross-compilation or not,
> >> >shouldn't they be common? I'm a bit lost about the intent here...
> >> They are common but split is needed to expose it at least. Also host for
> >> different arches can have some own opts already used that shouldn't be present
> >> for cross, better not mix it for safety.
> >
> >We want -Wmissing-prototypes and -Wstrict-prototypes for cross-compile
> >and non-cross-compile cases, right? So let's specify them as common
> >set of options, instead of relying on KBUILD_HOSTCFLAGS or
> >HOST_EXTRACFLAGS to have them. Otherwise we'll be getting extra
> >warnings for just cross-compile case, which is not good. If you are
> >worrying about having duplicate -W flags, seems like it's handled by
> >GCC already, so shouldn't be a problem.
>
> Ok, lets drop omit-frame-pointer.
>
> But then, lets do more radical step and drop
> KBUILD_HOSTCFLAGS & HOST_EXTRACFLAG in this patch:
Yeah, let's do this, if you confirmed that everything still works (and
I don't see a reason why it shouldn't). Thanks.
>
> -ifdef CROSS_COMPILE
> +TPROGS_CFLAGS += -Wall -O2
> +TPROGS_CFLAGS += -Wmissing-prototypes
> +TPROGS_CFLAGS += -Wstrict-prototypes
> -else
> -TPROGS_LDLIBS := $(KBUILD_HOSTLDLIBS)
> -TPROGS_CFLAGS += $(KBUILD_HOSTCFLAGS) $(HOST_EXTRACFLAGS)
> -endif
>
> At least it allows to use same options always for both, native and cross.
>
> I verified on native x86_64, arm64 and arm and cross for arm and arm64,
> but should work for others, at least it can be tuned explicitly and
> no need to depend on KBUILD and use "cross" fork here.
Yep, I like it.
>
> --
> Regards,
> Ivan Khoronzhuk
^ permalink raw reply
* Re: [PATCH 2/4] seccomp: add two missing ptrace ifdefines
From: Dmitry V. Levin @ 2019-09-19 18:30 UTC (permalink / raw)
To: Kees Cook
Cc: Tyler Hicks, Christian Brauner, luto, jannh, wad, shuah, ast,
daniel, kafai, songliubraving, yhs, linux-kernel, linux-kselftest,
netdev, bpf, Tycho Andersen, stable
In-Reply-To: <201909190918.443D6BC7@keescook>
On Thu, Sep 19, 2019 at 09:55:30AM -0700, Kees Cook wrote:
> On Thu, Sep 19, 2019 at 01:42:51PM +0300, Dmitry V. Levin wrote:
> > On Wed, Sep 18, 2019 at 10:33:09AM -0700, Kees Cook wrote:
> > > This is actually fixed in -next already (and, yes, with the Fixes line
> > > Tyler has mentioned):
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=next&id=69b2d3c5924273a0ae968d3818210fc57a1b9d07
> >
> > Excuse me, does it mean that you expect each selftest to be self-hosted?
> > I was (and still is) under impression that selftests should be built
> > with headers installed from the tree. Is it the case, or is it not?
>
> As you know (but to give others some context) there is a long-standing
> bug in the selftest build environment that causes these problems (it
> isn't including the uAPI headers) which you'd proposed to be fixed
> recently[1]. Did that ever get sent as a "real" patch? I don't see it
> in Shuah's tree; can you send it to Shuah?
>
> [1] https://lore.kernel.org/lkml/20190805094719.GA1693@altlinux.org/
The [1] was an idea rather than a patch, it didn't take arch uapi headers
into account. OK, I'll try to come up with a proper fix then.
--
ldv
^ permalink raw reply
* Re: [PATCH bpf] libbpf: fix version identification on busybox
From: Ivan Khoronzhuk @ 2019-09-19 18:45 UTC (permalink / raw)
To: ast, daniel, kafai, andriin; +Cc: yhs, netdev, bpf, linux-kernel
In-Reply-To: <20190919160518.25901-1-ivan.khoronzhuk@linaro.org>
On Thu, Sep 19, 2019 at 07:05:18PM +0300, Ivan Khoronzhuk wrote:
>It's very often for embedded to have stripped version of sort in
>busybox, when no -V option present. It breaks build natively on target
>board causing recursive loop.
>
>BusyBox v1.24.1 (2019-04-06 04:09:16 UTC) multi-call binary. \
>Usage: sort [-nrugMcszbdfimSTokt] [-o FILE] [-k \
>start[.offset][opts][,end[.offset][opts]] [-t CHAR] [FILE]...
>
>Lets modify command a little to avoid -V option.
>
>Fixes: dadb81d0afe732 ("libbpf: make libbpf.map source of truth for libbpf version")
>
>Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>---
>
>Based on bpf/master
>
> tools/lib/bpf/Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
>index c6f94cffe06e..a12490ad6215 100644
>--- a/tools/lib/bpf/Makefile
>+++ b/tools/lib/bpf/Makefile
>@@ -3,7 +3,7 @@
>
> LIBBPF_VERSION := $(shell \
> grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \
>- sort -rV | head -n1 | cut -d'_' -f2)
>+ cut -d'_' -f2 | sort -r | head -n1)
> LIBBPF_MAJOR_VERSION := $(firstword $(subst ., ,$(LIBBPF_VERSION)))
Also can be replaced a lidder harder, with:
LIBBPFMAP := $(shell cat libbpf.map)
LIBBPF_VERSIONS := $(sort $(patsubst %;,%,$(patsubst LIBBPF_%,%,$(filter LIBBPF_%, $(LIBBPFMAP)))))
LIBBPF_VERSION := $(word $(words $(LIBBPF_VERSIONS)), $(LIBBPF_VERSIONS))
You choose, I'm not sure in "sort" of make the same on all systems.
>
> MAKEFLAGS += --no-print-directory
>--
>2.17.1
>
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* Re: [PATCH net 0/3] Fix Qdisc destroy issues caused by adding fine-grained locking to filter API
From: Cong Wang @ 2019-09-19 19:13 UTC (permalink / raw)
To: Vlad Buslov
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
In-Reply-To: <vbfy2ykk6ps.fsf@mellanox.com>
On Thu, Sep 19, 2019 at 1:53 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
>
> On Thu 19 Sep 2019 at 01:50, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Wed, Sep 18, 2019 at 12:32 AM Vlad Buslov <vladbu@mellanox.com> wrote:
> >>
> >> TC filter API unlocking introduced several new fine-grained locks. The
> >> change caused sleeping-while-atomic BUGs in several Qdiscs that call cls
> >> APIs which need to obtain new mutex while holding sch tree spinlock. This
> >> series fixes affected Qdiscs by ensuring that cls API that became sleeping
> >> is only called outside of sch tree lock critical section.
> >
> > Sorry I just took a deeper look. It seems harder than just moving it
> > out of the critical section.
> >
> > qdisc_destroy() calls ops->reset() which usually purges queues,
> > I don't see how it is safe to move it out of tree spinlock without
> > respecting fast path.
> >
> > What do you think?
>
> Hmm, maybe we can split qdisc destruction in two stage process for
> affected qdiscs? Rough sketch:
>
> 1. Call qdisc_reset() (or qdisc_purge_queue()) on qdisc that are being
> deleted under sch tree lock protection.
>
> 2. Call new qdisc_put_empty() function after releasing the lock. This
> function would implement same functionality as a regular qdisc_put()
> besides resetting the Qdisc and freeing skb in its queues (already
> done by qdisc_reset())
>
> In fact, affected queues already do the same or something similar:
>
> - htb_change_class() calls qdisc_purge_queue() that calls qdisc_reset(),
> which makes reset inside qdisc_destroy() redundant.
>
> - multiq_tune() calls qdisc_tree_flush_backlog() that has the same
> implementation as qdisc_purge_queue() minus actually resetting the
> Qdisc. Can we substitute first function with the second one here?
>
> - sfb_change() - same as multiq_tune().
>
> Do you think that would work?
I think they have to call qdisc_purge_queue() or whatever that calls
qdisc_reset() to reset all queues including qdisc->gso_skb and
qdisc->skb_bad_txq before releasing sch tree lock.
qdisc_tree_flush_backlog() is not sufficient.
^ permalink raw reply
* Re: [PATCH v1 1/3] seccomp: add SECCOMP_USER_NOTIF_FLAG_CONTINUE
From: Jann Horn @ 2019-09-19 19:37 UTC (permalink / raw)
To: Christian Brauner
Cc: Kees Cook, Andy Lutomirski, Will Drewry, Shuah Khan,
Alexei Starovoitov, Daniel Borkmann, kafai, Song Liu, yhs,
kernel list, open list:KERNEL SELFTEST FRAMEWORK,
Network Development, bpf, Tycho Andersen, Tyler Hicks
In-Reply-To: <20190919095903.19370-2-christian.brauner@ubuntu.com>
On Thu, Sep 19, 2019 at 11:59 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
> This allows the seccomp notifier to continue a syscall.
[...]
> Recently we landed seccomp support for SECCOMP_RET_USER_NOTIF (cf. [4])
> which enables a process (watchee) to retrieve an fd for its seccomp
> filter. This fd can then be handed to another (usually more privileged)
> process (watcher). The watcher will then be able to receive seccomp
> messages about the syscalls having been performed by the watchee.
[...]
> This can be solved by
> telling seccomp to resume the syscall.
[...]
> @@ -780,8 +783,14 @@ static void seccomp_do_user_notification(int this_syscall,
> list_del(&n.list);
> out:
> mutex_unlock(&match->notify_lock);
> +
> + /* Userspace requests to continue the syscall. */
> + if (flags & SECCOMP_USER_NOTIF_FLAG_CONTINUE)
> + return 0;
> +
> syscall_set_return_value(current, task_pt_regs(current),
> err, ret);
> + return -1;
> }
Seccomp currently expects the various seccomp return values to be
fully ordered based on how much action the kernel should take against
the requested syscall. Currently, the range of return values is
basically divided into three regions: "block syscall in some way"
(from SECCOMP_RET_KILL_PROCESS to SECCOMP_RET_USER_NOTIF), "let ptrace
decide" (SECCOMP_RET_TRACE) and "allow" (SECCOMP_RET_LOG and
SECCOMP_RET_ALLOW). If SECCOMP_RET_USER_NOTIF becomes able to allow
syscalls, it will be able to override a negative decision from
SECCOMP_RET_TRACE.
In practice, that's probably not a big deal, since I'm not aware of
anyone actually using SECCOMP_RET_TRACE for security purposes, and on
top of that, you'd have to allow ioctl(..., SECCOMP_IOCTL_NOTIF_SEND,
...) and seccomp() with SECCOMP_FILTER_FLAG_NEW_LISTENER in your
seccomp policy for this to work.
More interestingly, what about the case where two
SECCOMP_RET_USER_NOTIF filters are installed? The most recently
installed filter takes precedence if the return values's action parts
are the same (and this is also documented in the manpage); so if a
container engine installs a filter that always intercepts sys_foobar()
(and never uses SECCOMP_USER_NOTIF_FLAG_CONTINUE), and then something
inside the container also installs a filter that always intercepts
sys_foobar() (and always uses SECCOMP_USER_NOTIF_FLAG_CONTINUE), the
container engine's filter will become ineffective.
With my tendency to overcomplicate things, I'm thinking that maybe it
might be a good idea to:
- collect a list of all filters that returned SECCOMP_RET_USER_NOTIF,
as well as the highest-precedence return value that was less strict
than SECCOMP_RET_USER_NOTIF
- sequentially send notifications to all of the
SECCOMP_RET_USER_NOTIF filters until one doesn't return
SECCOMP_USER_NOTIF_FLAG_CONTINUE
- if all returned SECCOMP_USER_NOTIF_FLAG_CONTINUE, go with the
highest-precedence return value that was less strict than
SECCOMP_RET_USER_NOTIF, or allow if no such return value was
encountered
But perhaps, for now, it would also be enough to just expand the big
fat warning note and tell people that if they allow the use of
SECCOMP_IOCTL_NOTIF_SEND and SECCOMP_FILTER_FLAG_NEW_LISTENER in their
filter, SECCOMP_RET_USER_NOTIF is bypassable. And if someone actually
has a usecase where SECCOMP_RET_USER_NOTIF should be secure and nested
SECCOMP_RET_USER_NOTIF support is needed, that more complicated logic
could be added later?
^ permalink raw reply
* Re: [PATCH bpf] libbpf: fix version identification on busybox
From: Andrii Nakryiko @ 2019-09-19 20:02 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: Alexei Starovoitov, Daniel Borkmann, Martin Lau, Andrii Nakryiko,
Yonghong Song, Networking, bpf, open list
In-Reply-To: <20190919160518.25901-1-ivan.khoronzhuk@linaro.org>
On Thu, Sep 19, 2019 at 11:22 AM Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
> It's very often for embedded to have stripped version of sort in
> busybox, when no -V option present. It breaks build natively on target
> board causing recursive loop.
>
> BusyBox v1.24.1 (2019-04-06 04:09:16 UTC) multi-call binary. \
> Usage: sort [-nrugMcszbdfimSTokt] [-o FILE] [-k \
> start[.offset][opts][,end[.offset][opts]] [-t CHAR] [FILE]...
>
> Lets modify command a little to avoid -V option.
>
> Fixes: dadb81d0afe732 ("libbpf: make libbpf.map source of truth for libbpf version")
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>
> Based on bpf/master
>
> tools/lib/bpf/Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> index c6f94cffe06e..a12490ad6215 100644
> --- a/tools/lib/bpf/Makefile
> +++ b/tools/lib/bpf/Makefile
> @@ -3,7 +3,7 @@
>
> LIBBPF_VERSION := $(shell \
> grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \
> - sort -rV | head -n1 | cut -d'_' -f2)
> + cut -d'_' -f2 | sort -r | head -n1)
You can't just sort alphabetically, because:
1.2
1.11
should be in that order. See discussion on mailing thread for original commit.
> LIBBPF_MAJOR_VERSION := $(firstword $(subst ., ,$(LIBBPF_VERSION)))
>
> MAKEFLAGS += --no-print-directory
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v4.14-stable 0/2] Fixes to commit fdfc5c8594c2 (tcp: remove empty skb from write queue in error cases)
From: Greg Kroah-Hartman @ 2019-09-19 20:07 UTC (permalink / raw)
To: Christoph Paasch; +Cc: Sasha Levin, stable, netdev, David Miller, Eric Dumazet
In-Reply-To: <CALMXkpbL+P8ZM+Z8NHg644X7++opx2He5256D7ZLncntQp+8vw@mail.gmail.com>
On Thu, Sep 19, 2019 at 08:21:43AM -0700, Christoph Paasch wrote:
> Hello Greg & Sasha,
>
> On Sat, Sep 14, 2019 at 12:20 AM Christoph Paasch <cpaasch@apple.com> wrote:
> >
> >
> > The above referenced commit has problems on older non-rbTree kernels.
> >
> > AFAICS, the commit has only been backported to 4.14 up to now, but the
> > commit that fdfc5c8594c2 is fixing (namely ce5ec440994b ("tcp: ensure epoll
> > edge trigger wakeup when write queue is empty"), is in v4.2.
> >
> > Christoph Paasch (2):
> > tcp: Reset send_head when removing skb from write-queue
> > tcp: Don't dequeue SYN/FIN-segments from write-queue
>
> I'm checking in on these two patches for the 4.14 stable-queue.
> Especially the panic fixed by patch 2 is pretty easy to trigger :-/
Dude, it's been less than a week. And it's the middle of the merge
window. And it's the week after Plumbers and Maintainer's summit.
Relax...
I'll go queue these up now, but I am worried about them, given this
total mess the backports seem to have caused.
Why isn't this needed in 4.9.y and 4.4.y also?
thanks,
greg k-h
^ permalink raw reply
* [PATCH net v2 0/3] Fix Qdisc destroy issues caused by adding fine-grained locking to filter API
From: Vlad Buslov @ 2019-09-19 20:14 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, Vlad Buslov
TC filter API unlocking introduced several new fine-grained locks. The
change caused sleeping-while-atomic BUGs in several Qdiscs that call cls
APIs which need to obtain new mutex while holding sch tree spinlock. This
series fixes affected Qdiscs by ensuring that cls API that became sleeping
is only called outside of sch tree lock critical section.
Vlad Buslov (3):
net: sched: sch_htb: don't call qdisc_put() while holding tree lock
net: sched: multiq: don't call qdisc_put() while holding tree lock
net: sched: sch_sfb: don't call qdisc_put() while holding tree lock
include/net/sch_generic.h | 1 +
net/sched/sch_generic.c | 37 +++++++++++++++++++++++++------------
net/sched/sch_htb.c | 4 +++-
net/sched/sch_multiq.c | 23 ++++++++++++++++-------
net/sched/sch_sfb.c | 7 ++++---
5 files changed, 49 insertions(+), 23 deletions(-)
--
2.21.0
^ permalink raw reply
* [PATCH net v2 1/3] net: sched: sch_htb: don't call qdisc_put() while holding tree lock
From: Vlad Buslov @ 2019-09-19 20:14 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, Vlad Buslov
In-Reply-To: <20190919201438.2383-1-vladbu@mellanox.com>
Recent changes that removed rtnl dependency from rules update path of tc
also made tcf_block_put() function sleeping. This function is called from
ops->destroy() of several Qdisc implementations, which in turn is called by
qdisc_put(). Some Qdiscs call qdisc_put() while holding sch tree spinlock,
which results sleeping-while-atomic BUG.
Steps to reproduce for htb:
tc qdisc add dev ens1f0 root handle 1: htb default 12
tc class add dev ens1f0 parent 1: classid 1:1 htb rate 100kbps ceil 100kbps
tc qdisc add dev ens1f0 parent 1:1 handle 40: sfq perturb 10
tc class add dev ens1f0 parent 1:1 classid 1:2 htb rate 100kbps ceil 100kbps
Resulting dmesg:
[ 4791.148551] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:909
[ 4791.151354] in_atomic(): 1, irqs_disabled(): 0, pid: 27273, name: tc
[ 4791.152805] INFO: lockdep is turned off.
[ 4791.153605] CPU: 19 PID: 27273 Comm: tc Tainted: G W 5.3.0-rc8+ #721
[ 4791.154336] Hardware name: Supermicro SYS-2028TP-DECR/X10DRT-P, BIOS 2.0b 03/30/2017
[ 4791.155075] Call Trace:
[ 4791.155803] dump_stack+0x85/0xc0
[ 4791.156529] ___might_sleep.cold+0xac/0xbc
[ 4791.157251] __mutex_lock+0x5b/0x960
[ 4791.157966] ? console_unlock+0x363/0x5d0
[ 4791.158676] ? tcf_chain0_head_change_cb_del.isra.0+0x1b/0xf0
[ 4791.159395] ? tcf_chain0_head_change_cb_del.isra.0+0x1b/0xf0
[ 4791.160103] tcf_chain0_head_change_cb_del.isra.0+0x1b/0xf0
[ 4791.160815] tcf_block_put_ext.part.0+0x21/0x50
[ 4791.161530] tcf_block_put+0x50/0x70
[ 4791.162233] sfq_destroy+0x15/0x50 [sch_sfq]
[ 4791.162936] qdisc_destroy+0x5f/0x160
[ 4791.163642] htb_change_class.cold+0x5df/0x69d [sch_htb]
[ 4791.164505] tc_ctl_tclass+0x19d/0x480
[ 4791.165360] rtnetlink_rcv_msg+0x170/0x4b0
[ 4791.166191] ? netlink_deliver_tap+0x95/0x400
[ 4791.166907] ? rtnl_dellink+0x2d0/0x2d0
[ 4791.167625] netlink_rcv_skb+0x49/0x110
[ 4791.168345] netlink_unicast+0x171/0x200
[ 4791.169058] netlink_sendmsg+0x224/0x3f0
[ 4791.169771] sock_sendmsg+0x5e/0x60
[ 4791.170475] ___sys_sendmsg+0x2ae/0x330
[ 4791.171183] ? ___sys_recvmsg+0x159/0x1f0
[ 4791.171894] ? do_wp_page+0x9c/0x790
[ 4791.172595] ? __handle_mm_fault+0xcd3/0x19e0
[ 4791.173309] __sys_sendmsg+0x59/0xa0
[ 4791.174024] do_syscall_64+0x5c/0xb0
[ 4791.174725] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 4791.175435] RIP: 0033:0x7f0aa41497b8
[ 4791.176129] Code: 89 02 48 c7 c0 ff ff ff ff eb bb 0f 1f 80 00 00 00 00 f3 0f 1e fa 48 8d 05 65 8f 0c 00 8b 00 85 c0 75 17 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 48 83 ec 28 89 5
4
[ 4791.177532] RSP: 002b:00007fff4e37d588 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[ 4791.178243] RAX: ffffffffffffffda RBX: 000000005d8132f7 RCX: 00007f0aa41497b8
[ 4791.178947] RDX: 0000000000000000 RSI: 00007fff4e37d5f0 RDI: 0000000000000003
[ 4791.179662] RBP: 0000000000000000 R08: 0000000000000001 R09: 00000000020149a0
[ 4791.180382] R10: 0000000000404eda R11: 0000000000000246 R12: 0000000000000001
[ 4791.181100] R13: 000000000047f640 R14: 0000000000000000 R15: 0000000000000000
Extend sch API with new qdisc_put_empty() function that has same
implementation as regular qdisc_put() but skips parts that reset qdisc and
free all packet buffers from gso_skb and skb_bad_txq queues. In
htb_change_class() function save parent->leaf.q to local temporary variable
and put reference to it after sch tree lock is released in order not to
call potentially sleeping cls API in atomic section. This is safe to do
because Qdisc has already been reset by qdisc_purge_queue() inside sch tree
lock critical section.
Fixes: c266f64dbfa2 ("net: sched: protect block state with mutex")
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
Notes:
Changes V1 -> V2:
- Extend sch API with new qdisc_put_empty() function that has same
implementation as regular qdisc_put() but skips parts that reset qdisc
and free all packet buffers from gso_skb and skb_bad_txq queues.
- Use new qdisc_put_empty() instead of regular qdisc_put() in
htb_change_class().
include/net/sch_generic.h | 1 +
net/sched/sch_generic.c | 37 +++++++++++++++++++++++++------------
net/sched/sch_htb.c | 4 +++-
3 files changed, 29 insertions(+), 13 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 43f5b7ed02bd..5687c342989c 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -623,6 +623,7 @@ struct Qdisc *dev_graft_qdisc(struct netdev_queue *dev_queue,
struct Qdisc *qdisc);
void qdisc_reset(struct Qdisc *qdisc);
void qdisc_put(struct Qdisc *qdisc);
+void qdisc_put_empty(struct Qdisc *qdisc);
void qdisc_put_unlocked(struct Qdisc *qdisc);
void qdisc_tree_reduce_backlog(struct Qdisc *qdisc, int n, int len);
#ifdef CONFIG_NET_SCHED
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 17bd8f539bc7..3e2cc8d97efe 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -951,7 +951,7 @@ static void qdisc_free_cb(struct rcu_head *head)
qdisc_free(q);
}
-static void qdisc_destroy(struct Qdisc *qdisc)
+static void qdisc_destroy(struct Qdisc *qdisc, bool reset)
{
const struct Qdisc_ops *ops = qdisc->ops;
struct sk_buff *skb, *tmp;
@@ -962,7 +962,7 @@ static void qdisc_destroy(struct Qdisc *qdisc)
qdisc_put_stab(rtnl_dereference(qdisc->stab));
#endif
gen_kill_estimator(&qdisc->rate_est);
- if (ops->reset)
+ if (reset && ops->reset)
ops->reset(qdisc);
if (ops->destroy)
ops->destroy(qdisc);
@@ -970,20 +970,22 @@ static void qdisc_destroy(struct Qdisc *qdisc)
module_put(ops->owner);
dev_put(qdisc_dev(qdisc));
- skb_queue_walk_safe(&qdisc->gso_skb, skb, tmp) {
- __skb_unlink(skb, &qdisc->gso_skb);
- kfree_skb_list(skb);
- }
+ if (reset) {
+ skb_queue_walk_safe(&qdisc->gso_skb, skb, tmp) {
+ __skb_unlink(skb, &qdisc->gso_skb);
+ kfree_skb_list(skb);
+ }
- skb_queue_walk_safe(&qdisc->skb_bad_txq, skb, tmp) {
- __skb_unlink(skb, &qdisc->skb_bad_txq);
- kfree_skb_list(skb);
+ skb_queue_walk_safe(&qdisc->skb_bad_txq, skb, tmp) {
+ __skb_unlink(skb, &qdisc->skb_bad_txq);
+ kfree_skb_list(skb);
+ }
}
call_rcu(&qdisc->rcu, qdisc_free_cb);
}
-void qdisc_put(struct Qdisc *qdisc)
+static void __qdisc_put(struct Qdisc *qdisc, bool reset)
{
if (!qdisc)
return;
@@ -992,10 +994,21 @@ void qdisc_put(struct Qdisc *qdisc)
!refcount_dec_and_test(&qdisc->refcnt))
return;
- qdisc_destroy(qdisc);
+ qdisc_destroy(qdisc, reset);
+}
+
+void qdisc_put(struct Qdisc *qdisc)
+{
+ __qdisc_put(qdisc, true);
}
EXPORT_SYMBOL(qdisc_put);
+void qdisc_put_empty(struct Qdisc *qdisc)
+{
+ __qdisc_put(qdisc, false);
+}
+EXPORT_SYMBOL(qdisc_put_empty);
+
/* Version of qdisc_put() that is called with rtnl mutex unlocked.
* Intended to be used as optimization, this function only takes rtnl lock if
* qdisc reference counter reached zero.
@@ -1007,7 +1020,7 @@ void qdisc_put_unlocked(struct Qdisc *qdisc)
!refcount_dec_and_rtnl_lock(&qdisc->refcnt))
return;
- qdisc_destroy(qdisc);
+ qdisc_destroy(qdisc, true);
rtnl_unlock();
}
EXPORT_SYMBOL(qdisc_put_unlocked);
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 7bcf20ef9145..44551f485ac8 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -1302,6 +1302,7 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
struct htb_class *cl = (struct htb_class *)*arg, *parent;
struct nlattr *opt = tca[TCA_OPTIONS];
struct nlattr *tb[TCA_HTB_MAX + 1];
+ struct Qdisc *parent_qdisc = NULL;
struct tc_htb_opt *hopt;
u64 rate64, ceil64;
int warn = 0;
@@ -1401,7 +1402,7 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
if (parent && !parent->level) {
/* turn parent into inner node */
qdisc_purge_queue(parent->leaf.q);
- qdisc_put(parent->leaf.q);
+ parent_qdisc = parent->leaf.q;
if (parent->prio_activity)
htb_deactivate(q, parent);
@@ -1480,6 +1481,7 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
cl->cbuffer = PSCHED_TICKS2NS(hopt->cbuffer);
sch_tree_unlock(sch);
+ qdisc_put_empty(parent_qdisc);
if (warn)
pr_warn("HTB: quantum of class %X is %s. Consider r2q change.\n",
--
2.21.0
^ permalink raw reply related
* [PATCH net v2 2/3] net: sched: multiq: don't call qdisc_put() while holding tree lock
From: Vlad Buslov @ 2019-09-19 20:14 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, Vlad Buslov
In-Reply-To: <20190919201438.2383-1-vladbu@mellanox.com>
Recent changes that removed rtnl dependency from rules update path of tc
also made tcf_block_put() function sleeping. This function is called from
ops->destroy() of several Qdisc implementations, which in turn is called by
qdisc_put(). Some Qdiscs call qdisc_put() while holding sch tree spinlock,
which results sleeping-while-atomic BUG.
Steps to reproduce for multiq:
tc qdisc add dev ens1f0 root handle 1: multiq
tc qdisc add dev ens1f0 parent 1:10 handle 50: sfq perturb 10
ethtool -L ens1f0 combined 2
tc qdisc change dev ens1f0 root handle 1: multiq
Resulting dmesg:
[ 5539.419344] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:909
[ 5539.420945] in_atomic(): 1, irqs_disabled(): 0, pid: 27658, name: tc
[ 5539.422435] INFO: lockdep is turned off.
[ 5539.423904] CPU: 21 PID: 27658 Comm: tc Tainted: G W 5.3.0-rc8+ #721
[ 5539.425400] Hardware name: Supermicro SYS-2028TP-DECR/X10DRT-P, BIOS 2.0b 03/30/2017
[ 5539.426911] Call Trace:
[ 5539.428380] dump_stack+0x85/0xc0
[ 5539.429823] ___might_sleep.cold+0xac/0xbc
[ 5539.431262] __mutex_lock+0x5b/0x960
[ 5539.432682] ? tcf_chain0_head_change_cb_del.isra.0+0x1b/0xf0
[ 5539.434103] ? __nla_validate_parse+0x51/0x840
[ 5539.435493] ? tcf_chain0_head_change_cb_del.isra.0+0x1b/0xf0
[ 5539.436903] tcf_chain0_head_change_cb_del.isra.0+0x1b/0xf0
[ 5539.438327] tcf_block_put_ext.part.0+0x21/0x50
[ 5539.439752] tcf_block_put+0x50/0x70
[ 5539.441165] sfq_destroy+0x15/0x50 [sch_sfq]
[ 5539.442570] qdisc_destroy+0x5f/0x160
[ 5539.444000] multiq_tune+0x14a/0x420 [sch_multiq]
[ 5539.445421] tc_modify_qdisc+0x324/0x840
[ 5539.446841] rtnetlink_rcv_msg+0x170/0x4b0
[ 5539.448269] ? netlink_deliver_tap+0x95/0x400
[ 5539.449691] ? rtnl_dellink+0x2d0/0x2d0
[ 5539.451116] netlink_rcv_skb+0x49/0x110
[ 5539.452522] netlink_unicast+0x171/0x200
[ 5539.453914] netlink_sendmsg+0x224/0x3f0
[ 5539.455304] sock_sendmsg+0x5e/0x60
[ 5539.456686] ___sys_sendmsg+0x2ae/0x330
[ 5539.458071] ? ___sys_recvmsg+0x159/0x1f0
[ 5539.459461] ? do_wp_page+0x9c/0x790
[ 5539.460846] ? __handle_mm_fault+0xcd3/0x19e0
[ 5539.462263] __sys_sendmsg+0x59/0xa0
[ 5539.463661] do_syscall_64+0x5c/0xb0
[ 5539.465044] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 5539.466454] RIP: 0033:0x7f1fe08177b8
[ 5539.467863] Code: 89 02 48 c7 c0 ff ff ff ff eb bb 0f 1f 80 00 00 00 00 f3 0f 1e fa 48 8d 05 65 8f 0c 00 8b 00 85 c0 75 17 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 48 83 ec 28 89 5
4
[ 5539.470906] RSP: 002b:00007ffe812de5d8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[ 5539.472483] RAX: ffffffffffffffda RBX: 000000005d8135e3 RCX: 00007f1fe08177b8
[ 5539.474069] RDX: 0000000000000000 RSI: 00007ffe812de640 RDI: 0000000000000003
[ 5539.475655] RBP: 0000000000000000 R08: 0000000000000001 R09: 000000000182e9b0
[ 5539.477203] R10: 0000000000404eda R11: 0000000000000246 R12: 0000000000000001
[ 5539.478699] R13: 000000000047f640 R14: 0000000000000000 R15: 0000000000000000
Rearrange locking in multiq_tune() in following ways:
- In loop that removes Qdiscs from disabled queues, call
qdisc_purge_queue() instead of qdisc_tree_flush_backlog() on Qdisc that
is being destroyed. Save the Qdisc in temporary allocated array and call
qdisc_put_empty() on each element of the array after sch tree lock is
released. This is safe to do because Qdiscs have already been reset by
qdisc_purge_queue() inside sch tree lock critical section.
- Do the same change for second loop that initializes Qdiscs for newly
enabled queues in multiq_tune() function. Since sch tree lock is obtained
and released on each iteration of this loop, just call qdisc_put_empty()
directly outside of critical section. Don't verify that old Qdisc is not
noop_qdisc before releasing reference to it because such check is already
performed by qdisc_put*() functions.
Fixes: c266f64dbfa2 ("net: sched: protect block state with mutex")
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
Notes:
Changes V1 -> V2:
- Refactor first loop in multiq_tune() to save child Qdiscs that are being
removed into an array and call qdisc_put_empty() on all of its elements
after sch tree lock critical section. Revert the change in V1 that
obtained and released sch tree lock on every loop iteration.
- Use qdisc_purge_queue() instead of qdisc_tree_flush_backlog() to properly
reset Qdiscs inside sch tree lock critical section.
- Use qdisc_put_empty() in both loops of multiq_tune() instead of regular
qdisc_put().
net/sched/sch_multiq.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/net/sched/sch_multiq.c b/net/sched/sch_multiq.c
index e1087746f6a2..52595b40efbd 100644
--- a/net/sched/sch_multiq.c
+++ b/net/sched/sch_multiq.c
@@ -174,7 +174,8 @@ static int multiq_tune(struct Qdisc *sch, struct nlattr *opt,
{
struct multiq_sched_data *q = qdisc_priv(sch);
struct tc_multiq_qopt *qopt;
- int i;
+ struct Qdisc **removed;
+ int i, n_removed = 0;
if (!netif_is_multiqueue(qdisc_dev(sch)))
return -EOPNOTSUPP;
@@ -185,6 +186,11 @@ static int multiq_tune(struct Qdisc *sch, struct nlattr *opt,
qopt->bands = qdisc_dev(sch)->real_num_tx_queues;
+ removed = kmalloc(sizeof(*removed) * (q->max_bands - q->bands),
+ GFP_KERNEL);
+ if (!removed)
+ return -ENOMEM;
+
sch_tree_lock(sch);
q->bands = qopt->bands;
for (i = q->bands; i < q->max_bands; i++) {
@@ -192,13 +198,17 @@ static int multiq_tune(struct Qdisc *sch, struct nlattr *opt,
struct Qdisc *child = q->queues[i];
q->queues[i] = &noop_qdisc;
- qdisc_tree_flush_backlog(child);
- qdisc_put(child);
+ qdisc_purge_queue(child);
+ removed[n_removed++] = child;
}
}
sch_tree_unlock(sch);
+ for (i = 0; i < n_removed; i++)
+ qdisc_put_empty(removed[i]);
+ kfree(removed);
+
for (i = 0; i < q->bands; i++) {
if (q->queues[i] == &noop_qdisc) {
struct Qdisc *child, *old;
@@ -213,11 +223,10 @@ static int multiq_tune(struct Qdisc *sch, struct nlattr *opt,
if (child != &noop_qdisc)
qdisc_hash_add(child, true);
- if (old != &noop_qdisc) {
- qdisc_tree_flush_backlog(old);
- qdisc_put(old);
- }
+ if (old != &noop_qdisc)
+ qdisc_purge_queue(old);
sch_tree_unlock(sch);
+ qdisc_put_empty(old);
}
}
}
--
2.21.0
^ permalink raw reply related
* [PATCH net v2 3/3] net: sched: sch_sfb: don't call qdisc_put() while holding tree lock
From: Vlad Buslov @ 2019-09-19 20:14 UTC (permalink / raw)
To: netdev
Cc: jhs, xiyou.wangcong, jiri, davem, Vlad Buslov,
syzbot+ac54455281db908c581e
In-Reply-To: <20190919201438.2383-1-vladbu@mellanox.com>
Recent changes that removed rtnl dependency from rules update path of tc
also made tcf_block_put() function sleeping. This function is called from
ops->destroy() of several Qdisc implementations, which in turn is called by
qdisc_put(). Some Qdiscs call qdisc_put() while holding sch tree spinlock,
which results sleeping-while-atomic BUG.
Steps to reproduce for sfb:
tc qdisc add dev ens1f0 handle 1: root sfb
tc qdisc add dev ens1f0 parent 1:10 handle 50: sfq perturb 10
tc qdisc change dev ens1f0 root handle 1: sfb
Resulting dmesg:
[ 7265.938717] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:909
[ 7265.940152] in_atomic(): 1, irqs_disabled(): 0, pid: 28579, name: tc
[ 7265.941455] INFO: lockdep is turned off.
[ 7265.942744] CPU: 11 PID: 28579 Comm: tc Tainted: G W 5.3.0-rc8+ #721
[ 7265.944065] Hardware name: Supermicro SYS-2028TP-DECR/X10DRT-P, BIOS 2.0b 03/30/2017
[ 7265.945396] Call Trace:
[ 7265.946709] dump_stack+0x85/0xc0
[ 7265.947994] ___might_sleep.cold+0xac/0xbc
[ 7265.949282] __mutex_lock+0x5b/0x960
[ 7265.950543] ? tcf_chain0_head_change_cb_del.isra.0+0x1b/0xf0
[ 7265.951803] ? tcf_chain0_head_change_cb_del.isra.0+0x1b/0xf0
[ 7265.953022] tcf_chain0_head_change_cb_del.isra.0+0x1b/0xf0
[ 7265.954248] tcf_block_put_ext.part.0+0x21/0x50
[ 7265.955478] tcf_block_put+0x50/0x70
[ 7265.956694] sfq_destroy+0x15/0x50 [sch_sfq]
[ 7265.957898] qdisc_destroy+0x5f/0x160
[ 7265.959099] sfb_change+0x175/0x330 [sch_sfb]
[ 7265.960304] tc_modify_qdisc+0x324/0x840
[ 7265.961503] rtnetlink_rcv_msg+0x170/0x4b0
[ 7265.962692] ? netlink_deliver_tap+0x95/0x400
[ 7265.963876] ? rtnl_dellink+0x2d0/0x2d0
[ 7265.965064] netlink_rcv_skb+0x49/0x110
[ 7265.966251] netlink_unicast+0x171/0x200
[ 7265.967427] netlink_sendmsg+0x224/0x3f0
[ 7265.968595] sock_sendmsg+0x5e/0x60
[ 7265.969753] ___sys_sendmsg+0x2ae/0x330
[ 7265.970916] ? ___sys_recvmsg+0x159/0x1f0
[ 7265.972074] ? do_wp_page+0x9c/0x790
[ 7265.973233] ? __handle_mm_fault+0xcd3/0x19e0
[ 7265.974407] __sys_sendmsg+0x59/0xa0
[ 7265.975591] do_syscall_64+0x5c/0xb0
[ 7265.976753] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 7265.977938] RIP: 0033:0x7f229069f7b8
[ 7265.979117] Code: 89 02 48 c7 c0 ff ff ff ff eb bb 0f 1f 80 00 00 00 00 f3 0f 1e fa 48 8d 05 65 8f 0c 00 8b 00 85 c0 75 17 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 48 83 ec 28 89 5
4
[ 7265.981681] RSP: 002b:00007ffd7ed2d158 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[ 7265.983001] RAX: ffffffffffffffda RBX: 000000005d813ca1 RCX: 00007f229069f7b8
[ 7265.984336] RDX: 0000000000000000 RSI: 00007ffd7ed2d1c0 RDI: 0000000000000003
[ 7265.985682] RBP: 0000000000000000 R08: 0000000000000001 R09: 000000000165c9a0
[ 7265.987021] R10: 0000000000404eda R11: 0000000000000246 R12: 0000000000000001
[ 7265.988309] R13: 000000000047f640 R14: 0000000000000000 R15: 0000000000000000
In sfb_change() function use qdisc_purge_queue() instead of
qdisc_tree_flush_backlog() to properly reset old child Qdisc and save
pointer to it into local temporary variable. Put reference to Qdisc after
sch tree lock is released in order not to call potentially sleeping cls API
in atomic section. This is safe to do because Qdisc has already been reset
by qdisc_purge_queue() inside sch tree lock critical section.
Reported-by: syzbot+ac54455281db908c581e@syzkaller.appspotmail.com
Fixes: c266f64dbfa2 ("net: sched: protect block state with mutex")
Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
Notes:
Changes V1 -> V2:
- Use qdisc_purge_queue() instead of qdisc_tree_flush_backlog() to properly
reset Qdiscs inside sch tree lock critical section.
- Call qdisc_put_empty() instead of regular qdisc_put() after sch tree lock
is released.
net/sched/sch_sfb.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/sched/sch_sfb.c b/net/sched/sch_sfb.c
index 1dff8506a715..23b2e492c587 100644
--- a/net/sched/sch_sfb.c
+++ b/net/sched/sch_sfb.c
@@ -488,7 +488,7 @@ static int sfb_change(struct Qdisc *sch, struct nlattr *opt,
struct netlink_ext_ack *extack)
{
struct sfb_sched_data *q = qdisc_priv(sch);
- struct Qdisc *child;
+ struct Qdisc *child, *old;
struct nlattr *tb[TCA_SFB_MAX + 1];
const struct tc_sfb_qopt *ctl = &sfb_default_ops;
u32 limit;
@@ -518,8 +518,8 @@ static int sfb_change(struct Qdisc *sch, struct nlattr *opt,
qdisc_hash_add(child, true);
sch_tree_lock(sch);
- qdisc_tree_flush_backlog(q->qdisc);
- qdisc_put(q->qdisc);
+ qdisc_purge_queue(q->qdisc);
+ old = q->qdisc;
q->qdisc = child;
q->rehash_interval = msecs_to_jiffies(ctl->rehash_interval);
@@ -542,6 +542,7 @@ static int sfb_change(struct Qdisc *sch, struct nlattr *opt,
sfb_init_perturbation(1, q);
sch_tree_unlock(sch);
+ qdisc_put_empty(old);
return 0;
}
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v4.14-stable 0/2] Fixes to commit fdfc5c8594c2 (tcp: remove empty skb from write queue in error cases)
From: Christoph Paasch @ 2019-09-19 20:17 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Sasha Levin, stable, netdev, David Miller, Eric Dumazet
In-Reply-To: <20190919200726.GA252076@kroah.com>
On Thu, Sep 19, 2019 at 1:07 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Sep 19, 2019 at 08:21:43AM -0700, Christoph Paasch wrote:
> > Hello Greg & Sasha,
> >
> > On Sat, Sep 14, 2019 at 12:20 AM Christoph Paasch <cpaasch@apple.com> wrote:
> > >
> > >
> > > The above referenced commit has problems on older non-rbTree kernels.
> > >
> > > AFAICS, the commit has only been backported to 4.14 up to now, but the
> > > commit that fdfc5c8594c2 is fixing (namely ce5ec440994b ("tcp: ensure epoll
> > > edge trigger wakeup when write queue is empty"), is in v4.2.
> > >
> > > Christoph Paasch (2):
> > > tcp: Reset send_head when removing skb from write-queue
> > > tcp: Don't dequeue SYN/FIN-segments from write-queue
> >
> > I'm checking in on these two patches for the 4.14 stable-queue.
> > Especially the panic fixed by patch 2 is pretty easy to trigger :-/
>
> Dude, it's been less than a week. And it's the middle of the merge
> window. And it's the week after Plumbers and Maintainer's summit.
>
> Relax...
Sorry!
> I'll go queue these up now, but I am worried about them, given this
> total mess the backports seem to have caused.
>
> Why isn't this needed in 4.9.y and 4.4.y also?
From what I see, commit fdfc5c8594c2 has not been backported to 4.9
and older. But I can imagine that eventually it will have to be
backported (I guess Eric can confirm or deny).
If it gets backported to 4.9 and 4.4, my 2 patches will have to come with them.
Christoph
^ permalink raw reply
* Re: [PATCH 2/2] ptp: Add a ptp clock driver for IDT ClockMatrix.
From: Vincent Cheng @ 2019-09-19 20:24 UTC (permalink / raw)
To: Andrew Lunn
Cc: robh+dt@kernel.org, mark.rutland@arm.com,
richardcochran@gmail.com, devicetree@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20190918211803.GO9591@lunn.ch>
Hi Andrew,
On Wed, Sep 18, 2019 at 05:18:03PM EDT, Andrew Lunn wrote:
>On Wed, Sep 18, 2019 at 04:06:38PM -0400, vincent.cheng.xh@renesas.com wrote:
>
>> +static s32 idtcm_xfer(struct idtcm *idtcm,
>> + u8 regaddr,
>> + u8 *buf,
>> + u16 count,
>> + bool write)
>> +{
>> + struct i2c_client *client = idtcm->client;
>> + struct i2c_msg msg[2];
>> + s32 cnt;
>> +
>> + msg[0].addr = client->addr;
>> + msg[0].flags = 0;
>> + msg[0].len = 1;
>> + msg[0].buf = ®addr;
>> +
>> + msg[1].addr = client->addr;
>> + msg[1].flags = write ? 0 : I2C_M_RD;
>> + msg[1].len = count;
>> + msg[1].buf = buf;
>> +
>> + cnt = i2c_transfer(client->adapter, msg, 2);
>> +
>> + if (cnt < 0) {
>> + pr_err("i2c_transfer returned %d\n", cnt);
>
>dev_err(client->dev, "i2c_transfer returned %d\n", cnt);
>
>We then have an idea which device has a transfer error.
>
>Please try to not use pr_err() when you have some sort of device.
Sure thing, will replace pr_err() with dev_err().
>> +static s32 idtcm_state_machine_reset(struct idtcm *idtcm)
>> +{
>> + s32 err;
>> + u8 byte = SM_RESET_CMD;
>> +
>> + err = idtcm_write(idtcm, RESET_CTRL, SM_RESET, &byte, sizeof(byte));
>> +
>> + if (!err) {
>> + /* delay */
>> + set_current_state(TASK_INTERRUPTIBLE);
>> + schedule_timeout(_msecs_to_jiffies(POST_SM_RESET_DELAY_MS));
>
>Maybe use msleep_interruptable()?
Yes, will try using msleep_interruptable() and will replace if it works.
>> +static s32 idtcm_load_firmware(struct idtcm *idtcm,
>> + struct device *dev)
>> +{
>> + const struct firmware *fw;
>> + struct idtcm_fwrc *rec;
>> + u32 regaddr;
>> + s32 err;
>> + s32 len;
>> + u8 val;
>> + u8 loaddr;
>> +
>> + pr_info("requesting firmware '%s'\n", FW_FILENAME);
>
>dev_debug()
Thanks, will make the change.
>> +
>> + err = request_firmware(&fw, FW_FILENAME, dev);
>> +
>> + if (err)
>> + return err;
>> +
>> + pr_info("firmware size %zu bytes\n", fw->size);
>
>dev_debug()
>
>Maybe look through all your pr_info and downgrade most of them to
>dev_debug()
Yes, will go through and downgrade to dev_debug() accordingly.
Thanks,
Vincent
^ permalink raw reply
* Re: Bug report (with fix) for DEC Tulip driver (de2104x.c)
From: Sven Schnelle @ 2019-09-19 20:31 UTC (permalink / raw)
To: Helge Deller
Cc: John David Anglin, Arlie Davis, Andrew Lunn, netdev, linux-parisc
In-Reply-To: <f71e9773-5cfb-f20b-956f-d98b11a5d4a7@gmx.de>
Hi,
On Wed, Sep 18, 2019 at 07:56:16AM +0200, Helge Deller wrote:
> On 18.09.19 00:51, John David Anglin wrote:
> > On 2019-09-17 5:36 p.m., Arlie Davis wrote:
> >> Likewise, I'm at a loss for testing with real hardware. It's hard to
> >> find such things, now.
> > How does de2104x compare to ds2142/43? I have a c3750 with ds2142/43 tulip. Helge
> > or some others might have a machine with a de2104x.
>
> The machines we could test are
> * a C240 with a DS21140 tulip chip (Sven has one),
My C240 identifies as C240+:
[ 0.000000] model 9000/782/C240+
which has a 21143 (verified by looking at the board):
root@c240:/# lspci -kvvnn -s 00:14.0
00:14.0 Ethernet controller [0200]: Digital Equipment Corporation DECchip 21142/43 [1011:0019] (rev 30)
Subsystem: Hewlett-Packard Company DECchip 21142/43 [103c:104f]
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV+ VGASnoop- ParErr+ Stepping- SERR+ FastB2B- DisINTx-
Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 255 (5000ns min, 10000ns max), Cache Line Size: 32 bytes
Interrupt: pin A routed to IRQ 23
Region 0: I/O ports at 0080 [size=128]
Region 1: Memory at f2802000 (32-bit, non-prefetchable) [size=128]
Expansion ROM at f2f80000 [disabled] [size=256K]
Kernel driver in use: tulip
Regards
Sven
^ permalink raw reply
* Re: mt76x2e hardware restart
From: Oleksandr Natalenko @ 2019-09-19 21:22 UTC (permalink / raw)
To: linux-mediatek
Cc: Felix Fietkau, Lorenzo Bianconi, Lorenzo Bianconi,
Stanislaw Gruszka, Ryder Lee, Roy Luo, Kalle Valo,
David S. Miller, Matthias Brugger, linux-wireless, netdev,
linux-arm-kernel, linux-kernel
In-Reply-To: <deaafa7a3e9ea2111ebb5106430849c6@natalenko.name>
On 19.09.2019 18:24, Oleksandr Natalenko wrote:
> [ +9,979664] mt76x2e 0000:01:00.0: Firmware Version: 0.0.00
> [ +0,000014] mt76x2e 0000:01:00.0: Build: 1
> [ +0,000010] mt76x2e 0000:01:00.0: Build Time: 201507311614____
> [ +0,018017] mt76x2e 0000:01:00.0: Firmware running!
> [ +0,001101] ieee80211 phy4: Hardware restart was requested
IIUC, this happens due to watchdog. I think the following applies.
Watchdog is started here:
=== mt76x02_util.c
130 void mt76x02_init_device(struct mt76x02_dev *dev)
131 {
...
155 INIT_DELAYED_WORK(&dev->wdt_work, mt76x02_wdt_work);
===
It checks for TX hang here:
=== mt76x02_mmio.c
557 void mt76x02_wdt_work(struct work_struct *work)
558 {
...
562 mt76x02_check_tx_hang(dev);
===
Conditions:
=== mt76x02_mmio.c
530 static void mt76x02_check_tx_hang(struct mt76x02_dev *dev)
531 {
532 if (mt76x02_tx_hang(dev)) {
533 if (++dev->tx_hang_check >= MT_TX_HANG_TH)
534 goto restart;
535 } else {
536 dev->tx_hang_check = 0;
537 }
538
539 if (dev->mcu_timeout)
540 goto restart;
541
542 return;
543
544 restart:
545 mt76x02_watchdog_reset(dev);
===
Actual check:
=== mt76x02_mmio.c
367 static bool mt76x02_tx_hang(struct mt76x02_dev *dev)
368 {
369 u32 dma_idx, prev_dma_idx;
370 struct mt76_queue *q;
371 int i;
372
373 for (i = 0; i < 4; i++) {
374 q = dev->mt76.q_tx[i].q;
375
376 if (!q->queued)
377 continue;
378
379 prev_dma_idx = dev->mt76.tx_dma_idx[i];
380 dma_idx = readl(&q->regs->dma_idx);
381 dev->mt76.tx_dma_idx[i] = dma_idx;
382
383 if (prev_dma_idx == dma_idx)
384 break;
385 }
386
387 return i < 4;
388 }
===
(I don't quite understand what it does here; why 4? does each device
have 4 queues? maybe, my does not? I guess this is where watchdog is
triggered, though, because otherwise I'd see mcu_timeout message like
"MCU message %d (seq %d) timed out\n")
Once it detects TX hang, the reset is triggered:
=== mt76x02_mmio.c
446 static void mt76x02_watchdog_reset(struct mt76x02_dev *dev)
447 {
...
485 if (restart)
486 mt76_mcu_restart(dev);
===
mt76_mcu_restart() is just a define for this series here:
=== mt76.h
555 #define mt76_mcu_restart(dev, ...)
(dev)->mt76.mcu_ops->mcu_restart(&((dev)->mt76))
===
Actual OP:
=== mt76x2/pci_mcu.c
188 int mt76x2_mcu_init(struct mt76x02_dev *dev)
189 {
190 static const struct mt76_mcu_ops mt76x2_mcu_ops = {
191 .mcu_restart = mt76pci_mcu_restart,
192 .mcu_send_msg = mt76x02_mcu_msg_send,
193 };
===
This triggers loading the firmware:
=== mt76x2/pci_mcu.c
168 static int
169 mt76pci_mcu_restart(struct mt76_dev *mdev)
170 {
...
179 ret = mt76pci_load_firmware(dev);
===
which does the printout I observe:
=== mt76x2/pci_mcu.c
91 static int
92 mt76pci_load_firmware(struct mt76x02_dev *dev)
93 {
...
156 dev_info(dev->mt76.dev, "Firmware running!\n");
===
Too bad it doesn't show the actual watchdog message, IOW, why the reset
happens. I guess I will have to insert some pr_infos here and there.
Does it make sense? Any ideas why this can happen?
More info on the device during boot:
===
[ +0,333233] mt76x2e 0000:01:00.0: enabling device (0000 -> 0002)
[ +0,000571] mt76x2e 0000:01:00.0: ASIC revision: 76120044
[ +0,017806] mt76x2e 0000:01:00.0: ROM patch build: 20141115060606a
===
--
Oleksandr Natalenko (post-factum)
^ permalink raw reply
* Re: [PATCH net-next] net: dsa: mv88e6xxx: Add support for port mirroring
From: Jason Cobham @ 2019-09-19 21:30 UTC (permalink / raw)
To: Iwan R Timmer
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
netdev
Hi Iwan,
>Hi Andrew,
>
>I only own a simple 5 ports switch (88E6176) which has no problem of mirroring the other ports to a single port. Except for a bandwith shortage ofcourse. While I thought I checked adding and removing ports, I seemed to forgot to check removing ingress traffic as it will now >disable mirroring egress traffic. Searching for how I can distinct ingress from egress mirroring in port_mirror_del, I saw there is a variable in the mirror struct called ingress. Which seems strange, because why is it a seperate argument to the port_mirror_add function?
>
>Origally I planned to be able to set the egress and ingress mirror seperatly. But in my laziness when I saw there already was a function to configure the destination port this functionality was lost.
>
>Because the other drivers which implemented the port_mirror_add (b53 and
>ksz9477) also lacks additional checks to prevent new mirror filters from breaking previous ones I assumed they were not necessary.
>
>At least I will soon sent a new version with at least the issue of removing mirror ingress traffic fixed and the ability to define a seperate ingress and egress port.
>
>Regards,
>Iwan
I have a similar patch set for port mirror from a few years ago. I'd
also like to see this functionality in mainline. One issue I ran into
is when doing port mirror in a cross-chip dsa configuration. If the
ingress and egress ports are on different chips, the ingress chip
needs to set the egress to the cross-chip dsa port and the cross-chip
egress port needs to be set appropriately. I also had the
functionality to mirror egress from a port to a destination port.
Is it appropriate to send my patch to the mailing list for review or
should we work on this off-line?
Thanks,
Jason
^ permalink raw reply
* [PATCH RFC] thermal: Fix broken registration if a sensor OF node is missing
From: Peter Mamonov @ 2019-09-19 21:40 UTC (permalink / raw)
To: rui.zhang, edubezval, daniel.lezcano
Cc: andrew, davem, netdev, linux-pm, Peter Mamonov, stable
When devm_thermal_zone_of_sensor_register() is called from
hwmon_thermal_add_sensor() it is possible that the relevant sensor is
missing an OF node. In this case thermal_zone_of_sensor_register() returns
-EINVAL which causes hwmon_thermal_add_sensor() to fail as well. This patch
changes relevant return code of thermal_zone_of_sensor_register() to
-ENODEV, which is tolerated by hwmon_thermal_add_sensor().
Here is a particular case of such behaviour: the Marvell ethernet PHYs
driver registers hwmon device for the built-in temperature sensor (see
drivers/net/phy/marvell.c). Since the sensor doesn't have associated OF
node devm_hwmon_device_register() returns error which ultimately causes
failure of the PHY driver's probe function.
Fixes: 4e5e4705bf69 ("thermal: introduce device tree parser")
Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Cc: stable@vger.kernel.org
---
drivers/thermal/of-thermal.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index dc5093be553e..34b0cc173f4a 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -493,7 +493,7 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data,
if (!dev || !dev->of_node) {
of_node_put(np);
- return ERR_PTR(-EINVAL);
+ return ERR_PTR(-ENODEV);
}
sensor_np = of_node_get(dev->of_node);
--
2.23.0
^ permalink raw reply related
* Re: [PATCH v1 3/3] seccomp: test SECCOMP_USER_NOTIF_FLAG_CONTINUE
From: Christian Brauner @ 2019-09-19 21:50 UTC (permalink / raw)
To: shuah
Cc: keescook, luto, jannh, wad, ast, daniel, kafai, songliubraving,
yhs, linux-kernel, linux-kselftest, netdev, bpf, Tycho Andersen,
Tyler Hicks, stable
In-Reply-To: <ad7d2901-6639-3684-b71c-bdc1a6a020cc@kernel.org>
On Thu, Sep 19, 2019 at 11:13:46AM -0600, shuah wrote:
> On 9/19/19 3:59 AM, Christian Brauner wrote:
> > Test whether a syscall can be performed after having been intercepted by
> > the seccomp notifier. The test uses dup() and kcmp() since it allows us to
> > nicely test whether the dup() syscall actually succeeded by comparing whether
> > the fds refer to the same underlying struct file.
> >
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Andy Lutomirski <luto@amacapital.net>
> > Cc: Will Drewry <wad@chromium.org>
> > Cc: Shuah Khan <shuah@kernel.org>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: Martin KaFai Lau <kafai@fb.com>
> > Cc: Song Liu <songliubraving@fb.com>
> > Cc: Yonghong Song <yhs@fb.com>
> > Cc: Tycho Andersen <tycho@tycho.ws>
> > CC: Tyler Hicks <tyhicks@canonical.com>
> > Cc: stable@vger.kernel.org
> > Cc: linux-kselftest@vger.kernel.org
> > Cc: netdev@vger.kernel.org
> > Cc: bpf@vger.kernel.org
> > ---
> > /* v1 */
> > - Christian Brauner <christian.brauner@ubuntu.com>:
> > - adapt to new flag name SECCOMP_USER_NOTIF_FLAG_CONTINUE
> >
> > /* v0 */
> > Link: https://lore.kernel.org/r/20190918084833.9369-5-christian.brauner@ubuntu.com
> > ---
> > tools/testing/selftests/seccomp/seccomp_bpf.c | 102 ++++++++++++++++++
> > 1 file changed, 102 insertions(+)
> >
> > diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> > index e996d7b7fd6e..b0966599acb5 100644
> > --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> > +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> > @@ -44,6 +44,7 @@
> > #include <sys/times.h>
> > #include <sys/socket.h>
> > #include <sys/ioctl.h>
> > +#include <linux/kcmp.h>
> > #include <unistd.h>
> > #include <sys/syscall.h>
> > @@ -167,6 +168,10 @@ struct seccomp_metadata {
> > #define SECCOMP_RET_USER_NOTIF 0x7fc00000U
> > +#ifndef SECCOMP_USER_NOTIF_FLAG_CONTINUE
> > +#define SECCOMP_USER_NOTIF_FLAG_CONTINUE 0x00000001
> > +#endif
> > +
> > #define SECCOMP_IOC_MAGIC '!'
> > #define SECCOMP_IO(nr) _IO(SECCOMP_IOC_MAGIC, nr)
> > #define SECCOMP_IOR(nr, type) _IOR(SECCOMP_IOC_MAGIC, nr, type)
> > @@ -3481,6 +3486,103 @@ TEST(seccomp_get_notif_sizes)
> > EXPECT_EQ(sizes.seccomp_notif_resp, sizeof(struct seccomp_notif_resp));
> > }
> > +static int filecmp(pid_t pid1, pid_t pid2, int fd1, int fd2)
> > +{
> > +#ifdef __NR_kcmp
> > + return syscall(__NR_kcmp, pid1, pid2, KCMP_FILE, fd1, fd2);
> > +#else
> > + errno = ENOSYS;
> > + return -1;
>
> This should be SKIP for kselftest so this isn't counted a failure.
> In this case test can't be run because of a missing dependency.
Right, I can just ifdef the whole test and report a skip.
^ permalink raw reply
* [PATCH iproute2 master] bpf: Fix race condition with map pinning
From: Joe Stringer @ 2019-09-19 22:07 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, daniel
If two processes attempt to invoke bpf_map_attach() at the same time,
then they will both create maps, then the first will successfully pin
the map to the filesystem and the second will not pin the map, but will
continue operating with a reference to its own copy of the map. As a
result, the sharing of the same map will be broken from the two programs
that were concurrently loaded via loaders using this library.
Fix this by adding a retry in the case where the pinning fails because
the map already exists on the filesystem. In that case, re-attempt
opening a fd to the map on the filesystem as it shows that another
program already created and pinned a map at that location.
Signed-off-by: Joe Stringer <joe@wand.net.nz>
---
lib/bpf.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/lib/bpf.c b/lib/bpf.c
index f64b58c3bb19..23eb8952cc28 100644
--- a/lib/bpf.c
+++ b/lib/bpf.c
@@ -1625,7 +1625,9 @@ static int bpf_map_attach(const char *name, struct bpf_elf_ctx *ctx,
int *have_map_in_map)
{
int fd, ifindex, ret, map_inner_fd = 0;
+ bool retried = false;
+probe:
fd = bpf_probe_pinned(name, ctx, map->pinning);
if (fd > 0) {
ret = bpf_map_selfcheck_pinned(fd, map, ext,
@@ -1674,7 +1676,11 @@ static int bpf_map_attach(const char *name, struct bpf_elf_ctx *ctx,
}
ret = bpf_place_pinned(fd, name, ctx, map->pinning);
- if (ret < 0 && errno != EEXIST) {
+ if (ret < 0) {
+ if (!retried && errno == EEXIST) {
+ retried = true;
+ goto probe;
+ }
fprintf(stderr, "Could not pin %s map: %s\n", name,
strerror(errno));
close(fd);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net-next] net: dsa: mv88e6xxx: Add support for port mirroring
From: Florian Fainelli @ 2019-09-19 22:12 UTC (permalink / raw)
To: Jason Cobham, Iwan R Timmer
Cc: Andrew Lunn, Vivien Didelot, David S. Miller, netdev
In-Reply-To: <CAKu_b=+0=KXnT-b8L2qkUxT2jSMAJaiMBNAeSjJ3hPqZgx4PGw@mail.gmail.com>
On 9/19/19 2:30 PM, Jason Cobham wrote:
> Hi Iwan,
>
>> Hi Andrew,
>>
>> I only own a simple 5 ports switch (88E6176) which has no problem of mirroring the other ports to a single port. Except for a bandwith shortage ofcourse. While I thought I checked adding and removing ports, I seemed to forgot to check removing ingress traffic as it will now >disable mirroring egress traffic. Searching for how I can distinct ingress from egress mirroring in port_mirror_del, I saw there is a variable in the mirror struct called ingress. Which seems strange, because why is it a seperate argument to the port_mirror_add function?
>>
>> Origally I planned to be able to set the egress and ingress mirror seperatly. But in my laziness when I saw there already was a function to configure the destination port this functionality was lost.
>>
>> Because the other drivers which implemented the port_mirror_add (b53 and
>> ksz9477) also lacks additional checks to prevent new mirror filters from breaking previous ones I assumed they were not necessary.
>>
>> At least I will soon sent a new version with at least the issue of removing mirror ingress traffic fixed and the ability to define a seperate ingress and egress port.
>>
>> Regards,
>> Iwan
>
> I have a similar patch set for port mirror from a few years ago. I'd
> also like to see this functionality in mainline. One issue I ran into
> is when doing port mirror in a cross-chip dsa configuration. If the
> ingress and egress ports are on different chips, the ingress chip
> needs to set the egress to the cross-chip dsa port and the cross-chip
> egress port needs to be set appropriately. I also had the
> functionality to mirror egress from a port to a destination port.
>
> Is it appropriate to send my patch to the mailing list for review or
> should we work on this off-line?
Given that the net-next tree is closed at the moment, working offline
and posting a combined version of a patch that supports port mirroring
for cross chip configurations as well as standalone sounds good to me.
Thanks!
--
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