* [PATCH bpf-next 0/2] bpftool improvements for xlated dump
From: Daniel Borkmann @ 2017-12-20 12:42 UTC (permalink / raw)
To: alexei.starovoitov
Cc: netdev, jakub.kicinski, quentin.monnet, Daniel Borkmann
This work adds correlation of maps and calls into the bpftool
xlated dump in order to help debugging and introspection of
loaded BPF progs. First patch makes kallsyms work on subprogs
with bpf calls, and second implements the actual correlation.
Details and example output can be found in the 2nd patch.
Thanks a lot!
Daniel Borkmann (2):
bpf: fix kallsyms handling for subprogs
bpf: allow for correlation of maps and helpers in dump
include/linux/filter.h | 9 +++
kernel/bpf/core.c | 4 +-
kernel/bpf/disasm.c | 65 ++++++++++++++---
kernel/bpf/disasm.h | 29 ++++++--
kernel/bpf/syscall.c | 93 ++++++++++++++++++++++--
kernel/bpf/verifier.c | 33 +++++++--
tools/bpf/bpftool/prog.c | 181 ++++++++++++++++++++++++++++++++++++++++++++---
7 files changed, 379 insertions(+), 35 deletions(-)
--
2.9.5
^ permalink raw reply
* [PATCH bpf-next 1/2] bpf: fix kallsyms handling for subprogs
From: Daniel Borkmann @ 2017-12-20 12:42 UTC (permalink / raw)
To: alexei.starovoitov
Cc: netdev, jakub.kicinski, quentin.monnet, Daniel Borkmann
In-Reply-To: <20171220124257.4512-1-daniel@iogearbox.net>
Right now kallsyms handling is not working with JITed subprogs.
The reason is that when in 1c2a088a6626 ("bpf: x64: add JIT support
for multi-function programs") in jit_subprogs() they are passed
to bpf_prog_kallsyms_add(), then their prog type is 0, which BPF
core will think it's a cBPF program as only cBPF programs have a
0 type. Thus, they need to inherit the type from the main prog.
Once that is fixed, they are indeed added to the BPF kallsyms
infra, but their tag is 0. Therefore, since intention is to add
them as bpf_prog_F_<tag>, we need to pass them to bpf_prog_calc_tag()
first. And once this is resolved, there is a use-after-free on
prog cleanup: we remove the kallsyms entry from the main prog,
later walk all subprogs and call bpf_jit_free() on them. However,
the kallsyms linkage was never released on them. Thus, do that
for all subprogs right in __bpf_prog_put() when refcount hits 0.
Fixes: 1c2a088a6626 ("bpf: x64: add JIT support for multi-function programs")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
kernel/bpf/syscall.c | 6 ++++++
kernel/bpf/verifier.c | 3 +++
2 files changed, 9 insertions(+)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index e2e1c78..30e728d 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -937,10 +937,16 @@ static void __bpf_prog_put_rcu(struct rcu_head *rcu)
static void __bpf_prog_put(struct bpf_prog *prog, bool do_idr_lock)
{
if (atomic_dec_and_test(&prog->aux->refcnt)) {
+ int i;
+
trace_bpf_prog_put_rcu(prog);
/* bpf_prog_free_id() must be called first */
bpf_prog_free_id(prog, do_idr_lock);
+
+ for (i = 0; i < prog->aux->func_cnt; i++)
+ bpf_prog_kallsyms_del(prog->aux->func[i]);
bpf_prog_kallsyms_del(prog);
+
call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu);
}
}
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 48b2901..811e6e9 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5062,7 +5062,10 @@ static int jit_subprogs(struct bpf_verifier_env *env)
goto out_free;
memcpy(func[i]->insnsi, &prog->insnsi[subprog_start],
len * sizeof(struct bpf_insn));
+ func[i]->type = prog->type;
func[i]->len = len;
+ if (bpf_prog_calc_tag(func[i]))
+ goto out_free;
func[i]->is_func = 1;
/* Use bpf_prog_F_tag to indicate functions in stack traces.
* Long term would need debug info to populate names
--
2.9.5
^ permalink raw reply related
* [PATCH bpf-next 2/2] bpf: allow for correlation of maps and helpers in dump
From: Daniel Borkmann @ 2017-12-20 12:42 UTC (permalink / raw)
To: alexei.starovoitov
Cc: netdev, jakub.kicinski, quentin.monnet, Daniel Borkmann
In-Reply-To: <20171220124257.4512-1-daniel@iogearbox.net>
Currently a dump of an xlated prog (post verifier stage) doesn't
correlate used helpers as well as maps. The prog info lists
involved map ids, however there's no correlation of where in the
program they are used as of today. Likewise, bpftool does not
correlate helper calls with the target functions.
The latter can be done w/o any kernel changes through kallsyms,
and also has the advantage that this works with inlined helpers
and BPF calls.
Example, via interpreter:
# tc filter show dev foo ingress
filter protocol all pref 49152 bpf chain 0
filter protocol all pref 49152 bpf chain 0 handle 0x1 foo.o:[ingress] \
direct-action not_in_hw id 1 tag c74773051b364165 <-- prog id:1
* Output before patch (calls/maps remain unclear):
# bpftool prog dump xlated id 1 <-- dump prog id:1
0: (b7) r1 = 2
1: (63) *(u32 *)(r10 -4) = r1
2: (bf) r2 = r10
3: (07) r2 += -4
4: (18) r1 = 0xffff95c47a8d4800
6: (85) call unknown#73040
7: (15) if r0 == 0x0 goto pc+18
8: (bf) r2 = r10
9: (07) r2 += -4
10: (bf) r1 = r0
11: (85) call unknown#73040
12: (15) if r0 == 0x0 goto pc+23
[...]
* Output after patch:
# bpftool prog dump xlated id 1
0: (b7) r1 = 2
1: (63) *(u32 *)(r10 -4) = r1
2: (bf) r2 = r10
3: (07) r2 += -4
4: (18) r1 = map[id:2] <-- map id:2
6: (85) call bpf_map_lookup_elem#73424 <-- helper call
7: (15) if r0 == 0x0 goto pc+18
8: (bf) r2 = r10
9: (07) r2 += -4
10: (bf) r1 = r0
11: (85) call bpf_map_lookup_elem#73424
12: (15) if r0 == 0x0 goto pc+23
[...]
# bpftool map show id 2 <-- show/dump/etc map id:2
2: hash_of_maps flags 0x0
key 4B value 4B max_entries 3 memlock 4096B
Example, JITed, same prog:
# tc filter show dev foo ingress
filter protocol all pref 49152 bpf chain 0
filter protocol all pref 49152 bpf chain 0 handle 0x1 foo.o:[ingress] \
direct-action not_in_hw id 3 tag c74773051b364165 jited
# bpftool prog show id 3
3: sched_cls tag c74773051b364165
loaded_at Dec 19/13:48 uid 0
xlated 384B jited 257B memlock 4096B map_ids 2
# bpftool prog dump xlated id 3
0: (b7) r1 = 2
1: (63) *(u32 *)(r10 -4) = r1
2: (bf) r2 = r10
3: (07) r2 += -4
4: (18) r1 = map[id:2] <-- map id:2
6: (85) call __htab_map_lookup_elem#77408 <-+ inlined rewrite
7: (15) if r0 == 0x0 goto pc+2 |
8: (07) r0 += 56 |
9: (79) r0 = *(u64 *)(r0 +0) <-+
10: (15) if r0 == 0x0 goto pc+24
11: (bf) r2 = r10
12: (07) r2 += -4
[...]
Example, same prog, but kallsyms disabled (in that case we are
also not allowed to pass any relative offsets, etc, so prog
becomes pointer sanitized on dump):
# sysctl kernel.kptr_restrict=2
kernel.kptr_restrict = 2
# bpftool prog dump xlated id 3
0: (b7) r1 = 2
1: (63) *(u32 *)(r10 -4) = r1
2: (bf) r2 = r10
3: (07) r2 += -4
4: (18) r1 = map[id:2]
6: (85) call bpf_unspec#0
7: (15) if r0 == 0x0 goto pc+2
[...]
Example, BPF calls via interpreter:
# bpftool prog dump xlated id 1
0: (85) call pc+2#__bpf_prog_run_args32
1: (b7) r0 = 1
2: (95) exit
3: (b7) r0 = 2
4: (95) exit
Example, BPF calls via JIT:
# sysctl net.core.bpf_jit_enable=1
net.core.bpf_jit_enable = 1
# sysctl net.core.bpf_jit_kallsyms=1
net.core.bpf_jit_kallsyms = 1
# bpftool prog dump xlated id 1
0: (85) call pc+2#bpf_prog_3b185187f1855c4c_F
1: (b7) r0 = 1
2: (95) exit
3: (b7) r0 = 2
4: (95) exit
And finally, an example for tail calls that is now working
as well wrt correlation:
# bpftool prog dump xlated id 2
[...]
10: (b7) r2 = 8
11: (85) call bpf_trace_printk#-41312
12: (bf) r1 = r6
13: (18) r2 = map[id:1]
15: (b7) r3 = 0
16: (85) call bpf_tail_call#12
17: (b7) r1 = 42
18: (6b) *(u16 *)(r6 +46) = r1
19: (b7) r0 = 0
20: (95) exit
# bpftool map show id 1
1: prog_array flags 0x0
key 4B value 4B max_entries 1 memlock 4096B
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
include/linux/filter.h | 9 +++
kernel/bpf/core.c | 4 +-
kernel/bpf/disasm.c | 65 ++++++++++++++---
kernel/bpf/disasm.h | 29 ++++++--
kernel/bpf/syscall.c | 87 +++++++++++++++++++++--
kernel/bpf/verifier.c | 30 ++++++--
tools/bpf/bpftool/prog.c | 181 ++++++++++++++++++++++++++++++++++++++++++++---
7 files changed, 370 insertions(+), 35 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index e872b4e..2b0df27 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -18,6 +18,7 @@
#include <linux/capability.h>
#include <linux/cryptohash.h>
#include <linux/set_memory.h>
+#include <linux/kallsyms.h>
#include <net/sch_generic.h>
@@ -724,6 +725,14 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog);
void bpf_jit_compile(struct bpf_prog *prog);
bool bpf_helper_changes_pkt_data(void *func);
+static inline bool bpf_dump_raw_ok(void)
+{
+ /* Reconstruction of call-sites is dependent on kallsyms,
+ * thus make dump the same restriction.
+ */
+ return kallsyms_show_value() == 1;
+}
+
struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
const struct bpf_insn *patch, u32 len);
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 768e0a0..70a5345 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -771,7 +771,9 @@ struct bpf_prog *bpf_jit_blind_constants(struct bpf_prog *prog)
/* Base function for offset calculation. Needs to go into .text section,
* therefore keeping it non-static as well; will also be used by JITs
- * anyway later on, so do not let the compiler omit it.
+ * anyway later on, so do not let the compiler omit it. This also needs
+ * to go into kallsyms for correlation from e.g. bpftool, so naming
+ * must not change.
*/
noinline u64 __bpf_call_base(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5)
{
diff --git a/kernel/bpf/disasm.c b/kernel/bpf/disasm.c
index 883f88f..8740406 100644
--- a/kernel/bpf/disasm.c
+++ b/kernel/bpf/disasm.c
@@ -21,10 +21,39 @@ static const char * const func_id_str[] = {
};
#undef __BPF_FUNC_STR_FN
-const char *func_id_name(int id)
+static const char *__func_get_name(const struct bpf_insn_cbs *cbs,
+ const struct bpf_insn *insn,
+ char *buff, size_t len)
{
BUILD_BUG_ON(ARRAY_SIZE(func_id_str) != __BPF_FUNC_MAX_ID);
+ if (insn->src_reg != BPF_PSEUDO_CALL &&
+ insn->imm >= 0 && insn->imm < __BPF_FUNC_MAX_ID &&
+ func_id_str[insn->imm])
+ return func_id_str[insn->imm];
+
+ if (cbs && cbs->cb_call)
+ return cbs->cb_call(cbs->private_data, insn);
+
+ if (insn->src_reg == BPF_PSEUDO_CALL)
+ snprintf(buff, len, "%+d", insn->imm);
+
+ return buff;
+}
+
+static const char *__func_imm_name(const struct bpf_insn_cbs *cbs,
+ const struct bpf_insn *insn,
+ u64 full_imm, char *buff, size_t len)
+{
+ if (cbs && cbs->cb_imm)
+ return cbs->cb_imm(cbs->private_data, insn, full_imm);
+
+ snprintf(buff, len, "0x%llx", (unsigned long long)full_imm);
+ return buff;
+}
+
+const char *func_id_name(int id)
+{
if (id >= 0 && id < __BPF_FUNC_MAX_ID && func_id_str[id])
return func_id_str[id];
else
@@ -83,7 +112,7 @@ static const char *const bpf_jmp_string[16] = {
[BPF_EXIT >> 4] = "exit",
};
-static void print_bpf_end_insn(bpf_insn_print_cb verbose,
+static void print_bpf_end_insn(bpf_insn_print_t verbose,
struct bpf_verifier_env *env,
const struct bpf_insn *insn)
{
@@ -92,9 +121,12 @@ static void print_bpf_end_insn(bpf_insn_print_cb verbose,
insn->imm, insn->dst_reg);
}
-void print_bpf_insn(bpf_insn_print_cb verbose, struct bpf_verifier_env *env,
- const struct bpf_insn *insn, bool allow_ptr_leaks)
+void print_bpf_insn(const struct bpf_insn_cbs *cbs,
+ struct bpf_verifier_env *env,
+ const struct bpf_insn *insn,
+ bool allow_ptr_leaks)
{
+ const bpf_insn_print_t verbose = cbs->cb_print;
u8 class = BPF_CLASS(insn->code);
if (class == BPF_ALU || class == BPF_ALU64) {
@@ -175,12 +207,15 @@ void print_bpf_insn(bpf_insn_print_cb verbose, struct bpf_verifier_env *env,
*/
u64 imm = ((u64)(insn + 1)->imm << 32) | (u32)insn->imm;
bool map_ptr = insn->src_reg == BPF_PSEUDO_MAP_FD;
+ char tmp[64];
if (map_ptr && !allow_ptr_leaks)
imm = 0;
- verbose(env, "(%02x) r%d = 0x%llx\n", insn->code,
- insn->dst_reg, (unsigned long long)imm);
+ verbose(env, "(%02x) r%d = %s\n",
+ insn->code, insn->dst_reg,
+ __func_imm_name(cbs, insn, imm,
+ tmp, sizeof(tmp)));
} else {
verbose(env, "BUG_ld_%02x\n", insn->code);
return;
@@ -189,12 +224,20 @@ void print_bpf_insn(bpf_insn_print_cb verbose, struct bpf_verifier_env *env,
u8 opcode = BPF_OP(insn->code);
if (opcode == BPF_CALL) {
- if (insn->src_reg == BPF_PSEUDO_CALL)
- verbose(env, "(%02x) call pc%+d\n", insn->code,
- insn->imm);
- else
+ char tmp[64];
+
+ if (insn->src_reg == BPF_PSEUDO_CALL) {
+ verbose(env, "(%02x) call pc%s\n",
+ insn->code,
+ __func_get_name(cbs, insn,
+ tmp, sizeof(tmp)));
+ } else {
+ strcpy(tmp, "unknown");
verbose(env, "(%02x) call %s#%d\n", insn->code,
- func_id_name(insn->imm), insn->imm);
+ __func_get_name(cbs, insn,
+ tmp, sizeof(tmp)),
+ insn->imm);
+ }
} else if (insn->code == (BPF_JMP | BPF_JA)) {
verbose(env, "(%02x) goto pc%+d\n",
insn->code, insn->off);
diff --git a/kernel/bpf/disasm.h b/kernel/bpf/disasm.h
index 8de977e..e0857d0 100644
--- a/kernel/bpf/disasm.h
+++ b/kernel/bpf/disasm.h
@@ -17,16 +17,35 @@
#include <linux/bpf.h>
#include <linux/kernel.h>
#include <linux/stringify.h>
+#ifndef __KERNEL__
+#include <stdio.h>
+#include <string.h>
+#endif
+
+struct bpf_verifier_env;
extern const char *const bpf_alu_string[16];
extern const char *const bpf_class_string[8];
const char *func_id_name(int id);
-struct bpf_verifier_env;
-typedef void (*bpf_insn_print_cb)(struct bpf_verifier_env *env,
- const char *, ...);
-void print_bpf_insn(bpf_insn_print_cb verbose, struct bpf_verifier_env *env,
- const struct bpf_insn *insn, bool allow_ptr_leaks);
+typedef void (*bpf_insn_print_t)(struct bpf_verifier_env *env,
+ const char *, ...);
+typedef const char *(*bpf_insn_revmap_call_t)(void *private_data,
+ const struct bpf_insn *insn);
+typedef const char *(*bpf_insn_print_imm_t)(void *private_data,
+ const struct bpf_insn *insn,
+ __u64 full_imm);
+
+struct bpf_insn_cbs {
+ bpf_insn_print_t cb_print;
+ bpf_insn_revmap_call_t cb_call;
+ bpf_insn_print_imm_t cb_imm;
+ void *private_data;
+};
+void print_bpf_insn(const struct bpf_insn_cbs *cbs,
+ struct bpf_verifier_env *env,
+ const struct bpf_insn *insn,
+ bool allow_ptr_leaks);
#endif
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 30e728d..007802c 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1558,6 +1558,67 @@ static int bpf_map_get_fd_by_id(const union bpf_attr *attr)
return fd;
}
+static const struct bpf_map *bpf_map_from_imm(const struct bpf_prog *prog,
+ unsigned long addr)
+{
+ int i;
+
+ for (i = 0; i < prog->aux->used_map_cnt; i++)
+ if (prog->aux->used_maps[i] == (void *)addr)
+ return prog->aux->used_maps[i];
+ return NULL;
+}
+
+static struct bpf_insn *bpf_insn_prepare_dump(const struct bpf_prog *prog)
+{
+ const struct bpf_map *map;
+ struct bpf_insn *insns;
+ u64 imm;
+ int i;
+
+ insns = kmemdup(prog->insnsi, bpf_prog_insn_size(prog),
+ GFP_USER);
+ if (!insns)
+ return insns;
+
+ for (i = 0; i < prog->len; i++) {
+ if (insns[i].code == (BPF_JMP | BPF_TAIL_CALL)) {
+ insns[i].code = BPF_JMP | BPF_CALL;
+ insns[i].imm = BPF_FUNC_tail_call;
+ /* fall-through */
+ }
+ if (insns[i].code == (BPF_JMP | BPF_CALL) ||
+ insns[i].code == (BPF_JMP | BPF_CALL_ARGS)) {
+ if (insns[i].code == (BPF_JMP | BPF_CALL_ARGS))
+ insns[i].code = BPF_JMP | BPF_CALL;
+ if (!bpf_dump_raw_ok())
+ insns[i].imm = 0;
+ continue;
+ }
+
+ if (insns[i].code != (BPF_LD | BPF_IMM | BPF_DW))
+ continue;
+
+ imm = ((u64)insns[i + 1].imm << 32) | (u32)insns[i].imm;
+ map = bpf_map_from_imm(prog, imm);
+ if (map) {
+ insns[i].src_reg = BPF_PSEUDO_MAP_FD;
+ insns[i].imm = map->id;
+ insns[i + 1].imm = 0;
+ continue;
+ }
+
+ if (!bpf_dump_raw_ok() &&
+ imm == (unsigned long)prog->aux) {
+ insns[i].imm = 0;
+ insns[i + 1].imm = 0;
+ continue;
+ }
+ }
+
+ return insns;
+}
+
static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
const union bpf_attr *attr,
union bpf_attr __user *uattr)
@@ -1608,18 +1669,34 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
ulen = info.jited_prog_len;
info.jited_prog_len = prog->jited_len;
if (info.jited_prog_len && ulen) {
- uinsns = u64_to_user_ptr(info.jited_prog_insns);
- ulen = min_t(u32, info.jited_prog_len, ulen);
- if (copy_to_user(uinsns, prog->bpf_func, ulen))
- return -EFAULT;
+ if (bpf_dump_raw_ok()) {
+ uinsns = u64_to_user_ptr(info.jited_prog_insns);
+ ulen = min_t(u32, info.jited_prog_len, ulen);
+ if (copy_to_user(uinsns, prog->bpf_func, ulen))
+ return -EFAULT;
+ } else {
+ info.jited_prog_insns = 0;
+ }
}
ulen = info.xlated_prog_len;
info.xlated_prog_len = bpf_prog_insn_size(prog);
if (info.xlated_prog_len && ulen) {
+ struct bpf_insn *insns_sanitized;
+ bool fault;
+
+ if (prog->blinded && !bpf_dump_raw_ok()) {
+ info.xlated_prog_insns = 0;
+ goto done;
+ }
+ insns_sanitized = bpf_insn_prepare_dump(prog);
+ if (!insns_sanitized)
+ return -ENOMEM;
uinsns = u64_to_user_ptr(info.xlated_prog_insns);
ulen = min_t(u32, info.xlated_prog_len, ulen);
- if (copy_to_user(uinsns, prog->insnsi, ulen))
+ fault = copy_to_user(uinsns, insns_sanitized, ulen);
+ kfree(insns_sanitized);
+ if (fault)
return -EFAULT;
}
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 811e6e9..f423726 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4426,9 +4426,12 @@ static int do_check(struct bpf_verifier_env *env)
}
if (env->log.level) {
+ const struct bpf_insn_cbs cbs = {
+ .cb_print = verbose,
+ };
+
verbose(env, "%d: ", insn_idx);
- print_bpf_insn(verbose, env, insn,
- env->allow_ptr_leaks);
+ print_bpf_insn(&cbs, env, insn, env->allow_ptr_leaks);
}
err = ext_analyzer_insn_hook(env, insn_idx, prev_insn_idx);
@@ -5016,14 +5019,14 @@ static int jit_subprogs(struct bpf_verifier_env *env)
{
struct bpf_prog *prog = env->prog, **func, *tmp;
int i, j, subprog_start, subprog_end = 0, len, subprog;
- struct bpf_insn *insn = prog->insnsi;
+ struct bpf_insn *insn;
void *old_bpf_func;
int err = -ENOMEM;
if (env->subprog_cnt == 0)
return 0;
- for (i = 0; i < prog->len; i++, insn++) {
+ for (i = 0, insn = prog->insnsi; i < prog->len; i++, insn++) {
if (insn->code != (BPF_JMP | BPF_CALL) ||
insn->src_reg != BPF_PSEUDO_CALL)
continue;
@@ -5115,6 +5118,25 @@ static int jit_subprogs(struct bpf_verifier_env *env)
bpf_prog_lock_ro(func[i]);
bpf_prog_kallsyms_add(func[i]);
}
+
+ /* Last step: make now unused interpreter insns from main
+ * prog consistent for later dump requests, so they can
+ * later look the same as if they were interpreted only.
+ */
+ for (i = 0, insn = prog->insnsi; i < prog->len; i++, insn++) {
+ unsigned long addr;
+
+ if (insn->code != (BPF_JMP | BPF_CALL) ||
+ insn->src_reg != BPF_PSEUDO_CALL)
+ continue;
+ insn->off = env->insn_aux_data[i].call_imm;
+ subprog = find_subprog(env, i + insn->off + 1);
+ addr = (unsigned long)func[subprog + 1]->bpf_func;
+ addr &= PAGE_MASK;
+ insn->imm = (u64 (*)(u64, u64, u64, u64, u64))
+ addr - __bpf_call_base;
+ }
+
prog->jited = 1;
prog->bpf_func = func[0]->bpf_func;
prog->aux->func = func;
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 037484c..42ee889 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -401,6 +401,88 @@ static int do_show(int argc, char **argv)
return err;
}
+#define SYM_MAX_NAME 256
+
+struct kernel_sym {
+ unsigned long address;
+ char name[SYM_MAX_NAME];
+};
+
+struct dump_data {
+ unsigned long address_call_base;
+ struct kernel_sym *sym_mapping;
+ __u32 sym_count;
+ char scratch_buff[SYM_MAX_NAME];
+};
+
+static int kernel_syms_cmp(const void *sym_a, const void *sym_b)
+{
+ return ((struct kernel_sym *)sym_a)->address -
+ ((struct kernel_sym *)sym_b)->address;
+}
+
+static void kernel_syms_load(struct dump_data *dd)
+{
+ struct kernel_sym *sym;
+ char buff[256];
+ void *tmp, *address;
+ FILE *fp;
+
+ fp = fopen("/proc/kallsyms", "r");
+ if (!fp)
+ return;
+
+ while (!feof(fp)) {
+ if (!fgets(buff, sizeof(buff), fp))
+ break;
+ tmp = realloc(dd->sym_mapping,
+ (dd->sym_count + 1) *
+ sizeof(*dd->sym_mapping));
+ if (!tmp) {
+out:
+ free(dd->sym_mapping);
+ dd->sym_mapping = NULL;
+ fclose(fp);
+ return;
+ }
+ dd->sym_mapping = tmp;
+ sym = &dd->sym_mapping[dd->sym_count];
+ if (sscanf(buff, "%p %*c %s", &address, sym->name) != 2)
+ continue;
+ sym->address = (unsigned long)address;
+ if (!strcmp(sym->name, "__bpf_call_base")) {
+ dd->address_call_base = sym->address;
+ /* sysctl kernel.kptr_restrict was set */
+ if (!sym->address)
+ goto out;
+ }
+ if (sym->address)
+ dd->sym_count++;
+ }
+
+ fclose(fp);
+
+ qsort(dd->sym_mapping, dd->sym_count,
+ sizeof(*dd->sym_mapping), kernel_syms_cmp);
+}
+
+static void kernel_syms_destroy(struct dump_data *dd)
+{
+ free(dd->sym_mapping);
+}
+
+static struct kernel_sym *kernel_syms_search(struct dump_data *dd,
+ unsigned long key)
+{
+ struct kernel_sym sym = {
+ .address = key,
+ };
+
+ return dd->sym_mapping ?
+ bsearch(&sym, dd->sym_mapping, dd->sym_count,
+ sizeof(*dd->sym_mapping), kernel_syms_cmp) : NULL;
+}
+
static void print_insn(struct bpf_verifier_env *env, const char *fmt, ...)
{
va_list args;
@@ -410,8 +492,71 @@ static void print_insn(struct bpf_verifier_env *env, const char *fmt, ...)
va_end(args);
}
-static void dump_xlated_plain(void *buf, unsigned int len, bool opcodes)
+static const char *print_call_pcrel(struct dump_data *dd,
+ struct kernel_sym *sym,
+ unsigned long address,
+ const struct bpf_insn *insn)
{
+ if (sym)
+ snprintf(dd->scratch_buff, sizeof(dd->scratch_buff),
+ "%+d#%s", insn->off, sym->name);
+ else
+ snprintf(dd->scratch_buff, sizeof(dd->scratch_buff),
+ "%+d#0x%lx", insn->off, address);
+ return dd->scratch_buff;
+}
+
+static const char *print_call_helper(struct dump_data *dd,
+ struct kernel_sym *sym,
+ unsigned long address)
+{
+ if (sym)
+ snprintf(dd->scratch_buff, sizeof(dd->scratch_buff),
+ "%s", sym->name);
+ else
+ snprintf(dd->scratch_buff, sizeof(dd->scratch_buff),
+ "0x%lx", address);
+ return dd->scratch_buff;
+}
+
+static const char *print_call(void *private_data,
+ const struct bpf_insn *insn)
+{
+ struct dump_data *dd = private_data;
+ unsigned long address = dd->address_call_base + insn->imm;
+ struct kernel_sym *sym;
+
+ sym = kernel_syms_search(dd, address);
+ if (insn->src_reg == BPF_PSEUDO_CALL)
+ return print_call_pcrel(dd, sym, address, insn);
+ else
+ return print_call_helper(dd, sym, address);
+}
+
+static const char *print_imm(void *private_data,
+ const struct bpf_insn *insn,
+ __u64 full_imm)
+{
+ struct dump_data *dd = private_data;
+
+ if (insn->src_reg == BPF_PSEUDO_MAP_FD)
+ snprintf(dd->scratch_buff, sizeof(dd->scratch_buff),
+ "map[id:%u]", insn->imm);
+ else
+ snprintf(dd->scratch_buff, sizeof(dd->scratch_buff),
+ "0x%llx", (unsigned long long)full_imm);
+ return dd->scratch_buff;
+}
+
+static void dump_xlated_plain(struct dump_data *dd, void *buf,
+ unsigned int len, bool opcodes)
+{
+ const struct bpf_insn_cbs cbs = {
+ .cb_print = print_insn,
+ .cb_call = print_call,
+ .cb_imm = print_imm,
+ .private_data = dd,
+ };
struct bpf_insn *insn = buf;
bool double_insn = false;
unsigned int i;
@@ -425,7 +570,7 @@ static void dump_xlated_plain(void *buf, unsigned int len, bool opcodes)
double_insn = insn[i].code == (BPF_LD | BPF_IMM | BPF_DW);
printf("% 4d: ", i);
- print_bpf_insn(print_insn, NULL, insn + i, true);
+ print_bpf_insn(&cbs, NULL, insn + i, true);
if (opcodes) {
printf(" ");
@@ -454,8 +599,15 @@ static void print_insn_json(struct bpf_verifier_env *env, const char *fmt, ...)
va_end(args);
}
-static void dump_xlated_json(void *buf, unsigned int len, bool opcodes)
+static void dump_xlated_json(struct dump_data *dd, void *buf,
+ unsigned int len, bool opcodes)
{
+ const struct bpf_insn_cbs cbs = {
+ .cb_print = print_insn_json,
+ .cb_call = print_call,
+ .cb_imm = print_imm,
+ .private_data = dd,
+ };
struct bpf_insn *insn = buf;
bool double_insn = false;
unsigned int i;
@@ -470,7 +622,7 @@ static void dump_xlated_json(void *buf, unsigned int len, bool opcodes)
jsonw_start_object(json_wtr);
jsonw_name(json_wtr, "disasm");
- print_bpf_insn(print_insn_json, NULL, insn + i, true);
+ print_bpf_insn(&cbs, NULL, insn + i, true);
if (opcodes) {
jsonw_name(json_wtr, "opcodes");
@@ -505,6 +657,7 @@ static void dump_xlated_json(void *buf, unsigned int len, bool opcodes)
static int do_dump(int argc, char **argv)
{
struct bpf_prog_info info = {};
+ struct dump_data dd = {};
__u32 len = sizeof(info);
unsigned int buf_size;
char *filepath = NULL;
@@ -592,6 +745,14 @@ static int do_dump(int argc, char **argv)
goto err_free;
}
+ if ((member_len == &info.jited_prog_len &&
+ info.jited_prog_insns == 0) ||
+ (member_len == &info.xlated_prog_len &&
+ info.xlated_prog_insns == 0)) {
+ p_err("error retrieving insn dump: kernel.kptr_restrict set?");
+ goto err_free;
+ }
+
if (filepath) {
fd = open(filepath, O_WRONLY | O_CREAT | O_TRUNC, 0600);
if (fd < 0) {
@@ -608,17 +769,19 @@ static int do_dump(int argc, char **argv)
goto err_free;
}
} else {
- if (member_len == &info.jited_prog_len)
+ if (member_len == &info.jited_prog_len) {
disasm_print_insn(buf, *member_len, opcodes);
- else
+ } else {
+ kernel_syms_load(&dd);
if (json_output)
- dump_xlated_json(buf, *member_len, opcodes);
+ dump_xlated_json(&dd, buf, *member_len, opcodes);
else
- dump_xlated_plain(buf, *member_len, opcodes);
+ dump_xlated_plain(&dd, buf, *member_len, opcodes);
+ kernel_syms_destroy(&dd);
+ }
}
free(buf);
-
return 0;
err_free:
--
2.9.5
^ permalink raw reply related
* Re: sparc64 verifier failures..
From: Daniel Borkmann @ 2017-12-20 13:05 UTC (permalink / raw)
To: David Miller; +Cc: netdev, alexei.starovoitov
In-Reply-To: <20171219.153636.783486472609136549.davem@davemloft.net>
On 12/19/2017 09:36 PM, David Miller wrote:
>
> I'm getting about 100 verifier failures on sparc64.
>
> The vast majority of them seem to be due to misaligned packet
> accesses. Here is a sample of some of the failures.
Thanks, I'll check it next days. It would probably make sense to have
a --strict-align mode for test_verifier that enables it on all tests
unconditionally so selftests suite would always run in both modes (at
least on archs that don't have this restriction).
^ permalink raw reply
* [PATCH net-next] net: packet: allow bind to device which is !IFF_UP
From: yuan linyu @ 2017-12-20 13:20 UTC (permalink / raw)
To: netdev; +Cc: David S . Miller, yuan linyu
From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
this try to allow tcpdump to capture packet once device IFF_UP
Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
---
net/packet/af_packet.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index da215e5..11b19fc 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -3124,13 +3124,8 @@ static int packet_do_bind(struct sock *sk, const char *name, int ifindex,
if (proto == 0 || !need_rehook)
goto out_unlock;
- if (!unlisted && (!dev || (dev->flags & IFF_UP))) {
+ if (!unlisted)
register_prot_hook(sk);
- } else {
- sk->sk_err = ENETDOWN;
- if (!sock_flag(sk, SOCK_DEAD))
- sk->sk_error_report(sk);
- }
out_unlock:
rcu_read_unlock();
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v3,net-next 2/2] ip6_gre: fix potential memory leak in ip6erspan_rcv
From: William Tu @ 2017-12-20 14:11 UTC (permalink / raw)
To: Haishuang Yan
Cc: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
Linux Kernel Network Developers, linux-kernel
In-Reply-To: <1513735621-21913-3-git-send-email-yanhaishuang@cmss.chinamobile.com>
On Tue, Dec 19, 2017 at 6:07 PM, Haishuang Yan
<yanhaishuang@cmss.chinamobile.com> wrote:
> If md is NULL, tun_dst must be freed, otherwise it will cause memory
> leak.
>
> Fixes: ef7baf5e083c ("ip6_gre: add ip6 erspan collect_md mode")
> Cc: William Tu <u9012063@gmail.com>
> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
>
> ---
> Changes since v3:
> * Rebase on latest master branch.
> * Fix wrong commit information.
> ---
> net/ipv6/ip6_gre.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index 9bd1103..45038a9 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -550,8 +550,10 @@ static int ip6erspan_rcv(struct sk_buff *skb, int gre_hdr_len,
>
> info = &tun_dst->u.tun_info;
> md = ip_tunnel_info_opts(info);
> - if (!md)
> + if (!md) {
> + dst_release((struct dst_entry *)tun_dst);
> return PACKET_REJECT;
isn't md allocated previously at
tun_dst = ipv6_tun_rx_dst(skb, flags, tun_id,
sizeof(*md));
so md should never be null after we check tun_dst?
William
^ permalink raw reply
* Re: [PATCH v1] net: bonding: Replace mac address parsing
From: Andy Gospodarek @ 2017-12-20 14:25 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Jay Vosburgh, Veaceslav Falico, netdev
In-Reply-To: <20171219182044.5678-1-andriy.shevchenko@linux.intel.com>
On Tue, Dec 19, 2017 at 08:20:44PM +0200, Andy Shevchenko wrote:
> Replace sscanf() with mac_pton().
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Nice cleanup. Thanks!
Acked-by: Andy Gospodarek <andy@greyhouse.net>
> ---
> drivers/net/bonding/bond_options.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> index 8a9b085c2a98..58c705f24f96 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -1431,13 +1431,9 @@ static int bond_option_ad_actor_system_set(struct bonding *bond,
> {
> u8 macaddr[ETH_ALEN];
> u8 *mac;
> - int i;
>
> if (newval->string) {
> - i = sscanf(newval->string, "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx",
> - &macaddr[0], &macaddr[1], &macaddr[2],
> - &macaddr[3], &macaddr[4], &macaddr[5]);
> - if (i != ETH_ALEN)
> + if (!mac_pton(newval->string, macaddr))
> goto err;
> mac = macaddr;
> } else {
> --
> 2.15.1
>
^ permalink raw reply
* Re: [PATCH 0/5] VSOCK: add vsock_test test suite
From: Jorgen S. Hansen @ 2017-12-20 14:48 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: netdev@vger.kernel.org, Dexuan Cui
In-Reply-To: <20171213144911.6428-1-stefanha@redhat.com>
> On Dec 13, 2017, at 3:49 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> The vsock_diag.ko module already has a test suite but the core AF_VSOCK
> functionality has no tests. This patch series adds several test cases that
> exercise AF_VSOCK SOCK_STREAM socket semantics (send/recv, connect/accept,
> half-closed connections, simultaneous connections).
>
> The test suite is modest but I hope to cover additional cases in the future.
> My goal is to have a shared test suite so VMCI, Hyper-V, and KVM can ensure
> that our transports behave the same.
>
> I have tested virtio-vsock.
>
> Jorgen: Please test the VMCI transport and let me know if anything needs to be
> adjusted. See tools/testing/vsock/README for information on how to run the
> test suite.
>
I tried running the vsock_test on VMCI, and all the tests failed in one way or
another:
1) connection reset test: when the guest tries to connect to the host, we
get EINVAL as the error instead of ECONNRESET. I’ll fix that.
2) client close and server close tests: On the host side, VMCI doesn’t
support reading data from a socket that has been closed by the
guest. When the guest closes a connection, all data is gone, and
we return EOF on the host side. So the tests that try to read data
after close, should not attempt that on VMCI host side. I got the
tests to pass by adding a getsockname call to determine if
the local CID was the host CID, and then skip the read attempt
in that case. We could add a vmci flag, that would enable
this behavior.
3) send_byte(fd, -EPIPE): for the VMCI transport, the close
isn’t necessarily visible immediately on the peer. So in most
cases, these send operations would complete with success.
I was running these tests using nested virtualization, so I
suspect that the problem is more likely to occur here, but
I had to add a sleep to be sure to get the EPIPE error.
4) server close test: the connect would sometimes fail. This looks
like an issue where we detect the peer close on the client side
before we complete the connection handshake on the client
side. There are two different channels used for the connection
handshake and the disconnect. I’ll look into this to see what
exactly is going on.
5) multiple connections tests: with the standard socket sizes,
VMCI is only able to support about 100 concurrent stream
connections so this test passes with MULTICONN_NFDS
set to 100.
Thanks,
Jorgen
^ permalink raw reply
* Re: [PATCH] net: Fix double free and memory corruption in get_net_ns_by_id()
From: Nicolas Dichtel @ 2017-12-20 15:00 UTC (permalink / raw)
To: Eric W. Biederman, netdev
Cc: David Miller, ktkhai, security, secalert, eric.dumazet, stephen
In-Reply-To: <87d13aaaqr.fsf@xmission.com>
Le 19/12/2017 à 18:27, Eric W. Biederman a écrit :
>
> (I can trivially verify that that idr_remove in cleanup_net happens
> after the network namespace count has dropped to zero --EWB)
>
> Function get_net_ns_by_id() does not check for net::count
> after it has found a peer in netns_ids idr.
>
> It may dereference a peer, after its count has already been
> finaly decremented. This leads to double free and memory
> corruption:
>
> put_net(peer) rtnl_lock()
> atomic_dec_and_test(&peer->count) [count=0] ...
> __put_net(peer) get_net_ns_by_id(net, id)
> spin_lock(&cleanup_list_lock)
> list_add(&net->cleanup_list, &cleanup_list)
> spin_unlock(&cleanup_list_lock)
> queue_work() peer = idr_find(&net->netns_ids, id)
> | get_net(peer) [count=1]
> | ...
> | (use after final put)
> v ...
> cleanup_net() ...
> spin_lock(&cleanup_list_lock) ...
> list_replace_init(&cleanup_list, ..) ...
> spin_unlock(&cleanup_list_lock) ...
> ... ...
> ... put_net(peer)
> ... atomic_dec_and_test(&peer->count) [count=0]
> ... spin_lock(&cleanup_list_lock)
> ... list_add(&net->cleanup_list, &cleanup_list)
> ... spin_unlock(&cleanup_list_lock)
> ... queue_work()
> ... rtnl_unlock()
> rtnl_lock() ...
> for_each_net(tmp) { ...
> id = __peernet2id(tmp, peer) ...
> spin_lock_irq(&tmp->nsid_lock) ...
> idr_remove(&tmp->netns_ids, id) ...
> ... ...
> net_drop_ns() ...
> net_free(peer) ...
> } ...
> |
> v
> cleanup_net()
> ...
> (Second free of peer)
>
> Also, put_net() on the right cpu may reorder with left's cpu
> list_replace_init(&cleanup_list, ..), and then cleanup_list
> will be corrupted.
>
> Since cleanup_net() is executed in worker thread, while
> put_net(peer) can happen everywhere, there should be
> enough time for concurrent get_net_ns_by_id() to pick
> the peer up, and the race does not seem to be unlikely.
> The patch fixes the problem in standard way.
>
> (Also, there is possible problem in peernet2id_alloc(), which requires
> check for net::count under nsid_lock and maybe_get_net(peer), but
> in current stable kernel it's used under rtnl_lock() and it has to be
> safe. Openswitch begun to use peernet2id_alloc(), and possibly it should
> be fixed too. While this is not in stable kernel yet, so I'll send
> a separate message to netdev@ later).
>
> Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> Fixes: 0c7aecd4bde4 "netns: add rtnl cmd to add and get peer netns ids"
> Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
Good catch, thank you.
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
^ permalink raw reply
* Re: [PATCH bpf-next 1/8] bpf: offload: don't require rtnl for dev list manipulation
From: Kirill Tkhai @ 2017-12-20 15:00 UTC (permalink / raw)
To: Jakub Kicinski, netdev, alexei.starovoitov, daniel; +Cc: oss-drivers
In-Reply-To: <20171220041006.25629-2-jakub.kicinski@netronome.com>
Hi, Jakub,
thanks for looking into this.
Sadly, that __bpf_prog_offload_destroy() needs rtnl_lock() context,
but rwsem is still good as it became useful for next patches from the series.
Please, see one small minor nit near the last hunk. Everything else looks good
for me.
On 20.12.2017 07:09, Jakub Kicinski wrote:
> We only need to hold rtnl_lock() around ndo calls. The device
> offload initialization doesn't require it. Neither will soon-
> -to-come querying the offload info. Use struct rw_semaphore
> because map offload will require sleeping with the semaphore
> held for read.
>
> Suggested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
> kernel/bpf/offload.c | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
> index 8455b89d1bbf..b88e5ebdc61d 100644
> --- a/kernel/bpf/offload.c
> +++ b/kernel/bpf/offload.c
> @@ -20,8 +20,12 @@
> #include <linux/netdevice.h>
> #include <linux/printk.h>
> #include <linux/rtnetlink.h>
> +#include <linux/rwsem.h>
>
> -/* protected by RTNL */
> +/* Protects bpf_prog_offload_devs and offload members of all progs.
> + * RTNL lock cannot be taken when holding this lock.
> + */
> +static struct rw_semaphore bpf_devs_lock;
> static LIST_HEAD(bpf_prog_offload_devs);
>
> int bpf_prog_offload_init(struct bpf_prog *prog, union bpf_attr *attr)
> @@ -43,17 +47,21 @@ int bpf_prog_offload_init(struct bpf_prog *prog, union bpf_attr *attr)
> offload->prog = prog;
> init_waitqueue_head(&offload->verifier_done);
>
> - rtnl_lock();
> + /* Our UNREGISTER notifier will grab bpf_devs_lock, so we are safe
> + * to assume the netdev doesn't get unregistered as long as we hold
> + * bpf_devs_lock.
> + */
> + down_write(&bpf_devs_lock);
> offload->netdev = __dev_get_by_index(net, attr->prog_ifindex);
> if (!offload->netdev) {
> - rtnl_unlock();
> + up_write(&bpf_devs_lock);
> kfree(offload);
> return -EINVAL;
> }
>
> prog->aux->offload = offload;
> list_add_tail(&offload->offloads, &bpf_prog_offload_devs);
> - rtnl_unlock();
> + up_write(&bpf_devs_lock);
>
> return 0;
> }
> @@ -126,7 +134,9 @@ void bpf_prog_offload_destroy(struct bpf_prog *prog)
> wake_up(&offload->verifier_done);
>
> rtnl_lock();
> + down_write(&bpf_devs_lock);
> __bpf_prog_offload_destroy(prog);
> + up_write(&bpf_devs_lock);
> rtnl_unlock();
>
> kfree(offload);
> @@ -181,11 +191,13 @@ static int bpf_offload_notification(struct notifier_block *notifier,
> if (netdev->reg_state != NETREG_UNREGISTERING)
> break;
>
> + down_write(&bpf_devs_lock);
> list_for_each_entry_safe(offload, tmp, &bpf_prog_offload_devs,
> offloads) {
> if (offload->netdev == netdev)
> __bpf_prog_offload_destroy(offload->prog);
> }
> + up_write(&bpf_devs_lock);
> break;
> default:
> break;
> @@ -199,6 +211,7 @@ static struct notifier_block bpf_offload_notifier = {
>
> static int __init bpf_offload_init(void)
> {
> + init_rwsem(&bpf_devs_lock);
DECLARE_RWSEM() could be used instead of this.
> register_netdevice_notifier(&bpf_offload_notifier);
> return 0;
> }
>
^ permalink raw reply
* Re: [patch iproute2] tc: add -bs option for batch mode
From: Stephen Hemminger @ 2017-12-20 15:17 UTC (permalink / raw)
To: Chris Mi; +Cc: netdev@vger.kernel.org, gerlitz.or@gmail.com
In-Reply-To: <VI1PR0501MB21433CE21CEF8212A8BBE21AAB0C0@VI1PR0501MB2143.eurprd05.prod.outlook.com>
On Wed, 20 Dec 2017 09:23:34 +0000
Chris Mi <chrism@mellanox.com> wrote:
> > Your real performance win is just not asking for ACK for every rule.
> No. Even if batch_size > 1, we ack every rule. The real performance win is
> to send multiple rules in one system call. If we are not asking for ACK for every rule,
> the performance will be improved further.
Try the no ACK method.
When we were optimizing routing daemons like Quagga, it was discovered
that an ACK for every route insert was the main bottleneck. Doing asynchronous
error handling got a bigger win than your batching.
Please try that, doing multiple messages using iov is not necessary.
^ permalink raw reply
* Re: [PATCH net v2] ipv4: Fix use-after-free when flushing FIB tables
From: Alexander Duyck @ 2017-12-20 15:32 UTC (permalink / raw)
To: Ido Schimmel
Cc: Netdev, David Miller, Duyck, Alexander H, David Ahern,
Fengguang Wu, mlxsw
In-Reply-To: <20171220085156.27991-1-idosch@mellanox.com>
On Wed, Dec 20, 2017 at 12:51 AM, Ido Schimmel <idosch@mellanox.com> wrote:
> Since commit 0ddcf43d5d4a ("ipv4: FIB Local/MAIN table collapse") the
> local table uses the same trie allocated for the main table when custom
> rules are not in use.
>
> When a net namespace is dismantled, the main table is flushed and freed
> (via an RCU callback) before the local table. In case the callback is
> invoked before the local table is iterated, a use-after-free can occur.
>
> Fix this by iterating over the FIB tables in reverse order, so that the
> main table is always freed after the local table.
>
> v2: Add a comment to make the fix more explicit per Dave's and Alex's
> feedback.
>
> Fixes: 0ddcf43d5d4a ("ipv4: FIB Local/MAIN table collapse")
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> ---
> net/ipv4/fib_frontend.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
> index f52d27a422c3..08ce7f14ede1 100644
> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
> @@ -1298,14 +1298,17 @@ static int __net_init ip_fib_net_init(struct net *net)
>
> static void ip_fib_net_exit(struct net *net)
> {
> - unsigned int i;
> + int i;
>
> rtnl_lock();
> #ifdef CONFIG_IP_MULTIPLE_TABLES
> RCU_INIT_POINTER(net->ipv4.fib_main, NULL);
> RCU_INIT_POINTER(net->ipv4.fib_default, NULL);
> #endif
> - for (i = 0; i < FIB_TABLE_HASHSZ; i++) {
> + /* The local table must be destroyed before the main table,
> + * as it might be using main's trie.
> + */
I think we might want even more description here. Specifically why
reversing the order allows local to be destroyed before main. I was
thinking something along the lines of:
Destroy the tables in reverse order to guarantee that the local table,
ID 255, is destroyed before main table, ID 254. This is necessary as
local may contain references to data contained in main.
> + for (i = FIB_TABLE_HASHSZ - 1; i >= 0; i--) {
> struct hlist_head *head = &net->ipv4.fib_table_hash[i];
> struct hlist_node *tmp;
> struct fib_table *tb;
> --
> 2.14.3
>
^ permalink raw reply
* [PATCH net v2] openvswitch: Fix pop_vlan action for double tagged frames
From: Eric Garver @ 2017-12-20 15:39 UTC (permalink / raw)
To: netdev; +Cc: ovs-dev, Jiri Benc
skb_vlan_pop() expects skb->protocol to be a valid TPID for double
tagged frames, but skb->protocol is set to the ethertype by
key_extract(). So temporarily set it to the TPID when doing a pop_vlan.
Fixes: 5108bbaddc37 ("openvswitch: add processing of L3 packets")
Signed-off-by: Eric Garver <e@erig.me>
---
net/openvswitch/actions.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 30a5df27116e..c484e0941047 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -280,6 +280,13 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *flow_key,
static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key)
{
int err;
+ __be16 proto = skb->protocol;
+
+ /* skb->protocol is set to the inner most parsed ethertype. To satisfy
+ * skb_vlan_pop() for multi-tagged frames we must set it to the tpid.
+ */
+ if (is_flow_key_valid(key) && key->eth.vlan.tci && key->eth.cvlan.tci)
+ skb->protocol = key->eth.cvlan.tpid;
err = skb_vlan_pop(skb);
if (skb_vlan_tag_present(skb)) {
@@ -288,6 +295,9 @@ static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key)
key->eth.vlan.tci = 0;
key->eth.vlan.tpid = 0;
}
+
+ skb->protocol = proto;
+
return err;
}
--
2.12.0
^ permalink raw reply related
* Re: [PATCH v1 2/4] lib/net_utils: Introduce mac_pton_from_user()
From: David Miller @ 2017-12-20 15:51 UTC (permalink / raw)
To: gregkh; +Cc: andriy.shevchenko, netdev, Larry.Finger, florian.c.schilhabel,
devel
In-Reply-To: <20171220071355.GB1957@kroah.com>
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: Wed, 20 Dec 2017 08:13:55 +0100
> On Tue, Dec 19, 2017 at 09:14:10PM +0200, Andy Shevchenko wrote:
>> Some drivers are getting MAC from user space. Make a helper for them.
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>> include/linux/kernel.h | 1 +
>> lib/net_utils.c | 12 ++++++++++++
>> 2 files changed, 13 insertions(+)
>
> Don't do this just for some horrid staging drivers. They can just drop
> that functionality entirely and use the "normal" way of doing this if
> they really want it.
Agreed.
^ permalink raw reply
* Re: [Intel-wired-lan] v4.15-rc2 on thinkpad x60: ethernet stopped working
From: Pavel Machek @ 2017-12-20 15:54 UTC (permalink / raw)
To: Fujinaka, Todd
Cc: Neftin, Sasha, Keller, Jacob E, bpoirier@suse.com,
nix.or.die@gmail.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
lsorense@csclub.uwaterloo.ca, David Miller
In-Reply-To: <9B4A1B1917080E46B64F07F2989DADD69845BBE2@ORSMSX110.amr.corp.intel.com>
[-- Attachment #1: Type: text/plain, Size: 619 bytes --]
Hi!
> >> Before ask for reverting 19110cfbb..., please, check if follow patch
> >> of Benjamin work for you http://patchwork.ozlabs.org/patch/846825/
> >
> Pavel, before ask for revert - let's check Benjamin's patch following to his previous patch. Previous patch was not competed and latest one come to complete changes.
>
v4.15-rc4+:
Ethernet works with 19110cfbb reverted.
Ethernet works With patchwork.ozlabs.org/patch/846825/ applied.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: Linux 4.14 - regression: broken tun/tap / bridge network with virtio - bisected
From: Andreas Hartmann @ 2017-12-20 15:56 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Michal Kubecek, Jason Wang, David Miller, Network Development
In-Reply-To: <6f75bdf5-839b-8c84-c8be-e83d071b245e@maya.org>
On 12/18/2017 at 06:11 PM Andreas Hartmann wrote:
> On 12/17/2017 at 11:33 PM Willem de Bruijn wrote:
[...]
>> I have been able to reproduce the hang by sending a UFO packet
>> between two guests running v4.13 on a host running v4.15-rc1.
>>
>> The vhost_net_ubuf_ref refcount indeed hits overflow (-1) from
>> vhost_zerocopy_callback being called for each segment of a
>> segmented UFO skb. This refcount is decremented then on each
>> segment, but incremented only once for the entire UFO skb.
>>
>> Before v4.14, these packets would be converted in skb_segment to
>> regular copy packets with skb_orphan_frags and the callback function
>> called once at this point. v4.14 added support for reference counted
>> zerocopy skb that can pass through skb_orphan_frags unmodified and
>> have their zerocopy state safely cloned with skb_zerocopy_clone.
>>
>> The call to skb_zerocopy_clone must come after skb_orphan_frags
>> to limit cloning of this state to those skbs that can do so safely.
>>
>> Please try a host with the following patch. This fixes it for me. I intend to
>> send it to net.
>>
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index a592ca025fc4..d2d985418819 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -3654,8 +3654,6 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>>
>> skb_shinfo(nskb)->tx_flags |= skb_shinfo(head_skb)->tx_flags &
>> SKBTX_SHARED_FRAG;
>> - if (skb_zerocopy_clone(nskb, head_skb, GFP_ATOMIC))
>> - goto err;
>>
>> while (pos < offset + len) {
>> if (i >= nfrags) {
>> @@ -3681,6 +3679,8 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>>
>> if (unlikely(skb_orphan_frags(frag_skb, GFP_ATOMIC)))
>> goto err;
>> + if (skb_zerocopy_clone(nskb, frag_skb, GFP_ATOMIC))
>> + goto err;
>>
>> *nskb_frag = *frag;
>> __skb_frag_ref(nskb_frag);
>>
>>
>> This is relatively inefficient, as it calls skb_zerocopy_clone for each frag
>> in the frags[] array. I will follow-up with a patch to net-next that only
>> checks once per skb:
>>
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index 466581cf4cdc..a293a33604ec 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -3662,7 +3662,8 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>>
>> skb_shinfo(nskb)->tx_flags |= skb_shinfo(head_skb)->tx_flags &
>> SKBTX_SHARED_FRAG;
>> - if (skb_zerocopy_clone(nskb, head_skb, GFP_ATOMIC))
>> + if (skb_orphan_frags(frag_skb, GFP_ATOMIC) ||
>> + skb_zerocopy_clone(nskb, frag_skb, GFP_ATOMIC))
>> goto err;
>>
>> while (pos < offset + len) {
>> @@ -3676,6 +3677,11 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>>
>> BUG_ON(!nfrags);
>>
>> + if (skb_orphan_frags(frag_skb, GFP_ATOMIC) ||
>> + skb_zerocopy_clone(nskb, frag_skb,
>> + GFP_ATOMIC))
>> + goto err;
>> +
>> list_skb = list_skb->next;
>> }
>>
>> @@ -3687,9 +3693,6 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>> goto err;
>> }
>>
>> - if (unlikely(skb_orphan_frags(frag_skb, GFP_ATOMIC)))
>> - goto err;
>> -
>
> I'm currently testing this one.
>
Test is in progress. I'm testing w/ 4.14.7, which already contains "net:
accept UFO datagrams from tuntap and packet".
At first, I tested an unpatched 4.14.7 - the problem (no more killable
qemu-process) did occur promptly on shutdown of the machine. This was
expected.
Next, I applied the above patch (the second one). Until now, I didn't
face any problem any more on shutdown of VMs. Looks promising.
Thanks,
regards,
Andreas
^ permalink raw reply
* Re: [Intel-wired-lan] v4.15-rc2 on thinkpad x60: ethernet stopped working
From: Pavel Machek @ 2017-12-20 16:01 UTC (permalink / raw)
To: Fujinaka, Todd
Cc: Neftin, Sasha, Keller, Jacob E, bpoirier@suse.com,
nix.or.die@gmail.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
lsorense@csclub.uwaterloo.ca, David Miller
In-Reply-To: <20171220155421.GA3849@amd>
[-- Attachment #1: Type: text/plain, Size: 962 bytes --]
On Wed 2017-12-20 16:54:21, Pavel Machek wrote:
> Hi!
>
> > >> Before ask for reverting 19110cfbb..., please, check if follow patch
> > >> of Benjamin work for you http://patchwork.ozlabs.org/patch/846825/
>
> > >
> > Pavel, before ask for revert - let's check Benjamin's patch following to his previous patch. Previous patch was not competed and latest one come to complete changes.
> >
>
> v4.15-rc4+:
>
> Ethernet works with 19110cfbb reverted.
>
> Ethernet works With patchwork.ozlabs.org/patch/846825/ applied.
Hmm. So... ethernet originally did not work with patch/846825/ applied
or 19110cfbb reverted, so I re-plugged ethernet cables. Now it works
even with plain v4.15-rc4+.
So it looks like the bug was fixed in the mainline in the meantime...?
Sorry for the noise,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* [PATCH v2] net: ibm: emac: support RGMII-[RX|TX]ID phymode
From: Christian Lamparter @ 2017-12-20 16:02 UTC (permalink / raw)
To: netdev; +Cc: David S . Miller, Andrew Lunn, Christophe Jaillet
The RGMII spec allows compliance for devices that implement an internal
delay on TXC and/or RXC inside the transmitter. This patch adds the
necessary RGMII_[RX|TX]ID mode code to handle such PHYs with the
emac driver.
Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
v2: - utilize phy_interface_mode_is_rgmii()
---
drivers/net/ethernet/ibm/emac/core.c | 4 ++--
drivers/net/ethernet/ibm/emac/emac.h | 3 +++
drivers/net/ethernet/ibm/emac/rgmii.c | 10 ++++++++--
3 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index 7feff2450ed6..043e72e28bba 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -199,8 +199,8 @@ static void __emac_set_multicast_list(struct emac_instance *dev);
static inline int emac_phy_supports_gige(int phy_mode)
{
- return phy_mode == PHY_MODE_GMII ||
- phy_mode == PHY_MODE_RGMII ||
+ return phy_interface_mode_is_rgmii(phy_mode) ||
+ phy_mode == PHY_MODE_GMII ||
phy_mode == PHY_MODE_SGMII ||
phy_mode == PHY_MODE_TBI ||
phy_mode == PHY_MODE_RTBI;
diff --git a/drivers/net/ethernet/ibm/emac/emac.h b/drivers/net/ethernet/ibm/emac/emac.h
index 5afcc27ceebb..8c6d2af7281b 100644
--- a/drivers/net/ethernet/ibm/emac/emac.h
+++ b/drivers/net/ethernet/ibm/emac/emac.h
@@ -112,6 +112,9 @@ struct emac_regs {
#define PHY_MODE_RMII PHY_INTERFACE_MODE_RMII
#define PHY_MODE_SMII PHY_INTERFACE_MODE_SMII
#define PHY_MODE_RGMII PHY_INTERFACE_MODE_RGMII
+#define PHY_MODE_RGMII_ID PHY_INTERFACE_MODE_RGMII_ID
+#define PHY_MODE_RGMII_RXID PHY_INTERFACE_MODE_RGMII_RXID
+#define PHY_MODE_RGMII_TXID PHY_INTERFACE_MODE_RGMII_TXID
#define PHY_MODE_TBI PHY_INTERFACE_MODE_TBI
#define PHY_MODE_GMII PHY_INTERFACE_MODE_GMII
#define PHY_MODE_RTBI PHY_INTERFACE_MODE_RTBI
diff --git a/drivers/net/ethernet/ibm/emac/rgmii.c b/drivers/net/ethernet/ibm/emac/rgmii.c
index c4a1ac38bba8..124b0473d2b7 100644
--- a/drivers/net/ethernet/ibm/emac/rgmii.c
+++ b/drivers/net/ethernet/ibm/emac/rgmii.c
@@ -52,9 +52,9 @@
/* RGMII bridge supports only GMII/TBI and RGMII/RTBI PHYs */
static inline int rgmii_valid_mode(int phy_mode)
{
- return phy_mode == PHY_MODE_GMII ||
+ return phy_interface_mode_is_rgmii(phy_mode) ||
+ phy_mode == PHY_MODE_GMII ||
phy_mode == PHY_MODE_MII ||
- phy_mode == PHY_MODE_RGMII ||
phy_mode == PHY_MODE_TBI ||
phy_mode == PHY_MODE_RTBI;
}
@@ -63,6 +63,9 @@ static inline const char *rgmii_mode_name(int mode)
{
switch (mode) {
case PHY_MODE_RGMII:
+ case PHY_MODE_RGMII_ID:
+ case PHY_MODE_RGMII_RXID:
+ case PHY_MODE_RGMII_TXID:
return "RGMII";
case PHY_MODE_TBI:
return "TBI";
@@ -81,6 +84,9 @@ static inline u32 rgmii_mode_mask(int mode, int input)
{
switch (mode) {
case PHY_MODE_RGMII:
+ case PHY_MODE_RGMII_ID:
+ case PHY_MODE_RGMII_RXID:
+ case PHY_MODE_RGMII_TXID:
return RGMII_FER_RGMII(input);
case PHY_MODE_TBI:
return RGMII_FER_TBI(input);
--
2.15.1
^ permalink raw reply related
* Re: [PATCH v3 ipsec-next 3/3] xfrm: wrap xfrmdev_ops with offload config
From: Marcelo Ricardo Leitner @ 2017-12-20 16:03 UTC (permalink / raw)
To: Shannon Nelson; +Cc: steffen.klassert, netdev
In-Reply-To: <1513726549-7065-4-git-send-email-shannon.nelson@oracle.com>
On Tue, Dec 19, 2017 at 03:35:49PM -0800, Shannon Nelson wrote:
> There's no reason to define netdev->xfrmdev_ops if
> the offload facility is not CONFIG'd in.
>
> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
This one could use a Fixes tag perhaps:
Fixes: d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
as in theory the build was broken since then, as it added:
+#ifdef CONFIG_XFRM_OFFLOAD
+struct xfrmdev_ops {
...
+#ifdef CONFIG_XFRM
+ const struct xfrmdev_ops *xfrmdev_ops;
So the pointer would have an undefined type
if CONFIG_XFRM && !CONFIG_XFRM_OFFLOAD
Though I couldn't reproduce this, not sure why.
But.. is it buildable with this patch? I mine failed:
obj-$(CONFIG_XFRM) := xfrm_policy.o xfrm_state.o xfrm_hash.o \
xfrm_input.o xfrm_output.o \
xfrm_sysctl.o xfrm_replay.o xfrm_device.o
so xfrm_device is always in if CONFIG_XFRM is there,
xfrm_dev_init(), via xfrm_dev_notifier -> xfrm_dev_event() ->
xfrm_dev_register() and then:
static int xfrm_dev_register(struct net_device *dev)
{
if ((dev->features & NETIF_F_HW_ESP) && !dev->xfrmdev_ops)
^^^^^^^^^^^^^^^^
We can't control CONFIG_XFRM_OFFLOAD directly, so unless you
unselected other offloadings such as INET_ESP_OFFLOAD, it is still on.
linux/net/xfrm/xfrm_device.c: In function ‘xfrm_dev_register’:
linux/net/xfrm/xfrm_device.c:147:48: error: ‘struct net_device’ has no member named ‘xfrmdev_ops’; did you mean ‘netdev_ops’?
if ((dev->features & NETIF_F_HW_ESP) && !dev->xfrmdev_ops)
^~~~~~~~~~~
netdev_ops
> ---
> include/linux/netdevice.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 2eaac7d..145d0de 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1697,7 +1697,7 @@ struct net_device {
> const struct ndisc_ops *ndisc_ops;
> #endif
>
> -#ifdef CONFIG_XFRM
> +#ifdef CONFIG_XFRM_OFFLOAD
> const struct xfrmdev_ops *xfrmdev_ops;
> #endif
>
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH net-next] dev: Correctly get length of alias string in dev_set_alias()
From: David Miller @ 2017-12-20 16:05 UTC (permalink / raw)
To: serhe.popovych; +Cc: netdev
In-Reply-To: <1513633115-16940-1-git-send-email-serhe.popovych@gmail.com>
From: Serhey Popovych <serhe.popovych@gmail.com>
Date: Mon, 18 Dec 2017 23:38:35 +0200
> We supply number of bytes available in @alias via @len
> parameter to dev_set_alias() which is not the same
> as zero terminated string length that can be shorter.
>
> Both dev_set_alias() users (rtnetlink and sysfs) can
> submit number of bytes up to IFALIASZ with actual string
> length slightly shorter by putting '\0' not at @len - 1.
>
> Use strnlen() to get length of zero terminated string
> and not access beyond @len. Correct comment about @len
> and explain how to unset alias (i.e. use zero for @len).
>
> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
I don't really see this as useful, really.
In the sysfs case, we are not presented with a NULL terminated string.
Instead, the net sysfs code gives us a length that goes up until the
trailing newline character. The sysfs case is never larger than the
actual string size + 1.
The netlink attribute is usually sized appropriately for whatever the
string length actually is.
This therefore just seems to add an new strnlen() unnecessarily to
this code path, which rarely does anything helpful.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 net-next 0/6] tls: Add generic NIC offload infrastructure
From: David Miller @ 2017-12-20 16:12 UTC (permalink / raw)
To: borisp; +Cc: jiri, ilyal, netdev, davejwatson, tom, hannes, aviadye, liranl
In-Reply-To: <HE1PR0501MB223554DC2840A95B594ED0D1B00C0@HE1PR0501MB2235.eurprd05.prod.outlook.com>
From: Boris Pismenny <borisp@mellanox.com>
Date: Wed, 20 Dec 2017 08:28:03 +0000
> Dave, would you prefer to get the driver patches that use this infra
> before the infra?
The arguments you present are silly.
In order to analyze any proposed API, the users of it must be presented
for the reviewers to see as well.
Logically, you must have tried to make use of the APIs to see how well
they work and are usable for at least one such user, right?
Therefore, the use case exists, and you must present it alongside the
API proposal.
Whether you provide the API addition patches and the user in the same
patch series, or a separate one, doesn't really matter. What is
important is that this is accessible to the reviewer at the same
time.
^ permalink raw reply
* Re: sparc64 verifier failures..
From: David Miller @ 2017-12-20 16:17 UTC (permalink / raw)
To: daniel; +Cc: netdev, alexei.starovoitov
In-Reply-To: <58565df5-2c45-6ab3-fb48-de011da70e6a@iogearbox.net>
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Wed, 20 Dec 2017 14:05:36 +0100
> On 12/19/2017 09:36 PM, David Miller wrote:
>>
>> I'm getting about 100 verifier failures on sparc64.
>>
>> The vast majority of them seem to be due to misaligned packet
>> accesses. Here is a sample of some of the failures.
>
> Thanks, I'll check it next days. It would probably make sense to have
> a --strict-align mode for test_verifier that enables it on all tests
> unconditionally so selftests suite would always run in both modes (at
> least on archs that don't have this restriction).
Indeed, and for some reason I thought we were already doing this.
^ permalink raw reply
* RE: [PATCH v3 net-next 0/6] tls: Add generic NIC offload infrastructure
From: Ilya Lesokhin @ 2017-12-20 16:23 UTC (permalink / raw)
To: David Miller, Boris Pismenny
Cc: jiri@resnulli.us, netdev@vger.kernel.org, davejwatson@fb.com,
tom@herbertland.com, hannes@stressinduktion.org, Aviad Yehezkel,
Liran Liss
In-Reply-To: <20171220.111208.1328340432834146497.davem@davemloft.net>
>
> > Dave, would you prefer to get the driver patches that use this infra
> > before the infra?
>
> The arguments you present are silly.
>
> In order to analyze any proposed API, the users of it must be presented for the
> reviewers to see as well.
>
> Logically, you must have tried to make use of the APIs to see how well they
> work and are usable for at least one such user, right?
Right, we agree.
>
> Therefore, the use case exists, and you must present it alongside the API
> proposal.
>
> Whether you provide the API addition patches and the user in the same patch
> series, or a separate one, doesn't really matter. What is important is that this
> is accessible to the reviewer at the same time.
Note that we did provide a user in an accessible place.
https://github.com/Mellanox/tls-offload/tree/tls_device_v3
The link was at the bottom of the cover letter.
We just feel that the code there is not yet ready for upstream submission, and it might have
conflicts with other stuff submitted by Mellanox.
Would it be better if we submitted the mlx5e TLS support as an RFC alongside the TLS
Infrastructure patches?
^ permalink raw reply
* Re: [PATCH v3 ipsec-next 3/3] xfrm: wrap xfrmdev_ops with offload config
From: Shannon Nelson @ 2017-12-20 16:22 UTC (permalink / raw)
To: Marcelo Ricardo Leitner; +Cc: steffen.klassert, netdev
In-Reply-To: <20171220160240.GK6122@localhost.localdomain>
On 12/20/2017 8:03 AM, Marcelo Ricardo Leitner wrote:
> On Tue, Dec 19, 2017 at 03:35:49PM -0800, Shannon Nelson wrote:
>> There's no reason to define netdev->xfrmdev_ops if
>> the offload facility is not CONFIG'd in.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
>
> This one could use a Fixes tag perhaps:
> Fixes: d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
>
> as in theory the build was broken since then, as it added:
> +#ifdef CONFIG_XFRM_OFFLOAD
> +struct xfrmdev_ops {
> ...
> +#ifdef CONFIG_XFRM
> + const struct xfrmdev_ops *xfrmdev_ops;
>
> So the pointer would have an undefined type
> if CONFIG_XFRM && !CONFIG_XFRM_OFFLOAD
> Though I couldn't reproduce this, not sure why.
Hmmm, I don't think this requires a "Fixes" tag, as the code all worked
just fine, I'm just doing a little cleaning.
Patch 2/3 adds a more intense look at the data structure, so I needed to
change it to the CONFIG_XFRM_OFFLOAD so as to not break the build.
Since the xfrmdev_ops field is now never used unless we have
CONFIG_XFRM_OFFLOAD, we can change the net_device definition to be just
a bit smaller without it.
>
> But.. is it buildable with this patch? I mine failed:
>
> obj-$(CONFIG_XFRM) := xfrm_policy.o xfrm_state.o xfrm_hash.o \
> xfrm_input.o xfrm_output.o \
> xfrm_sysctl.o xfrm_replay.o xfrm_device.o
>
> so xfrm_device is always in if CONFIG_XFRM is there,
> xfrm_dev_init(), via xfrm_dev_notifier -> xfrm_dev_event() ->
> xfrm_dev_register() and then:
>
> static int xfrm_dev_register(struct net_device *dev)
> {
> if ((dev->features & NETIF_F_HW_ESP) && !dev->xfrmdev_ops)
This looks like you haven't applied version 3 of the 2nd patch "xfrm:
check for xdo_dev_ops add and delete". I missed this in the earlier
version (not enough compile tests), but version 3 of patch 2/3 should
address it.
sln
> ^^^^^^^^^^^^^^^^
>
> We can't control CONFIG_XFRM_OFFLOAD directly, so unless you
> unselected other offloadings such as INET_ESP_OFFLOAD, it is still on.
>
> linux/net/xfrm/xfrm_device.c: In function ‘xfrm_dev_register’:
> linux/net/xfrm/xfrm_device.c:147:48: error: ‘struct net_device’ has no member named ‘xfrmdev_ops’; did you mean ‘netdev_ops’?
> if ((dev->features & NETIF_F_HW_ESP) && !dev->xfrmdev_ops)
> ^~~~~~~~~~~
> netdev_ops
>
>
>> ---
>> include/linux/netdevice.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 2eaac7d..145d0de 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -1697,7 +1697,7 @@ struct net_device {
>> const struct ndisc_ops *ndisc_ops;
>> #endif
>>
>> -#ifdef CONFIG_XFRM
>> +#ifdef CONFIG_XFRM_OFFLOAD
>> const struct xfrmdev_ops *xfrmdev_ops;
>> #endif
>>
>> --
>> 2.7.4
>>
^ permalink raw reply
* Re: [QUESTION] Doubt about NAPI_GRO_CB(skb)->is_atomic in tcpv4 gro process
From: Alexander Duyck @ 2017-12-20 16:24 UTC (permalink / raw)
To: Yunsheng Lin
Cc: netdev@vger.kernel.org, davem@davemloft.net, linuxarm@huawei.com,
yuxiaowu, wzhen.wang, Xuehuahu
In-Reply-To: <ca367d0d-b0d7-3357-7196-c0da17ef9890@huawei.com>
On Wed, Dec 20, 2017 at 1:09 AM, Yunsheng Lin <linyunsheng@huawei.com> wrote:
> Hi, all
> I have some doubt about NAPI_GRO_CB(skb)->is_atomic when
> analyzing the tcpv4 gro process:
>
> Firstly we set NAPI_GRO_CB(skb)->is_atomic to 1 in dev_gro_receive:
> https://elixir.free-electrons.com/linux/v4.15-rc4/source/net/core/dev.c#L4838
>
> And then in inet_gro_receive, we check the NAPI_GRO_CB(skb)->is_atomic
> before setting NAPI_GRO_CB(skb)->is_atomic according to IP_DF bit in the ip header:
> https://elixir.free-electrons.com/linux/v4.15-rc4/source/net/ipv4/af_inet.c#L1319
>
> struct sk_buff **inet_gro_receive(struct sk_buff **head, struct sk_buff *skb)
> {
> .....................
> for (p = *head; p; p = p->next) {
> ........................
>
> /* If the previous IP ID value was based on an atomic
> * datagram we can overwrite the value and ignore it.
> */
> if (NAPI_GRO_CB(skb)->is_atomic) //we check it here
> NAPI_GRO_CB(p)->flush_id = flush_id;
> else
> NAPI_GRO_CB(p)->flush_id |= flush_id;
> }
>
> NAPI_GRO_CB(skb)->is_atomic = !!(iph->frag_off & htons(IP_DF)); //we set it here
> NAPI_GRO_CB(skb)->flush |= flush;
> skb_set_network_header(skb, off);
> ................................
> }
>
> My question is whether we should check the NAPI_GRO_CB(skb)->is_atomic or NAPI_GRO_CB(p)->is_atomic?
> If we should check NAPI_GRO_CB(skb)->is_atomic, then maybe it is unnecessary because it is alway true.
> If we should check NAPI_GRO_CB(p)->is_atomic, maybe there is a bug here.
>
> So what is the logic here? I am just start analyzing the gro, maybe I miss something obvious here.
The logic there is to address the multiple IP header case where there
are 2 or more IP headers due to things like VXLAN or GRE tunnels. So
what will happen is that an outer IP header will end up being sent
with DF not set and will clear the is_atomic value then we want to OR
in the next header that is applied. It defaults to assignment on
is_atomic because the first IP header will encounter flush_id with no
previous configuration occupying it.
The part I am not sure about is if we should be using assignment for
is_atomic or using an "&=" to clear the bit and leave it cleared. I
don't know if there has been much testing of multiple levels of tunnel
header.
Thanks.
- Alex
^ 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