* Re: [PATCH net-next 2/2] ipv6: fix ip6_tnl_parse_tlv_enc_lim()
From: Eric Dumazet @ 2017-01-24 0:06 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Willem de Bruijn, Alexei Starovoitov,
Eric Dumazet
In-Reply-To: <20170123235804.29342-3-edumazet@google.com>
On Mon, Jan 23, 2017 at 3:58 PM, Eric Dumazet <edumazet@google.com> wrote:
> This function suffers from multiple issues.
>
> First one is that pskb_may_pull() may reallocate skb->head,
> so the 'raw' pointer needs either to be reloaded or not used at all.
>
> Second issue is that NEXTHDR_DEST handling does not validate
> that the options are present in skb->data, so we might read
> garbage or access non existent memory.
>
> With help from Willem de Bruijn.
Hmm, I've added a bug. Will send a V2, sorry for this.
^ permalink raw reply
* [PATCH net-next 2/5] bpf: enable load bytes helper for filter/reuseport progs
From: Daniel Borkmann @ 2017-01-24 0:06 UTC (permalink / raw)
To: davem; +Cc: netdev, ast, Daniel Borkmann
In-Reply-To: <cover.1485214851.git.daniel@iogearbox.net>
BPF_PROG_TYPE_SOCKET_FILTER are used in various facilities such as
for SO_REUSEPORT and packet fanout demuxing, packet filtering, kcm,
etc, and yet the only facility they can use is BPF_LD with {BPF_ABS,
BPF_IND} for single byte/half/word access.
Direct packet access is only restricted to tc programs right now,
but we can still facilitate usage by allowing skb_load_bytes() helper
added back then in 05c74e5e53f6 ("bpf: add bpf_skb_load_bytes helper")
that calls skb_header_pointer() similarly to bpf_load_pointer(), but
for stack buffers with larger access size.
Name the previous sk_filter_func_proto() as bpf_base_func_proto()
since this is used everywhere else as well, similarly for the ctx
converter, that is, bpf_convert_ctx_access().
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
net/core/filter.c | 41 ++++++++++++++++++++++++++---------------
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index 883975f..e2263da 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2598,7 +2598,7 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
};
static const struct bpf_func_proto *
-sk_filter_func_proto(enum bpf_func_id func_id)
+bpf_base_func_proto(enum bpf_func_id func_id)
{
switch (func_id) {
case BPF_FUNC_map_lookup_elem:
@@ -2626,6 +2626,17 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
}
static const struct bpf_func_proto *
+sk_filter_func_proto(enum bpf_func_id func_id)
+{
+ switch (func_id) {
+ case BPF_FUNC_skb_load_bytes:
+ return &bpf_skb_load_bytes_proto;
+ default:
+ return bpf_base_func_proto(func_id);
+ }
+}
+
+static const struct bpf_func_proto *
tc_cls_act_func_proto(enum bpf_func_id func_id)
{
switch (func_id) {
@@ -2680,7 +2691,7 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
case BPF_FUNC_skb_under_cgroup:
return &bpf_skb_under_cgroup_proto;
default:
- return sk_filter_func_proto(func_id);
+ return bpf_base_func_proto(func_id);
}
}
@@ -2695,7 +2706,7 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
case BPF_FUNC_xdp_adjust_head:
return &bpf_xdp_adjust_head_proto;
default:
- return sk_filter_func_proto(func_id);
+ return bpf_base_func_proto(func_id);
}
}
@@ -2706,7 +2717,7 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
case BPF_FUNC_skb_load_bytes:
return &bpf_skb_load_bytes_proto;
default:
- return sk_filter_func_proto(func_id);
+ return bpf_base_func_proto(func_id);
}
}
@@ -2733,7 +2744,7 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
case BPF_FUNC_skb_under_cgroup:
return &bpf_skb_under_cgroup_proto;
default:
- return sk_filter_func_proto(func_id);
+ return bpf_base_func_proto(func_id);
}
}
@@ -2983,10 +2994,10 @@ void bpf_warn_invalid_xdp_action(u32 act)
}
EXPORT_SYMBOL_GPL(bpf_warn_invalid_xdp_action);
-static u32 sk_filter_convert_ctx_access(enum bpf_access_type type,
- const struct bpf_insn *si,
- struct bpf_insn *insn_buf,
- struct bpf_prog *prog)
+static u32 bpf_convert_ctx_access(enum bpf_access_type type,
+ const struct bpf_insn *si,
+ struct bpf_insn *insn_buf,
+ struct bpf_prog *prog)
{
struct bpf_insn *insn = insn_buf;
int off;
@@ -3210,7 +3221,7 @@ static u32 tc_cls_act_convert_ctx_access(enum bpf_access_type type,
offsetof(struct net_device, ifindex));
break;
default:
- return sk_filter_convert_ctx_access(type, si, insn_buf, prog);
+ return bpf_convert_ctx_access(type, si, insn_buf, prog);
}
return insn - insn_buf;
@@ -3242,7 +3253,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
static const struct bpf_verifier_ops sk_filter_ops = {
.get_func_proto = sk_filter_func_proto,
.is_valid_access = sk_filter_is_valid_access,
- .convert_ctx_access = sk_filter_convert_ctx_access,
+ .convert_ctx_access = bpf_convert_ctx_access,
};
static const struct bpf_verifier_ops tc_cls_act_ops = {
@@ -3261,24 +3272,24 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
static const struct bpf_verifier_ops cg_skb_ops = {
.get_func_proto = cg_skb_func_proto,
.is_valid_access = sk_filter_is_valid_access,
- .convert_ctx_access = sk_filter_convert_ctx_access,
+ .convert_ctx_access = bpf_convert_ctx_access,
};
static const struct bpf_verifier_ops lwt_inout_ops = {
.get_func_proto = lwt_inout_func_proto,
.is_valid_access = lwt_is_valid_access,
- .convert_ctx_access = sk_filter_convert_ctx_access,
+ .convert_ctx_access = bpf_convert_ctx_access,
};
static const struct bpf_verifier_ops lwt_xmit_ops = {
.get_func_proto = lwt_xmit_func_proto,
.is_valid_access = lwt_is_valid_access,
- .convert_ctx_access = sk_filter_convert_ctx_access,
+ .convert_ctx_access = bpf_convert_ctx_access,
.gen_prologue = tc_cls_act_prologue,
};
static const struct bpf_verifier_ops cg_sock_ops = {
- .get_func_proto = sk_filter_func_proto,
+ .get_func_proto = bpf_base_func_proto,
.is_valid_access = sock_filter_is_valid_access,
.convert_ctx_access = sock_filter_convert_ctx_access,
};
--
1.9.3
^ permalink raw reply related
* [PATCH net-next 4/5] bpf: add prog tag test case to bpf selftests
From: Daniel Borkmann @ 2017-01-24 0:06 UTC (permalink / raw)
To: davem; +Cc: netdev, ast, Daniel Borkmann
In-Reply-To: <cover.1485214851.git.daniel@iogearbox.net>
Add the test case used to compare the results from fdinfo with
af_alg's output on the tag. Tests are from min to max sized
programs, with and without maps included.
# ./test_tag
test_tag: OK (40945 tests)
Tested on x86_64 and s390x.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
tools/testing/selftests/bpf/Makefile | 4 +-
tools/testing/selftests/bpf/test_tag.c | 202 +++++++++++++++++++++++++++++++++
2 files changed, 204 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/bpf/test_tag.c
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 064a3e5..769a6cb 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -1,8 +1,8 @@
CFLAGS += -Wall -O2 -I../../../../usr/include
-test_objs = test_verifier test_maps test_lru_map test_lpm_map
+test_objs = test_verifier test_tag test_maps test_lru_map test_lpm_map
-TEST_PROGS := test_verifier test_maps test_lru_map test_lpm_map test_kmod.sh
+TEST_PROGS := $(test_objs) test_kmod.sh
TEST_FILES := $(test_objs)
all: $(test_objs)
diff --git a/tools/testing/selftests/bpf/test_tag.c b/tools/testing/selftests/bpf/test_tag.c
new file mode 100644
index 0000000..6ab4793
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_tag.c
@@ -0,0 +1,202 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <ctype.h>
+#include <time.h>
+#include <errno.h>
+#include <unistd.h>
+#include <string.h>
+#include <sched.h>
+#include <limits.h>
+#include <assert.h>
+
+#include <sys/socket.h>
+#include <sys/resource.h>
+
+#include <linux/filter.h>
+#include <linux/bpf.h>
+#include <linux/if_alg.h>
+
+#include "../../../include/linux/filter.h"
+
+#include "bpf_sys.h"
+
+static struct bpf_insn prog[BPF_MAXINSNS];
+
+static void bpf_gen_imm_prog(unsigned int insns, int fd_map)
+{
+ int i;
+
+ srand(time(NULL));
+ for (i = 0; i < insns; i++)
+ prog[i] = BPF_ALU64_IMM(BPF_MOV, i % BPF_REG_10, rand());
+ prog[i - 1] = BPF_EXIT_INSN();
+}
+
+static void bpf_gen_map_prog(unsigned int insns, int fd_map)
+{
+ int i, j = 0;
+
+ for (i = 0; i + 1 < insns; i += 2) {
+ struct bpf_insn tmp[] = {
+ BPF_LD_MAP_FD(j++ % BPF_REG_10, fd_map)
+ };
+
+ memcpy(&prog[i], tmp, sizeof(tmp));
+ }
+ if (insns % 2 == 0)
+ prog[insns - 2] = BPF_ALU64_IMM(BPF_MOV, i % BPF_REG_10, 42);
+ prog[insns - 1] = BPF_EXIT_INSN();
+}
+
+static int bpf_try_load_prog(int insns, int fd_map,
+ void (*bpf_filler)(unsigned int insns,
+ int fd_map))
+{
+ int fd_prog;
+
+ bpf_filler(insns, fd_map);
+ fd_prog = bpf_prog_load(BPF_PROG_TYPE_SCHED_CLS, prog, insns *
+ sizeof(struct bpf_insn), "", NULL, 0);
+ assert(fd_prog > 0);
+ if (fd_map > 0)
+ bpf_filler(insns, 0);
+ return fd_prog;
+}
+
+static int __hex2bin(char ch)
+{
+ if ((ch >= '0') && (ch <= '9'))
+ return ch - '0';
+ ch = tolower(ch);
+ if ((ch >= 'a') && (ch <= 'f'))
+ return ch - 'a' + 10;
+ return -1;
+}
+
+static int hex2bin(uint8_t *dst, const char *src, size_t count)
+{
+ while (count--) {
+ int hi = __hex2bin(*src++);
+ int lo = __hex2bin(*src++);
+
+ if ((hi < 0) || (lo < 0))
+ return -1;
+ *dst++ = (hi << 4) | lo;
+ }
+ return 0;
+}
+
+static void tag_from_fdinfo(int fd_prog, uint8_t *tag, uint32_t len)
+{
+ const int prefix_len = sizeof("prog_tag:\t") - 1;
+ char buff[256];
+ int ret = -1;
+ FILE *fp;
+
+ snprintf(buff, sizeof(buff), "/proc/%d/fdinfo/%d", getpid(),
+ fd_prog);
+ fp = fopen(buff, "r");
+ assert(fp);
+
+ while (fgets(buff, sizeof(buff), fp)) {
+ if (strncmp(buff, "prog_tag:\t", len))
+ continue;
+ ret = hex2bin(tag, buff + prefix_len, len);
+ break;
+ }
+
+ fclose(fp);
+ assert(!ret);
+}
+
+static void tag_from_alg(int insns, uint8_t *tag, uint32_t len)
+{
+ static const struct sockaddr_alg alg = {
+ .salg_family = AF_ALG,
+ .salg_type = "hash",
+ .salg_name = "sha1",
+ };
+ int fd_base, fd_alg, ret;
+ ssize_t size;
+
+ fd_base = socket(AF_ALG, SOCK_SEQPACKET, 0);
+ assert(fd_base > 0);
+
+ ret = bind(fd_base, (struct sockaddr *)&alg, sizeof(alg));
+ assert(!ret);
+
+ fd_alg = accept(fd_base, NULL, 0);
+ assert(fd_alg > 0);
+
+ insns *= sizeof(struct bpf_insn);
+ size = write(fd_alg, prog, insns);
+ assert(size == insns);
+
+ size = read(fd_alg, tag, len);
+ assert(size == len);
+
+ close(fd_alg);
+ close(fd_base);
+}
+
+static void tag_dump(const char *prefix, uint8_t *tag, uint32_t len)
+{
+ int i;
+
+ printf("%s", prefix);
+ for (i = 0; i < len; i++)
+ printf("%02x", tag[i]);
+ printf("\n");
+}
+
+static void tag_exit_report(int insns, int fd_map, uint8_t *ftag,
+ uint8_t *atag, uint32_t len)
+{
+ printf("Program tag mismatch for %d insns%s!\n", insns,
+ fd_map < 0 ? "" : " with map");
+
+ tag_dump(" fdinfo result: ", ftag, len);
+ tag_dump(" af_alg result: ", atag, len);
+ exit(1);
+}
+
+static void do_test(uint32_t *tests, int start_insns, int fd_map,
+ void (*bpf_filler)(unsigned int insns, int fd))
+{
+ int i, fd_prog;
+
+ for (i = start_insns; i <= BPF_MAXINSNS; i++) {
+ uint8_t ftag[8], atag[sizeof(ftag)];
+
+ fd_prog = bpf_try_load_prog(i, fd_map, bpf_filler);
+ tag_from_fdinfo(fd_prog, ftag, sizeof(ftag));
+ tag_from_alg(i, atag, sizeof(atag));
+ if (memcmp(ftag, atag, sizeof(ftag)))
+ tag_exit_report(i, fd_map, ftag, atag, sizeof(ftag));
+
+ close(fd_prog);
+ sched_yield();
+ (*tests)++;
+ }
+}
+
+int main(void)
+{
+ struct rlimit rinf = { RLIM_INFINITY, RLIM_INFINITY };
+ uint32_t tests = 0;
+ int i, fd_map;
+
+ setrlimit(RLIMIT_MEMLOCK, &rinf);
+ fd_map = bpf_map_create(BPF_MAP_TYPE_HASH, sizeof(int),
+ sizeof(int), 1, BPF_F_NO_PREALLOC);
+ assert(fd_map > 0);
+
+ for (i = 0; i < 5; i++) {
+ do_test(&tests, 2, -1, bpf_gen_imm_prog);
+ do_test(&tests, 3, fd_map, bpf_gen_map_prog);
+ }
+
+ printf("test_tag: OK (%u tests)\n", tests);
+ close(fd_map);
+ return 0;
+}
--
1.9.3
^ permalink raw reply related
* [PATCH net-next 0/5] Misc BPF improvements
From: Daniel Borkmann @ 2017-01-24 0:06 UTC (permalink / raw)
To: davem; +Cc: netdev, ast, Daniel Borkmann
This series adds various misc improvements to BPF, f.e. allowing
skb_load_bytes() helper to be used with filter/reuseport programs
to facilitate programming, test cases for program tag, etc. For
details, please see individual patches.
Thanks!
Daniel Borkmann (5):
bpf: simplify __is_valid_access test on cb
bpf: enable load bytes helper for filter/reuseport progs
bpf: allow option for setting bpf_l4_csum_replace from scratch
bpf: add prog tag test case to bpf selftests
bpf: enable verifier to better track const alu ops
include/uapi/linux/bpf.h | 1 +
kernel/bpf/verifier.c | 64 ++++++---
net/core/filter.c | 63 ++++-----
tools/testing/selftests/bpf/Makefile | 4 +-
tools/testing/selftests/bpf/test_tag.c | 202 ++++++++++++++++++++++++++++
tools/testing/selftests/bpf/test_verifier.c | 82 +++++++++++
6 files changed, 364 insertions(+), 52 deletions(-)
create mode 100644 tools/testing/selftests/bpf/test_tag.c
--
1.9.3
^ permalink raw reply
* [PATCH net-next 5/5] bpf: enable verifier to better track const alu ops
From: Daniel Borkmann @ 2017-01-24 0:06 UTC (permalink / raw)
To: davem; +Cc: netdev, ast, Daniel Borkmann, Gianluca Borello, William Tu
In-Reply-To: <cover.1485214851.git.daniel@iogearbox.net>
William reported couple of issues in relation to direct packet
access. Typical scheme is to check for data + [off] <= data_end,
where [off] can be either immediate or coming from a tracked
register that contains an immediate, depending on the branch, we
can then access the data. However, in case of calculating [off]
for either the mentioned test itself or for access after the test
in a more "complex" way, then the verifier will stop tracking the
CONST_IMM marked register and will mark it as UNKNOWN_VALUE one.
Adding that UNKNOWN_VALUE typed register to a pkt() marked
register, the verifier then bails out in check_packet_ptr_add()
as it finds the registers imm value below 48. In the first below
example, that is due to evaluate_reg_imm_alu() not handling right
shifts and thus marking the register as UNKNOWN_VALUE via helper
__mark_reg_unknown_value() that resets imm to 0.
In the second case the same happens at the time when r4 is set
to r4 &= r5, where it transitions to UNKNOWN_VALUE from
evaluate_reg_imm_alu(). Later on r4 we shift right by 3 inside
evaluate_reg_alu(), where the register's imm turns into 3. That
is, for registers with type UNKNOWN_VALUE, imm of 0 means that
we don't know what value the register has, and for imm > 0 it
means that the value has [imm] upper zero bits. F.e. when shifting
an UNKNOWN_VALUE register by 3 to the right, no matter what value
it had, we know that the 3 upper most bits must be zero now.
This is to make sure that ALU operations with unknown registers
don't overflow. Meaning, once we know that we have more than 48
upper zero bits, or, in other words cannot go beyond 0xffff offset
with ALU ops, such an addition will track the target register
as a new pkt() register with a new id, but 0 offset and 0 range,
so for that a new data/data_end test will be required. Is the source
register a CONST_IMM one that is to be added to the pkt() register,
or the source instruction is an add instruction with immediate
value, then it will get added if it stays within max 0xffff bounds.
>From there, pkt() type, can be accessed should reg->off + imm be
within the access range of pkt().
[...]
from 28 to 30: R0=imm1,min_value=1,max_value=1
R1=pkt(id=0,off=0,r=22) R2=pkt_end
R3=imm144,min_value=144,max_value=144
R4=imm0,min_value=0,max_value=0
R5=inv48,min_value=2054,max_value=2054 R10=fp
30: (bf) r5 = r3
31: (07) r5 += 23
32: (77) r5 >>= 3
33: (bf) r6 = r1
34: (0f) r6 += r5
cannot add integer value with 0 upper zero bits to ptr_to_packet
[...]
from 52 to 80: R0=imm1,min_value=1,max_value=1
R1=pkt(id=0,off=0,r=34) R2=pkt_end R3=inv
R4=imm272 R5=inv56,min_value=17,max_value=17
R6=pkt(id=0,off=26,r=34) R10=fp
80: (07) r4 += 71
81: (18) r5 = 0xfffffff8
83: (5f) r4 &= r5
84: (77) r4 >>= 3
85: (0f) r1 += r4
cannot add integer value with 3 upper zero bits to ptr_to_packet
Thus to get above use-cases working, evaluate_reg_imm_alu() has
been extended for further ALU ops. This is fine, because we only
operate strictly within realm of CONST_IMM types, so here we don't
care about overflows as they will happen in the simulated but also
real execution and interaction with pkt() in check_packet_ptr_add()
will check actual imm value once added to pkt(), but it's irrelevant
before.
With regards to 06c1c049721a ("bpf: allow helpers access to variable
memory") that works on UNKNOWN_VALUE registers, the verifier becomes
now a bit smarter as it can better resolve ALU ops, so we need to
adapt two test cases there, as min/max bound tracking only becomes
necessary when registers were spilled to stack. So while mask was
set before to track upper bound for UNKNOWN_VALUE case, it's now
resolved directly as CONST_IMM, and such contructs are only necessary
when f.e. registers are spilled.
For commit 6b17387307ba ("bpf: recognize 64bit immediate loads as
consts") that initially enabled dw load tracking only for nfp jit/
analyzer, I did couple of tests on large, complex programs and we
don't increase complexity badly (my tests were in ~3% range on avg).
I've added a couple of tests similar to affected code above, and
it works fine with verifier now.
Reported-by: William Tu <u9012063@gmail.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Gianluca Borello <g.borello@gmail.com>
Cc: William Tu <u9012063@gmail.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
kernel/bpf/verifier.c | 64 +++++++++++++++-------
tools/testing/selftests/bpf/test_verifier.c | 82 +++++++++++++++++++++++++++++
2 files changed, 127 insertions(+), 19 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8f69df7..fb3513b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1566,22 +1566,54 @@ static int evaluate_reg_imm_alu(struct bpf_verifier_env *env,
struct bpf_reg_state *dst_reg = ®s[insn->dst_reg];
struct bpf_reg_state *src_reg = ®s[insn->src_reg];
u8 opcode = BPF_OP(insn->code);
+ u64 dst_imm = dst_reg->imm;
- /* dst_reg->type == CONST_IMM here, simulate execution of 'add'/'or'
- * insn. Don't care about overflow or negative values, just add them
+ /* dst_reg->type == CONST_IMM here. Simulate execution of insns
+ * containing ALU ops. Don't care about overflow or negative
+ * values, just add/sub/... them; registers are in u64.
*/
- if (opcode == BPF_ADD && BPF_SRC(insn->code) == BPF_K)
- dst_reg->imm += insn->imm;
- else if (opcode == BPF_ADD && BPF_SRC(insn->code) == BPF_X &&
- src_reg->type == CONST_IMM)
- dst_reg->imm += src_reg->imm;
- else if (opcode == BPF_OR && BPF_SRC(insn->code) == BPF_K)
- dst_reg->imm |= insn->imm;
- else if (opcode == BPF_OR && BPF_SRC(insn->code) == BPF_X &&
- src_reg->type == CONST_IMM)
- dst_reg->imm |= src_reg->imm;
- else
+ if (opcode == BPF_ADD && BPF_SRC(insn->code) == BPF_K) {
+ dst_imm += insn->imm;
+ } else if (opcode == BPF_ADD && BPF_SRC(insn->code) == BPF_X &&
+ src_reg->type == CONST_IMM) {
+ dst_imm += src_reg->imm;
+ } else if (opcode == BPF_SUB && BPF_SRC(insn->code) == BPF_K) {
+ dst_imm -= insn->imm;
+ } else if (opcode == BPF_SUB && BPF_SRC(insn->code) == BPF_X &&
+ src_reg->type == CONST_IMM) {
+ dst_imm -= src_reg->imm;
+ } else if (opcode == BPF_MUL && BPF_SRC(insn->code) == BPF_K) {
+ dst_imm *= insn->imm;
+ } else if (opcode == BPF_MUL && BPF_SRC(insn->code) == BPF_X &&
+ src_reg->type == CONST_IMM) {
+ dst_imm *= src_reg->imm;
+ } else if (opcode == BPF_OR && BPF_SRC(insn->code) == BPF_K) {
+ dst_imm |= insn->imm;
+ } else if (opcode == BPF_OR && BPF_SRC(insn->code) == BPF_X &&
+ src_reg->type == CONST_IMM) {
+ dst_imm |= src_reg->imm;
+ } else if (opcode == BPF_AND && BPF_SRC(insn->code) == BPF_K) {
+ dst_imm &= insn->imm;
+ } else if (opcode == BPF_AND && BPF_SRC(insn->code) == BPF_X &&
+ src_reg->type == CONST_IMM) {
+ dst_imm &= src_reg->imm;
+ } else if (opcode == BPF_RSH && BPF_SRC(insn->code) == BPF_K) {
+ dst_imm >>= insn->imm;
+ } else if (opcode == BPF_RSH && BPF_SRC(insn->code) == BPF_X &&
+ src_reg->type == CONST_IMM) {
+ dst_imm >>= src_reg->imm;
+ } else if (opcode == BPF_LSH && BPF_SRC(insn->code) == BPF_K) {
+ dst_imm <<= insn->imm;
+ } else if (opcode == BPF_LSH && BPF_SRC(insn->code) == BPF_X &&
+ src_reg->type == CONST_IMM) {
+ dst_imm <<= src_reg->imm;
+ } else {
mark_reg_unknown_value(regs, insn->dst_reg);
+ goto out;
+ }
+
+ dst_reg->imm = dst_imm;
+out:
return 0;
}
@@ -2225,14 +2257,8 @@ static int check_ld_imm(struct bpf_verifier_env *env, struct bpf_insn *insn)
return err;
if (insn->src_reg == 0) {
- /* generic move 64-bit immediate into a register,
- * only analyzer needs to collect the ld_imm value.
- */
u64 imm = ((u64)(insn + 1)->imm << 32) | (u32)insn->imm;
- if (!env->analyzer_ops)
- return 0;
-
regs[insn->dst_reg].type = CONST_IMM;
regs[insn->dst_reg].imm = imm;
return 0;
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 1aa7324..0d0912c 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -2326,6 +2326,84 @@ struct test_val {
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
},
{
+ "direct packet access: test11 (shift, good access)",
+ .insns = {
+ BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
+ offsetof(struct __sk_buff, data)),
+ BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
+ offsetof(struct __sk_buff, data_end)),
+ BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 22),
+ BPF_JMP_REG(BPF_JGT, BPF_REG_0, BPF_REG_3, 8),
+ BPF_MOV64_IMM(BPF_REG_3, 144),
+ BPF_MOV64_REG(BPF_REG_5, BPF_REG_3),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, 23),
+ BPF_ALU64_IMM(BPF_RSH, BPF_REG_5, 3),
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_2),
+ BPF_ALU64_REG(BPF_ADD, BPF_REG_6, BPF_REG_5),
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .result = ACCEPT,
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ },
+ {
+ "direct packet access: test12 (and, good access)",
+ .insns = {
+ BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
+ offsetof(struct __sk_buff, data)),
+ BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
+ offsetof(struct __sk_buff, data_end)),
+ BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 22),
+ BPF_JMP_REG(BPF_JGT, BPF_REG_0, BPF_REG_3, 8),
+ BPF_MOV64_IMM(BPF_REG_3, 144),
+ BPF_MOV64_REG(BPF_REG_5, BPF_REG_3),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, 23),
+ BPF_ALU64_IMM(BPF_AND, BPF_REG_5, 15),
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_2),
+ BPF_ALU64_REG(BPF_ADD, BPF_REG_6, BPF_REG_5),
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .result = ACCEPT,
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ },
+ {
+ "direct packet access: test13 (branches, good access)",
+ .insns = {
+ BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
+ offsetof(struct __sk_buff, data)),
+ BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
+ offsetof(struct __sk_buff, data_end)),
+ BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 22),
+ BPF_JMP_REG(BPF_JGT, BPF_REG_0, BPF_REG_3, 13),
+ BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
+ offsetof(struct __sk_buff, mark)),
+ BPF_MOV64_IMM(BPF_REG_4, 1),
+ BPF_JMP_REG(BPF_JGT, BPF_REG_3, BPF_REG_4, 2),
+ BPF_MOV64_IMM(BPF_REG_3, 14),
+ BPF_JMP_IMM(BPF_JA, 0, 0, 1),
+ BPF_MOV64_IMM(BPF_REG_3, 24),
+ BPF_MOV64_REG(BPF_REG_5, BPF_REG_3),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, 23),
+ BPF_ALU64_IMM(BPF_AND, BPF_REG_5, 15),
+ BPF_MOV64_REG(BPF_REG_6, BPF_REG_2),
+ BPF_ALU64_REG(BPF_ADD, BPF_REG_6, BPF_REG_5),
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .result = ACCEPT,
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ },
+ {
"helper access to packet: test1, valid packet_ptr range",
.insns = {
BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
@@ -4208,6 +4286,8 @@ struct test_val {
.insns = {
BPF_MOV64_IMM(BPF_REG_1, 0),
BPF_MOV64_IMM(BPF_REG_2, 0),
+ BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_2, -128),
+ BPF_LDX_MEM(BPF_DW, BPF_REG_2, BPF_REG_10, -128),
BPF_ALU64_IMM(BPF_AND, BPF_REG_2, 64),
BPF_MOV64_IMM(BPF_REG_3, 0),
BPF_MOV64_IMM(BPF_REG_4, 0),
@@ -4251,6 +4331,8 @@ struct test_val {
BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_0, -16),
BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_0, -8),
BPF_MOV64_IMM(BPF_REG_2, 0),
+ BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_2, -128),
+ BPF_LDX_MEM(BPF_DW, BPF_REG_2, BPF_REG_10, -128),
BPF_ALU64_IMM(BPF_AND, BPF_REG_2, 63),
BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, 1),
BPF_MOV64_IMM(BPF_REG_3, 0),
--
1.9.3
^ permalink raw reply related
* [PATCH net-next 1/5] bpf: simplify __is_valid_access test on cb
From: Daniel Borkmann @ 2017-01-24 0:06 UTC (permalink / raw)
To: davem; +Cc: netdev, ast, Daniel Borkmann
In-Reply-To: <cover.1485214851.git.daniel@iogearbox.net>
The __is_valid_access() test for cb[] from 62c7989b24db ("bpf: allow
b/h/w/dw access for bpf's cb in ctx") was done unnecessarily complex,
we can just simplify it the same way as recent fix from 2d071c643f1c
("bpf, trace: make ctx access checks more robust") did. Overflow can
never happen as size is 1/2/4/8 depending on access.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
net/core/filter.c | 15 ++-------------
1 file changed, 2 insertions(+), 13 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index 9038386..883975f 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2784,19 +2784,8 @@ static bool __is_valid_access(int off, int size)
switch (off) {
case offsetof(struct __sk_buff, cb[0]) ...
offsetof(struct __sk_buff, cb[4]) + sizeof(__u32) - 1:
- if (size == sizeof(__u16) &&
- off > offsetof(struct __sk_buff, cb[4]) + sizeof(__u16))
- return false;
- if (size == sizeof(__u32) &&
- off > offsetof(struct __sk_buff, cb[4]))
- return false;
- if (size == sizeof(__u64) &&
- off > offsetof(struct __sk_buff, cb[2]))
- return false;
- if (size != sizeof(__u8) &&
- size != sizeof(__u16) &&
- size != sizeof(__u32) &&
- size != sizeof(__u64))
+ if (off + size >
+ offsetof(struct __sk_buff, cb[4]) + sizeof(__u32))
return false;
break;
default:
--
1.9.3
^ permalink raw reply related
* [PATCH net-next 3/5] bpf: allow option for setting bpf_l4_csum_replace from scratch
From: Daniel Borkmann @ 2017-01-24 0:06 UTC (permalink / raw)
To: davem; +Cc: netdev, ast, Daniel Borkmann
In-Reply-To: <cover.1485214851.git.daniel@iogearbox.net>
When programs need to calculate the csum from scratch for small UDP
packets and use bpf_l4_csum_replace() to feed the result from helpers
like bpf_csum_diff(), then we need a flag besides BPF_F_MARK_MANGLED_0
that would ignore the case of current csum being 0, and which would
still allow for the helper to set the csum and transform when needed
to CSUM_MANGLED_0.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
include/uapi/linux/bpf.h | 1 +
net/core/filter.c | 7 ++++---
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index bd30684..e07fd5a 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -522,6 +522,7 @@ enum bpf_func_id {
/* BPF_FUNC_l4_csum_replace flags. */
#define BPF_F_PSEUDO_HDR (1ULL << 4)
#define BPF_F_MARK_MANGLED_0 (1ULL << 5)
+#define BPF_F_MARK_ENFORCE (1ULL << 6)
/* BPF_FUNC_clone_redirect and BPF_FUNC_redirect flags. */
#define BPF_F_INGRESS (1ULL << 0)
diff --git a/net/core/filter.c b/net/core/filter.c
index e2263da..1e00737 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1522,10 +1522,11 @@ static inline void bpf_pull_mac_rcsum(struct sk_buff *skb)
{
bool is_pseudo = flags & BPF_F_PSEUDO_HDR;
bool is_mmzero = flags & BPF_F_MARK_MANGLED_0;
+ bool do_mforce = flags & BPF_F_MARK_ENFORCE;
__sum16 *ptr;
- if (unlikely(flags & ~(BPF_F_MARK_MANGLED_0 | BPF_F_PSEUDO_HDR |
- BPF_F_HDR_FIELD_MASK)))
+ if (unlikely(flags & ~(BPF_F_MARK_MANGLED_0 | BPF_F_MARK_ENFORCE |
+ BPF_F_PSEUDO_HDR | BPF_F_HDR_FIELD_MASK)))
return -EINVAL;
if (unlikely(offset > 0xffff || offset & 1))
return -EFAULT;
@@ -1533,7 +1534,7 @@ static inline void bpf_pull_mac_rcsum(struct sk_buff *skb)
return -EFAULT;
ptr = (__sum16 *)(skb->data + offset);
- if (is_mmzero && !*ptr)
+ if (is_mmzero && !do_mforce && !*ptr)
return 0;
switch (flags & BPF_F_HDR_FIELD_MASK) {
--
1.9.3
^ permalink raw reply related
* Re: [RFC PATCH net-next 0/5] bridge: per vlan lwt and dst_metadata support
From: Roopa Prabhu @ 2017-01-24 0:09 UTC (permalink / raw)
To: Or Gerlitz
Cc: Jiri Benc, Jiri Pirko, Linux Netdev List, David Miller,
Stephen Hemminger, Nikolay Aleksandrov, Thomas Graf,
Hannes Frederic Sowa, pravin shelar, David Ahern,
Jamal Hadi Salim
In-Reply-To: <CAJ3xEMiC5xJ+rex8xMnyuGj5QKj+sYA9A6JjOM0xQaZraFSHig@mail.gmail.com>
On 1/23/17, 9:03 AM, Or Gerlitz wrote:
> On Mon, Jan 23, 2017 at 6:13 PM, Roopa Prabhu <roopa@cumulusnetworks.com>
> wrote:
>
>> Also, the goal is to reduce the number of vxlan devices from say 4k to 1.
>> I don't think replacing it with 8k (egress + ingress) rules is going in the
>> right direction.
>>
> Can't you take advantage of the shared vxlan device configuration
> introduced throughout the LWT work such that you have single device dealing
> with many tunnels? why?
>
I tried to cover this in my initial paragraph in the cover letter:
"lwt and dst_metadata/collect_metadata have enabled vxlan l3 deployments to use a 'single vxlan
netdev for multiple vnis' eliminating the scalability problem with using a 'single vxlan netdev per vni'.
This series tries to do the same for vxlan netdevs in pure l2 bridged networks. Use-case/deployment and
details are below." there is more in the cover letter on this.
There is no route pointing to the vxlan device here. vxlan device is a bridged port. And it bridges local host ports to remote vxlan tunnels
vlan-to-vxlan.
^ permalink raw reply
* Re: [PATCH cumulus-4.1.y 1/5] vxlan: flush fdb entries on oper down
From: Roopa Prabhu @ 2017-01-24 0:11 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: davem, netdev, ramanb, jbenc, pshelar
In-Reply-To: <20170123085945.36404cf0@xeon-e3>
On 1/23/17, 8:59 AM, Stephen Hemminger wrote:
> On Fri, 20 Jan 2017 23:40:06 -0800
> Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
>
>> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
>> index 19b1653..15b1c23 100644
>> --- a/drivers/net/vxlan.c
>> +++ b/drivers/net/vxlan.c
>> @@ -3276,6 +3276,12 @@ static int vxlan_netdevice_event(struct notifier_block *unused,
>> vxlan_handle_lowerdev_unregister(vn, dev);
>> else if (event == NETDEV_UDP_TUNNEL_PUSH_INFO)
>> vxlan_push_rx_ports(dev);
>> + else if (event == NETDEV_CHANGE) {
>> + if (dev->netdev_ops == &vxlan_netdev_ops) {
>> + if (netif_running(dev) && !netif_oper_up(dev))
>> + vxlan_flush(netdev_priv(dev));
>> + }
>> + }
> Looks correct.
> Maybe logic would be clearer with a switch() statement here.
ack. this was an internal patch i accidentally sent. I have refined it a bit for upstream since then.
will post an update..
thanks
^ permalink raw reply
* Re: [PATCH net-next 1/2] vxlan: don't flush static fdb entries on admin down
From: Roopa Prabhu @ 2017-01-24 0:14 UTC (permalink / raw)
To: David Miller; +Cc: netdev, ramanb, stephen, jbenc, pshelar
In-Reply-To: <20170123.160741.1558621534864494201.davem@davemloft.net>
On 1/23/17, 1:07 PM, David Miller wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> Date: Fri, 20 Jan 2017 23:43:18 -0800
>
>> /* Purge the forwarding table */
>> -static void vxlan_flush(struct vxlan_dev *vxlan)
>> +static void vxlan_flush(struct vxlan_dev *vxlan, int do_all)
> Please use 'bool' and true/false for this new argument.
>
> FWIW, I am fine with changing the default behavior in this way.
ack, will fix and post a v2, thanks.
^ permalink raw reply
* Re: [RFC PATCH] mlx5: Fix page rfcnt issue
From: David Miller @ 2017-01-24 0:16 UTC (permalink / raw)
To: tom; +Cc: netdev, tariqt, saeedm, kernel-team
In-Reply-To: <20170123235658.3872770-1-tom@herbertland.com>
From: Tom Herbert <tom@herbertland.com>
Date: Mon, 23 Jan 2017 15:56:58 -0800
> One other potential problem in the driver is the use of put_page in
> release pages. Comparing how the allocation is done in other drivers
> (for instance comparing to ixgbe) some seem to use __free_pages instead.
> I don't know which is correct to use, but somehow it doesn't seem like
> they can both be right.
The only difference I can see is that put_page() does a
__page_cache_release() which shouldn't be necessary for driver RX
pages, so it could be unnecessary overhead.
^ permalink raw reply
* Re: [PATCH cumulus-4.1.y 2/5] vxlan: don't replace fdb entry if nothing changed
From: Roopa Prabhu @ 2017-01-24 0:14 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: davem, netdev, ramanb, jbenc, pshelar
In-Reply-To: <20170123090211.6bcd2afc@xeon-e3>
On 1/23/17, 9:02 AM, Stephen Hemminger wrote:
> On Fri, 20 Jan 2017 23:40:07 -0800
> Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
>
>> + if (!vxlan_addr_equal(&rd->remote_ip, ip) ||
>> + rd->remote_port != port ||
>> + rd->remote_vni != vni ||
>> + rd->remote_ifindex != ifindex) {
>> + dst_cache_reset(&rd->dst_cache);
>> + rd->remote_ip = *ip;
>> + rd->remote_port = port;
>> + rd->remote_vni = vni;
>> + rd->remote_ifindex = ifindex;
>> + return 1;
>> + }
>> +
> I think it would be clearer if negative logic was avoided.
>
> if (vxlan_addr_equal(&rd->remote_ip, ip) &&
> rd->remote_port == port &&
> rd->remote_vni == vni &&
> rd->ermote_ifindex == ifndex)
> return 1;
>
> dst_cache_reset ...
ack, this was an accidental hit on send as well.
It is on my upstream patch stack..but i think this patch is not really needed upstream because
a previous call to vxlan_fdb_find_rdst in vxlan_fdb_replace does the same thing.
I will test again and repost if needed, thanks.
^ permalink raw reply
* [PATCH net-next] bpf, lpm: fix kfree of im_node in trie_update_elem
From: Daniel Borkmann @ 2017-01-24 0:26 UTC (permalink / raw)
To: davem; +Cc: ast, daniel, netdev, Daniel Borkmann
We need to initialize im_node to NULL, otherwise in case of error path
it gets passed to kfree() as uninitialized pointer.
Fixes: b95a5c4db09b ("bpf: add a longest prefix match trie map implementation")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
Mentioned it in http://patchwork.ozlabs.org/patch/718070/, but
was probably overlooked.
kernel/bpf/lpm_trie.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c
index ba19241d..144e976 100644
--- a/kernel/bpf/lpm_trie.c
+++ b/kernel/bpf/lpm_trie.c
@@ -262,7 +262,7 @@ static int trie_update_elem(struct bpf_map *map,
void *_key, void *value, u64 flags)
{
struct lpm_trie *trie = container_of(map, struct lpm_trie, map);
- struct lpm_trie_node *node, *im_node, *new_node = NULL;
+ struct lpm_trie_node *node, *im_node = NULL, *new_node = NULL;
struct lpm_trie_node __rcu **slot;
struct bpf_lpm_trie_key *key = _key;
unsigned long irq_flags;
--
1.9.3
^ permalink raw reply related
* Re: [PATCH net-next] bpf, lpm: fix kfree of im_node in trie_update_elem
From: Alexei Starovoitov @ 2017-01-24 0:37 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, ast, daniel, netdev
In-Reply-To: <6052894aa1bd4df5a5c7aaa05f5b63a980cbe371.1485217298.git.daniel@iogearbox.net>
On Tue, Jan 24, 2017 at 01:26:46AM +0100, Daniel Borkmann wrote:
> We need to initialize im_node to NULL, otherwise in case of error path
> it gets passed to kfree() as uninitialized pointer.
>
> Fixes: b95a5c4db09b ("bpf: add a longest prefix match trie map implementation")
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Great catch.
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* linux-next: manual merge of the net-next tree with the net tree
From: Stephen Rothwell @ 2017-01-24 0:38 UTC (permalink / raw)
To: David Miller, Networking
Cc: linux-next, linux-kernel, David Ahern, Robert Shearman
Hi all,
Today's linux-next merge of the net-next tree got a conflict in:
net/mpls/af_mpls.c
between commit:
9f427a0e474a ("net: mpls: Fix multipath selection for LSR use case")
from the net tree and commit:
27d691056bde ("mpls: Packet stats")
from the net-next tree.
I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging. You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.
--
Cheers,
Stephen Rothwell
diff --cc net/mpls/af_mpls.c
index 5b77377e5a15,4dc81963af8f..000000000000
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@@ -98,10 -94,35 +94,35 @@@ bool mpls_pkt_too_big(const struct sk_b
}
EXPORT_SYMBOL_GPL(mpls_pkt_too_big);
+ void mpls_stats_inc_outucastpkts(struct net_device *dev,
+ const struct sk_buff *skb)
+ {
+ struct mpls_dev *mdev;
+
+ if (skb->protocol == htons(ETH_P_MPLS_UC)) {
+ mdev = mpls_dev_get(dev);
+ if (mdev)
+ MPLS_INC_STATS_LEN(mdev, skb->len,
+ tx_packets,
+ tx_bytes);
+ } else if (skb->protocol == htons(ETH_P_IP)) {
+ IP_UPD_PO_STATS(dev_net(dev), IPSTATS_MIB_OUT, skb->len);
+ #if IS_ENABLED(CONFIG_IPV6)
+ } else if (skb->protocol == htons(ETH_P_IPV6)) {
+ struct inet6_dev *in6dev = __in6_dev_get(dev);
+
+ if (in6dev)
+ IP6_UPD_PO_STATS(dev_net(dev), in6dev,
+ IPSTATS_MIB_OUT, skb->len);
+ #endif
+ }
+ }
+ EXPORT_SYMBOL_GPL(mpls_stats_inc_outucastpkts);
+
-static u32 mpls_multipath_hash(struct mpls_route *rt,
- struct sk_buff *skb, bool bos)
+static u32 mpls_multipath_hash(struct mpls_route *rt, struct sk_buff *skb)
{
struct mpls_entry_decoded dec;
+ unsigned int mpls_hdr_len = 0;
struct mpls_shim_hdr *hdr;
bool eli_seen = false;
int label_index;
@@@ -280,27 -308,24 +310,24 @@@ static int mpls_forward(struct sk_buff
hdr = mpls_hdr(skb);
dec = mpls_entry_decode(hdr);
- /* Pop the label */
- skb_pull(skb, sizeof(*hdr));
- skb_reset_network_header(skb);
-
- skb_orphan(skb);
-
rt = mpls_route_input_rcu(net, dec.label);
- if (!rt)
+ if (!rt) {
+ MPLS_INC_STATS(mdev, rx_noroute);
goto drop;
+ }
- nh = mpls_select_multipath(rt, skb, dec.bos);
+ nh = mpls_select_multipath(rt, skb);
if (!nh)
- goto drop;
-
- /* Find the output device */
- out_dev = rcu_dereference(nh->nh_dev);
- if (!mpls_output_possible(out_dev))
- goto drop;
+ goto err;
+ /* Pop the label */
+ skb_pull(skb, sizeof(*hdr));
+ skb_reset_network_header(skb);
+
+ skb_orphan(skb);
+
if (skb_warn_if_lro(skb))
- goto drop;
+ goto err;
skb_forward_csum(skb);
^ permalink raw reply
* [PATCH v2 net 0/2] ipv6: fix ip6_tnl_parse_tlv_enc_lim() issues
From: Eric Dumazet @ 2017-01-24 0:43 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Willem de Bruijn, Alexei Starovoitov,
Eric Dumazet
First patch fixes ip6_tnl_parse_tlv_enc_lim() callers,
bug added in linux-3.7
Second patch fixes ip6_tnl_parse_tlv_enc_lim() itself,
bug predates linux-2.6.12
Based on a report from Dmitry Vyukov, thanks to KASAN.
Eric Dumazet (2):
ip6_tunnel: must reload ipv6h in ip6ip6_tnl_xmit()
ipv6: fix ip6_tnl_parse_tlv_enc_lim()
net/ipv6/ip6_gre.c | 3 +++
net/ipv6/ip6_tunnel.c | 36 ++++++++++++++++++++++++------------
2 files changed, 27 insertions(+), 12 deletions(-)
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply
* [PATCH v2 net 1/2] ip6_tunnel: must reload ipv6h in ip6ip6_tnl_xmit()
From: Eric Dumazet @ 2017-01-24 0:43 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Willem de Bruijn, Alexei Starovoitov,
Eric Dumazet, Dmitry Kozlov
In-Reply-To: <20170124004306.19236-1-edumazet@google.com>
Since ip6_tnl_parse_tlv_enc_lim() can call pskb_may_pull(),
we must reload any pointer that was related to skb->head
(or skb->data), or risk use after free.
Fixes: c12b395a4664 ("gre: Support GRE over IPv6")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Dmitry Kozlov <xeb@mail.ru>
---
net/ipv6/ip6_gre.c | 3 +++
net/ipv6/ip6_tunnel.c | 2 ++
2 files changed, 5 insertions(+)
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 75b6108234dd05a54af0ae51c7c11eaf1ca2..558631860d91bbcb1321a4b566b6e92ddcaf 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -582,6 +582,9 @@ static inline int ip6gre_xmit_ipv6(struct sk_buff *skb, struct net_device *dev)
return -1;
offset = ip6_tnl_parse_tlv_enc_lim(skb, skb_network_header(skb));
+ /* ip6_tnl_parse_tlv_enc_lim() might have reallocated skb->head */
+ ipv6h = ipv6_hdr(skb);
+
if (offset > 0) {
struct ipv6_tlv_tnl_enc_lim *tel;
tel = (struct ipv6_tlv_tnl_enc_lim *)&skb_network_header(skb)[offset];
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 753d6d0860fb14c100ab8b20799782ab8160..02923f956ac8ba5f3270e8d5282fd9637c2d 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1303,6 +1303,8 @@ ip6ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev)
fl6.flowlabel = key->label;
} else {
offset = ip6_tnl_parse_tlv_enc_lim(skb, skb_network_header(skb));
+ /* ip6_tnl_parse_tlv_enc_lim() might have reallocated skb->head */
+ ipv6h = ipv6_hdr(skb);
if (offset > 0) {
struct ipv6_tlv_tnl_enc_lim *tel;
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v2 net 2/2] ipv6: fix ip6_tnl_parse_tlv_enc_lim()
From: Eric Dumazet @ 2017-01-24 0:43 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Willem de Bruijn, Alexei Starovoitov,
Eric Dumazet
In-Reply-To: <20170124004306.19236-1-edumazet@google.com>
This function suffers from multiple issues.
First one is that pskb_may_pull() may reallocate skb->head,
so the 'raw' pointer needs either to be reloaded or not used at all.
Second issue is that NEXTHDR_DEST handling does not validate
that the options are present in skb->data, so we might read
garbage or access non existent memory.
With help from Willem de Bruijn.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Willem de Bruijn <willemb@google.com>
---
net/ipv6/ip6_tunnel.c | 34 ++++++++++++++++++++++------------
1 file changed, 22 insertions(+), 12 deletions(-)
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 02923f956ac8ba5f3270e8d5282fd9637c2d..ff8ee06491c335d209e86bb15f2526ab1915 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -400,18 +400,19 @@ ip6_tnl_dev_uninit(struct net_device *dev)
__u16 ip6_tnl_parse_tlv_enc_lim(struct sk_buff *skb, __u8 *raw)
{
- const struct ipv6hdr *ipv6h = (const struct ipv6hdr *) raw;
- __u8 nexthdr = ipv6h->nexthdr;
- __u16 off = sizeof(*ipv6h);
+ const struct ipv6hdr *ipv6h = (const struct ipv6hdr *)raw;
+ unsigned int nhoff = raw - skb->data;
+ unsigned int off = nhoff + sizeof(*ipv6h);
+ u8 next, nexthdr = ipv6h->nexthdr;
while (ipv6_ext_hdr(nexthdr) && nexthdr != NEXTHDR_NONE) {
- __u16 optlen = 0;
struct ipv6_opt_hdr *hdr;
- if (raw + off + sizeof(*hdr) > skb->data &&
- !pskb_may_pull(skb, raw - skb->data + off + sizeof (*hdr)))
+ u16 optlen;
+
+ if (!pskb_may_pull(skb, off + sizeof(*hdr)))
break;
- hdr = (struct ipv6_opt_hdr *) (raw + off);
+ hdr = (struct ipv6_opt_hdr *)(skb->data + off);
if (nexthdr == NEXTHDR_FRAGMENT) {
struct frag_hdr *frag_hdr = (struct frag_hdr *) hdr;
if (frag_hdr->frag_off)
@@ -422,20 +423,29 @@ __u16 ip6_tnl_parse_tlv_enc_lim(struct sk_buff *skb, __u8 *raw)
} else {
optlen = ipv6_optlen(hdr);
}
+ /* cache hdr->nexthdr, since pskb_may_pull() might
+ * invalidate hdr
+ */
+ next = hdr->nexthdr;
if (nexthdr == NEXTHDR_DEST) {
- __u16 i = off + 2;
+ u16 i = 2;
+
+ /* Remember : hdr is no longer valid at this point. */
+ if (!pskb_may_pull(skb, off + optlen))
+ break;
+
while (1) {
struct ipv6_tlv_tnl_enc_lim *tel;
/* No more room for encapsulation limit */
- if (i + sizeof (*tel) > off + optlen)
+ if (i + sizeof(*tel) > optlen)
break;
- tel = (struct ipv6_tlv_tnl_enc_lim *) &raw[i];
+ tel = (struct ipv6_tlv_tnl_enc_lim *) skb->data + off + i;
/* return index of option if found and valid */
if (tel->type == IPV6_TLV_TNL_ENCAP_LIMIT &&
tel->length == 1)
- return i;
+ return i + off - nhoff;
/* else jump to next option */
if (tel->type)
i += tel->length + 2;
@@ -443,7 +453,7 @@ __u16 ip6_tnl_parse_tlv_enc_lim(struct sk_buff *skb, __u8 *raw)
i++;
}
}
- nexthdr = hdr->nexthdr;
+ nexthdr = next;
off += optlen;
}
return 0;
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* Re: XDP offload to hypervisor
From: Alexei Starovoitov @ 2017-01-24 1:02 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: John Fastabend, jasowang, john.r.fastabend, netdev, daniel
In-Reply-To: <20170123230727-mutt-send-email-mst@kernel.org>
On Mon, Jan 23, 2017 at 11:40:29PM +0200, Michael S. Tsirkin wrote:
> I've been thinking about passing XDP programs from guest to the
> hypervisor. Basically, after getting an incoming packet, we could run
> an XDP program in host kernel.
>
> If the result is XDP_DROP or XDP_TX we don't need to wake up the guest at all!
that's an interesting idea!
Long term 'xdp offload' needs to be defined, since NICs become smarter
and can accelerate xdp programs.
So pushing the xdp program down from virtio in the guest into host
and from x86 into nic cpu should probably be handled through the same api.
> When using tun for networking - especially with adjust_head - this
> unfortunately probably means we need to do a data copy unless there is
> enough headroom. How much is enough though?
Frankly I don't understand the whole virtio nit picking that was happening.
imo virtio+xdp by itself is only useful for debugging, development and testing
of xdp programs in a VM. The discussion about performance of virtio+xdp
will only be meaningful when corresponding host part is done.
Likely in the form of vhost extensions and may be driver changes.
Trying to optimize virtio+xdp when host is doing traditional skb+vhost
isn't going to be impactful.
But when host can do xdp in phyiscal NIC that can deliver raw
pages into vhost that gets picked up by guest virtio, then we hopefully
will be around 10G line rate. page pool is likely needed in such scenario.
Some new xdp action like xdp_tx_into_vhost or whatever.
And guest will be seeing full pages that host nic provided and discussion
about headroom will be automatically solved.
Arguing that skb has 64-byte headroom and therefore we need to
reduce XDP_PACKET_HEADROOM is really upside down.
> Another issue is around host/guest ABI. Guest BPF could add new features
> at any point. What if hypervisor can not support it all? I guess we
> could try loading program into hypervisor and run it within guest on
> failure to load, but this ignores question of cross-version
> compatibility - someone might start guest on a new host
> then try to move to an old one. So we will need an option
> "behave like an older host" such that guest can start and then
> move to an older host later. This will likely mean
> implementing this validation of programs in qemu userspace unless linux
> can supply something like this. Is this (disabling some features)
> something that might be of interest to larger bpf community?
In case of x86->nic offload not all xdp features will be supported
by the nic and that is expected. The user will request 'offload of xdp prog'
in some form and if it cannot be done, then xdp programs will run
on x86 as before. Same thing, I imagine, is applicable to virtio->host
offload. Therefore I don't see a need for user space visible
feature negotiation.
> With a device such as macvtap there exist configurations where a single
> guest is in control of the device (aka passthrough mode) in that case
> there's a potential to run xdp on host before host skb is built, unless
> host already has an xdp program attached. If it does we could run the
> program within guest, but what if a guest program got attached first?
> Maybe we should pass a flag in the packet "xdp passed on this packet in
> host". Then, guest can skip running it. Unless we do a full reset
> there's always a potential for packets to slip through, e.g. on xdp
> program changes. Maybe a flush command is needed, or force queue or
> device reset to make sure nothing is going on. Does this make sense?
All valid questions and concerns.
Since there is still no xdp_adjust_head support in virtio,
it feels kinda early to get into detailed 'virtio offload' discussion.
^ permalink raw reply
* [PATCH] net: ks8851: Drop eeprom_size structure member
From: Stephen Boyd @ 2017-01-24 1:49 UTC (permalink / raw)
To: David S. Miller; +Cc: linux-kernel, netdev
After commit 51b7b1c34e19 (KSZ8851-SNL: Add ethtool support for
EEPROM via eeprom_93cx6, 2011-11-21) this structure member is
unused. Delete it.
Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
---
Found while debugging an eeprom issue.
drivers/net/ethernet/micrel/ks8851.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index e7e1aff40bd9..955d69a8e8d3 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -84,7 +84,6 @@ union ks8851_tx_hdr {
* @rc_ier: Cached copy of KS_IER.
* @rc_ccr: Cached copy of KS_CCR.
* @rc_rxqcr: Cached copy of KS_RXQCR.
- * @eeprom_size: Companion eeprom size in Bytes, 0 if no eeprom
* @eeprom: 93CX6 EEPROM state for accessing on-board EEPROM.
* @vdd_reg: Optional regulator supplying the chip
* @vdd_io: Optional digital power supply for IO
@@ -120,7 +119,6 @@ struct ks8851_net {
u16 rc_ier;
u16 rc_rxqcr;
u16 rc_ccr;
- u16 eeprom_size;
struct mii_if_info mii;
struct ks8851_rxctrl rxctrl;
@@ -1533,11 +1531,6 @@ static int ks8851_probe(struct spi_device *spi)
/* cache the contents of the CCR register for EEPROM, etc. */
ks->rc_ccr = ks8851_rdreg16(ks, KS_CCR);
- if (ks->rc_ccr & CCR_EEPROM)
- ks->eeprom_size = 128;
- else
- ks->eeprom_size = 0;
-
ks8851_read_selftest(ks);
ks8851_init_mac(ks);
--
2.10.0.297.gf6727b0
^ permalink raw reply related
* Re: [PATCH v2] bpf: Restrict cgroup bpf hooks to the init netns
From: Alexei Starovoitov @ 2017-01-24 2:09 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Network Development, David S. Miller, Daniel Borkmann,
Alexei Starovoitov, David Ahern
In-Reply-To: <ddd9e097f54e894197a2925731de02d1d3cae137.1485203708.git.luto@kernel.org>
On Mon, Jan 23, 2017 at 12:36:08PM -0800, Andy Lutomirski wrote:
> To see how cgroup+bpf interacts with network namespaces, I wrote a
> little program called show_bind that calls getsockopt(...,
> SO_BINDTODEVICE, ...) and prints the result. It did this:
>
> # ./ip link add dev vrf0 type vrf table 10
> # ./ip vrf exec vrf0 ./show_bind
> Default binding is "vrf0"
> # ./ip vrf exec vrf0 unshare -n ./show_bind
> show_bind: getsockopt: No such device
>
> What's happening here is that "ip vrf" looks up vrf0's ifindex in
> the init netns and installs a hook that binds sockets to that
> ifindex. When the hook runs in a different netns, it sets
> sk_bound_dev_if to an ifindex from the wrong netns, resulting in
> incorrect behavior. In this particular example, the ifindex was 4
> and there was no ifindex 4 in the new netns. If there had been,
> this test would have malfunctioned differently
>
> Since it's rather late in the release cycle, let's punt. This patch
> makes it impossible to install cgroup+bpf hooks outside the init
> netns and makes them not run on sockets that aren't in the init
> netns.
>
> In a future release, it should be relatively straightforward to make
> these hooks be local to a netns and, if needed, to add a flag so
> that hooks can be made global if necessary. Global hooks should
> presumably be constrained so that they can't write to any ifindex
> fields.
>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: David Ahern <dsa@cumulusnetworks.com>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>
> DaveM, this mitigates a bug in a feature that's new in 4.10, and the
> bug can be hit using current iproute2 -git. please consider this for
> -net.
>
> Changes from v1:
> - Fix the commit message. 'git commit' was very clever and thought that
> all the interesting bits of the test case were intended to be comments
> and stripped them. Whoops!
>
> kernel/bpf/cgroup.c | 21 +++++++++++++++++++++
> kernel/bpf/syscall.c | 11 +++++++++++
> 2 files changed, 32 insertions(+)
>
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index a515f7b007c6..a824f543de69 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -143,6 +143,17 @@ int __cgroup_bpf_run_filter_skb(struct sock *sk,
> if (!sk || !sk_fullsock(sk))
> return 0;
>
> + /*
> + * For now, socket bpf hooks attached to cgroups can only be
> + * installed in the init netns and only affect the init netns.
> + * This could be relaxed in the future once some semantic issues
> + * are resolved. For example, ifindexes belonging to one netns
> + * should probably not be visible to hooks installed by programs
> + * running in a different netns.
> + */
> + if (sock_net(sk) != &init_net)
> + return 0;
Nack.
Such check will create absolutely broken abi that we won't be able to fix later.
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index e89acea22ecf..c0bbc55e244d 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -902,6 +902,17 @@ static int bpf_prog_attach(const union bpf_attr *attr)
> struct cgroup *cgrp;
> enum bpf_prog_type ptype;
>
> + /*
> + * For now, socket bpf hooks attached to cgroups can only be
> + * installed in the init netns and only affect the init netns.
> + * This could be relaxed in the future once some semantic issues
> + * are resolved. For example, ifindexes belonging to one netns
> + * should probably not be visible to hooks installed by programs
> + * running in a different netns.
the comment is bogus and shows complete lack of understanding
how bpf is working and how it's being used.
See SKF_AD_IFINDEX that was in classic bpf forever.
> + */
> + if (current->nsproxy->net_ns != &init_net)
> + return -EINVAL;
this restriction I actually don't mind, since it indeed can be
relaxed later, but please make it proper with net_eq()
^ permalink raw reply
* Re: [PATCH net-next] bpf, lpm: fix kfree of im_node in trie_update_elem
From: David Miller @ 2017-01-24 2:18 UTC (permalink / raw)
To: daniel; +Cc: ast, daniel, netdev
In-Reply-To: <6052894aa1bd4df5a5c7aaa05f5b63a980cbe371.1485217298.git.daniel@iogearbox.net>
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Tue, 24 Jan 2017 01:26:46 +0100
> We need to initialize im_node to NULL, otherwise in case of error path
> it gets passed to kfree() as uninitialized pointer.
>
> Fixes: b95a5c4db09b ("bpf: add a longest prefix match trie map implementation")
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
> Mentioned it in http://patchwork.ozlabs.org/patch/718070/, but
> was probably overlooked.
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] Bluetooth: hidp: might sleep error in hidp_session_thread
From: Brian Norris @ 2017-01-24 2:31 UTC (permalink / raw)
To: Jeffy Chen
Cc: linux-bluetooth, Douglas Anderson, Johan Hedberg, netdev,
linux-kernel, David S. Miller, Marcel Holtmann, Gustavo Padovan,
Peter Hurley, Peter Zijlstra, Guenter Roeck
In-Reply-To: <1484920328-20522-1-git-send-email-jeffy.chen@rock-chips.com>
Hi Jeffy,
On Fri, Jan 20, 2017 at 09:52:08PM +0800, Jeffy Chen wrote:
> [ 39.044329] do not call blocking ops when !TASK_RUNNING; state=1 set
> at [<ffffffbffc290358>] hidp_session_thread+0x110/0x568 [hidp]
> ...
> [ 40.159664] Call trace:
> [ 40.162122] [<ffffffc00024ae08>] __might_sleep+0x64/0x90
> [ 40.167443] [<ffffffc00080568c>] lock_sock_nested+0x30/0x78
> [ 40.173047] [<ffffffbffc1b3ca0>] l2cap_sock_sendmsg+0x90/0xf0
> [bluetooth]
> [ 40.179842] [<ffffffc0008012c4>] sock_sendmsg+0x4c/0x68
> [ 40.185072] [<ffffffc000801414>] kernel_sendmsg+0x54/0x68
> [ 40.190477] [<ffffffbffc28f4d0>] hidp_send_frame+0x78/0xa0 [hidp]
> [ 40.196574] [<ffffffbffc28f53c>] hidp_process_transmit+0x44/0x98
> [hidp]
> [ 40.203191] [<ffffffbffc2905ac>] hidp_session_thread+0x364/0x568
> [hidp]
Am I crazy, or are several other protocols broken like this too? I see a
similar structure in net/bluetooth/bnep/core.c and
net/bluetooth/cmtp/core.c, at least, each of which also call
kernel_sendmsg(), which might be an l2cap socket (...I think? I'm not a
bluetooth expert really).
>
> Following (https://lwn.net/Articles/628628/).
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
> net/bluetooth/hidp/core.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index 0bec458..bfd3fb8 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -1180,7 +1180,9 @@ static void hidp_session_run(struct hidp_session *session)
> struct sock *ctrl_sk = session->ctrl_sock->sk;
> struct sock *intr_sk = session->intr_sock->sk;
> struct sk_buff *skb;
> + DEFINE_WAIT_FUNC(wait, woken_wake_function);
>
> + add_wait_queue(sk_sleep(intr_sk), &wait);
> for (;;) {
> /*
> * This thread can be woken up two ways:
> @@ -1188,12 +1190,10 @@ static void hidp_session_run(struct hidp_session *session)
> * session->terminate flag and wakes this thread up.
> * - Via modifying the socket state of ctrl/intr_sock. This
> * thread is woken up by ->sk_state_changed().
> - *
> - * Note: set_current_state() performs any necessary
> - * memory-barriers for us.
> */
> - set_current_state(TASK_INTERRUPTIBLE);
>
> + /* Ensure session->terminate is updated */
> + smp_mb__before_atomic();
> if (atomic_read(&session->terminate))
> break;
>
> @@ -1227,11 +1227,14 @@ static void hidp_session_run(struct hidp_session *session)
> hidp_process_transmit(session, &session->ctrl_transmit,
> session->ctrl_sock);
>
> - schedule();
> + wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
I think this looks mostly good, except what about the
hidp_session_terminate() condition? In that case, you're running
wake_up_process() -- which won't set WQ_FLAG_WOKEN for us. So what
happens if we see a hidp_session_terminate() call in between the check
for the ->terminate count, but before we call wait_woken()? IIUC, I
think we'll just ignore the call and keep waiting for the next wake
signal.
I think you might need to rework hidp_session_terminate() too, to
actually target the wait queue and not just the processes.
IIUC, of course. I could be wrong...
Brian
> }
> + remove_wait_queue(sk_sleep(intr_sk), &wait);
>
> atomic_inc(&session->terminate);
> - set_current_state(TASK_RUNNING);
> +
> + /* Ensure session->terminate is updated */
> + smp_mb__after_atomic();
> }
>
> /*
> --
> 2.1.4
>
>
^ permalink raw reply
* Re: [PATCH v2] bpf: Restrict cgroup bpf hooks to the init netns
From: David Ahern @ 2017-01-24 2:31 UTC (permalink / raw)
To: Alexei Starovoitov, Andy Lutomirski
Cc: Network Development, David S. Miller, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <20170124020950.GA72992@ast-mbp.thefacebook.com>
On 1/23/17 7:09 PM, Alexei Starovoitov wrote:
>> + */
>> + if (current->nsproxy->net_ns != &init_net)
>> + return -EINVAL;
>
> this restriction I actually don't mind, since it indeed can be
> relaxed later, but please make it proper with net_eq()
>
I do mind. Why have different restrictions for the skb and sk filters?
I have already shown that ip can alleviate the management aspects of the implementation -- just like ip netns does.
^ permalink raw reply
* Re: [PATCH v2] bpf: Restrict cgroup bpf hooks to the init netns
From: Andy Lutomirski @ 2017-01-24 2:39 UTC (permalink / raw)
To: David Ahern
Cc: Alexei Starovoitov, Andy Lutomirski, Network Development,
David S. Miller, Daniel Borkmann, Alexei Starovoitov
In-Reply-To: <678f4d6f-e2f8-eb37-207a-c77269507f9d@cumulusnetworks.com>
On Mon, Jan 23, 2017 at 6:31 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
> On 1/23/17 7:09 PM, Alexei Starovoitov wrote:
>>> + */
>>> + if (current->nsproxy->net_ns != &init_net)
>>> + return -EINVAL;
>>
>> this restriction I actually don't mind, since it indeed can be
>> relaxed later, but please make it proper with net_eq()
>>
>
> I do mind. Why have different restrictions for the skb and sk filters?
>
> I have already shown that ip can alleviate the management aspects of the implementation -- just like ip netns does.
I'm not sure I followed what you meant. If I understood right (which
is a big if) you're saying that ip vrf when run in a netns works
correctly in that netns. I agree, but I think that it would continue
to work (even more reliably) if the hooks only executed in the netns
in which they were created. I think that would probably be a good
improvement, and it could be done on top of my patch, but I'm not
about to write that code for 4.10.
--Andy
^ 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