* [PATCH bpf-next v4 2/2] bpf: Add selftests for bpf_perf_event_output
From: allanzhang @ 2019-06-25 17:27 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, David S. Miller, netdev, bpf, linux-kernel
Cc: allanzhang
In-Reply-To: <20190625172717.158613-1-allanzhang@google.com>
Software event output is only enabled by a few prog types.
This test is to ensure that all supported types are enbled for
bpf_perf_event_output sucessfully.
v4:
* Reformating log message
v3:
* Reformating log message
v2:
* Reformating log message
Signed-off-by: allanzhang <allanzhang@google.com>
---
tools/testing/selftests/bpf/test_verifier.c | 33 ++++++-
.../selftests/bpf/verifier/event_output.c | 94 +++++++++++++++++++
2 files changed, 126 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/verifier/event_output.c
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index c5514daf8865..901a188e1eea 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -50,7 +50,7 @@
#define MAX_INSNS BPF_MAXINSNS
#define MAX_TEST_INSNS 1000000
#define MAX_FIXUPS 8
-#define MAX_NR_MAPS 18
+#define MAX_NR_MAPS 19
#define MAX_TEST_RUNS 8
#define POINTER_VALUE 0xcafe4all
#define TEST_DATA_LEN 64
@@ -84,6 +84,7 @@ struct bpf_test {
int fixup_map_array_wo[MAX_FIXUPS];
int fixup_map_array_small[MAX_FIXUPS];
int fixup_sk_storage_map[MAX_FIXUPS];
+ int fixup_map_event_output[MAX_FIXUPS];
const char *errstr;
const char *errstr_unpriv;
uint32_t retval, retval_unpriv, insn_processed;
@@ -604,6 +605,28 @@ static int create_sk_storage_map(void)
return fd;
}
+static int create_event_output_map(void)
+{
+ struct bpf_create_map_attr attr = {
+ .name = "test_map",
+ .map_type = BPF_MAP_TYPE_PERF_EVENT_ARRAY,
+ .key_size = 4,
+ .value_size = 4,
+ .max_entries = 1,
+ };
+ int fd, btf_fd;
+
+ btf_fd = load_btf();
+ if (btf_fd < 0)
+ return -1;
+ attr.btf_fd = btf_fd;
+ fd = bpf_create_map_xattr(&attr);
+ close(attr.btf_fd);
+ if (fd < 0)
+ printf("Failed to create event_output\n");
+ return fd;
+}
+
static char bpf_vlog[UINT_MAX >> 8];
static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
@@ -627,6 +650,7 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
int *fixup_map_array_wo = test->fixup_map_array_wo;
int *fixup_map_array_small = test->fixup_map_array_small;
int *fixup_sk_storage_map = test->fixup_sk_storage_map;
+ int *fixup_map_event_output = test->fixup_map_event_output;
if (test->fill_helper) {
test->fill_insns = calloc(MAX_TEST_INSNS, sizeof(struct bpf_insn));
@@ -788,6 +812,13 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
fixup_sk_storage_map++;
} while (*fixup_sk_storage_map);
}
+ if (*fixup_map_event_output) {
+ map_fds[18] = create_event_output_map();
+ do {
+ prog[*fixup_map_event_output].imm = map_fds[18];
+ fixup_map_event_output++;
+ } while (*fixup_map_event_output);
+ }
}
static int set_admin(bool admin)
diff --git a/tools/testing/selftests/bpf/verifier/event_output.c b/tools/testing/selftests/bpf/verifier/event_output.c
new file mode 100644
index 000000000000..b25eabcfaa56
--- /dev/null
+++ b/tools/testing/selftests/bpf/verifier/event_output.c
@@ -0,0 +1,94 @@
+/* instructions used to output a skb based software event, produced
+ * from code snippet:
+struct TMP {
+ uint64_t tmp;
+} tt;
+tt.tmp = 5;
+bpf_perf_event_output(skb, &connection_tracking_event_map, 0,
+ &tt, sizeof(tt));
+return 1;
+
+the bpf assembly from llvm is:
+ 0: b7 02 00 00 05 00 00 00 r2 = 5
+ 1: 7b 2a f8 ff 00 00 00 00 *(u64 *)(r10 - 8) = r2
+ 2: bf a4 00 00 00 00 00 00 r4 = r10
+ 3: 07 04 00 00 f8 ff ff ff r4 += -8
+ 4: 18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0ll
+ 6: b7 03 00 00 00 00 00 00 r3 = 0
+ 7: b7 05 00 00 08 00 00 00 r5 = 8
+ 8: 85 00 00 00 19 00 00 00 call 25
+ 9: b7 00 00 00 01 00 00 00 r0 = 1
+ 10: 95 00 00 00 00 00 00 00 exit
+
+ The reason I put the code here instead of fill_helpers is that map fixup is
+ against the insns, instead of filled prog.
+*/
+
+#define __PERF_EVENT_INSNS__ \
+ BPF_MOV64_IMM(BPF_REG_2, 5), \
+ BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_2, -8), \
+ BPF_MOV64_REG(BPF_REG_4, BPF_REG_10), \
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -8), \
+ BPF_LD_MAP_FD(BPF_REG_2, 0), \
+ BPF_MOV64_IMM(BPF_REG_3, 0), \
+ BPF_MOV64_IMM(BPF_REG_5, 8), \
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, \
+ BPF_FUNC_perf_event_output), \
+ BPF_MOV64_IMM(BPF_REG_0, 1), \
+ BPF_EXIT_INSN(),
+{
+ "perfevent for sockops",
+ .insns = { __PERF_EVENT_INSNS__ },
+ .prog_type = BPF_PROG_TYPE_SOCK_OPS,
+ .fixup_map_event_output = { 4 },
+ .result = ACCEPT,
+ .retval = 1,
+},
+{
+ "perfevent for tc",
+ .insns = { __PERF_EVENT_INSNS__ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .fixup_map_event_output = { 4 },
+ .result = ACCEPT,
+ .retval = 1,
+},
+{
+ "perfevent for lwt out",
+ .insns = { __PERF_EVENT_INSNS__ },
+ .prog_type = BPF_PROG_TYPE_LWT_OUT,
+ .fixup_map_event_output = { 4 },
+ .result = ACCEPT,
+ .retval = 1,
+},
+{
+ "perfevent for xdp",
+ .insns = { __PERF_EVENT_INSNS__ },
+ .prog_type = BPF_PROG_TYPE_XDP,
+ .fixup_map_event_output = { 4 },
+ .result = ACCEPT,
+ .retval = 1,
+},
+{
+ "perfevent for socket filter",
+ .insns = { __PERF_EVENT_INSNS__ },
+ .prog_type = BPF_PROG_TYPE_SOCKET_FILTER,
+ .fixup_map_event_output = { 4 },
+ .result = ACCEPT,
+ .retval = 1,
+},
+{
+ "perfevent for sk_skb",
+ .insns = { __PERF_EVENT_INSNS__ },
+ .prog_type = BPF_PROG_TYPE_SK_SKB,
+ .fixup_map_event_output = { 4 },
+ .result = ACCEPT,
+ .retval = 1,
+},
+{
+ "perfevent for cgroup skb",
+ .insns = { __PERF_EVENT_INSNS__ },
+ .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
+ .fixup_map_event_output = { 4 },
+ .result = ACCEPT,
+ .retval = 1,
+},
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next v4 1/2] bpf: Allow bpf_skb_event_output for a few prog types
From: allanzhang @ 2019-06-25 17:27 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, David S. Miller, netdev, bpf, linux-kernel
Cc: allanzhang
In-Reply-To: <20190625172717.158613-1-allanzhang@google.com>
Software event output is only enabled by a few prog types right now (TC,
LWT out, XDP, sockops). Many other skb based prog types need
bpf_skb_event_output to produce software event.
Added socket_filter, cg_skb, sk_skb prog types to generate sw event.
Test bpf code is generated from code snippet:
struct TMP {
uint64_t tmp;
} tt;
tt.tmp = 5;
bpf_perf_event_output(skb, &connection_tracking_event_map, 0,
&tt, sizeof(tt));
return 1;
the bpf assembly from llvm is:
0: b7 02 00 00 05 00 00 00 r2 = 5
1: 7b 2a f8 ff 00 00 00 00 *(u64 *)(r10 - 8) = r2
2: bf a4 00 00 00 00 00 00 r4 = r10
3: 07 04 00 00 f8 ff ff ff r4 += -8
4: 18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0ll
6: b7 03 00 00 00 00 00 00 r3 = 0
7: b7 05 00 00 08 00 00 00 r5 = 8
8: 85 00 00 00 19 00 00 00 call 25
9: b7 00 00 00 01 00 00 00 r0 = 1
10: 95 00 00 00 00 00 00 00 exit
Signed-off-by: allanzhang <allanzhang@google.com>
v4:
* Reformating log message
v3:
* Reformating log message
v2:
* Reformating log message
---
net/core/filter.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/net/core/filter.c b/net/core/filter.c
index 2014d76e0d2a..b75fcf412628 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5958,6 +5958,8 @@ sk_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_get_socket_cookie_proto;
case BPF_FUNC_get_socket_uid:
return &bpf_get_socket_uid_proto;
+ case BPF_FUNC_perf_event_output:
+ return &bpf_skb_event_output_proto;
default:
return bpf_base_func_proto(func_id);
}
@@ -5978,6 +5980,8 @@ cg_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_sk_storage_get_proto;
case BPF_FUNC_sk_storage_delete:
return &bpf_sk_storage_delete_proto;
+ case BPF_FUNC_perf_event_output:
+ return &bpf_skb_event_output_proto;
#ifdef CONFIG_SOCK_CGROUP_DATA
case BPF_FUNC_skb_cgroup_id:
return &bpf_skb_cgroup_id_proto;
@@ -6226,6 +6230,8 @@ sk_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_sk_redirect_map_proto;
case BPF_FUNC_sk_redirect_hash:
return &bpf_sk_redirect_hash_proto;
+ case BPF_FUNC_perf_event_output:
+ return &bpf_skb_event_output_proto;
#ifdef CONFIG_INET
case BPF_FUNC_sk_lookup_tcp:
return &bpf_sk_lookup_tcp_proto;
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next v4 0/2] bpf: Allow bpf_skb_event_output for more prog types
From: allanzhang @ 2019-06-25 17:27 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, David S. Miller, netdev, bpf, linux-kernel
Cc: allanzhang
Software event output is only enabled by a few prog types right now (TC,
LWT out, XDP, sockops). Many other skb based prog types need
bpf_skb_event_output to produce software event.
Added socket_filter, cg_skb, sk_skb prog types to generate sw event.
allanzhang (2):
bpf: Allow bpf_skb_event_output for a few prog types
bpf: Add selftests for bpf_perf_event_output
net/core/filter.c | 6 ++
tools/testing/selftests/bpf/test_verifier.c | 33 ++++++-
.../selftests/bpf/verifier/event_output.c | 94 +++++++++++++++++++
3 files changed, 132 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/verifier/event_output.c
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* Re: [PATCH V3 12/15] ARM: vexpress: cleanup cppcheck shifting error
From: Sudeep Holla @ 2019-06-25 17:24 UTC (permalink / raw)
To: Phong Tran
Cc: acme, alexander.shishkin, alexander.sverdlin, allison, andrew,
ast, bgolaszewski, bpf, daniel, daniel, dmg, festevam, gerg,
gregkh, gregory.clement, haojian.zhuang, hsweeten,
illusionist.neo, info, jason, jolsa, kafai, kernel, kgene, krzk,
kstewart, linux-arm-kernel, linux-imx, linux-kernel, linux-omap,
linux-samsung-soc, linux, liviu.dudau, lkundrak,
lorenzo.pieralisi, mark.rutland, mingo, namhyung, netdev, nsekhar,
peterz, robert.jarzmik, s.hauer, sebastian.hesselbarth, shawnguo,
songliubraving, swinslow, tglx, tony, will, yhs, Sudeep Holla
In-Reply-To: <20190625040356.27473-13-tranmanphong@gmail.com>
On Tue, Jun 25, 2019 at 11:03:53AM +0700, Phong Tran wrote:
> There is error from cppcheck tool
> "Shifting signed 32-bit value by 31 bits is undefined behaviour errors"
> change to use BIT() marco for improvement.
>
What's your plan for merging this series ? I can take this for v5.4
If not,
Acked-by: Sudeep Holla <sudeep.holla@arm.com>
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH bpf] tools: bpftool: use correct argument in cgroup errors
From: Roman Gushchin @ 2019-06-25 17:21 UTC (permalink / raw)
To: Jakub Kicinski
Cc: alexei.starovoitov@gmail.com, daniel@iogearbox.net,
netdev@vger.kernel.org, bpf@vger.kernel.org,
oss-drivers@netronome.com, sdf@google.com, Quentin Monnet
In-Reply-To: <20190625165631.18928-1-jakub.kicinski@netronome.com>
On Tue, Jun 25, 2019 at 09:56:31AM -0700, Jakub Kicinski wrote:
> cgroup code tries to use argv[0] as the cgroup path,
> but if it fails uses argv[1] to report errors.
>
> Fixes: 5ccda64d38cc ("bpftool: implement cgroup bpf operations")
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
Acked-by: Roman Gushchin <guro@fb.com>
Thanks, Jakub!
> ---
> tools/bpf/bpftool/cgroup.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
> index 73ec8ea33fb4..a13fb7265d1a 100644
> --- a/tools/bpf/bpftool/cgroup.c
> +++ b/tools/bpf/bpftool/cgroup.c
> @@ -168,7 +168,7 @@ static int do_show(int argc, char **argv)
>
> cgroup_fd = open(argv[0], O_RDONLY);
> if (cgroup_fd < 0) {
> - p_err("can't open cgroup %s", argv[1]);
> + p_err("can't open cgroup %s", argv[0]);
> goto exit;
> }
>
> @@ -356,7 +356,7 @@ static int do_attach(int argc, char **argv)
>
> cgroup_fd = open(argv[0], O_RDONLY);
> if (cgroup_fd < 0) {
> - p_err("can't open cgroup %s", argv[1]);
> + p_err("can't open cgroup %s", argv[0]);
> goto exit;
> }
>
> @@ -414,7 +414,7 @@ static int do_detach(int argc, char **argv)
>
> cgroup_fd = open(argv[0], O_RDONLY);
> if (cgroup_fd < 0) {
> - p_err("can't open cgroup %s", argv[1]);
> + p_err("can't open cgroup %s", argv[0]);
> goto exit;
> }
>
> --
> 2.21.0
>
^ permalink raw reply
* Re: [PATCH V3 01/15] arm: perf: cleanup cppcheck shifting error
From: Will Deacon @ 2019-06-25 17:21 UTC (permalink / raw)
To: Phong Tran
Cc: acme, alexander.shishkin, alexander.sverdlin, allison, andrew,
ast, bgolaszewski, bpf, daniel, daniel, dmg, festevam, gerg,
gregkh, gregory.clement, haojian.zhuang, hsweeten,
illusionist.neo, info, jason, jolsa, kafai, kernel, kgene, krzk,
kstewart, linux-arm-kernel, linux-imx, linux-kernel, linux-omap,
linux-samsung-soc, linux, liviu.dudau, lkundrak,
lorenzo.pieralisi, mark.rutland, mingo, namhyung, netdev, nsekhar,
peterz, robert.jarzmik, s.hauer, sebastian.hesselbarth, shawnguo,
songliubraving, sudeep.holla, swinslow, tglx, tony, yhs
In-Reply-To: <20190625040356.27473-2-tranmanphong@gmail.com>
On Tue, Jun 25, 2019 at 11:03:42AM +0700, Phong Tran wrote:
> There is error from cppcheck tool
> "Shifting signed 32-bit value by 31 bits is undefined behaviour errors"
> change to use BIT() marco for improvement.
s/marco/macro/
As Peter pointed out, this "error" is also a false positive also for the
kernel.
> Signed-off-by: Phong Tran <tranmanphong@gmail.com>
> ---
> arch/arm/kernel/perf_event_v7.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c
> index a4fb0f8b8f84..2924d7910b10 100644
> --- a/arch/arm/kernel/perf_event_v7.c
> +++ b/arch/arm/kernel/perf_event_v7.c
> @@ -697,9 +697,9 @@ static struct attribute_group armv7_pmuv2_events_attr_group = {
> /*
> * Event filters for PMUv2
> */
> -#define ARMV7_EXCLUDE_PL1 (1 << 31)
> -#define ARMV7_EXCLUDE_USER (1 << 30)
> -#define ARMV7_INCLUDE_HYP (1 << 27)
> +#define ARMV7_EXCLUDE_PL1 BIT(31)
> +#define ARMV7_EXCLUDE_USER BIT(30)
> +#define ARMV7_INCLUDE_HYP BIT(27)
Acked-by: Will Deacon <will.deacon@arm.com>
You can drop this into Russell's patch system[1].
Will
[1] https://www.arm.linux.org.uk/developer/patches/
^ permalink raw reply
* Re: [PATCH] perf cs-etm: Improve completeness for kernel address space
From: Mathieu Poirier @ 2019-06-25 17:14 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Leo Yan, Linux Kernel Mailing List, linux-arm-kernel, netdev, bpf,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Peter Zijlstra,
Suzuki Poulouse, Coresight ML
In-Reply-To: <20190624190009.GE4181@kernel.org>
On Mon, 24 Jun 2019 at 13:00, Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> Em Thu, Jun 20, 2019 at 08:58:29AM +0800, Leo Yan escreveu:
> > Hi Mathieu,
> >
> > On Wed, Jun 19, 2019 at 11:49:44AM -0600, Mathieu Poirier wrote:
> >
> > [...]
> >
> > > > diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> > > > index 51dd00f65709..4776c2c1fb6d 100644
> > > > --- a/tools/perf/Makefile.config
> > > > +++ b/tools/perf/Makefile.config
> > > > @@ -418,6 +418,30 @@ ifdef CORESIGHT
> > > > endif
> > > > LDFLAGS += $(LIBOPENCSD_LDFLAGS)
> > > > EXTLIBS += $(OPENCSDLIBS)
> > > > + ifneq ($(wildcard $(srctree)/arch/arm64/kernel/vmlinux.lds),)
> > > > + # Extract info from lds:
> > > > + # . = ((((((((0xffffffffffffffff)) - (((1)) << (48)) + 1) + (0)) + (0x08000000))) + (0x08000000))) + 0x00080000;
> > > > + # ARM64_PRE_START_SIZE := (0x08000000 + 0x08000000 + 0x00080000)
> > > > + ARM64_PRE_START_SIZE := $(shell egrep ' \. \= \({8}0x[0-9a-fA-F]+\){2}' \
> > > > + $(srctree)/arch/arm64/kernel/vmlinux.lds | \
> > > > + sed -e 's/[(|)|.|=|+|<|;|-]//g' -e 's/ \+/ /g' -e 's/^[ \t]*//' | \
> > > > + awk -F' ' '{print "("$$6 "+" $$7 "+" $$8")"}' 2>/dev/null)
> > > > + else
> > > > + ARM64_PRE_START_SIZE := 0
> > > > + endif
> > > > + CFLAGS += -DARM64_PRE_START_SIZE="$(ARM64_PRE_START_SIZE)"
> > > > + ifneq ($(wildcard $(srctree)/arch/arm/kernel/vmlinux.lds),)
> > > > + # Extract info from lds:
> > > > + # . = ((0xC0000000)) + 0x00208000;
> > > > + # ARM_PRE_START_SIZE := 0x00208000
> > > > + ARM_PRE_START_SIZE := $(shell egrep ' \. \= \({2}0x[0-9a-fA-F]+\){2}' \
> > > > + $(srctree)/arch/arm/kernel/vmlinux.lds | \
> > > > + sed -e 's/[(|)|.|=|+|<|;|-]//g' -e 's/ \+/ /g' -e 's/^[ \t]*//' | \
> > > > + awk -F' ' '{print "("$$2")"}' 2>/dev/null)
> > > > + else
> > > > + ARM_PRE_START_SIZE := 0
> > > > + endif
> > > > + CFLAGS += -DARM_PRE_START_SIZE="$(ARM_PRE_START_SIZE)"
> > > > $(call detected,CONFIG_LIBOPENCSD)
> > > > ifdef CSTRACE_RAW
> > > > CFLAGS += -DCS_DEBUG_RAW
> > > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> > > > index 0c7776b51045..ae831f836c70 100644
> > > > --- a/tools/perf/util/cs-etm.c
> > > > +++ b/tools/perf/util/cs-etm.c
> > > > @@ -613,10 +613,34 @@ static void cs_etm__free(struct perf_session *session)
> > > > static u8 cs_etm__cpu_mode(struct cs_etm_queue *etmq, u64 address)
> > > > {
> > > > struct machine *machine;
> > > > + u64 fixup_kernel_start = 0;
> > > > + const char *arch;
> > > >
> > > > machine = etmq->etm->machine;
> > > > + arch = perf_env__arch(machine->env);
> > > >
> > > > - if (address >= etmq->etm->kernel_start) {
> > > > + /*
> > > > + * Since arm and arm64 specify some memory regions prior to
> > > > + * 'kernel_start', kernel addresses can be less than 'kernel_start'.
> > > > + *
> > > > + * For arm architecture, the 16MB virtual memory space prior to
> > > > + * 'kernel_start' is allocated to device modules, a PMD table if
> > > > + * CONFIG_HIGHMEM is enabled and a PGD table.
> > > > + *
> > > > + * For arm64 architecture, the root PGD table, device module memory
> > > > + * region and BPF jit region are prior to 'kernel_start'.
> > > > + *
> > > > + * To reflect the complete kernel address space, compensate these
> > > > + * pre-defined regions for kernel start address.
> > > > + */
> > > > + if (!strcmp(arch, "arm64"))
> > > > + fixup_kernel_start = etmq->etm->kernel_start -
> > > > + ARM64_PRE_START_SIZE;
> > > > + else if (!strcmp(arch, "arm"))
> > > > + fixup_kernel_start = etmq->etm->kernel_start -
> > > > + ARM_PRE_START_SIZE;
> > >
> > > I will test your work but from a quick look wouldn't it be better to
> > > have a single define name here? From looking at the modifications you
> > > did to Makefile.config there doesn't seem to be a reason to have two.
> >
> > Thanks for suggestion. I changed to use single define
> > ARM_PRE_START_SIZE and sent patch v2 [1].
> >
> > If possible, please test patch v2.
> >
> > Thanks,
> > Leo Yan
>
> So just for the record, I'm waiting for Mathieu on this one, i.e. for
> him to test/ack v3.
Right, please give me some time to test this. As Leo indicated the
procedure is time consuming.
Thanks,
Mathieu
>
> - Arnaldo
>
> > [1] https://lore.kernel.org/linux-arm-kernel/20190620005428.20883-1-leo.yan@linaro.org/T/#u
> >
> > > > +
> > > > + if (address >= fixup_kernel_start) {
> > > > if (machine__is_host(machine))
> > > > return PERF_RECORD_MISC_KERNEL;
> > > > else
> > > > --
> > > > 2.17.1
> > > >
>
> --
>
> - Arnaldo
^ permalink raw reply
* Re: Warnings generated from tcp_sacktag_write_queue.
From: Eric Dumazet @ 2019-06-25 17:10 UTC (permalink / raw)
To: Chinmay Agarwal, netdev; +Cc: sharathv, kapandey
In-Reply-To: <20190625155734.GA31551@chinagar-linux.qualcomm.com>
On 6/25/19 8:57 AM, Chinmay Agarwal wrote:
>
> The kernel version used is 4.14.
>
Do not use this old version please.
^ permalink raw reply
* Re: Removing skb_orphan() from ip_rcv_core()
From: Eric Dumazet @ 2019-06-25 17:03 UTC (permalink / raw)
To: Daniel Borkmann, Joe Stringer, Florian Westphal
Cc: netdev, john fastabend, Lorenz Bauer, Jakub Sitnicki, Paolo Abeni,
Flavio Leitner, ast
In-Reply-To: <4deff7d7-cc10-090d-86f2-850148fdf032@iogearbox.net>
On 6/25/19 2:35 AM, Daniel Borkmann wrote:
>
> But wasn't the whole point of 9c4c325252c5 ("skbuff: preserve sock reference when
> scrubbing the skb.") to defer orphaning to as late as possible? If I'm not missing
> anything, then above would reintroduce the issues that 9c4c325252c5 was trying to
> solve wrt TSQ/XPS/etc when skb was sent via veth based data path to cross netns and
> then forwarded to phys dev for transmission; meaning, skb->sk is lost at the point
> of dev_queue_xmit() for the latter. A side-effect this would also have is that this
> changes behavior again for tc egress programs sitting on phys dev (e.g. querying
> sock cookie or other related features).
Unless we can detect/decide that a packet going through veth pair is going to be locally
consumed, or forwarded to a physical device (another ndo_start_xmit()), we need
to skb_orphan() the packet, exactly the same way than loopback ndo_start_xmit()
(We could have setups where these packets going through lo interface could be forwarded
to a NIC...)
Backpressure is a best effort, we should not make it an absolute requirement and
prevent doing early demux as early as possible in RX path.
^ permalink raw reply
* [PATCH net-next] Revert "net: ena: ethtool: add extra properties retrieval via get_priv_flags"
From: Jakub Kicinski @ 2019-06-25 16:59 UTC (permalink / raw)
To: davem
Cc: netdev, oss-drivers, nafea, dwmw2, sameehj, zorik, saeedb,
netanel, Jakub Kicinski
This reverts commit 315c28d2b714 ("net: ena: ethtool: add extra properties retrieval via get_priv_flags").
As discussed at netconf and on the mailing list we can't allow
for the the abuse of private flags for exposing arbitrary device
labels.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
.../net/ethernet/amazon/ena/ena_admin_defs.h | 16 ----
drivers/net/ethernet/amazon/ena/ena_com.c | 56 --------------
drivers/net/ethernet/amazon/ena/ena_com.h | 32 --------
drivers/net/ethernet/amazon/ena/ena_ethtool.c | 75 +++----------------
drivers/net/ethernet/amazon/ena/ena_netdev.c | 14 ----
drivers/net/ethernet/amazon/ena/ena_netdev.h | 2 -
6 files changed, 11 insertions(+), 184 deletions(-)
diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
index c8638f7b5b8e..d19f2ecf8e84 100644
--- a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
+++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
@@ -32,8 +32,6 @@
#ifndef _ENA_ADMIN_H_
#define _ENA_ADMIN_H_
-#define ENA_ADMIN_EXTRA_PROPERTIES_STRING_LEN 32
-#define ENA_ADMIN_EXTRA_PROPERTIES_COUNT 32
enum ena_admin_aq_opcode {
ENA_ADMIN_CREATE_SQ = 1,
@@ -62,8 +60,6 @@ enum ena_admin_aq_feature_id {
ENA_ADMIN_MAX_QUEUES_NUM = 2,
ENA_ADMIN_HW_HINTS = 3,
ENA_ADMIN_LLQ = 4,
- ENA_ADMIN_EXTRA_PROPERTIES_STRINGS = 5,
- ENA_ADMIN_EXTRA_PROPERTIES_FLAGS = 6,
ENA_ADMIN_MAX_QUEUES_EXT = 7,
ENA_ADMIN_RSS_HASH_FUNCTION = 10,
ENA_ADMIN_STATELESS_OFFLOAD_CONFIG = 11,
@@ -599,14 +595,6 @@ struct ena_admin_set_feature_mtu_desc {
u32 mtu;
};
-struct ena_admin_get_extra_properties_strings_desc {
- u32 count;
-};
-
-struct ena_admin_get_extra_properties_flags_desc {
- u32 flags;
-};
-
struct ena_admin_set_feature_host_attr_desc {
/* host OS info base address in OS memory. host info is 4KB of
* physically contiguous
@@ -926,10 +914,6 @@ struct ena_admin_get_feat_resp {
struct ena_admin_feature_intr_moder_desc intr_moderation;
struct ena_admin_ena_hw_hints hw_hints;
-
- struct ena_admin_get_extra_properties_strings_desc extra_properties_strings;
-
- struct ena_admin_get_extra_properties_flags_desc extra_properties_flags;
} u;
};
diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
index 56781609c3af..911a2e7a375a 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -1896,62 +1896,6 @@ int ena_com_get_link_params(struct ena_com_dev *ena_dev,
return ena_com_get_feature(ena_dev, resp, ENA_ADMIN_LINK_CONFIG, 0);
}
-int ena_com_extra_properties_strings_init(struct ena_com_dev *ena_dev)
-{
- struct ena_admin_get_feat_resp resp;
- struct ena_extra_properties_strings *extra_properties_strings =
- &ena_dev->extra_properties_strings;
- u32 rc;
-
- extra_properties_strings->size = ENA_ADMIN_EXTRA_PROPERTIES_COUNT *
- ENA_ADMIN_EXTRA_PROPERTIES_STRING_LEN;
-
- extra_properties_strings->virt_addr =
- dma_alloc_coherent(ena_dev->dmadev,
- extra_properties_strings->size,
- &extra_properties_strings->dma_addr,
- GFP_KERNEL);
- if (unlikely(!extra_properties_strings->virt_addr)) {
- pr_err("Failed to allocate extra properties strings\n");
- return 0;
- }
-
- rc = ena_com_get_feature_ex(ena_dev, &resp,
- ENA_ADMIN_EXTRA_PROPERTIES_STRINGS,
- extra_properties_strings->dma_addr,
- extra_properties_strings->size, 0);
- if (rc) {
- pr_debug("Failed to get extra properties strings\n");
- goto err;
- }
-
- return resp.u.extra_properties_strings.count;
-err:
- ena_com_delete_extra_properties_strings(ena_dev);
- return 0;
-}
-
-void ena_com_delete_extra_properties_strings(struct ena_com_dev *ena_dev)
-{
- struct ena_extra_properties_strings *extra_properties_strings =
- &ena_dev->extra_properties_strings;
-
- if (extra_properties_strings->virt_addr) {
- dma_free_coherent(ena_dev->dmadev,
- extra_properties_strings->size,
- extra_properties_strings->virt_addr,
- extra_properties_strings->dma_addr);
- extra_properties_strings->virt_addr = NULL;
- }
-}
-
-int ena_com_get_extra_properties_flags(struct ena_com_dev *ena_dev,
- struct ena_admin_get_feat_resp *resp)
-{
- return ena_com_get_feature(ena_dev, resp,
- ENA_ADMIN_EXTRA_PROPERTIES_FLAGS, 0);
-}
-
int ena_com_get_dev_attr_feat(struct ena_com_dev *ena_dev,
struct ena_com_dev_get_features_ctx *get_feat_ctx)
{
diff --git a/drivers/net/ethernet/amazon/ena/ena_com.h b/drivers/net/ethernet/amazon/ena/ena_com.h
index 4700d92a317b..0d3664fe260d 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.h
+++ b/drivers/net/ethernet/amazon/ena/ena_com.h
@@ -352,12 +352,6 @@ struct ena_host_attribute {
dma_addr_t host_info_dma_addr;
};
-struct ena_extra_properties_strings {
- u8 *virt_addr;
- dma_addr_t dma_addr;
- u32 size;
-};
-
/* Each ena_dev is a PCI function. */
struct ena_com_dev {
struct ena_com_admin_queue admin_queue;
@@ -386,7 +380,6 @@ struct ena_com_dev {
struct ena_intr_moder_entry *intr_moder_tbl;
struct ena_com_llq_info llq_info;
- struct ena_extra_properties_strings extra_properties_strings;
};
struct ena_com_dev_get_features_ctx {
@@ -620,31 +613,6 @@ int ena_com_validate_version(struct ena_com_dev *ena_dev);
int ena_com_get_link_params(struct ena_com_dev *ena_dev,
struct ena_admin_get_feat_resp *resp);
-/* ena_com_extra_properties_strings_init - Initialize the extra properties strings buffer.
- * @ena_dev: ENA communication layer struct
- *
- * Initialize the extra properties strings buffer.
- */
-int ena_com_extra_properties_strings_init(struct ena_com_dev *ena_dev);
-
-/* ena_com_delete_extra_properties_strings - Free the extra properties strings buffer.
- * @ena_dev: ENA communication layer struct
- *
- * Free the allocated extra properties strings buffer.
- */
-void ena_com_delete_extra_properties_strings(struct ena_com_dev *ena_dev);
-
-/* ena_com_get_extra_properties_flags - Retrieve extra properties flags.
- * @ena_dev: ENA communication layer struct
- * @resp: Extra properties flags.
- *
- * Retrieve the extra properties flags.
- *
- * @return - 0 on Success negative value otherwise.
- */
-int ena_com_get_extra_properties_flags(struct ena_com_dev *ena_dev,
- struct ena_admin_get_feat_resp *resp);
-
/* ena_com_get_dma_width - Retrieve physical dma address width the device
* supports.
* @ena_dev: ENA communication layer struct
diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
index b46f069ac0eb..b997c3ce9e2b 100644
--- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
+++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
@@ -198,24 +198,15 @@ static void ena_get_ethtool_stats(struct net_device *netdev,
ena_dev_admin_queue_stats(adapter, &data);
}
-static int get_stats_sset_count(struct ena_adapter *adapter)
-{
- return adapter->num_queues * (ENA_STATS_ARRAY_TX + ENA_STATS_ARRAY_RX)
- + ENA_STATS_ARRAY_GLOBAL + ENA_STATS_ARRAY_ENA_COM;
-}
-
int ena_get_sset_count(struct net_device *netdev, int sset)
{
struct ena_adapter *adapter = netdev_priv(netdev);
- switch (sset) {
- case ETH_SS_STATS:
- return get_stats_sset_count(adapter);
- case ETH_SS_PRIV_FLAGS:
- return adapter->ena_extra_properties_count;
- default:
+ if (sset != ETH_SS_STATS)
return -EOPNOTSUPP;
- }
+
+ return adapter->num_queues * (ENA_STATS_ARRAY_TX + ENA_STATS_ARRAY_RX)
+ + ENA_STATS_ARRAY_GLOBAL + ENA_STATS_ARRAY_ENA_COM;
}
static void ena_queue_strings(struct ena_adapter *adapter, u8 **data)
@@ -257,54 +248,26 @@ static void ena_com_dev_strings(u8 **data)
}
}
-static void get_stats_strings(struct ena_adapter *adapter, u8 *data)
+static void ena_get_strings(struct net_device *netdev, u32 sset, u8 *data)
{
+ struct ena_adapter *adapter = netdev_priv(netdev);
const struct ena_stats *ena_stats;
int i;
+ if (sset != ETH_SS_STATS)
+ return;
+
for (i = 0; i < ENA_STATS_ARRAY_GLOBAL; i++) {
ena_stats = &ena_stats_global_strings[i];
+
memcpy(data, ena_stats->name, ETH_GSTRING_LEN);
data += ETH_GSTRING_LEN;
}
+
ena_queue_strings(adapter, &data);
ena_com_dev_strings(&data);
}
-static void get_private_flags_strings(struct ena_adapter *adapter, u8 *data)
-{
- struct ena_com_dev *ena_dev = adapter->ena_dev;
- u8 *strings = ena_dev->extra_properties_strings.virt_addr;
- int i;
-
- if (unlikely(!strings)) {
- adapter->ena_extra_properties_count = 0;
- return;
- }
-
- for (i = 0; i < adapter->ena_extra_properties_count; i++) {
- strlcpy(data, strings + ENA_ADMIN_EXTRA_PROPERTIES_STRING_LEN * i,
- ETH_GSTRING_LEN);
- data += ETH_GSTRING_LEN;
- }
-}
-
-static void ena_get_strings(struct net_device *netdev, u32 sset, u8 *data)
-{
- struct ena_adapter *adapter = netdev_priv(netdev);
-
- switch (sset) {
- case ETH_SS_STATS:
- get_stats_strings(adapter, data);
- break;
- case ETH_SS_PRIV_FLAGS:
- get_private_flags_strings(adapter, data);
- break;
- default:
- break;
- }
-}
-
static int ena_get_link_ksettings(struct net_device *netdev,
struct ethtool_link_ksettings *link_ksettings)
{
@@ -479,7 +442,6 @@ static void ena_get_drvinfo(struct net_device *dev,
strlcpy(info->version, DRV_MODULE_VERSION, sizeof(info->version));
strlcpy(info->bus_info, pci_name(adapter->pdev),
sizeof(info->bus_info));
- info->n_priv_flags = adapter->ena_extra_properties_count;
}
static void ena_get_ringparam(struct net_device *netdev,
@@ -856,20 +818,6 @@ static int ena_set_tunable(struct net_device *netdev,
return ret;
}
-static u32 ena_get_priv_flags(struct net_device *netdev)
-{
- struct ena_adapter *adapter = netdev_priv(netdev);
- struct ena_com_dev *ena_dev = adapter->ena_dev;
- struct ena_admin_get_feat_resp get_resp;
- u32 rc;
-
- rc = ena_com_get_extra_properties_flags(ena_dev, &get_resp);
- if (!rc)
- return get_resp.u.extra_properties_flags.flags;
-
- return 0;
-}
-
static const struct ethtool_ops ena_ethtool_ops = {
.get_link_ksettings = ena_get_link_ksettings,
.get_drvinfo = ena_get_drvinfo,
@@ -892,7 +840,6 @@ static const struct ethtool_ops ena_ethtool_ops = {
.get_channels = ena_get_channels,
.get_tunable = ena_get_tunable,
.set_tunable = ena_set_tunable,
- .get_priv_flags = ena_get_priv_flags,
};
void ena_set_ethtool_ops(struct net_device *netdev)
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 20ec8ff03aaf..664e3ed97ea9 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -2472,14 +2472,6 @@ static void ena_config_debug_area(struct ena_adapter *adapter)
ena_com_delete_debug_area(adapter->ena_dev);
}
-static void ena_extra_properties_strings_destroy(struct net_device *netdev)
-{
- struct ena_adapter *adapter = netdev_priv(netdev);
-
- ena_com_delete_extra_properties_strings(adapter->ena_dev);
- adapter->ena_extra_properties_count = 0;
-}
-
static void ena_get_stats64(struct net_device *netdev,
struct rtnl_link_stats64 *stats)
{
@@ -3578,9 +3570,6 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
ena_config_debug_area(adapter);
- adapter->ena_extra_properties_count =
- ena_com_extra_properties_strings_init(ena_dev);
-
memcpy(adapter->netdev->perm_addr, adapter->mac_addr, netdev->addr_len);
netif_carrier_off(netdev);
@@ -3620,7 +3609,6 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
return 0;
err_rss:
- ena_extra_properties_strings_destroy(netdev);
ena_com_delete_debug_area(ena_dev);
ena_com_rss_destroy(ena_dev);
err_free_msix:
@@ -3687,8 +3675,6 @@ static void ena_remove(struct pci_dev *pdev)
ena_com_delete_host_info(ena_dev);
- ena_extra_properties_strings_destroy(netdev);
-
ena_release_bars(ena_dev, pdev);
pci_disable_device(pdev);
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
index f2b6e2e0504d..efbcffd22215 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
@@ -378,8 +378,6 @@ struct ena_adapter {
u32 last_monitored_tx_qid;
enum ena_regs_reset_reason_types reset_reason;
-
- u8 ena_extra_properties_count;
};
void ena_set_ethtool_ops(struct net_device *netdev);
--
2.21.0
^ permalink raw reply related
* [PATCH bpf] tools: bpftool: use correct argument in cgroup errors
From: Jakub Kicinski @ 2019-06-25 16:56 UTC (permalink / raw)
To: alexei.starovoitov, daniel
Cc: netdev, bpf, oss-drivers, guro, sdf, Jakub Kicinski,
Quentin Monnet
cgroup code tries to use argv[0] as the cgroup path,
but if it fails uses argv[1] to report errors.
Fixes: 5ccda64d38cc ("bpftool: implement cgroup bpf operations")
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
tools/bpf/bpftool/cgroup.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
index 73ec8ea33fb4..a13fb7265d1a 100644
--- a/tools/bpf/bpftool/cgroup.c
+++ b/tools/bpf/bpftool/cgroup.c
@@ -168,7 +168,7 @@ static int do_show(int argc, char **argv)
cgroup_fd = open(argv[0], O_RDONLY);
if (cgroup_fd < 0) {
- p_err("can't open cgroup %s", argv[1]);
+ p_err("can't open cgroup %s", argv[0]);
goto exit;
}
@@ -356,7 +356,7 @@ static int do_attach(int argc, char **argv)
cgroup_fd = open(argv[0], O_RDONLY);
if (cgroup_fd < 0) {
- p_err("can't open cgroup %s", argv[1]);
+ p_err("can't open cgroup %s", argv[0]);
goto exit;
}
@@ -414,7 +414,7 @@ static int do_detach(int argc, char **argv)
cgroup_fd = open(argv[0], O_RDONLY);
if (cgroup_fd < 0) {
- p_err("can't open cgroup %s", argv[1]);
+ p_err("can't open cgroup %s", argv[0]);
goto exit;
}
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v2 net-next] ipv6: Convert gateway validation to use fib6_info
From: Wei Wang @ 2019-06-25 16:54 UTC (permalink / raw)
To: David Ahern
Cc: David S . Miller, Linux Kernel Network Developers,
Martin KaFai Lau, David Ahern
In-Reply-To: <20190624204451.10929-1-dsahern@kernel.org>
On Mon, Jun 24, 2019 at 1:44 PM David Ahern <dsahern@kernel.org> wrote:
>
> From: David Ahern <dsahern@gmail.com>
>
> Gateway validation does not need a dst_entry, it only needs the fib
> entry to validate the gateway resolution and egress device. So,
> convert ip6_nh_lookup_table from ip6_pol_route to fib6_table_lookup
> and ip6_route_check_nh to use fib6_lookup over rt6_lookup.
>
> ip6_pol_route is a call to fib6_table_lookup and if successful a call
> to fib6_select_path. From there the exception cache is searched for an
> entry or a dst_entry is created to return to the caller. The exception
> entry is not relevant for gateway validation, so what matters are the
> calls to fib6_table_lookup and then fib6_select_path.
>
> Similarly, rt6_lookup can be replaced with a call to fib6_lookup with
> RT6_LOOKUP_F_IFACE set in flags. Again, the exception cache search is
> not relevant, only the lookup with path selection. The primary difference
> in the lookup paths is the use of rt6_select with fib6_lookup versus
> rt6_device_match with rt6_lookup. When you remove complexities in the
> rt6_select path, e.g.,
> 1. saddr is not set for gateway validation, so RT6_LOOKUP_F_HAS_SADDR
> is not relevant
> 2. rt6_check_neigh is not called so that removes the RT6_NUD_FAIL_DO_RR
> return and round-robin logic.
>
> the code paths are believed to be equivalent for the given use case -
> validate the gateway and optionally given the device. Furthermore, it
> aligns the validation with onlink code path and the lookup path actually
> used for rx and tx.
>
> Adjust the users, ip6_route_check_nh_onlink and ip6_route_check_nh to
> handle a fib6_info vs a rt6_info when performing validation checks.
>
> Existing selftests fib-onlink-tests.sh and fib_tests.sh are used to
> verify the changes.
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
Reviewed-by: Wei Wang <weiwan@google.com>
> ---
> v2
> - use in6_dev_get versus __in6_dev_get + in6_dev_hold (comment from Wei)
> - updated commit message
>
> net/ipv6/route.c | 118 ++++++++++++++++++++++++++-----------------------------
> 1 file changed, 56 insertions(+), 62 deletions(-)
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index be5e65c97652..5fe0fd6f2909 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -3144,10 +3144,9 @@ static int ip6_dst_gc(struct dst_ops *ops)
> return entries > rt_max_size;
> }
>
> -static struct rt6_info *ip6_nh_lookup_table(struct net *net,
> - struct fib6_config *cfg,
> - const struct in6_addr *gw_addr,
> - u32 tbid, int flags)
> +static int ip6_nh_lookup_table(struct net *net, struct fib6_config *cfg,
> + const struct in6_addr *gw_addr, u32 tbid,
> + int flags, struct fib6_result *res)
> {
> struct flowi6 fl6 = {
> .flowi6_oif = cfg->fc_ifindex,
> @@ -3155,25 +3154,23 @@ static struct rt6_info *ip6_nh_lookup_table(struct net *net,
> .saddr = cfg->fc_prefsrc,
> };
> struct fib6_table *table;
> - struct rt6_info *rt;
> + int err;
>
> table = fib6_get_table(net, tbid);
> if (!table)
> - return NULL;
> + return -EINVAL;
>
> if (!ipv6_addr_any(&cfg->fc_prefsrc))
> flags |= RT6_LOOKUP_F_HAS_SADDR;
>
> flags |= RT6_LOOKUP_F_IGNORE_LINKSTATE;
> - rt = ip6_pol_route(net, table, cfg->fc_ifindex, &fl6, NULL, flags);
>
> - /* if table lookup failed, fall back to full lookup */
> - if (rt == net->ipv6.ip6_null_entry) {
> - ip6_rt_put(rt);
> - rt = NULL;
> - }
> + err = fib6_table_lookup(net, table, cfg->fc_ifindex, &fl6, res, flags);
> + if (!err && res->f6i != net->ipv6.fib6_null_entry)
> + fib6_select_path(net, res, &fl6, cfg->fc_ifindex,
> + cfg->fc_ifindex != 0, NULL, flags);
>
> - return rt;
> + return err;
> }
>
> static int ip6_route_check_nh_onlink(struct net *net,
> @@ -3181,29 +3178,19 @@ static int ip6_route_check_nh_onlink(struct net *net,
> const struct net_device *dev,
> struct netlink_ext_ack *extack)
> {
> - u32 tbid = l3mdev_fib_table(dev) ? : RT_TABLE_MAIN;
> + u32 tbid = l3mdev_fib_table_rcu(dev) ? : RT_TABLE_MAIN;
> const struct in6_addr *gw_addr = &cfg->fc_gateway;
> - u32 flags = RTF_LOCAL | RTF_ANYCAST | RTF_REJECT;
> - struct fib6_info *from;
> - struct rt6_info *grt;
> + struct fib6_result res = {};
> int err;
>
> - err = 0;
> - grt = ip6_nh_lookup_table(net, cfg, gw_addr, tbid, 0);
> - if (grt) {
> - rcu_read_lock();
> - from = rcu_dereference(grt->from);
> - if (!grt->dst.error &&
> - /* ignore match if it is the default route */
> - from && !ipv6_addr_any(&from->fib6_dst.addr) &&
> - (grt->rt6i_flags & flags || dev != grt->dst.dev)) {
> - NL_SET_ERR_MSG(extack,
> - "Nexthop has invalid gateway or device mismatch");
> - err = -EINVAL;
> - }
> - rcu_read_unlock();
> -
> - ip6_rt_put(grt);
> + err = ip6_nh_lookup_table(net, cfg, gw_addr, tbid, 0, &res);
> + if (!err && !(res.fib6_flags & RTF_REJECT) &&
> + /* ignore match if it is the default route */
> + !ipv6_addr_any(&res.f6i->fib6_dst.addr) &&
> + (res.fib6_type != RTN_UNICAST || dev != res.nh->fib_nh_dev)) {
> + NL_SET_ERR_MSG(extack,
> + "Nexthop has invalid gateway or device mismatch");
> + err = -EINVAL;
> }
>
> return err;
> @@ -3216,47 +3203,50 @@ static int ip6_route_check_nh(struct net *net,
> {
> const struct in6_addr *gw_addr = &cfg->fc_gateway;
> struct net_device *dev = _dev ? *_dev : NULL;
> - struct rt6_info *grt = NULL;
> + int flags = RT6_LOOKUP_F_IFACE;
> + struct fib6_result res = {};
> int err = -EHOSTUNREACH;
>
> if (cfg->fc_table) {
> - int flags = RT6_LOOKUP_F_IFACE;
> -
> - grt = ip6_nh_lookup_table(net, cfg, gw_addr,
> - cfg->fc_table, flags);
> - if (grt) {
> - if (grt->rt6i_flags & RTF_GATEWAY ||
> - (dev && dev != grt->dst.dev)) {
> - ip6_rt_put(grt);
> - grt = NULL;
> - }
> - }
> + err = ip6_nh_lookup_table(net, cfg, gw_addr,
> + cfg->fc_table, flags, &res);
> + /* gw_addr can not require a gateway or resolve to a reject
> + * route. If a device is given, it must match the result.
> + */
> + if (err || res.fib6_flags & RTF_REJECT ||
> + res.nh->fib_nh_gw_family ||
> + (dev && dev != res.nh->fib_nh_dev))
> + err = -EHOSTUNREACH;
> }
>
> - if (!grt)
> - grt = rt6_lookup(net, gw_addr, NULL, cfg->fc_ifindex, NULL, 1);
> + if (err < 0) {
> + struct flowi6 fl6 = {
> + .flowi6_oif = cfg->fc_ifindex,
> + .daddr = *gw_addr,
> + };
>
> - if (!grt)
> - goto out;
> + err = fib6_lookup(net, cfg->fc_ifindex, &fl6, &res, flags);
> + if (err || res.fib6_flags & RTF_REJECT ||
> + res.nh->fib_nh_gw_family)
> + err = -EHOSTUNREACH;
> +
> + if (err)
> + return err;
> +
> + fib6_select_path(net, &res, &fl6, cfg->fc_ifindex,
> + cfg->fc_ifindex != 0, NULL, flags);
> + }
>
> + err = 0;
> if (dev) {
> - if (dev != grt->dst.dev) {
> - ip6_rt_put(grt);
> - goto out;
> - }
> + if (dev != res.nh->fib_nh_dev)
> + err = -EHOSTUNREACH;
> } else {
> - *_dev = dev = grt->dst.dev;
> - *idev = grt->rt6i_idev;
> + *_dev = dev = res.nh->fib_nh_dev;
> dev_hold(dev);
> - in6_dev_hold(grt->rt6i_idev);
> + *idev = in6_dev_get(dev);
> }
>
> - if (!(grt->rt6i_flags & RTF_GATEWAY))
> - err = 0;
> -
> - ip6_rt_put(grt);
> -
> -out:
> return err;
> }
>
> @@ -3297,11 +3287,15 @@ static int ip6_validate_gw(struct net *net, struct fib6_config *cfg,
> goto out;
> }
>
> + rcu_read_lock();
> +
> if (cfg->fc_flags & RTNH_F_ONLINK)
> err = ip6_route_check_nh_onlink(net, cfg, dev, extack);
> else
> err = ip6_route_check_nh(net, cfg, _dev, idev);
>
> + rcu_read_unlock();
> +
> if (err)
> goto out;
> }
> --
> 2.11.0
>
^ permalink raw reply
* Re: 4.19: Traced deadlock during xfrm_user module load
From: Florian Westphal @ 2019-06-25 16:53 UTC (permalink / raw)
To: Thomas Jarosch; +Cc: netdev, netfilter-devel, Juliana Rodrigueiro
In-Reply-To: <20190625155509.pgcxwgclqx3lfxxr@intra2net.com>
Thomas Jarosch <thomas.jarosch@intra2net.com> wrote:
> we're in the process of upgrading to kernel 4.19 and hit
> a very rare lockup on boot during "xfrm_user" module load.
> The tested kernel was 4.19.55.
>
> When the strongswan IPsec service starts, it loads the xfrm_user module.
> -> modprobe hangs forever.
>
> Also network services like ssh or apache stop responding,
> ICMP ping still works.
>
> By chance we had magic sysRq enabled and were able to get some meaningful stack
> traces. We've rebuilt the kernel with LOCKDEP + DEBUG_INFO + DEBUG_INFO_REDUCED,
> but so far failed to reproduce the issue even when hammering the suspected
> deadlock case. Though it's just hammering it for a few hours yet.
>
> Preliminary analysis:
>
> "modprobe xfrm_user":
> xfrm_user_init()
> register_pernet_subsys()
> -> grab pernet_ops_rwsem
> ..
> netlink_table_grab()
> calls schedule() as "nl_table_users" is non-zero
>
>
> conntrack netlink related program "info_iponline" does this in parallel:
> netlink_bind()
> netlink_lock_table() -> increases "nl_table_users"
> nfnetlink_bind()
> # does not unlock the table as it's locked by netlink_bind()
> __request_module()
> call_usermodehelper_exec()
>
>
> "modprobe nf_conntrack_netlink" runs and inits nf_conntrack_netlink:
> ctnetlink_init()
> register_pernet_subsys()
> -> blocks on "pernet_ops_rwsem" thanks to xfrm_user module
> -> schedule()
> -> deadlock forever
>
Thanks for this detailed analysis.
In this specific case I think this is enough:
diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 92077d459109..61ba92415480 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -578,7 +578,8 @@ static int nfnetlink_bind(struct net *net, int group)
ss = nfnetlink_get_subsys(type << 8);
rcu_read_unlock();
if (!ss)
- request_module("nfnetlink-subsys-%d", type);
+ request_module_nowait("nfnetlink-subsys-%d", type);
return 0;
}
#endif
^ permalink raw reply related
* [PATCH bpf] bpf: fix BPF_ALU32 | BPF_ARSH on BE arches
From: Jiong Wang @ 2019-06-25 16:41 UTC (permalink / raw)
To: alexei.starovoitov, daniel
Cc: yauheni.kaliuta, bpf, netdev, oss-drivers, Jiong Wang
Yauheni reported the following code do not work correctly on BE arches:
ALU_ARSH_X:
DST = (u64) (u32) ((*(s32 *) &DST) >> SRC);
CONT;
ALU_ARSH_K:
DST = (u64) (u32) ((*(s32 *) &DST) >> IMM);
CONT;
and are causing failure of test_verifier test 'arsh32 on imm 2' on BE
arches.
The code is taking address and interpreting memory directly, so is not
endianness neutral. We should instead perform standard C type casting on
the variable. A u64 to s32 conversion will drop the high 32-bit and reserve
the low 32-bit as signed integer, this is all we want.
Fixes: 2dc6b100f928 ("bpf: interpreter support BPF_ALU | BPF_ARSH")
Reported-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
---
kernel/bpf/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 080e2bb..f2148db 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1364,10 +1364,10 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
insn++;
CONT;
ALU_ARSH_X:
- DST = (u64) (u32) ((*(s32 *) &DST) >> SRC);
+ DST = (u64) (u32) (((s32) DST) >> SRC);
CONT;
ALU_ARSH_K:
- DST = (u64) (u32) ((*(s32 *) &DST) >> IMM);
+ DST = (u64) (u32) (((s32) DST) >> IMM);
CONT;
ALU64_ARSH_X:
(*(s64 *) &DST) >>= SRC;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net] net/sched: flower: fix infinite loop in fl_walk()
From: Davide Caratti @ 2019-06-25 16:23 UTC (permalink / raw)
To: Cong Wang
Cc: Vlad Buslov, David S. Miller, Linux Kernel Network Developers,
Lucas Bates
In-Reply-To: <6650f0da68982ffa5bb71a773c5a3d588bd972c4.camel@redhat.com>
On Tue, 2019-06-25 at 17:47 +0200, Davide Caratti wrote:
> On Thu, 2019-06-20 at 10:33 -0700, Cong Wang wrote:
> > On Thu, Jun 20, 2019 at 5:52 AM Davide Caratti <dcaratti@redhat.com> wrote:
> > > hello Cong, thanks for reading.
> > >
> > > On Wed, 2019-06-19 at 15:04 -0700, Cong Wang wrote:
> > > > On Wed, Jun 19, 2019 at 2:10 PM Davide Caratti <dcaratti@redhat.com> wrote:
> > > > > on some CPUs (e.g. i686), tcf_walker.cookie has the same size as the IDR.
> > > > > In this situation, the following script:
> > > > >
> > > > > # tc filter add dev eth0 ingress handle 0xffffffff flower action ok
> > > > > # tc filter show dev eth0 ingress
> > > > >
> > > > > results in an infinite loop.
>
> So, when the radix tree contains one slot with index equal to ULONG_MAX,
> whatever can be the value of 'id',
oops, this phrase is of course wrong. the value of 'id' matters to
determine the condition of the if().
> the condition in that if() will always
> be false (and the function will keep returning non-NULL, hence the
> infinite loop).
what I wanted to say is, when the radix tree contains a single slot with
index equal to ULONG_MAX, whatever value I put in 'id' the function will
always return a pointer to that slot.
--
davide
^ permalink raw reply
* Re: [PATCH v3 net] af_packet: Block execution of tasks waiting for transmit to complete in AF_PACKET
From: Neil Horman @ 2019-06-25 16:20 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, Matteo Croce, David S. Miller
In-Reply-To: <CAF=yD-KEZBds_SRDFnOjqvidW30E=NG-2X=hBdcGx_--PAmjew@mail.gmail.com>
On Tue, Jun 25, 2019 at 09:37:17AM -0400, Willem de Bruijn wrote:
> On Tue, Jun 25, 2019 at 7:03 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > On Mon, Jun 24, 2019 at 06:15:29PM -0400, Willem de Bruijn wrote:
> > > > > > + if (need_wait && !packet_next_frame(po, &po->tx_ring, TP_STATUS_SEND_REQUEST)) {
> > > > > > + po->wait_on_complete = 1;
> > > > > > + timeo = sock_sndtimeo(&po->sk, msg->msg_flags & MSG_DONTWAIT);
> > > > >
> > > > > This resets timeout on every loop. should only set above the loop once.
> > > > >
> > > > I explained exactly why I did that in the change log. Its because I reuse the
> > > > timeout variable to get the return value of the wait_for_complete call.
> > > > Otherwise I need to add additional data to the stack, which I don't want to do.
> > > > Sock_sndtimeo is an inline function and really doesn't add any overhead to this
> > > > path, so I see no reason not to reuse the variable.
> > >
> > > The issue isn't the reuse. It is that timeo is reset to sk_sndtimeo
> > > each time. Whereas wait_for_common and its variants return the
> > > number of jiffies left in case the loop needs to sleep again later.
> > >
> > > Reading sock_sndtimeo once and passing it to wait_.. repeatedly is a
> > > common pattern across the stack.
> > >
> > But those patterns are unique to those situations. For instance, in
> > tcp_sendmsg_locked, we aquire the value of the socket timeout, and use that to
> > wait for the entire message send operation to complete, which consists of
> > potentially several blocking operations (waiting for the tcp connection to be
> > established, waiting for socket memory, etc). In that situation we want to wait
> > for all of those operations to complete to send a single message, and fail if
> > they exceed the timeout in aggregate. The semantics are different with
> > AF_PACKET. In this use case, the message is in effect empty, and just used to
> > pass some control information. tpacket_snd, sends as many frames from the
> > memory mapped buffer as possible, and on each iteration we want to wait for the
> > specified timeout for those frames to complete sending. I think resetting the
> > timeout on each wait instance is the right way to go here.
>
> I disagree. If a SO_SNDTIMEO of a given time is set, the thread should
> not wait beyond that. Else what is the point of passing a specific
> duration in the syscall?
>
I can appreciate that, but you said yourself that you wanted to minimize this
change. The current behavior of AF_PACKET is to:
a) check for their being no more packets ready to send
b) if (a) is false we schedule() (nominally allowing other tasks to run)
c) when (b) is complete, check for additional frames to send, and exit if there
are not, otherwise, continue sending and waiting
In that model, a single task calling sendmsg can run in the kernel indefinately,
if userspace continues to fill the memory mapped buffer
Given that model, resetting the timeout on each call to wait_for_completion
keeps that behavior. In fact, if we allow the timeout value to get continuously
decremented, it will be possible for a call to sendmsg in this protocol to
_always_ return -ETIMEDOUT (if we keep the buffer full in userspace long enough,
then the sending task will eventually decrement timeo to zero, and force an
-ETIMEDOUT call).
I'm convinced that resetting the timeout here is the right way to go.
> Btw, we can always drop the timeo and go back to unconditional (bar
> signals) waiting.
>
We could, but it would be nice to implement the timeout feature here if
possible.
> >
> > > > > > @@ -2728,6 +2755,11 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > > > > > err = net_xmit_errno(err);
> > > > > > if (err && __packet_get_status(po, ph) ==
> > > > > > TP_STATUS_AVAILABLE) {
> > > > > > + /* re-init completion queue to avoid subsequent fallthrough
> > > > > > + * on a future thread calling wait_on_complete_interruptible_timeout
> > > > > > + */
> > > > > > + po->wait_on_complete = 0;
> > > > >
> > > > > If setting where sleeping, no need for resetting if a failure happens
> > > > > between those blocks.
> > > > >
> > > > > > + init_completion(&po->skb_completion);
> > > > >
> > > > > no need to reinit between each use?
> > > > >
> > > > I explained exactly why I did this in the comment above. We have to set
> > > > wait_for_complete prior to calling transmit, so as to ensure that we call
> > > > wait_for_completion before we exit the loop. However, in this error case, we
> > > > exit the loop prior to calling wait_for_complete, so we need to reset the
> > > > completion variable and the wait_for_complete flag. Otherwise we will be in a
> > > > case where, on the next entrace to this loop we will have a completion variable
> > > > with completion->done > 0, meaning the next wait will be a fall through case,
> > > > which we don't want.
> > >
> > > By moving back to the point where schedule() is called, hopefully this
> > > complexity automatically goes away. Same as my comment to the line
> > > immediately above.
> > >
> > Its going to change what the complexity is, actually. I was looking at this
> > last night, and I realized that your assertion that we could remove
> > packet_next_frame came at a cost. This is because we need to determine if we
> > have to wait before we call po->xmit, but we need to actually do the wait after
> > po->xmit
>
> Why? The existing method using schedule() doesn't.
>
Because the existing method using schedule doesn't have to rely on an
asynchronous event to wake the sending task back up. Specifically, we need to
set some state that indicates tpacket_destruct_skb should call complete, and
since tpacket_destruct_skb is called asynchronously in a separate execution
context, we need to ensure there is no race condition whereby we execute
tpacket_destruct_skb before we set the state that we also use to determine if we
should call wait_for_complete. ie:
1) tpacket_send_skb calls po->xmit
2) tpacket_send_skb loops around and checks to see if there are more packets to
send. If not and if need_wait is set we call wait_for_complete
If tpacket_destruct_skb is called after (2), we're fine. But if its called
between (1) and (2), then tpacket_destruct_skb won't call complete (because
wait_on_complete isn't set yet), causing a hang.
Because you wanted to remove packet_next_frame, we have no way to determine if
we're done sending frames prior to calling po->xmit, which is the point past
which tpacket_destruct_skb might be called, so now I have to find an alternate
state condition to key off of.
> Can we not just loop while sending and sleep immediately when
> __packet_get_status returns !TP_STATUS_AVAILABLE?
>
> I don't understand the need to probe the next packet to send instead
> of the current.
>
See above, we can definately check the return of ph at the top of the loop, and
sleep if its NULL, but in so doing we cant use po->wait_on_complete as a gating
variable because we've already called po->xmit. Once we call that function, if
we haven't already set po->wait_on_complete to true, its possible
tpacket_destruct_skb will already have been called before we get to the point
where we check wait_for_completion. That means we will wait on a completion
variable that never gets completed, so I need to find a new way to track weather
or not we are waiting. Its entirely doable, I'm sure, its just not as straight
forward as your making it out to be.
> This seems to be the crux of the disagreement. My guess is that it has
> to do with setting wait_on_complete, but I don't see the problem. It
> can be set immediately before going to sleep.
>
The reason has to do with the fact that
tpacket_destruct_skb can be called from ksoftirq context (i.e. it will be called
asynchrnously, not from the thread running in tpacket_snd). Condsider this
flow, assuming we do as you suggest, and just set po->wait_on_complete=1
immediately prior to calling wait_for_complete_interruptible_timeout:
1) tpacket_snd gets a frame, builds the skb for transmission
2) tpakcket_snd calls po->xmit(skb), sending the frame to the driver
3) tpacket_snd, iterates through back to the top of the loop, and determines
that we need to wait for the previous packet to complete
4) The driver gets a tx completion interrupt, interrupting the cpu on which we
are executing, and calls kfree_skb_irq on the skb we just transmitted
5) the ksoftirq runs on the cpu, calling kfree_skb on our skb
6) (5) calls skb->destruct (which is tpakcet_skb_destruct)
7) tpacket_skb_destruct executes, sees that po->wait_on_completion is zero, and
skips calling complete()
8) the thread running tpacket_snd gets scheduled back on the cpu and now sets
po->wait_on_complete, then calls wait_for_completion_interruptible_timeout, but
since the skb we are waiting to complete has already been freed, we will never
get the completion notification, and so we will wait for the maximal timeout,
which is absolutely not what we want.
Interestingly (Ironically), that control flow will never happen in the use case
I'm trying to fix here, because its SCHED_FIFO, and will run until such time as
we call wait_for_completion_interuptible_timeout, so in this case we're safe.
But in the nominal case, where the sending task is acturally SCHED_OTHER, the
above can aboslutely happen, and thats very broken.
> I don't meant to draw this out, btw, or add to your workload. If you
> prefer, I can take a stab at my (admittedly hand-wavy) suggestion.
>
No, I have another method in flight that I'm testing now, it removes the
po->wait_on_complete variable entirely, checking instead to make sure that
we've:
a) sent at least one frame
and
b) that we have a positive pending count
and
c) that we need to wait
and
d) that we have no more frames to send
This is why I was saying that your idea can be done, but it trades one form of
complexity for another.
Neil
> > (to ensure that wait_on_complete is set properly when the desructor is
> > called). By moving the wait to the top of the loop and getting rid of
> > packet_next_frame we now have a race condition in which we might call
> > tpacket_destruct_skb with wait_on_complete set to false, causing us to wait for
> > the maximum timeout erroneously. So I'm going to have to find a new way to
> > signal the need to call complete, which I think will introduce a different level
> > of complexity.
>
^ permalink raw reply
* Re: [PATCH net] vxlan: do not destroy fdb if register_netdevice() is failed
From: Taehee Yoo @ 2019-06-25 16:08 UTC (permalink / raw)
To: Roopa Prabhu; +Cc: David Miller, Netdev
In-Reply-To: <CAJieiUjri=-w2PqB9q5fEa=4jqkTWSfK0dUwnT7Cvxdo2sRRzg@mail.gmail.com>
On Tue, 25 Jun 2019 at 13:12, Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
>
Hi Roopa,
Thank you for the review!
> On Sun, Jun 23, 2019 at 7:18 PM Taehee Yoo <ap420073@gmail.com> wrote:
> >
> > On Mon, 24 Jun 2019 at 03:07, David Miller <davem@davemloft.net> wrote:
> > >
> >
> > Hi David,
> >
> > Thank you for the review!
> >
> > > From: Taehee Yoo <ap420073@gmail.com>
> > > Date: Thu, 20 Jun 2019 20:51:08 +0900
> > >
> > > > __vxlan_dev_create() destroys FDB using specific pointer which indicates
> > > > a fdb when error occurs.
> > > > But that pointer should not be used when register_netdevice() fails because
> > > > register_netdevice() internally destroys fdb when error occurs.
> > > >
> > > > In order to avoid un-registered dev's notification, fdb destroying routine
> > > > checks dev's register status before notification.
> > >
> > > Simply pass do_notify as false in this failure code path of __vxlan_dev_create(),
> > > thank you.
> >
> > Failure path of __vxlan_dev_create() can't handle do_notify in that case
> > because if register_netdevice() fails it internally calls
> > ->ndo_uninit() which is
> > vxlan_uninit().
> > vxlan_uninit() internally calls vxlan_fdb_delete_default() and it callls
> > vxlan_fdb_destroy().
> > do_notify of vxlan_fdb_destroy() in vxlan_fdb_delete_default() is always true.
> > So, failure path of __vxlan_dev_create() doesn't have any opportunity to
> > handle do_notify.
>
>
> I don't see register_netdevice calling ndo_uninit in case of all
> errors. In the case where it does not,
> does your patch leak the fdb entry ?.
>
> Wondering if we should just use vxlan_fdb_delete_default with a notify
> flag to delete the entry if exists.
> Will that help ?
>
> There is another commit that touched this code path:
> commit 6db9246871394b3a136cd52001a0763676563840
>
> Author: Petr Machata <petrm@mellanox.com>
> Date: Tue Dec 18 13:16:00 2018 +0000
> vxlan: Fix error path in __vxlan_dev_create()
I have checked up failure path of register_netdevice().
Yes, this patch leaks fdb entry.
There are 3 failure cases in the register_netdevice().
A. error occurs before calling ->ndo_init().
it doesn't call ->ndo_uninit().
B. error occurs after calling ->ndo_init().
it calls ->ndo_uninit() and dev->reg_state is NETREG_UNINITIALIZED.
C. error occurs after registering netdev. it calls rollback_registered().
rollback_registered() internally calls ->ndo_uninit()
and dev->reg_state is NETREG_UNREGISTERING.
A panic due to these problem could be fixed by using
vxlan_fdb_delete_default() with notify flag.
But notification problem could not be fixed clearly
because of the case C.
I don't have clear solution for the case C.
Please let me know, if you have any good idea for fixing the case C.
Thank you!
^ permalink raw reply
* [net-next 1/1] tipc: eliminate unnecessary skb expansion during retransmission
From: Jon Maloy @ 2019-06-25 16:08 UTC (permalink / raw)
To: davem, netdev
Cc: gordan.mihaljevic, tung.q.nguyen, hoang.h.le, jon.maloy,
canh.d.luu, ying.xue, tipc-discussion
We increase the allocated headroom for the buffer copies to be
retransmitted. This eliminates the need for the lower stack levels
(UDP/IP/L2) to expand the headroom in order to add their own headers.
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
net/tipc/link.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/tipc/link.c b/net/tipc/link.c
index af50b53..aa79bf8 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1125,7 +1125,7 @@ static int tipc_link_bc_retrans(struct tipc_link *l, struct tipc_link *r,
continue;
TIPC_SKB_CB(skb)->nxt_retr = jiffies + TIPC_BC_RETR_LIM;
}
- _skb = __pskb_copy(skb, MIN_H_SIZE, GFP_ATOMIC);
+ _skb = __pskb_copy(skb, LL_MAX_HEADER + MIN_H_SIZE, GFP_ATOMIC);
if (!_skb)
return 0;
hdr = buf_msg(_skb);
--
2.1.4
^ permalink raw reply related
* [PATCH] ipvs: allow tunneling with gre encapsulation
From: Vadim Fedorenko @ 2019-06-25 15:59 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Konstantin Khlebnikov, Wensong Zhang, Julian Anastasov,
Jozsef Kadlecsik, Florian Westphal, David S. Miller, netdev,
lvs-devel, netfilter-devel
windows real servers can handle gre tunnels, this patch allows
gre encapsulation with the tunneling method, thereby letting ipvs
be load balancer for windows-based services
Signed-off-by: Vadim Fedorenko <vfedorenko@yandex-team.ru>
---
include/uapi/linux/ip_vs.h | 1 +
net/netfilter/ipvs/ip_vs_xmit.c | 76 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 77 insertions(+)
diff --git a/include/uapi/linux/ip_vs.h b/include/uapi/linux/ip_vs.h
index e4f1806..4102ddc 100644
--- a/include/uapi/linux/ip_vs.h
+++ b/include/uapi/linux/ip_vs.h
@@ -128,6 +128,7 @@
enum {
IP_VS_CONN_F_TUNNEL_TYPE_IPIP = 0, /* IPIP */
IP_VS_CONN_F_TUNNEL_TYPE_GUE, /* GUE */
+ IP_VS_CONN_F_TUNNEL_TYPE_GRE, /* GRE */
IP_VS_CONN_F_TUNNEL_TYPE_MAX,
};
diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
index 71fc6d6..fad3f33 100644
--- a/net/netfilter/ipvs/ip_vs_xmit.c
+++ b/net/netfilter/ipvs/ip_vs_xmit.c
@@ -29,6 +29,7 @@
#include <linux/tcp.h> /* for tcphdr */
#include <net/ip.h>
#include <net/gue.h>
+#include <net/gre.h>
#include <net/tcp.h> /* for csum_tcpudp_magic */
#include <net/udp.h>
#include <net/icmp.h> /* for icmp_send */
@@ -389,6 +390,12 @@ static inline bool decrement_ttl(struct netns_ipvs *ipvs,
skb->ip_summed == CHECKSUM_PARTIAL)
mtu -= GUE_PLEN_REMCSUM + GUE_LEN_PRIV;
}
+ if (dest->tun_type == IP_VS_CONN_F_TUNNEL_TYPE_GRE) {
+ __be16 tflags = 0;
+ if (dest->tun_flags & IP_VS_TUNNEL_ENCAP_FLAG_CSUM)
+ tflags |= TUNNEL_CSUM;
+ mtu -= gre_calc_hlen(tflags);
+ }
if (mtu < 68) {
IP_VS_DBG_RL("%s(): mtu less than 68\n", __func__);
goto err_put;
@@ -549,6 +556,12 @@ static inline bool decrement_ttl(struct netns_ipvs *ipvs,
skb->ip_summed == CHECKSUM_PARTIAL)
mtu -= GUE_PLEN_REMCSUM + GUE_LEN_PRIV;
}
+ if (dest->tun_type == IP_VS_CONN_F_TUNNEL_TYPE_GRE) {
+ __be16 tflags = 0;
+ if (dest->tun_flags & IP_VS_TUNNEL_ENCAP_FLAG_CSUM)
+ tflags |= TUNNEL_CSUM;
+ mtu -= gre_calc_hlen(tflags);
+ }
if (mtu < IPV6_MIN_MTU) {
IP_VS_DBG_RL("%s(): mtu less than %d\n", __func__,
IPV6_MIN_MTU);
@@ -1079,6 +1092,24 @@ static inline int __tun_gso_type_mask(int encaps_af, int
orig_af)
return 0;
}
+static void
+ipvs_gre_encap(struct net *net, struct sk_buff *skb,
+ struct ip_vs_conn *cp, __u8 *next_protocol)
+{
+ size_t hdrlen;
+ __be16 tflags = 0;
+ __be16 proto = *next_protocol == IPPROTO_IPIP ? htons(ETH_P_IP) :
htons(ETH_P_IPV6);
+
+ if (cp->dest->tun_flags & IP_VS_TUNNEL_ENCAP_FLAG_CSUM)
+ tflags |= TUNNEL_CSUM;
+
+ hdrlen = gre_calc_hlen(tflags);
+
+ gre_build_header(skb, hdrlen, tflags, proto, 0, 0);
+
+ *next_protocol = IPPROTO_GRE;
+}
+
/*
* IP Tunneling transmitter
*
@@ -1153,6 +1184,18 @@ static inline int __tun_gso_type_mask(int encaps_af, int
orig_af)
max_headroom += sizeof(struct udphdr) + gue_hdrlen;
}
+ if (tun_type == IP_VS_CONN_F_TUNNEL_TYPE_GRE) {
+ size_t gre_hdrlen;
+ __be16 tflags = 0;
+
+ if (tun_flags & IP_VS_TUNNEL_ENCAP_FLAG_CSUM)
+ tflags |= TUNNEL_CSUM;
+
+ gre_hdrlen = gre_calc_hlen(tflags);
+
+ max_headroom += gre_hdrlen;
+ }
+
/* We only care about the df field if sysctl_pmtu_disc(ipvs) is set */
dfp = sysctl_pmtu_disc(ipvs) ? &df : NULL;
skb = ip_vs_prepare_tunneled_skb(skb, cp->af, max_headroom,
@@ -1174,6 +1217,13 @@ static inline int __tun_gso_type_mask(int encaps_af, int
orig_af)
}
}
+ if (tun_type == IP_VS_CONN_F_TUNNEL_TYPE_GRE) {
+ if (tun_flags & IP_VS_TUNNEL_ENCAP_FLAG_CSUM)
+ gso_type |= SKB_GSO_GRE_CSUM;
+ else
+ gso_type |= SKB_GSO_GRE;
+ }
+
if (iptunnel_handle_offloads(skb, gso_type))
goto tx_error;
@@ -1194,6 +1244,9 @@ static inline int __tun_gso_type_mask(int encaps_af, int
orig_af)
udp_set_csum(!check, skb, saddr, cp->daddr.ip, skb->len);
}
+ if (tun_type == IP_VS_CONN_F_TUNNEL_TYPE_GRE) {
+ ipvs_gre_encap(net, skb, cp, &next_protocol);
+ }
skb_push(skb, sizeof(struct iphdr));
skb_reset_network_header(skb);
@@ -1289,6 +1342,18 @@ static inline int __tun_gso_type_mask(int encaps_af, int
orig_af)
max_headroom += sizeof(struct udphdr) + gue_hdrlen;
}
+ if (tun_type == IP_VS_CONN_F_TUNNEL_TYPE_GRE) {
+ size_t gre_hdrlen;
+ __be16 tflags = 0;
+
+ if (tun_flags & IP_VS_TUNNEL_ENCAP_FLAG_CSUM)
+ tflags |= TUNNEL_CSUM;
+
+ gre_hdrlen = gre_calc_hlen(tflags);
+
+ max_headroom += gre_hdrlen;
+ }
+
skb = ip_vs_prepare_tunneled_skb(skb, cp->af, max_headroom,
&next_protocol, &payload_len,
&dsfield, &ttl, NULL);
@@ -1308,6 +1373,13 @@ static inline int __tun_gso_type_mask(int encaps_af, int
orig_af)
}
}
+ if (tun_type == IP_VS_CONN_F_TUNNEL_TYPE_GRE) {
+ if (tun_flags & IP_VS_TUNNEL_ENCAP_FLAG_CSUM)
+ gso_type |= SKB_GSO_GRE_CSUM;
+ else
+ gso_type |= SKB_GSO_GRE;
+ }
+
if (iptunnel_handle_offloads(skb, gso_type))
goto tx_error;
@@ -1328,6 +1400,10 @@ static inline int __tun_gso_type_mask(int encaps_af, int
orig_af)
udp6_set_csum(!check, skb, &saddr, &cp->daddr.in6, skb->len);
}
+ if (tun_type == IP_VS_CONN_F_TUNNEL_TYPE_GRE) {
+ ipvs_gre_encap(net, skb, cp, &next_protocol);
+ }
+
skb_push(skb, sizeof(struct ipv6hdr));
skb_reset_network_header(skb);
memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt));
--
1.9.1
^ permalink raw reply related
* 4.19: Traced deadlock during xfrm_user module load
From: Thomas Jarosch @ 2019-06-25 15:55 UTC (permalink / raw)
To: netdev; +Cc: netfilter-devel, Juliana Rodrigueiro
Hi all,
we're in the process of upgrading to kernel 4.19 and hit
a very rare lockup on boot during "xfrm_user" module load.
The tested kernel was 4.19.55.
When the strongswan IPsec service starts, it loads the xfrm_user module.
-> modprobe hangs forever.
Also network services like ssh or apache stop responding,
ICMP ping still works.
By chance we had magic sysRq enabled and were able to get some meaningful stack
traces. We've rebuilt the kernel with LOCKDEP + DEBUG_INFO + DEBUG_INFO_REDUCED,
but so far failed to reproduce the issue even when hammering the suspected
deadlock case. Though it's just hammering it for a few hours yet.
Preliminary analysis:
"modprobe xfrm_user":
xfrm_user_init()
register_pernet_subsys()
-> grab pernet_ops_rwsem
..
netlink_table_grab()
calls schedule() as "nl_table_users" is non-zero
conntrack netlink related program "info_iponline" does this in parallel:
netlink_bind()
netlink_lock_table() -> increases "nl_table_users"
nfnetlink_bind()
# does not unlock the table as it's locked by netlink_bind()
__request_module()
call_usermodehelper_exec()
"modprobe nf_conntrack_netlink" runs and inits nf_conntrack_netlink:
ctnetlink_init()
register_pernet_subsys()
-> blocks on "pernet_ops_rwsem" thanks to xfrm_user module
-> schedule()
-> deadlock forever
Full stack traces:
strongswan starts on boot and loads "xfrm_user":
kernel: modprobe D 0 3825 3762 0x80000080
kernel: Call Trace:
kernel: __schedule+0x188/0x4d0
kernel: ? add_wait_queue_exclusive+0x4d/0x60
kernel: schedule+0x21/0x70
kernel: netlink_table_grab+0x9a/0xf0
kernel: ? wake_up_q+0x60/0x60
kernel: __netlink_kernel_create+0x15f/0x1e0
kernel: xfrm_user_net_init+0x45/0x80 [xfrm_user]
kernel: ? xfrm_user_net_exit+0x60/0x60 [xfrm_user]
kernel: ops_init+0x68/0x100
kernel: ? vprintk_emit+0x9e/0x1a0
kernel: ? 0xf826c000
kernel: ? xfrm_dump_sa+0x120/0x120 [xfrm_user]
kernel: register_pernet_operations+0xef/0x1d0
kernel: ? 0xf826c000
kernel: register_pernet_subsys+0x1c/0x30
kernel: xfrm_user_init+0x18/0x1000 [xfrm_user]
kernel: do_one_initcall+0x44/0x190
kernel: ? free_unref_page_commit+0x6a/0xd0
kernel: do_init_module+0x46/0x1c0
kernel: load_module+0x1dc1/0x2400
kernel: sys_init_module+0xed/0x120
kernel: do_fast_syscall_32+0x7a/0x200
kernel: entry_SYSENTER_32+0x6b/0xbe
Another program triggers a conntrack netlink operation in parallel:
kernel: info_iponline D 0 3787 3684 0x00000084
kernel: Call Trace:
kernel: __schedule+0x188/0x4d0
kernel: schedule+0x21/0x70
kernel: schedule_timeout+0x195/0x260
kernel: ? wake_up_process+0xf/0x20
kernel: ? insert_work+0x86/0xa0
kernel: wait_for_common+0xe2/0x190
kernel: ? wake_up_q+0x60/0x60
kernel: wait_for_completion_killable+0x12/0x30
kernel: call_usermodehelper_exec+0xda/0x170
kernel: __request_module+0x115/0x2e0
kernel: ? __wake_up_common_lock+0x7a/0xa0
kernel: ? __wake_up+0xd/0x20
kernel: nfnetlink_bind+0x28/0x53 [nfnetlink]
kernel: netlink_bind+0x138/0x300
kernel: ? netlink_setsockopt+0x320/0x320
kernel: __sys_bind+0x65/0xb0
kernel: ? __audit_syscall_exit+0x1fb/0x270
kernel: ? __audit_syscall_entry+0xad/0xf0
kernel: sys_socketcall+0x2f0/0x350
kernel: ? _cond_resched+0x12/0x40
kernel: do_fast_syscall_32+0x7a/0x200
kernel: entry_SYSENTER_32+0x6b/0xbe
This triggers a module load by the kernel:
kernel: modprobe D 0 3827 1578 0x80000080
kernel: Call Trace:
kernel: __schedule+0x188/0x4d0
kernel: schedule+0x21/0x70
kernel: rwsem_down_write_failed+0xf5/0x210
kernel: ? 0xf8283000
kernel: call_rwsem_down_write_failed+0x9/0xc
kernel: down_write+0x18/0x30
kernel: register_pernet_subsys+0x10/0x30
kernel: ctnetlink_init+0x48/0x1000 [nf_conntrack_netlink]
kernel: do_one_initcall+0x44/0x190
kernel: ? free_unref_page_commit+0x6a/0xd0
kernel: do_init_module+0x46/0x1c0
kernel: load_module+0x1dc1/0x2400
kernel: sys_init_module+0xed/0x120
kernel: do_fast_syscall_32+0x7a/0x200
kernel: entry_SYSENTER_32+0x6b/0xbe
Other network related operations are deadlocked in netlink_table_grab():
kernel: ntpd D 0 3831 3830 0x00000080
kernel: Call Trace:
kernel: __schedule+0x188/0x4d0
kernel: ? add_wait_queue_exclusive+0x4d/0x60
kernel: schedule+0x21/0x70
kernel: netlink_table_grab+0x9a/0xf0
kernel: ? wake_up_q+0x60/0x60
kernel: netlink_release+0x14e/0x460
kernel: __sock_release+0x2d/0xb0
kernel: sock_close+0xd/0x20
kernel: __fput+0x93/0x1c0
kernel: ____fput+0x8/0x10
kernel: task_work_run+0x82/0xa0
kernel: exit_to_usermode_loop+0x8d/0x90
kernel: do_fast_syscall_32+0x1a7/0x200
kernel: entry_SYSENTER_32+0x6b/0xbe
The easy workaround for us is to load the conntrack netlink
modules before starting strongswan. Since we use classic init,
this works. In parallel booting systemd world, people might
still hit the issue, so it's worth fixing.
The related function netlink_create() unlocks the table
before requesting modules and locks it afterwards again.
We guess it's racy to call netlink_unlock_table()
before doing the
err = nlk->netlink_bind(net, group + 1);
call in netlink_bind() ?
What would be the best fix here?
Best regards,
Thomas Jarosch and Juliana Rodrigueiro
^ permalink raw reply
* Re: Warnings generated from tcp_sacktag_write_queue.
From: Chinmay Agarwal @ 2019-06-25 15:57 UTC (permalink / raw)
To: Eric Dumazet, netdev; +Cc: sharathv, kapandey
In-Reply-To: <ab6bb900-e9b7-f2b2-0a56-d1c9e14d2db6@gmail.com>
On Tue, Jun 25, 2019 at 04:24:14PM +0200, Eric Dumazet wrote:
>
>
> On 6/25/19 6:07 AM, Chinmay Agarwal wrote:
> > Dear All,
> >
> > We are hitting the following WARN_ON condition:
> >
> > WARN_ON((int)tcp_packets_in_flight(tp) < 0);
> >
> > tcp_packets_in_flight = packets_out –( lost_out +
> > sacked_out ) + retrans_out (This value is coming -ve)
> >
> > The tcp socket being used is in fin_wait_1 state.
> > The values for variables just before the crash:
> > packets_out = 0,
> > retrans_out = 28,
> > lost_out = 38,
> > sacked_out = 8
> >
> >
> > The only place I can find the packets_out value being set as 0 is:
> >
> > void tcp_write_queue_purge(struct sock *sk)
> > {
> > ...
> >
> > tcp_sk(sk)->packets_out = 0;
> > inet_csk(sk)->icsk_backoff = 0;
> > }
> >
> > Is there some code flow where packets_out can be set to 0 and other
> > values can remain unchanged?
> > If not, is there some scenario which may lead to "tcp_write_queue_purge"
> > called and not followed up by "tcp_clear_retrans"?
> >
> > According to my understanding we should call "tcp_clear_retrans" after
> > setting packets_out to 0.
> >
> > [ 1950.556150] Call trace:
> > [ 1950.558689] tcp_sacktag_write_queue+0x704/0x72c
> > [ 1950.561313] init: Untracked pid 10745 exited with status 0
> > [ 1950.563441] tcp_ack+0x3a4/0xd40
> > [ 1950.563447] tcp_rcv_state_process+0x1e8/0xbbc
> > [ 1950.563457] tcp_v4_do_rcv+0x18c/0x1cc
> > [ 1950.563461] tcp_v4_rcv+0x84c/0x8a8
> > [ 1950.563471] ip_protocol_deliver_rcu+0xdc/0x190
> > [ 1950.563474] ip_local_deliver_finish+0x64/0x80
> > [ 1950.563479] ip_local_deliver+0xc4/0xf8
> > [ 1950.563482] ip_rcv_finish+0x214/0x2e0
> > [ 1950.563486] ip_rcv+0x2fc/0x39c
> > [ 1950.563496] __netif_receive_skb_core+0x698/0x84c
> > [ 1950.563499] __netif_receive_skb+0x3c/0x7c
> > [ 1950.563503] process_backlog+0x98/0x148
> > [ 1950.563506] net_rx_action+0x128/0x388
> > [ 1950.563519] __do_softirq+0x20c/0x3f0
> > [ 1950.563528] irq_exit+0x9c/0xa8
> > [ 1950.563536] handle_IPI+0x174/0x278
> > [ 1950.563540] gic_handle_irq+0x124/0x1c0
> > [ 1950.563544] el1_irq+0xb4/0x12c
> > [ 1950.563556] lpm_cpuidle_enter+0x3f4/0x430
> > [ 1950.563561] cpuidle_enter_state+0x124/0x25c
> > [ 1950.563565] cpuidle_enter+0x30/0x40
> > [ 1950.563575] call_cpuidle+0x3c/0x60
> > [ 1950.563579] do_idle+0x190/0x228
> > [ 1950.563583] cpu_startup_entry+0x24/0x28
> > [ 1950.563588] secondary_start_kernel+0x12c/0x138
> >
>
>
> You do not provide what exact kernel version you are using,
> this is probably the most important information we need.
>
The kernel version used is 4.14.
^ permalink raw reply
* [net-next 1/1] tipc: simplify stale link failure criteria
From: Jon Maloy @ 2019-06-25 15:36 UTC (permalink / raw)
To: davem, netdev
Cc: gordan.mihaljevic, tung.q.nguyen, hoang.h.le, jon.maloy,
canh.d.luu, ying.xue, tipc-discussion
In commit a4dc70d46cf1 ("tipc: extend link reset criteria for stale
packet retransmission") we made link retransmission failure events
dependent on the link tolerance, and not only of the number of failed
retransmission attempts, as we did earlier. This works well. However,
keeping the original, additional criteria of 99 failed retransmissions
is now redundant, and may in some cases lead to failure detection
times in the order of minutes instead of the expected 1.5 sec link
tolerance value.
We now remove this criteria altogether.
Acked-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
net/tipc/link.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/net/tipc/link.c b/net/tipc/link.c
index bcfb0a4..af50b53 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -107,7 +107,6 @@ struct tipc_stats {
* @backlogq: queue for messages waiting to be sent
* @snt_nxt: next sequence number to use for outbound messages
* @prev_from: sequence number of most previous retransmission request
- * @stale_cnt: counter for number of identical retransmit attempts
* @stale_limit: time when repeated identical retransmits must force link reset
* @ackers: # of peers that needs to ack each packet before it can be released
* @acked: # last packet acked by a certain peer. Used for broadcast.
@@ -167,7 +166,6 @@ struct tipc_link {
u16 snd_nxt;
u16 prev_from;
u16 window;
- u16 stale_cnt;
unsigned long stale_limit;
/* Reception */
@@ -910,7 +908,6 @@ void tipc_link_reset(struct tipc_link *l)
l->acked = 0;
l->silent_intv_cnt = 0;
l->rst_cnt = 0;
- l->stale_cnt = 0;
l->bc_peer_is_up = false;
memset(&l->mon_state, 0, sizeof(l->mon_state));
tipc_link_reset_stats(l);
@@ -1068,8 +1065,7 @@ static bool link_retransmit_failure(struct tipc_link *l, struct tipc_link *r,
if (r->prev_from != from) {
r->prev_from = from;
r->stale_limit = jiffies + msecs_to_jiffies(r->tolerance);
- r->stale_cnt = 0;
- } else if (++r->stale_cnt > 99 && time_after(jiffies, r->stale_limit)) {
+ } else if (time_after(jiffies, r->stale_limit)) {
pr_warn("Retransmission failure on link <%s>\n", l->name);
link_print(l, "State of link ");
pr_info("Failed msg: usr %u, typ %u, len %u, err %u\n",
@@ -1515,7 +1511,6 @@ int tipc_link_rcv(struct tipc_link *l, struct sk_buff *skb,
/* Forward queues and wake up waiting users */
if (likely(tipc_link_release_pkts(l, msg_ack(hdr)))) {
- l->stale_cnt = 0;
tipc_link_advance_backlog(l, xmitq);
if (unlikely(!skb_queue_empty(&l->wakeupq)))
link_prepare_wakeup(l);
@@ -2584,7 +2579,7 @@ int tipc_link_dump(struct tipc_link *l, u16 dqueues, char *buf)
i += scnprintf(buf + i, sz - i, " %u", l->silent_intv_cnt);
i += scnprintf(buf + i, sz - i, " %u", l->rst_cnt);
i += scnprintf(buf + i, sz - i, " %u", l->prev_from);
- i += scnprintf(buf + i, sz - i, " %u", l->stale_cnt);
+ i += scnprintf(buf + i, sz - i, " %u", 0);
i += scnprintf(buf + i, sz - i, " %u", l->acked);
list = &l->transmq;
--
2.1.4
^ permalink raw reply related
* Re: [PATCH net] net/sched: flower: fix infinite loop in fl_walk()
From: Davide Caratti @ 2019-06-25 15:47 UTC (permalink / raw)
To: Cong Wang
Cc: Vlad Buslov, David S. Miller, Linux Kernel Network Developers,
Lucas Bates
In-Reply-To: <CAM_iQpVVMBUdhv3o=doLhpWxee91zUPKjAOtUwryUEj0pfowdg@mail.gmail.com>
On Thu, 2019-06-20 at 10:33 -0700, Cong Wang wrote:
> On Thu, Jun 20, 2019 at 5:52 AM Davide Caratti <dcaratti@redhat.com> wrote:
> > hello Cong, thanks for reading.
> >
> > On Wed, 2019-06-19 at 15:04 -0700, Cong Wang wrote:
> > > On Wed, Jun 19, 2019 at 2:10 PM Davide Caratti <dcaratti@redhat.com> wrote:
> > > > on some CPUs (e.g. i686), tcf_walker.cookie has the same size as the IDR.
> > > > In this situation, the following script:
> > > >
> > > > # tc filter add dev eth0 ingress handle 0xffffffff flower action ok
> > > > # tc filter show dev eth0 ingress
> > > >
> > > > results in an infinite loop.
[...]
> I am not sure it is better to handle this overflow inside idr_get_next_ul()
> or just let its callers to handle it. According to the comments above
> idr_get_next_ul() it sounds like it is not expected to overflow, so...
>
> diff --git a/lib/idr.c b/lib/idr.c
> index c34e256d2f01..a38f5e391cec 100644
> --- a/lib/idr.c
> +++ b/lib/idr.c
> @@ -267,6 +267,9 @@ void *idr_get_next_ul(struct idr *idr, unsigned
> long *nextid)
> if (!slot)
> return NULL;
>
> + /* overflow */
> + if (iter.index < id)
> + return NULL;
> *nextid = iter.index + base;
> return rcu_dereference_raw(*slot);
> }
hello Cong,
I tested the above patch, but I still see the infinite loop on kernel
5.2.0-0.rc5.git0.1.fc31.i686 .
idr_get_next_ul() returns the entry in the radix tree which is greater or
equal to '*nextid' (which has the same value as 'id' in the above hunk).
So, when the radix tree contains one slot with index equal to ULONG_MAX,
whatever can be the value of 'id', the condition in that if() will always
be false (and the function will keep returning non-NULL, hence the
infinite loop).
I also tried this:
if (iter.index == id && id == ULONG_MAX) {
return NULL;
}
it fixes the infinite loop, but it clearly breaks the function semantic
(and anyway, it's not sufficient to fix my test, at least with cls_flower
it still dumps the entry with id 0xffffffff several times). I'm for
fixing the callers of idr_get_next_ul(), and in details:
- apply this patch for cls_flower
- change tcf_dump_walker() in act_api.c as follows, and add a TDC testcase
for 'gact'.
index 4e5d2e9ace5d..f34888c8a952 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -228,8 +228,11 @@ static int tcf_dump_walker(struct tcf_idrinfo
*idrinfo, struct sk_buff *skb,
idr_for_each_entry_ul(idr, p, id) {
index++;
- if (index < s_i)
+ if (index < s_i) {
+ if (id == ULONG_MAX)
+ break;
continue;
+ }
if (jiffy_since &&
time_after(jiffy_since,
WDYT?
thanks a lot,
--
davide
^ permalink raw reply related
* Re: [PATCH v2] bpf: fix uapi bpf_prog_info fields alignment
From: Dmitry V. Levin @ 2019-06-25 15:36 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Baruch Siach, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Network Development,
bpf, Arnd Bergmann, linux-arch, Jiri Olsa, Geert Uytterhoeven,
Linus Torvalds
In-Reply-To: <CAADnVQJNLk7tAHRHr7V7ugvCX9iCjaH4_vS9YuNWcMpwnA6ZyA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2715 bytes --]
On Tue, Jun 25, 2019 at 08:19:35AM -0700, Alexei Starovoitov wrote:
> On Tue, Jun 25, 2019 at 8:08 AM Dmitry V. Levin <ldv@altlinux.org> wrote:
> > On Tue, Jun 25, 2019 at 07:16:55AM -0700, Alexei Starovoitov wrote:
> > > On Tue, Jun 25, 2019 at 4:07 AM Baruch Siach <baruch@tkos.co.il> wrote:
> > > >
> > > > Merge commit 1c8c5a9d38f60 ("Merge
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next") undid the
> > > > fix from commit 36f9814a494 ("bpf: fix uapi hole for 32 bit compat
> > > > applications") by taking the gpl_compatible 1-bit field definition from
> > > > commit b85fab0e67b162 ("bpf: Add gpl_compatible flag to struct
> > > > bpf_prog_info") as is. That breaks architectures with 16-bit alignment
> > > > like m68k. Embed gpl_compatible into an anonymous union with 32-bit pad
> > > > member to restore alignment of following fields.
> > > >
> > > > Thanks to Dmitry V. Levin his analysis of this bug history.
> > > >
> > > > Cc: Jiri Olsa <jolsa@kernel.org>
> > > > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > > > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > > > ---
> > > > v2:
> > > > Use anonymous union with pad to make it less likely to break again in
> > > > the future.
> > > > ---
> > > > include/uapi/linux/bpf.h | 5 ++++-
> > > > tools/include/uapi/linux/bpf.h | 5 ++++-
> > > > 2 files changed, 8 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > > index a8b823c30b43..766eae02d7ae 100644
> > > > --- a/include/uapi/linux/bpf.h
> > > > +++ b/include/uapi/linux/bpf.h
> > > > @@ -3142,7 +3142,10 @@ struct bpf_prog_info {
> > > > __aligned_u64 map_ids;
> > > > char name[BPF_OBJ_NAME_LEN];
> > > > __u32 ifindex;
> > > > - __u32 gpl_compatible:1;
> > > > + union {
> > > > + __u32 gpl_compatible:1;
> > > > + __u32 pad;
> > > > + };
> > >
> > > Nack for the reasons explained in the previous thread
> > > on the same subject.
> > > Why cannot you go with earlier suggestion of _u32 :31; ?
> >
> > By the way, why not use aligned types as suggested by Geert?
> > They are already used for other members of struct bpf_prog_info anyway.
> >
> > FWIW, we use aligned types for bpf in strace and that approach
> > proved to be more robust than manual padding.
>
> because __aligned_u64 is used for pointers.
Does the fact that __aligned_u64 is used for pointers mean that
__aligned_u64 should not be used for anything but pointers?
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: selftests: bpf: test_libbpf.sh failed at file test_l4lb.o
From: Dan Rue @ 2019-06-25 15:31 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andrii Nakryiko, Naresh Kamboju,
open list:KERNEL SELFTEST FRAMEWORK, bpf, Netdev, open list, Xdp,
David S. Miller, Daniel Borkmann, Alexei Starovoitov, Martin Lau,
Yonghong Song, john fastabend, Jesper Dangaard Brouer,
Jakub Kicinski, Shuah Khan
In-Reply-To: <CAADnVQKZycXgSw6C0qa7g0y=W3xRhM_4Rqcj7ZzL=rGh_n4mgA@mail.gmail.com>
On Mon, Jun 24, 2019 at 12:58:15PM -0700, Alexei Starovoitov wrote:
> On Mon, Jun 24, 2019 at 12:53 PM Dan Rue <dan.rue@linaro.org> wrote:
> >
> > I would say if it's not possible to check at runtime, and it requires
> > clang 9.0, that this test should not be enabled by default.
>
> The latest clang is the requirement.
> If environment has old clang or no clang at all these tests will be failing.
Hi Alexei!
I'm not certain if I'm interpreting you as you intended, but it sounds
like you're telling me that if the test build environment does not use
'latest clang' (i guess latest as of today?), that these tests will
fail, and that is how it is going to be. If I have that wrong, please
correct me and disregard the rest of my message.
Please understand where we are coming from. We (and many others) run
thousands of tests from a lot of test frameworks, and so our environment
often has mutually exclusive requirements when it comes to things like
toolchain selection.
We believe, strongly, that a test should not emit a "fail" for a missing
requirement. Fail is a serious thing, and should be reserved for an
actual issue that needs to be investigated, reported, and fixed.
This is how we treat test failures - we investigate, report, and fix
them when possible. When they're not real failures, we waste our time
(and yours, in this case).
By adding the tests to TEST_GEN_PROGS, you're adding them to the general
test set that those of us running test farms try to run continuously
across a wide range of hardware environments and kernel branches.
My suggestion is that if you do not want us running them, don't add them
to TEST_GEN_PROGS. I thought the suggestion of testing for adequate
clang support and adding them conditionally at build-time was an idea
worth consideration.
Thanks,
Dan
--
Linaro - Kernel Validation
^ 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