* [PATCH bpf-next v5 1/6] flow_dissector: switch kernel context to struct bpf_flow_dissector
2019-04-15 17:37 [PATCH bpf-next v5 0/6] net: flow_dissector: trigger BPF hook when called from eth_get_headlen Stanislav Fomichev
@ 2019-04-15 17:37 ` Stanislav Fomichev
2019-04-15 17:37 ` [PATCH bpf-next v5 2/6] bpf: when doing BPF_PROG_TEST_RUN for flow dissector use no-skb mode Stanislav Fomichev
` (4 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Stanislav Fomichev @ 2019-04-15 17:37 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, simon.horman, willemb, peterpenkov96,
Stanislav Fomichev
struct bpf_flow_dissector has a small subset of sk_buff fields that
flow dissector BPF program is allowed to access and an optional
pointer to real skb. Real skb is used only in bpf_skb_load_bytes
helper to read non-linear data.
The real motivation for this is to be able to call flow dissector
from eth_get_headlen context where we don't have an skb and need
to dissect raw bytes.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
include/linux/skbuff.h | 4 ++
include/net/flow_dissector.h | 7 +++
include/net/sch_generic.h | 11 ++--
net/bpf/test_run.c | 4 --
net/core/filter.c | 105 +++++++++++++++++++++++++++--------
net/core/flow_dissector.c | 45 +++++++--------
6 files changed, 117 insertions(+), 59 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a06275a618f0..4d2365ffa444 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1273,6 +1273,10 @@ static inline int skb_flow_dissector_bpf_prog_detach(const union bpf_attr *attr)
}
#endif
+struct bpf_flow_dissector;
+bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
+ __be16 proto, int nhoff, int hlen);
+
struct bpf_flow_keys;
bool __skb_flow_bpf_dissect(struct bpf_prog *prog,
const struct sk_buff *skb,
diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index 2b26979efb48..7c5a8d9a8d2a 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -305,4 +305,11 @@ static inline void *skb_flow_dissector_target(struct flow_dissector *flow_dissec
return ((char *)target_container) + flow_dissector->offset[key_id];
}
+struct bpf_flow_dissector {
+ struct bpf_flow_keys *flow_keys;
+ const struct sk_buff *skb;
+ void *data;
+ void *data_end;
+};
+
#endif
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index e8f85cd2afce..21f434f3ac9e 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -364,13 +364,10 @@ struct tcf_proto {
};
struct qdisc_skb_cb {
- union {
- struct {
- unsigned int pkt_len;
- u16 slave_dev_queue_mapping;
- u16 tc_classid;
- };
- struct bpf_flow_keys *flow_keys;
+ struct {
+ unsigned int pkt_len;
+ u16 slave_dev_queue_mapping;
+ u16 tc_classid;
};
#define QDISC_CB_PRIV_LEN 20
unsigned char data[QDISC_CB_PRIV_LEN];
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 2221573dacdb..006ad865f7fb 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -382,7 +382,6 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
u32 repeat = kattr->test.repeat;
struct bpf_flow_keys flow_keys;
u64 time_start, time_spent = 0;
- struct bpf_skb_data_end *cb;
u32 retval, duration;
struct sk_buff *skb;
struct sock *sk;
@@ -423,9 +422,6 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
current->nsproxy->net_ns->loopback_dev);
skb_reset_network_header(skb);
- cb = (struct bpf_skb_data_end *)skb->cb;
- cb->qdisc_cb.flow_keys = &flow_keys;
-
if (!repeat)
repeat = 1;
diff --git a/net/core/filter.c b/net/core/filter.c
index 95a27fdf9a40..52c2424226e9 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1730,6 +1730,40 @@ static const struct bpf_func_proto bpf_skb_load_bytes_proto = {
.arg4_type = ARG_CONST_SIZE,
};
+BPF_CALL_4(bpf_flow_dissector_load_bytes,
+ const struct bpf_flow_dissector *, ctx, u32, offset,
+ void *, to, u32, len)
+{
+ void *ptr;
+
+ if (unlikely(offset > 0xffff))
+ goto err_clear;
+
+ if (unlikely(!ctx->skb))
+ goto err_clear;
+
+ ptr = skb_header_pointer(ctx->skb, offset, len, to);
+ if (unlikely(!ptr))
+ goto err_clear;
+ if (ptr != to)
+ memcpy(to, ptr, len);
+
+ return 0;
+err_clear:
+ memset(to, 0, len);
+ return -EFAULT;
+}
+
+static const struct bpf_func_proto bpf_flow_dissector_load_bytes_proto = {
+ .func = bpf_flow_dissector_load_bytes,
+ .gpl_only = false,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_CTX,
+ .arg2_type = ARG_ANYTHING,
+ .arg3_type = ARG_PTR_TO_UNINIT_MEM,
+ .arg4_type = ARG_CONST_SIZE,
+};
+
BPF_CALL_5(bpf_skb_load_bytes_relative, const struct sk_buff *, skb,
u32, offset, void *, to, u32, len, u32, start_header)
{
@@ -6119,7 +6153,7 @@ flow_dissector_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
{
switch (func_id) {
case BPF_FUNC_skb_load_bytes:
- return &bpf_skb_load_bytes_proto;
+ return &bpf_flow_dissector_load_bytes_proto;
default:
return bpf_base_func_proto(func_id);
}
@@ -6246,9 +6280,7 @@ static bool bpf_skb_is_valid_access(int off, int size, enum bpf_access_type type
return false;
break;
case bpf_ctx_range_ptr(struct __sk_buff, flow_keys):
- if (size != sizeof(__u64))
- return false;
- break;
+ return false;
case bpf_ctx_range(struct __sk_buff, tstamp):
if (size != sizeof(__u64))
return false;
@@ -6283,7 +6315,6 @@ static bool sk_filter_is_valid_access(int off, int size,
case bpf_ctx_range(struct __sk_buff, data):
case bpf_ctx_range(struct __sk_buff, data_meta):
case bpf_ctx_range(struct __sk_buff, data_end):
- case bpf_ctx_range_ptr(struct __sk_buff, flow_keys):
case bpf_ctx_range_till(struct __sk_buff, family, local_port):
case bpf_ctx_range(struct __sk_buff, tstamp):
case bpf_ctx_range(struct __sk_buff, wire_len):
@@ -6310,7 +6341,6 @@ static bool cg_skb_is_valid_access(int off, int size,
switch (off) {
case bpf_ctx_range(struct __sk_buff, tc_classid):
case bpf_ctx_range(struct __sk_buff, data_meta):
- case bpf_ctx_range_ptr(struct __sk_buff, flow_keys):
case bpf_ctx_range(struct __sk_buff, wire_len):
return false;
case bpf_ctx_range(struct __sk_buff, data):
@@ -6356,7 +6386,6 @@ static bool lwt_is_valid_access(int off, int size,
case bpf_ctx_range(struct __sk_buff, tc_classid):
case bpf_ctx_range_till(struct __sk_buff, family, local_port):
case bpf_ctx_range(struct __sk_buff, data_meta):
- case bpf_ctx_range_ptr(struct __sk_buff, flow_keys):
case bpf_ctx_range(struct __sk_buff, tstamp):
case bpf_ctx_range(struct __sk_buff, wire_len):
return false;
@@ -6599,7 +6628,6 @@ static bool tc_cls_act_is_valid_access(int off, int size,
case bpf_ctx_range(struct __sk_buff, data_end):
info->reg_type = PTR_TO_PACKET_END;
break;
- case bpf_ctx_range_ptr(struct __sk_buff, flow_keys):
case bpf_ctx_range_till(struct __sk_buff, family, local_port):
return false;
}
@@ -6801,7 +6829,6 @@ static bool sk_skb_is_valid_access(int off, int size,
switch (off) {
case bpf_ctx_range(struct __sk_buff, tc_classid):
case bpf_ctx_range(struct __sk_buff, data_meta):
- case bpf_ctx_range_ptr(struct __sk_buff, flow_keys):
case bpf_ctx_range(struct __sk_buff, tstamp):
case bpf_ctx_range(struct __sk_buff, wire_len):
return false;
@@ -6875,24 +6902,65 @@ static bool flow_dissector_is_valid_access(int off, int size,
const struct bpf_prog *prog,
struct bpf_insn_access_aux *info)
{
+ const int size_default = sizeof(__u32);
+
+ if (off < 0 || off >= sizeof(struct __sk_buff))
+ return false;
+
if (type == BPF_WRITE)
return false;
switch (off) {
case bpf_ctx_range(struct __sk_buff, data):
+ if (size != size_default)
+ return false;
info->reg_type = PTR_TO_PACKET;
- break;
+ return true;
case bpf_ctx_range(struct __sk_buff, data_end):
+ if (size != size_default)
+ return false;
info->reg_type = PTR_TO_PACKET_END;
- break;
+ return true;
case bpf_ctx_range_ptr(struct __sk_buff, flow_keys):
+ if (size != sizeof(__u64))
+ return false;
info->reg_type = PTR_TO_FLOW_KEYS;
- break;
+ return true;
default:
return false;
}
+}
- return bpf_skb_is_valid_access(off, size, type, prog, info);
+static u32 flow_dissector_convert_ctx_access(enum bpf_access_type type,
+ const struct bpf_insn *si,
+ struct bpf_insn *insn_buf,
+ struct bpf_prog *prog,
+ u32 *target_size)
+
+{
+ struct bpf_insn *insn = insn_buf;
+
+ switch (si->off) {
+ case offsetof(struct __sk_buff, data):
+ *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct bpf_flow_dissector, data),
+ si->dst_reg, si->src_reg,
+ offsetof(struct bpf_flow_dissector, data));
+ break;
+
+ case offsetof(struct __sk_buff, data_end):
+ *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct bpf_flow_dissector, data_end),
+ si->dst_reg, si->src_reg,
+ offsetof(struct bpf_flow_dissector, data_end));
+ break;
+
+ case offsetof(struct __sk_buff, flow_keys):
+ *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct bpf_flow_dissector, flow_keys),
+ si->dst_reg, si->src_reg,
+ offsetof(struct bpf_flow_dissector, flow_keys));
+ break;
+ }
+
+ return insn - insn_buf;
}
static u32 bpf_convert_ctx_access(enum bpf_access_type type,
@@ -7199,15 +7267,6 @@ static u32 bpf_convert_ctx_access(enum bpf_access_type type,
skc_num, 2, target_size));
break;
- case offsetof(struct __sk_buff, flow_keys):
- off = si->off;
- off -= offsetof(struct __sk_buff, flow_keys);
- off += offsetof(struct sk_buff, cb);
- off += offsetof(struct qdisc_skb_cb, flow_keys);
- *insn++ = BPF_LDX_MEM(BPF_SIZEOF(void *), si->dst_reg,
- si->src_reg, off);
- break;
-
case offsetof(struct __sk_buff, tstamp):
BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, tstamp) != 8);
@@ -8212,7 +8271,7 @@ const struct bpf_prog_ops sk_msg_prog_ops = {
const struct bpf_verifier_ops flow_dissector_verifier_ops = {
.get_func_proto = flow_dissector_func_proto,
.is_valid_access = flow_dissector_is_valid_access,
- .convert_ctx_access = bpf_convert_ctx_access,
+ .convert_ctx_access = flow_dissector_convert_ctx_access,
};
const struct bpf_prog_ops flow_dissector_prog_ops = {
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 795449713ba4..ef6d9443cc75 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -688,39 +688,34 @@ bool __skb_flow_bpf_dissect(struct bpf_prog *prog,
struct flow_dissector *flow_dissector,
struct bpf_flow_keys *flow_keys)
{
- struct bpf_skb_data_end cb_saved;
- struct bpf_skb_data_end *cb;
- u32 result;
-
- /* Note that even though the const qualifier is discarded
- * throughout the execution of the BPF program, all changes(the
- * control block) are reverted after the BPF program returns.
- * Therefore, __skb_flow_dissect does not alter the skb.
- */
-
- cb = (struct bpf_skb_data_end *)skb->cb;
+ struct bpf_flow_dissector ctx = {
+ .flow_keys = flow_keys,
+ .skb = skb,
+ .data = skb->data,
+ .data_end = skb->data + skb_headlen(skb),
+ };
+
+ return bpf_flow_dissect(prog, &ctx, skb->protocol,
+ skb_network_offset(skb), skb_headlen(skb));
+}
- /* Save Control Block */
- memcpy(&cb_saved, cb, sizeof(cb_saved));
- memset(cb, 0, sizeof(*cb));
+bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
+ __be16 proto, int nhoff, int hlen)
+{
+ struct bpf_flow_keys *flow_keys = ctx->flow_keys;
+ u32 result;
/* Pass parameters to the BPF program */
memset(flow_keys, 0, sizeof(*flow_keys));
- cb->qdisc_cb.flow_keys = flow_keys;
- flow_keys->n_proto = skb->protocol;
- flow_keys->nhoff = skb_network_offset(skb);
+ flow_keys->n_proto = proto;
+ flow_keys->nhoff = nhoff;
flow_keys->thoff = flow_keys->nhoff;
- bpf_compute_data_pointers((struct sk_buff *)skb);
- result = BPF_PROG_RUN(prog, skb);
-
- /* Restore state */
- memcpy(cb, &cb_saved, sizeof(cb_saved));
+ result = BPF_PROG_RUN(prog, ctx);
- flow_keys->nhoff = clamp_t(u16, flow_keys->nhoff,
- skb_network_offset(skb), skb->len);
+ flow_keys->nhoff = clamp_t(u16, flow_keys->nhoff, nhoff, hlen);
flow_keys->thoff = clamp_t(u16, flow_keys->thoff,
- flow_keys->nhoff, skb->len);
+ flow_keys->nhoff, hlen);
return result == BPF_OK;
}
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH bpf-next v5 2/6] bpf: when doing BPF_PROG_TEST_RUN for flow dissector use no-skb mode
2019-04-15 17:37 [PATCH bpf-next v5 0/6] net: flow_dissector: trigger BPF hook when called from eth_get_headlen Stanislav Fomichev
2019-04-15 17:37 ` [PATCH bpf-next v5 1/6] flow_dissector: switch kernel context to struct bpf_flow_dissector Stanislav Fomichev
@ 2019-04-15 17:37 ` Stanislav Fomichev
2019-04-15 17:37 ` [PATCH bpf-next v5 3/6] net: plumb network namespace into __skb_flow_dissect Stanislav Fomichev
` (3 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Stanislav Fomichev @ 2019-04-15 17:37 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, simon.horman, willemb, peterpenkov96,
Stanislav Fomichev
Now that we have bpf_flow_dissect which can work on raw data,
use it when doing BPF_PROG_TEST_RUN for flow dissector.
Simplifies bpf_prog_test_run_flow_dissector and allows us to
test no-skb mode.
Note, that previously, with bpf_flow_dissect_skb we used to call
eth_type_trans which pulled L2 (ETH_HLEN) header and we explicitly called
skb_reset_network_header. That means flow_keys->nhoff would be
initialized to 0 (skb_network_offset) in init_flow_keys.
Now we call bpf_flow_dissect with nhoff set to ETH_HLEN and need
to undo it once the dissection is done to preserve the existing behavior.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
net/bpf/test_run.c | 47 +++++++++++++++++-----------------------------
1 file changed, 17 insertions(+), 30 deletions(-)
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 006ad865f7fb..db2ec88ab129 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -379,12 +379,12 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
union bpf_attr __user *uattr)
{
u32 size = kattr->test.data_size_in;
+ struct bpf_flow_dissector ctx = {};
u32 repeat = kattr->test.repeat;
struct bpf_flow_keys flow_keys;
u64 time_start, time_spent = 0;
+ const struct ethhdr *eth;
u32 retval, duration;
- struct sk_buff *skb;
- struct sock *sk;
void *data;
int ret;
u32 i;
@@ -395,43 +395,31 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
if (kattr->test.ctx_in || kattr->test.ctx_out)
return -EINVAL;
- data = bpf_test_init(kattr, size, NET_SKB_PAD + NET_IP_ALIGN,
- SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
+ if (size < ETH_HLEN)
+ return -EINVAL;
+
+ data = bpf_test_init(kattr, size, 0, 0);
if (IS_ERR(data))
return PTR_ERR(data);
- sk = kzalloc(sizeof(*sk), GFP_USER);
- if (!sk) {
- kfree(data);
- return -ENOMEM;
- }
- sock_net_set(sk, current->nsproxy->net_ns);
- sock_init_data(NULL, sk);
-
- skb = build_skb(data, 0);
- if (!skb) {
- kfree(data);
- kfree(sk);
- return -ENOMEM;
- }
- skb->sk = sk;
-
- skb_reserve(skb, NET_SKB_PAD + NET_IP_ALIGN);
- __skb_put(skb, size);
- skb->protocol = eth_type_trans(skb,
- current->nsproxy->net_ns->loopback_dev);
- skb_reset_network_header(skb);
+ eth = (struct ethhdr *)data;
if (!repeat)
repeat = 1;
+ ctx.flow_keys = &flow_keys;
+ ctx.data = data;
+ ctx.data_end = (__u8 *)data + size;
+
rcu_read_lock();
preempt_disable();
time_start = ktime_get_ns();
for (i = 0; i < repeat; i++) {
- retval = __skb_flow_bpf_dissect(prog, skb,
- &flow_keys_dissector,
- &flow_keys);
+ retval = bpf_flow_dissect(prog, &ctx, eth->h_proto, ETH_HLEN,
+ size);
+
+ flow_keys.nhoff -= ETH_HLEN;
+ flow_keys.thoff -= ETH_HLEN;
if (signal_pending(current)) {
preempt_enable();
@@ -464,7 +452,6 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
retval, duration);
out:
- kfree_skb(skb);
- kfree(sk);
+ kfree(data);
return ret;
}
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH bpf-next v5 3/6] net: plumb network namespace into __skb_flow_dissect
2019-04-15 17:37 [PATCH bpf-next v5 0/6] net: flow_dissector: trigger BPF hook when called from eth_get_headlen Stanislav Fomichev
2019-04-15 17:37 ` [PATCH bpf-next v5 1/6] flow_dissector: switch kernel context to struct bpf_flow_dissector Stanislav Fomichev
2019-04-15 17:37 ` [PATCH bpf-next v5 2/6] bpf: when doing BPF_PROG_TEST_RUN for flow dissector use no-skb mode Stanislav Fomichev
@ 2019-04-15 17:37 ` Stanislav Fomichev
2019-04-15 17:37 ` [PATCH bpf-next v5 4/6] flow_dissector: handle no-skb use case Stanislav Fomichev
` (2 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Stanislav Fomichev @ 2019-04-15 17:37 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, simon.horman, willemb, peterpenkov96,
Stanislav Fomichev
This new argument will be used in the next patches for the
eth_get_headlen use case. eth_get_headlen calls flow dissector
with only data (without skb) so there is currently no way to
pull attached BPF flow dissector program. With this new argument,
we can amend the callers to explicitly pass network namespace
so we can use attached BPF program.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
include/linux/skbuff.h | 19 +++++++++++--------
net/core/flow_dissector.c | 27 +++++++++++++++++----------
net/ethernet/eth.c | 5 +++--
3 files changed, 31 insertions(+), 20 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 4d2365ffa444..60cdbcde751b 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1282,7 +1282,8 @@ bool __skb_flow_bpf_dissect(struct bpf_prog *prog,
const struct sk_buff *skb,
struct flow_dissector *flow_dissector,
struct bpf_flow_keys *flow_keys);
-bool __skb_flow_dissect(const struct sk_buff *skb,
+bool __skb_flow_dissect(const struct net *net,
+ const struct sk_buff *skb,
struct flow_dissector *flow_dissector,
void *target_container,
void *data, __be16 proto, int nhoff, int hlen,
@@ -1292,8 +1293,8 @@ static inline bool skb_flow_dissect(const struct sk_buff *skb,
struct flow_dissector *flow_dissector,
void *target_container, unsigned int flags)
{
- return __skb_flow_dissect(skb, flow_dissector, target_container,
- NULL, 0, 0, 0, flags);
+ return __skb_flow_dissect(NULL, skb, flow_dissector,
+ target_container, NULL, 0, 0, 0, flags);
}
static inline bool skb_flow_dissect_flow_keys(const struct sk_buff *skb,
@@ -1301,18 +1302,19 @@ static inline bool skb_flow_dissect_flow_keys(const struct sk_buff *skb,
unsigned int flags)
{
memset(flow, 0, sizeof(*flow));
- return __skb_flow_dissect(skb, &flow_keys_dissector, flow,
- NULL, 0, 0, 0, flags);
+ return __skb_flow_dissect(NULL, skb, &flow_keys_dissector,
+ flow, NULL, 0, 0, 0, flags);
}
static inline bool
-skb_flow_dissect_flow_keys_basic(const struct sk_buff *skb,
+skb_flow_dissect_flow_keys_basic(const struct net *net,
+ const struct sk_buff *skb,
struct flow_keys_basic *flow, void *data,
__be16 proto, int nhoff, int hlen,
unsigned int flags)
{
memset(flow, 0, sizeof(*flow));
- return __skb_flow_dissect(skb, &flow_keys_basic_dissector, flow,
+ return __skb_flow_dissect(net, skb, &flow_keys_basic_dissector, flow,
data, proto, nhoff, hlen, flags);
}
@@ -2492,7 +2494,8 @@ static inline void skb_probe_transport_header(struct sk_buff *skb)
if (skb_transport_header_was_set(skb))
return;
- if (skb_flow_dissect_flow_keys_basic(skb, &keys, NULL, 0, 0, 0, 0))
+ if (skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
+ NULL, 0, 0, 0, 0))
skb_set_transport_header(skb, keys.control.thoff);
}
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index ef6d9443cc75..f32c7e737fc6 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -722,6 +722,7 @@ bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
/**
* __skb_flow_dissect - extract the flow_keys struct and return it
+ * @net: associated network namespace, derived from @skb if NULL
* @skb: sk_buff to extract the flow from, can be NULL if the rest are specified
* @flow_dissector: list of keys to dissect
* @target_container: target structure to put dissected values into
@@ -738,7 +739,8 @@ bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
*
* Caller must take care of zeroing target container memory.
*/
-bool __skb_flow_dissect(const struct sk_buff *skb,
+bool __skb_flow_dissect(const struct net *net,
+ const struct sk_buff *skb,
struct flow_dissector *flow_dissector,
void *target_container,
void *data, __be16 proto, int nhoff, int hlen,
@@ -797,13 +799,17 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
struct bpf_prog *attached = NULL;
rcu_read_lock();
+ if (!net) {
+ if (skb->dev)
+ net = dev_net(skb->dev);
+ else if (skb->sk)
+ net = sock_net(skb->sk);
+ else
+ WARN_ON_ONCE(1);
+ }
- if (skb->dev)
- attached = rcu_dereference(dev_net(skb->dev)->flow_dissector_prog);
- else if (skb->sk)
- attached = rcu_dereference(sock_net(skb->sk)->flow_dissector_prog);
- else
- WARN_ON_ONCE(1);
+ if (net)
+ attached = rcu_dereference(net->flow_dissector_prog);
if (attached) {
ret = __skb_flow_bpf_dissect(attached, skb,
@@ -1405,8 +1411,8 @@ u32 __skb_get_hash_symmetric(const struct sk_buff *skb)
__flow_hash_secret_init();
memset(&keys, 0, sizeof(keys));
- __skb_flow_dissect(skb, &flow_keys_dissector_symmetric, &keys,
- NULL, 0, 0, 0,
+ __skb_flow_dissect(NULL, skb, &flow_keys_dissector_symmetric,
+ &keys, NULL, 0, 0, 0,
FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL);
return __flow_hash_from_keys(&keys, hashrnd);
@@ -1507,7 +1513,8 @@ u32 skb_get_poff(const struct sk_buff *skb)
{
struct flow_keys_basic keys;
- if (!skb_flow_dissect_flow_keys_basic(skb, &keys, NULL, 0, 0, 0, 0))
+ if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
+ NULL, 0, 0, 0, 0))
return 0;
return __skb_get_poff(skb, skb->data, &keys, skb_headlen(skb));
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index f7a3d7a171c7..1e439549c419 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -136,8 +136,9 @@ u32 eth_get_headlen(void *data, unsigned int len)
return len;
/* parse any remaining L2/L3 headers, check for L4 */
- if (!skb_flow_dissect_flow_keys_basic(NULL, &keys, data, eth->h_proto,
- sizeof(*eth), len, flags))
+ if (!skb_flow_dissect_flow_keys_basic(NULL, NULL, &keys, data,
+ eth->h_proto, sizeof(*eth),
+ len, flags))
return max_t(u32, keys.control.thoff, sizeof(*eth));
/* parse for any L4 headers */
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH bpf-next v5 4/6] flow_dissector: handle no-skb use case
2019-04-15 17:37 [PATCH bpf-next v5 0/6] net: flow_dissector: trigger BPF hook when called from eth_get_headlen Stanislav Fomichev
` (2 preceding siblings ...)
2019-04-15 17:37 ` [PATCH bpf-next v5 3/6] net: plumb network namespace into __skb_flow_dissect Stanislav Fomichev
@ 2019-04-15 17:37 ` Stanislav Fomichev
2019-04-15 17:38 ` [PATCH bpf-next v5 5/6] net: pass net argument to the eth_get_headlen Stanislav Fomichev
2019-04-15 17:38 ` [PATCH bpf-next v5 6/6] selftests/bpf: add flow dissector bpf_skb_load_bytes helper test Stanislav Fomichev
5 siblings, 0 replies; 14+ messages in thread
From: Stanislav Fomichev @ 2019-04-15 17:37 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, simon.horman, willemb, peterpenkov96,
Stanislav Fomichev
When called without skb, gather all required data from the
__skb_flow_dissect's arguments and use recently introduces
no-skb mode of bpf flow dissector.
Note: WARN_ON_ONCE(!net) will now trigger for eth_get_headlen users.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
include/linux/skbuff.h | 5 ----
net/core/flow_dissector.c | 52 +++++++++++++++++++--------------------
2 files changed, 25 insertions(+), 32 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 60cdbcde751b..0bfc16d719b5 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1277,11 +1277,6 @@ struct bpf_flow_dissector;
bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
__be16 proto, int nhoff, int hlen);
-struct bpf_flow_keys;
-bool __skb_flow_bpf_dissect(struct bpf_prog *prog,
- const struct sk_buff *skb,
- struct flow_dissector *flow_dissector,
- struct bpf_flow_keys *flow_keys);
bool __skb_flow_dissect(const struct net *net,
const struct sk_buff *skb,
struct flow_dissector *flow_dissector,
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index f32c7e737fc6..fac712cee9d5 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -683,22 +683,6 @@ static void __skb_flow_bpf_to_target(const struct bpf_flow_keys *flow_keys,
}
}
-bool __skb_flow_bpf_dissect(struct bpf_prog *prog,
- const struct sk_buff *skb,
- struct flow_dissector *flow_dissector,
- struct bpf_flow_keys *flow_keys)
-{
- struct bpf_flow_dissector ctx = {
- .flow_keys = flow_keys,
- .skb = skb,
- .data = skb->data,
- .data_end = skb->data + skb_headlen(skb),
- };
-
- return bpf_flow_dissect(prog, &ctx, skb->protocol,
- skb_network_offset(skb), skb_headlen(skb));
-}
-
bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
__be16 proto, int nhoff, int hlen)
{
@@ -753,6 +737,7 @@ bool __skb_flow_dissect(const struct net *net,
struct flow_dissector_key_icmp *key_icmp;
struct flow_dissector_key_tags *key_tags;
struct flow_dissector_key_vlan *key_vlan;
+ struct bpf_prog *attached = NULL;
enum flow_dissect_ret fdret;
enum flow_dissector_key_id dissector_vlan = FLOW_DISSECTOR_KEY_MAX;
int num_hdrs = 0;
@@ -795,26 +780,39 @@ bool __skb_flow_dissect(const struct net *net,
target_container);
if (skb) {
- struct bpf_flow_keys flow_keys;
- struct bpf_prog *attached = NULL;
-
- rcu_read_lock();
if (!net) {
if (skb->dev)
net = dev_net(skb->dev);
else if (skb->sk)
net = sock_net(skb->sk);
- else
- WARN_ON_ONCE(1);
}
+ }
- if (net)
- attached = rcu_dereference(net->flow_dissector_prog);
+ WARN_ON_ONCE(!net);
+ if (net) {
+ rcu_read_lock();
+ attached = rcu_dereference(net->flow_dissector_prog);
if (attached) {
- ret = __skb_flow_bpf_dissect(attached, skb,
- flow_dissector,
- &flow_keys);
+ struct bpf_flow_keys flow_keys;
+ struct bpf_flow_dissector ctx = {
+ .flow_keys = &flow_keys,
+ .data = data,
+ .data_end = data + hlen,
+ };
+ __be16 n_proto = proto;
+
+ if (skb) {
+ ctx.skb = skb;
+ /* we can't use 'proto' in the skb case
+ * because it might be set to skb->vlan_proto
+ * which has been pulled from the data
+ */
+ n_proto = skb->protocol;
+ }
+
+ ret = bpf_flow_dissect(attached, &ctx, n_proto, nhoff,
+ hlen);
__skb_flow_bpf_to_target(&flow_keys, flow_dissector,
target_container);
rcu_read_unlock();
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH bpf-next v5 5/6] net: pass net argument to the eth_get_headlen
2019-04-15 17:37 [PATCH bpf-next v5 0/6] net: flow_dissector: trigger BPF hook when called from eth_get_headlen Stanislav Fomichev
` (3 preceding siblings ...)
2019-04-15 17:37 ` [PATCH bpf-next v5 4/6] flow_dissector: handle no-skb use case Stanislav Fomichev
@ 2019-04-15 17:38 ` Stanislav Fomichev
2019-04-19 0:28 ` Alexei Starovoitov
2019-04-15 17:38 ` [PATCH bpf-next v5 6/6] selftests/bpf: add flow dissector bpf_skb_load_bytes helper test Stanislav Fomichev
5 siblings, 1 reply; 14+ messages in thread
From: Stanislav Fomichev @ 2019-04-15 17:38 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, simon.horman, willemb, peterpenkov96,
Stanislav Fomichev, Maxim Krasnyansky, Saeed Mahameed,
Jeff Kirsher, intel-wired-lan, Yisen Zhuang, Salil Mehta,
Michael Chan, Igor Russkikh
Update all users eth_get_headlen to pass network namespace
and pass it down to the flow dissector. This commit is a noop
until administrator inserts BPF flow dissector program.
Cc: Maxim Krasnyansky <maxk@qti.qualcomm.com>
Cc: Saeed Mahameed <saeedm@mellanox.com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Cc: intel-wired-lan@lists.osuosl.org
Cc: Yisen Zhuang <yisen.zhuang@huawei.com>
Cc: Salil Mehta <salil.mehta@huawei.com>
Cc: Michael Chan <michael.chan@broadcom.com>
Cc: Igor Russkikh <igor.russkikh@aquantia.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
drivers/net/ethernet/aquantia/atlantic/aq_ring.c | 3 ++-
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
drivers/net/ethernet/hisilicon/hns/hns_enet.c | 3 ++-
drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 3 ++-
drivers/net/ethernet/intel/fm10k/fm10k_main.c | 2 +-
drivers/net/ethernet/intel/i40e/i40e_txrx.c | 3 ++-
drivers/net/ethernet/intel/iavf/iavf_txrx.c | 3 ++-
drivers/net/ethernet/intel/ice/ice_txrx.c | 3 ++-
drivers/net/ethernet/intel/igb/igb_main.c | 3 ++-
drivers/net/ethernet/intel/igc/igc_main.c | 3 ++-
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 3 ++-
drivers/net/ethernet/mellanox/mlx5/core/en_tx.c | 3 ++-
drivers/net/tun.c | 3 ++-
include/linux/etherdevice.h | 2 +-
net/ethernet/eth.c | 5 +++--
16 files changed, 29 insertions(+), 17 deletions(-)
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index c64e2fb5a4f1..1b3181f757b7 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -354,7 +354,8 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
hdr_len = buff->len;
if (hdr_len > AQ_CFG_RX_HDR_SIZE)
- hdr_len = eth_get_headlen(aq_buf_vaddr(&buff->rxdata),
+ hdr_len = eth_get_headlen(dev_net(skb->dev),
+ aq_buf_vaddr(&buff->rxdata),
AQ_CFG_RX_HDR_SIZE);
memcpy(__skb_put(skb, hdr_len), aq_buf_vaddr(&buff->rxdata),
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 6528a597367b..8bb5f708ccc6 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -899,7 +899,7 @@ static struct sk_buff *bnxt_rx_page_skb(struct bnxt *bp,
DMA_ATTR_WEAK_ORDERING);
if (unlikely(!payload))
- payload = eth_get_headlen(data_ptr, len);
+ payload = eth_get_headlen(dev_net(bp->dev), data_ptr, len);
skb = napi_alloc_skb(&rxr->bnapi->napi, payload);
if (!skb) {
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.c b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
index 297b95c1b3c1..f1ecc78d2323 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
@@ -598,7 +598,8 @@ static int hns_nic_poll_rx_skb(struct hns_nic_ring_data *ring_data,
} else {
ring->stats.seg_pkt_cnt++;
- pull_len = eth_get_headlen(va, HNS_RX_HEAD_SIZE);
+ pull_len = eth_get_headlen(dev_net(ndev), va,
+ HNS_RX_HEAD_SIZE);
memcpy(__skb_put(skb, pull_len), va,
ALIGN(pull_len, sizeof(long)));
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index b53b0911ec24..423d9ce0f6f8 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -2457,7 +2457,8 @@ static int hns3_alloc_skb(struct hns3_enet_ring *ring, int length,
ring->stats.seg_pkt_cnt++;
u64_stats_update_end(&ring->syncp);
- ring->pull_len = eth_get_headlen(va, HNS3_RX_HEAD_SIZE);
+ ring->pull_len = eth_get_headlen(dev_net(netdev), va,
+ HNS3_RX_HEAD_SIZE);
__skb_put(skb, ring->pull_len);
hns3_nic_reuse_page(skb, ring->frag_num++, ring, ring->pull_len,
desc_cb);
diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
index 2325cee76211..e2bee187d652 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
@@ -280,7 +280,7 @@ static bool fm10k_add_rx_frag(struct fm10k_rx_buffer *rx_buffer,
/* we need the header to contain the greater of either ETH_HLEN or
* 60 bytes if the skb->len is less than 60 for skb_pad.
*/
- pull_len = eth_get_headlen(va, FM10K_RX_HDR_LEN);
+ pull_len = eth_get_headlen(dev_net(skb->dev), va, FM10K_RX_HDR_LEN);
/* align pull length to size of long to optimize memcpy performance */
memcpy(__skb_put(skb, pull_len), va, ALIGN(pull_len, sizeof(long)));
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 1a95223c9f99..85c5b503e0a0 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -2035,7 +2035,8 @@ static struct sk_buff *i40e_construct_skb(struct i40e_ring *rx_ring,
/* Determine available headroom for copy */
headlen = size;
if (headlen > I40E_RX_HDR_SIZE)
- headlen = eth_get_headlen(xdp->data, I40E_RX_HDR_SIZE);
+ headlen = eth_get_headlen(dev_net(skb->dev), xdp->data,
+ I40E_RX_HDR_SIZE);
/* align pull length to size of long to optimize memcpy performance */
memcpy(__skb_put(skb, headlen), xdp->data,
diff --git a/drivers/net/ethernet/intel/iavf/iavf_txrx.c b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
index b64187753ad6..23a62d7d0f9f 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_txrx.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
@@ -1315,7 +1315,8 @@ static struct sk_buff *iavf_construct_skb(struct iavf_ring *rx_ring,
/* Determine available headroom for copy */
headlen = size;
if (headlen > IAVF_RX_HDR_SIZE)
- headlen = eth_get_headlen(va, IAVF_RX_HDR_SIZE);
+ headlen = eth_get_headlen(dev_net(skb->dev), va,
+ IAVF_RX_HDR_SIZE);
/* align pull length to size of long to optimize memcpy performance */
memcpy(__skb_put(skb, headlen), va, ALIGN(headlen, sizeof(long)));
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
index a6f7b7feaf3c..2692b9333055 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
@@ -698,7 +698,8 @@ ice_construct_skb(struct ice_ring *rx_ring, struct ice_rx_buf *rx_buf,
/* Determine available headroom for copy */
headlen = size;
if (headlen > ICE_RX_HDR_SIZE)
- headlen = eth_get_headlen(va, ICE_RX_HDR_SIZE);
+ headlen = eth_get_headlen(dev_net(skb->dev), va,
+ ICE_RX_HDR_SIZE);
/* align pull length to size of long to optimize memcpy performance */
memcpy(__skb_put(skb, headlen), va, ALIGN(headlen, sizeof(long)));
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index acbb5b4f333d..2023e1800c8d 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -8051,7 +8051,8 @@ static struct sk_buff *igb_construct_skb(struct igb_ring *rx_ring,
/* Determine available headroom for copy */
headlen = size;
if (headlen > IGB_RX_HDR_LEN)
- headlen = eth_get_headlen(va, IGB_RX_HDR_LEN);
+ headlen = eth_get_headlen(dev_net(skb->dev), va,
+ IGB_RX_HDR_LEN);
/* align pull length to size of long to optimize memcpy performance */
memcpy(__skb_put(skb, headlen), va, ALIGN(headlen, sizeof(long)));
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index f79728381e8a..265a9d8a8421 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1199,7 +1199,8 @@ static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring,
/* Determine available headroom for copy */
headlen = size;
if (headlen > IGC_RX_HDR_LEN)
- headlen = eth_get_headlen(va, IGC_RX_HDR_LEN);
+ headlen = eth_get_headlen(dev_net(skb->dev), va,
+ IGC_RX_HDR_LEN);
/* align pull length to size of long to optimize memcpy performance */
memcpy(__skb_put(skb, headlen), va, ALIGN(headlen, sizeof(long)));
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 60cec3540dd7..5e5294567ca1 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1800,7 +1800,7 @@ static void ixgbe_pull_tail(struct ixgbe_ring *rx_ring,
* we need the header to contain the greater of either ETH_HLEN or
* 60 bytes if the skb->len is less than 60 for skb_pad.
*/
- pull_len = eth_get_headlen(va, IXGBE_RX_HDR_SIZE);
+ pull_len = eth_get_headlen(dev_net(skb->dev), va, IXGBE_RX_HDR_SIZE);
/* align pull length to size of long to optimize memcpy performance */
skb_copy_to_linear_data(skb, va, ALIGN(pull_len, sizeof(long)));
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 49e23afa05a2..252fe0de6b56 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -895,7 +895,8 @@ struct sk_buff *ixgbevf_construct_skb(struct ixgbevf_ring *rx_ring,
/* Determine available headroom for copy */
headlen = size;
if (headlen > IXGBEVF_RX_HDR_SIZE)
- headlen = eth_get_headlen(xdp->data, IXGBEVF_RX_HDR_SIZE);
+ headlen = eth_get_headlen(dev_net(skb->dev), xdp->data,
+ IXGBEVF_RX_HDR_SIZE);
/* align pull length to size of long to optimize memcpy performance */
memcpy(__skb_put(skb, headlen), xdp->data,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
index 40f3f98aa279..efcc27756c7e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
@@ -163,7 +163,8 @@ static inline u16 mlx5e_calc_min_inline(enum mlx5_inline_modes mode,
case MLX5_INLINE_MODE_NONE:
return 0;
case MLX5_INLINE_MODE_TCP_UDP:
- hlen = eth_get_headlen(skb->data, skb_headlen(skb));
+ hlen = eth_get_headlen(dev_net(skb->dev), skb->data,
+ skb_headlen(skb));
if (hlen == ETH_HLEN && !skb_vlan_tag_present(skb))
hlen += VLAN_HLEN;
break;
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 24d0220b9ba0..6d5c8ecfea1e 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1965,7 +1965,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
if (frags) {
/* Exercise flow dissector code path. */
- u32 headlen = eth_get_headlen(skb->data, skb_headlen(skb));
+ u32 headlen = eth_get_headlen(dev_net(tun->dev), skb->data,
+ skb_headlen(skb));
if (unlikely(headlen > skb_headlen(skb))) {
this_cpu_inc(tun->pcpu_stats->rx_dropped);
diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index e2f3b21cd72a..71a441ffab3f 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -33,7 +33,7 @@ struct device;
int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr);
unsigned char *arch_get_platform_mac_address(void);
int nvmem_get_mac_address(struct device *dev, void *addrbuf);
-u32 eth_get_headlen(void *data, unsigned int max_len);
+u32 eth_get_headlen(const struct net *net, void *data, unsigned int max_len);
__be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev);
extern const struct header_ops eth_header_ops;
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 1e439549c419..0202e72e20a4 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -119,13 +119,14 @@ EXPORT_SYMBOL(eth_header);
/**
* eth_get_headlen - determine the length of header for an ethernet frame
+ * @net: pointer to device network namespace
* @data: pointer to start of frame
* @len: total length of frame
*
* Make a best effort attempt to pull the length for all of the headers for
* a given frame in a linear buffer.
*/
-u32 eth_get_headlen(void *data, unsigned int len)
+u32 eth_get_headlen(const struct net *net, void *data, unsigned int len)
{
const unsigned int flags = FLOW_DISSECTOR_F_PARSE_1ST_FRAG;
const struct ethhdr *eth = (const struct ethhdr *)data;
@@ -136,7 +137,7 @@ u32 eth_get_headlen(void *data, unsigned int len)
return len;
/* parse any remaining L2/L3 headers, check for L4 */
- if (!skb_flow_dissect_flow_keys_basic(NULL, NULL, &keys, data,
+ if (!skb_flow_dissect_flow_keys_basic(net, NULL, &keys, data,
eth->h_proto, sizeof(*eth),
len, flags))
return max_t(u32, keys.control.thoff, sizeof(*eth));
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH bpf-next v5 5/6] net: pass net argument to the eth_get_headlen
2019-04-15 17:38 ` [PATCH bpf-next v5 5/6] net: pass net argument to the eth_get_headlen Stanislav Fomichev
@ 2019-04-19 0:28 ` Alexei Starovoitov
2019-04-19 0:43 ` Stanislav Fomichev
0 siblings, 1 reply; 14+ messages in thread
From: Alexei Starovoitov @ 2019-04-19 0:28 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: netdev, bpf, davem, ast, daniel, simon.horman, willemb,
peterpenkov96, Maxim Krasnyansky, Saeed Mahameed, Jeff Kirsher,
intel-wired-lan, Yisen Zhuang, Salil Mehta, Michael Chan,
Igor Russkikh
On Mon, Apr 15, 2019 at 10:38:00AM -0700, Stanislav Fomichev wrote:
> Update all users eth_get_headlen to pass network namespace
> and pass it down to the flow dissector. This commit is a noop
> until administrator inserts BPF flow dissector program.
>
> Cc: Maxim Krasnyansky <maxk@qti.qualcomm.com>
> Cc: Saeed Mahameed <saeedm@mellanox.com>
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org
> Cc: Yisen Zhuang <yisen.zhuang@huawei.com>
> Cc: Salil Mehta <salil.mehta@huawei.com>
> Cc: Michael Chan <michael.chan@broadcom.com>
> Cc: Igor Russkikh <igor.russkikh@aquantia.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
> drivers/net/ethernet/aquantia/atlantic/aq_ring.c | 3 ++-
> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
> drivers/net/ethernet/hisilicon/hns/hns_enet.c | 3 ++-
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 3 ++-
> drivers/net/ethernet/intel/fm10k/fm10k_main.c | 2 +-
> drivers/net/ethernet/intel/i40e/i40e_txrx.c | 3 ++-
> drivers/net/ethernet/intel/iavf/iavf_txrx.c | 3 ++-
> drivers/net/ethernet/intel/ice/ice_txrx.c | 3 ++-
> drivers/net/ethernet/intel/igb/igb_main.c | 3 ++-
> drivers/net/ethernet/intel/igc/igc_main.c | 3 ++-
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
> drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 3 ++-
> drivers/net/ethernet/mellanox/mlx5/core/en_tx.c | 3 ++-
> drivers/net/tun.c | 3 ++-
> include/linux/etherdevice.h | 2 +-
> net/ethernet/eth.c | 5 +++--
> 16 files changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index c64e2fb5a4f1..1b3181f757b7 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> @@ -354,7 +354,8 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
>
> hdr_len = buff->len;
> if (hdr_len > AQ_CFG_RX_HDR_SIZE)
> - hdr_len = eth_get_headlen(aq_buf_vaddr(&buff->rxdata),
> + hdr_len = eth_get_headlen(dev_net(skb->dev),
> + aq_buf_vaddr(&buff->rxdata),
> AQ_CFG_RX_HDR_SIZE);
>
> memcpy(__skb_put(skb, hdr_len), aq_buf_vaddr(&buff->rxdata),
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 6528a597367b..8bb5f708ccc6 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -899,7 +899,7 @@ static struct sk_buff *bnxt_rx_page_skb(struct bnxt *bp,
> DMA_ATTR_WEAK_ORDERING);
>
> if (unlikely(!payload))
> - payload = eth_get_headlen(data_ptr, len);
> + payload = eth_get_headlen(dev_net(bp->dev), data_ptr, len);
>
> skb = napi_alloc_skb(&rxr->bnapi->napi, payload);
> if (!skb) {
> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.c b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
> index 297b95c1b3c1..f1ecc78d2323 100644
> --- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
> @@ -598,7 +598,8 @@ static int hns_nic_poll_rx_skb(struct hns_nic_ring_data *ring_data,
> } else {
> ring->stats.seg_pkt_cnt++;
>
> - pull_len = eth_get_headlen(va, HNS_RX_HEAD_SIZE);
> + pull_len = eth_get_headlen(dev_net(ndev), va,
> + HNS_RX_HEAD_SIZE);
> memcpy(__skb_put(skb, pull_len), va,
> ALIGN(pull_len, sizeof(long)));
>
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> index b53b0911ec24..423d9ce0f6f8 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> @@ -2457,7 +2457,8 @@ static int hns3_alloc_skb(struct hns3_enet_ring *ring, int length,
> ring->stats.seg_pkt_cnt++;
> u64_stats_update_end(&ring->syncp);
>
> - ring->pull_len = eth_get_headlen(va, HNS3_RX_HEAD_SIZE);
> + ring->pull_len = eth_get_headlen(dev_net(netdev), va,
> + HNS3_RX_HEAD_SIZE);
> __skb_put(skb, ring->pull_len);
> hns3_nic_reuse_page(skb, ring->frag_num++, ring, ring->pull_len,
> desc_cb);
> diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> index 2325cee76211..e2bee187d652 100644
> --- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> +++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> @@ -280,7 +280,7 @@ static bool fm10k_add_rx_frag(struct fm10k_rx_buffer *rx_buffer,
> /* we need the header to contain the greater of either ETH_HLEN or
> * 60 bytes if the skb->len is less than 60 for skb_pad.
> */
> - pull_len = eth_get_headlen(va, FM10K_RX_HDR_LEN);
> + pull_len = eth_get_headlen(dev_net(skb->dev), va, FM10K_RX_HDR_LEN);
>
> /* align pull length to size of long to optimize memcpy performance */
> memcpy(__skb_put(skb, pull_len), va, ALIGN(pull_len, sizeof(long)));
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> index 1a95223c9f99..85c5b503e0a0 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> @@ -2035,7 +2035,8 @@ static struct sk_buff *i40e_construct_skb(struct i40e_ring *rx_ring,
> /* Determine available headroom for copy */
> headlen = size;
> if (headlen > I40E_RX_HDR_SIZE)
> - headlen = eth_get_headlen(xdp->data, I40E_RX_HDR_SIZE);
> + headlen = eth_get_headlen(dev_net(skb->dev), xdp->data,
> + I40E_RX_HDR_SIZE);
>
> /* align pull length to size of long to optimize memcpy performance */
> memcpy(__skb_put(skb, headlen), xdp->data,
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_txrx.c b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
> index b64187753ad6..23a62d7d0f9f 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_txrx.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
> @@ -1315,7 +1315,8 @@ static struct sk_buff *iavf_construct_skb(struct iavf_ring *rx_ring,
> /* Determine available headroom for copy */
> headlen = size;
> if (headlen > IAVF_RX_HDR_SIZE)
> - headlen = eth_get_headlen(va, IAVF_RX_HDR_SIZE);
> + headlen = eth_get_headlen(dev_net(skb->dev), va,
> + IAVF_RX_HDR_SIZE);
>
> /* align pull length to size of long to optimize memcpy performance */
> memcpy(__skb_put(skb, headlen), va, ALIGN(headlen, sizeof(long)));
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> index a6f7b7feaf3c..2692b9333055 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> @@ -698,7 +698,8 @@ ice_construct_skb(struct ice_ring *rx_ring, struct ice_rx_buf *rx_buf,
> /* Determine available headroom for copy */
> headlen = size;
> if (headlen > ICE_RX_HDR_SIZE)
> - headlen = eth_get_headlen(va, ICE_RX_HDR_SIZE);
> + headlen = eth_get_headlen(dev_net(skb->dev), va,
> + ICE_RX_HDR_SIZE);
>
> /* align pull length to size of long to optimize memcpy performance */
> memcpy(__skb_put(skb, headlen), va, ALIGN(headlen, sizeof(long)));
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index acbb5b4f333d..2023e1800c8d 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -8051,7 +8051,8 @@ static struct sk_buff *igb_construct_skb(struct igb_ring *rx_ring,
> /* Determine available headroom for copy */
> headlen = size;
> if (headlen > IGB_RX_HDR_LEN)
> - headlen = eth_get_headlen(va, IGB_RX_HDR_LEN);
> + headlen = eth_get_headlen(dev_net(skb->dev), va,
> + IGB_RX_HDR_LEN);
>
> /* align pull length to size of long to optimize memcpy performance */
> memcpy(__skb_put(skb, headlen), va, ALIGN(headlen, sizeof(long)));
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index f79728381e8a..265a9d8a8421 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -1199,7 +1199,8 @@ static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring,
> /* Determine available headroom for copy */
> headlen = size;
> if (headlen > IGC_RX_HDR_LEN)
> - headlen = eth_get_headlen(va, IGC_RX_HDR_LEN);
> + headlen = eth_get_headlen(dev_net(skb->dev), va,
> + IGC_RX_HDR_LEN);
>
> /* align pull length to size of long to optimize memcpy performance */
> memcpy(__skb_put(skb, headlen), va, ALIGN(headlen, sizeof(long)));
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 60cec3540dd7..5e5294567ca1 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -1800,7 +1800,7 @@ static void ixgbe_pull_tail(struct ixgbe_ring *rx_ring,
> * we need the header to contain the greater of either ETH_HLEN or
> * 60 bytes if the skb->len is less than 60 for skb_pad.
> */
> - pull_len = eth_get_headlen(va, IXGBE_RX_HDR_SIZE);
> + pull_len = eth_get_headlen(dev_net(skb->dev), va, IXGBE_RX_HDR_SIZE);
>
> /* align pull length to size of long to optimize memcpy performance */
> skb_copy_to_linear_data(skb, va, ALIGN(pull_len, sizeof(long)));
> diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> index 49e23afa05a2..252fe0de6b56 100644
> --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> @@ -895,7 +895,8 @@ struct sk_buff *ixgbevf_construct_skb(struct ixgbevf_ring *rx_ring,
> /* Determine available headroom for copy */
> headlen = size;
> if (headlen > IXGBEVF_RX_HDR_SIZE)
> - headlen = eth_get_headlen(xdp->data, IXGBEVF_RX_HDR_SIZE);
> + headlen = eth_get_headlen(dev_net(skb->dev), xdp->data,
> + IXGBEVF_RX_HDR_SIZE);
>
> /* align pull length to size of long to optimize memcpy performance */
> memcpy(__skb_put(skb, headlen), xdp->data,
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> index 40f3f98aa279..efcc27756c7e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> @@ -163,7 +163,8 @@ static inline u16 mlx5e_calc_min_inline(enum mlx5_inline_modes mode,
> case MLX5_INLINE_MODE_NONE:
> return 0;
> case MLX5_INLINE_MODE_TCP_UDP:
> - hlen = eth_get_headlen(skb->data, skb_headlen(skb));
> + hlen = eth_get_headlen(dev_net(skb->dev), skb->data,
> + skb_headlen(skb));
> if (hlen == ETH_HLEN && !skb_vlan_tag_present(skb))
> hlen += VLAN_HLEN;
> break;
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 24d0220b9ba0..6d5c8ecfea1e 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1965,7 +1965,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>
> if (frags) {
> /* Exercise flow dissector code path. */
> - u32 headlen = eth_get_headlen(skb->data, skb_headlen(skb));
> + u32 headlen = eth_get_headlen(dev_net(tun->dev), skb->data,
> + skb_headlen(skb));
>
> if (unlikely(headlen > skb_headlen(skb))) {
> this_cpu_inc(tun->pcpu_stats->rx_dropped);
> diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> index e2f3b21cd72a..71a441ffab3f 100644
> --- a/include/linux/etherdevice.h
> +++ b/include/linux/etherdevice.h
> @@ -33,7 +33,7 @@ struct device;
> int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr);
> unsigned char *arch_get_platform_mac_address(void);
> int nvmem_get_mac_address(struct device *dev, void *addrbuf);
> -u32 eth_get_headlen(void *data, unsigned int max_len);
> +u32 eth_get_headlen(const struct net *net, void *data, unsigned int max_len);
> __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev);
> extern const struct header_ops eth_header_ops;
>
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index 1e439549c419..0202e72e20a4 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -119,13 +119,14 @@ EXPORT_SYMBOL(eth_header);
>
> /**
> * eth_get_headlen - determine the length of header for an ethernet frame
> + * @net: pointer to device network namespace
> * @data: pointer to start of frame
> * @len: total length of frame
> *
> * Make a best effort attempt to pull the length for all of the headers for
> * a given frame in a linear buffer.
> */
> -u32 eth_get_headlen(void *data, unsigned int len)
> +u32 eth_get_headlen(const struct net *net, void *data, unsigned int len)
would it make sense to future proof it a little bit and pass 'dev'
into eth_get_headlen() instead of 'net' ?
May be tomorrow we'd want different flow_dissectors per-device
in addition to per-net ?
Also please add C based test for skb-less flow_dissector.
Current test_flow_dissector.sh doesn't seem to cover it.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH bpf-next v5 5/6] net: pass net argument to the eth_get_headlen
2019-04-19 0:28 ` Alexei Starovoitov
@ 2019-04-19 0:43 ` Stanislav Fomichev
2019-04-19 4:50 ` Alexei Starovoitov
0 siblings, 1 reply; 14+ messages in thread
From: Stanislav Fomichev @ 2019-04-19 0:43 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Stanislav Fomichev, netdev, bpf, davem, ast, daniel,
simon.horman, willemb, peterpenkov96, Maxim Krasnyansky,
Saeed Mahameed, Jeff Kirsher, intel-wired-lan, Yisen Zhuang,
Salil Mehta, Michael Chan, Igor Russkikh
On 04/18, Alexei Starovoitov wrote:
> On Mon, Apr 15, 2019 at 10:38:00AM -0700, Stanislav Fomichev wrote:
> > Update all users eth_get_headlen to pass network namespace
> > and pass it down to the flow dissector. This commit is a noop
> > until administrator inserts BPF flow dissector program.
> >
> > Cc: Maxim Krasnyansky <maxk@qti.qualcomm.com>
> > Cc: Saeed Mahameed <saeedm@mellanox.com>
> > Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > Cc: intel-wired-lan@lists.osuosl.org
> > Cc: Yisen Zhuang <yisen.zhuang@huawei.com>
> > Cc: Salil Mehta <salil.mehta@huawei.com>
> > Cc: Michael Chan <michael.chan@broadcom.com>
> > Cc: Igor Russkikh <igor.russkikh@aquantia.com>
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> > drivers/net/ethernet/aquantia/atlantic/aq_ring.c | 3 ++-
> > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
> > drivers/net/ethernet/hisilicon/hns/hns_enet.c | 3 ++-
> > drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 3 ++-
> > drivers/net/ethernet/intel/fm10k/fm10k_main.c | 2 +-
> > drivers/net/ethernet/intel/i40e/i40e_txrx.c | 3 ++-
> > drivers/net/ethernet/intel/iavf/iavf_txrx.c | 3 ++-
> > drivers/net/ethernet/intel/ice/ice_txrx.c | 3 ++-
> > drivers/net/ethernet/intel/igb/igb_main.c | 3 ++-
> > drivers/net/ethernet/intel/igc/igc_main.c | 3 ++-
> > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
> > drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 3 ++-
> > drivers/net/ethernet/mellanox/mlx5/core/en_tx.c | 3 ++-
> > drivers/net/tun.c | 3 ++-
> > include/linux/etherdevice.h | 2 +-
> > net/ethernet/eth.c | 5 +++--
> > 16 files changed, 29 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> > index c64e2fb5a4f1..1b3181f757b7 100644
> > --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> > +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> > @@ -354,7 +354,8 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
> >
> > hdr_len = buff->len;
> > if (hdr_len > AQ_CFG_RX_HDR_SIZE)
> > - hdr_len = eth_get_headlen(aq_buf_vaddr(&buff->rxdata),
> > + hdr_len = eth_get_headlen(dev_net(skb->dev),
> > + aq_buf_vaddr(&buff->rxdata),
> > AQ_CFG_RX_HDR_SIZE);
> >
> > memcpy(__skb_put(skb, hdr_len), aq_buf_vaddr(&buff->rxdata),
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index 6528a597367b..8bb5f708ccc6 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > @@ -899,7 +899,7 @@ static struct sk_buff *bnxt_rx_page_skb(struct bnxt *bp,
> > DMA_ATTR_WEAK_ORDERING);
> >
> > if (unlikely(!payload))
> > - payload = eth_get_headlen(data_ptr, len);
> > + payload = eth_get_headlen(dev_net(bp->dev), data_ptr, len);
> >
> > skb = napi_alloc_skb(&rxr->bnapi->napi, payload);
> > if (!skb) {
> > diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.c b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
> > index 297b95c1b3c1..f1ecc78d2323 100644
> > --- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c
> > +++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
> > @@ -598,7 +598,8 @@ static int hns_nic_poll_rx_skb(struct hns_nic_ring_data *ring_data,
> > } else {
> > ring->stats.seg_pkt_cnt++;
> >
> > - pull_len = eth_get_headlen(va, HNS_RX_HEAD_SIZE);
> > + pull_len = eth_get_headlen(dev_net(ndev), va,
> > + HNS_RX_HEAD_SIZE);
> > memcpy(__skb_put(skb, pull_len), va,
> > ALIGN(pull_len, sizeof(long)));
> >
> > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> > index b53b0911ec24..423d9ce0f6f8 100644
> > --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> > @@ -2457,7 +2457,8 @@ static int hns3_alloc_skb(struct hns3_enet_ring *ring, int length,
> > ring->stats.seg_pkt_cnt++;
> > u64_stats_update_end(&ring->syncp);
> >
> > - ring->pull_len = eth_get_headlen(va, HNS3_RX_HEAD_SIZE);
> > + ring->pull_len = eth_get_headlen(dev_net(netdev), va,
> > + HNS3_RX_HEAD_SIZE);
> > __skb_put(skb, ring->pull_len);
> > hns3_nic_reuse_page(skb, ring->frag_num++, ring, ring->pull_len,
> > desc_cb);
> > diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> > index 2325cee76211..e2bee187d652 100644
> > --- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> > +++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> > @@ -280,7 +280,7 @@ static bool fm10k_add_rx_frag(struct fm10k_rx_buffer *rx_buffer,
> > /* we need the header to contain the greater of either ETH_HLEN or
> > * 60 bytes if the skb->len is less than 60 for skb_pad.
> > */
> > - pull_len = eth_get_headlen(va, FM10K_RX_HDR_LEN);
> > + pull_len = eth_get_headlen(dev_net(skb->dev), va, FM10K_RX_HDR_LEN);
> >
> > /* align pull length to size of long to optimize memcpy performance */
> > memcpy(__skb_put(skb, pull_len), va, ALIGN(pull_len, sizeof(long)));
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > index 1a95223c9f99..85c5b503e0a0 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> > @@ -2035,7 +2035,8 @@ static struct sk_buff *i40e_construct_skb(struct i40e_ring *rx_ring,
> > /* Determine available headroom for copy */
> > headlen = size;
> > if (headlen > I40E_RX_HDR_SIZE)
> > - headlen = eth_get_headlen(xdp->data, I40E_RX_HDR_SIZE);
> > + headlen = eth_get_headlen(dev_net(skb->dev), xdp->data,
> > + I40E_RX_HDR_SIZE);
> >
> > /* align pull length to size of long to optimize memcpy performance */
> > memcpy(__skb_put(skb, headlen), xdp->data,
> > diff --git a/drivers/net/ethernet/intel/iavf/iavf_txrx.c b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
> > index b64187753ad6..23a62d7d0f9f 100644
> > --- a/drivers/net/ethernet/intel/iavf/iavf_txrx.c
> > +++ b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
> > @@ -1315,7 +1315,8 @@ static struct sk_buff *iavf_construct_skb(struct iavf_ring *rx_ring,
> > /* Determine available headroom for copy */
> > headlen = size;
> > if (headlen > IAVF_RX_HDR_SIZE)
> > - headlen = eth_get_headlen(va, IAVF_RX_HDR_SIZE);
> > + headlen = eth_get_headlen(dev_net(skb->dev), va,
> > + IAVF_RX_HDR_SIZE);
> >
> > /* align pull length to size of long to optimize memcpy performance */
> > memcpy(__skb_put(skb, headlen), va, ALIGN(headlen, sizeof(long)));
> > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> > index a6f7b7feaf3c..2692b9333055 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> > @@ -698,7 +698,8 @@ ice_construct_skb(struct ice_ring *rx_ring, struct ice_rx_buf *rx_buf,
> > /* Determine available headroom for copy */
> > headlen = size;
> > if (headlen > ICE_RX_HDR_SIZE)
> > - headlen = eth_get_headlen(va, ICE_RX_HDR_SIZE);
> > + headlen = eth_get_headlen(dev_net(skb->dev), va,
> > + ICE_RX_HDR_SIZE);
> >
> > /* align pull length to size of long to optimize memcpy performance */
> > memcpy(__skb_put(skb, headlen), va, ALIGN(headlen, sizeof(long)));
> > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> > index acbb5b4f333d..2023e1800c8d 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_main.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> > @@ -8051,7 +8051,8 @@ static struct sk_buff *igb_construct_skb(struct igb_ring *rx_ring,
> > /* Determine available headroom for copy */
> > headlen = size;
> > if (headlen > IGB_RX_HDR_LEN)
> > - headlen = eth_get_headlen(va, IGB_RX_HDR_LEN);
> > + headlen = eth_get_headlen(dev_net(skb->dev), va,
> > + IGB_RX_HDR_LEN);
> >
> > /* align pull length to size of long to optimize memcpy performance */
> > memcpy(__skb_put(skb, headlen), va, ALIGN(headlen, sizeof(long)));
> > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> > index f79728381e8a..265a9d8a8421 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > @@ -1199,7 +1199,8 @@ static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring,
> > /* Determine available headroom for copy */
> > headlen = size;
> > if (headlen > IGC_RX_HDR_LEN)
> > - headlen = eth_get_headlen(va, IGC_RX_HDR_LEN);
> > + headlen = eth_get_headlen(dev_net(skb->dev), va,
> > + IGC_RX_HDR_LEN);
> >
> > /* align pull length to size of long to optimize memcpy performance */
> > memcpy(__skb_put(skb, headlen), va, ALIGN(headlen, sizeof(long)));
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > index 60cec3540dd7..5e5294567ca1 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > @@ -1800,7 +1800,7 @@ static void ixgbe_pull_tail(struct ixgbe_ring *rx_ring,
> > * we need the header to contain the greater of either ETH_HLEN or
> > * 60 bytes if the skb->len is less than 60 for skb_pad.
> > */
> > - pull_len = eth_get_headlen(va, IXGBE_RX_HDR_SIZE);
> > + pull_len = eth_get_headlen(dev_net(skb->dev), va, IXGBE_RX_HDR_SIZE);
> >
> > /* align pull length to size of long to optimize memcpy performance */
> > skb_copy_to_linear_data(skb, va, ALIGN(pull_len, sizeof(long)));
> > diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> > index 49e23afa05a2..252fe0de6b56 100644
> > --- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> > +++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
> > @@ -895,7 +895,8 @@ struct sk_buff *ixgbevf_construct_skb(struct ixgbevf_ring *rx_ring,
> > /* Determine available headroom for copy */
> > headlen = size;
> > if (headlen > IXGBEVF_RX_HDR_SIZE)
> > - headlen = eth_get_headlen(xdp->data, IXGBEVF_RX_HDR_SIZE);
> > + headlen = eth_get_headlen(dev_net(skb->dev), xdp->data,
> > + IXGBEVF_RX_HDR_SIZE);
> >
> > /* align pull length to size of long to optimize memcpy performance */
> > memcpy(__skb_put(skb, headlen), xdp->data,
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> > index 40f3f98aa279..efcc27756c7e 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> > @@ -163,7 +163,8 @@ static inline u16 mlx5e_calc_min_inline(enum mlx5_inline_modes mode,
> > case MLX5_INLINE_MODE_NONE:
> > return 0;
> > case MLX5_INLINE_MODE_TCP_UDP:
> > - hlen = eth_get_headlen(skb->data, skb_headlen(skb));
> > + hlen = eth_get_headlen(dev_net(skb->dev), skb->data,
> > + skb_headlen(skb));
> > if (hlen == ETH_HLEN && !skb_vlan_tag_present(skb))
> > hlen += VLAN_HLEN;
> > break;
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 24d0220b9ba0..6d5c8ecfea1e 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -1965,7 +1965,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
> >
> > if (frags) {
> > /* Exercise flow dissector code path. */
> > - u32 headlen = eth_get_headlen(skb->data, skb_headlen(skb));
> > + u32 headlen = eth_get_headlen(dev_net(tun->dev), skb->data,
> > + skb_headlen(skb));
> >
> > if (unlikely(headlen > skb_headlen(skb))) {
> > this_cpu_inc(tun->pcpu_stats->rx_dropped);
> > diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> > index e2f3b21cd72a..71a441ffab3f 100644
> > --- a/include/linux/etherdevice.h
> > +++ b/include/linux/etherdevice.h
> > @@ -33,7 +33,7 @@ struct device;
> > int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr);
> > unsigned char *arch_get_platform_mac_address(void);
> > int nvmem_get_mac_address(struct device *dev, void *addrbuf);
> > -u32 eth_get_headlen(void *data, unsigned int max_len);
> > +u32 eth_get_headlen(const struct net *net, void *data, unsigned int max_len);
> > __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev);
> > extern const struct header_ops eth_header_ops;
> >
> > diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> > index 1e439549c419..0202e72e20a4 100644
> > --- a/net/ethernet/eth.c
> > +++ b/net/ethernet/eth.c
> > @@ -119,13 +119,14 @@ EXPORT_SYMBOL(eth_header);
> >
> > /**
> > * eth_get_headlen - determine the length of header for an ethernet frame
> > + * @net: pointer to device network namespace
> > * @data: pointer to start of frame
> > * @len: total length of frame
> > *
> > * Make a best effort attempt to pull the length for all of the headers for
> > * a given frame in a linear buffer.
> > */
> > -u32 eth_get_headlen(void *data, unsigned int len)
> > +u32 eth_get_headlen(const struct net *net, void *data, unsigned int len)
>
> would it make sense to future proof it a little bit and pass 'dev'
> into eth_get_headlen() instead of 'net' ?
> May be tomorrow we'd want different flow_dissectors per-device
> in addition to per-net ?
Good point, will use net_device.
> Also please add C based test for skb-less flow_dissector.
> Current test_flow_dissector.sh doesn't seem to cover it.
It doesn't look like we can exercise skb-less flow dissector from
test_flow_dissector.sh; we need to trigger some driver code, which is
hard when we send the packets on the localhost in
test_flow_dissector.sh.
To test skb-less dissector I convert BPF_PROG_TEST_RUN to always use skb-less
mode. test_flow_dissector.sh tests skb-mode, prog_tests/flow_dissector.c
tests skb-less mode.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH bpf-next v5 5/6] net: pass net argument to the eth_get_headlen
2019-04-19 0:43 ` Stanislav Fomichev
@ 2019-04-19 4:50 ` Alexei Starovoitov
2019-04-19 23:29 ` Stanislav Fomichev
0 siblings, 1 reply; 14+ messages in thread
From: Alexei Starovoitov @ 2019-04-19 4:50 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Stanislav Fomichev, netdev, bpf, davem, ast, daniel,
simon.horman, willemb, peterpenkov96, Maxim Krasnyansky,
Saeed Mahameed, Jeff Kirsher, intel-wired-lan, Yisen Zhuang,
Salil Mehta, Michael Chan, Igor Russkikh
On Thu, Apr 18, 2019 at 05:43:50PM -0700, Stanislav Fomichev wrote:
> On 04/18, Alexei Starovoitov wrote:
> > On Mon, Apr 15, 2019 at 10:38:00AM -0700, Stanislav Fomichev wrote:
> > > Update all users eth_get_headlen to pass network namespace
> > > and pass it down to the flow dissector. This commit is a noop
> > > until administrator inserts BPF flow dissector program.
> > >
> > > Cc: Maxim Krasnyansky <maxk@qti.qualcomm.com>
> > > Cc: Saeed Mahameed <saeedm@mellanox.com>
> > > Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > > Cc: intel-wired-lan@lists.osuosl.org
> > > Cc: Yisen Zhuang <yisen.zhuang@huawei.com>
> > > Cc: Salil Mehta <salil.mehta@huawei.com>
> > > Cc: Michael Chan <michael.chan@broadcom.com>
> > > Cc: Igor Russkikh <igor.russkikh@aquantia.com>
> > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
...
> > Also please add C based test for skb-less flow_dissector.
> > Current test_flow_dissector.sh doesn't seem to cover it.
> It doesn't look like we can exercise skb-less flow dissector from
> test_flow_dissector.sh; we need to trigger some driver code, which is
> hard when we send the packets on the localhost in
> test_flow_dissector.sh.
>
> To test skb-less dissector I convert BPF_PROG_TEST_RUN to always use skb-less
> mode. test_flow_dissector.sh tests skb-mode, prog_tests/flow_dissector.c
> tests skb-less mode.
I saw that but I'm afraid it's not enough.
tun_get_user() is calling it, so it should be possible to test
skb-less mode via tun.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next v5 5/6] net: pass net argument to the eth_get_headlen
2019-04-19 4:50 ` Alexei Starovoitov
@ 2019-04-19 23:29 ` Stanislav Fomichev
2019-04-19 23:37 ` Alexei Starovoitov
0 siblings, 1 reply; 14+ messages in thread
From: Stanislav Fomichev @ 2019-04-19 23:29 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Stanislav Fomichev, netdev, bpf, davem, ast, daniel,
simon.horman, willemb, peterpenkov96, Maxim Krasnyansky,
Saeed Mahameed, Jeff Kirsher, intel-wired-lan, Yisen Zhuang,
Salil Mehta, Michael Chan, Igor Russkikh
On 04/18, Alexei Starovoitov wrote:
> On Thu, Apr 18, 2019 at 05:43:50PM -0700, Stanislav Fomichev wrote:
> > On 04/18, Alexei Starovoitov wrote:
> > > On Mon, Apr 15, 2019 at 10:38:00AM -0700, Stanislav Fomichev wrote:
> > > > Update all users eth_get_headlen to pass network namespace
> > > > and pass it down to the flow dissector. This commit is a noop
> > > > until administrator inserts BPF flow dissector program.
> > > >
> > > > Cc: Maxim Krasnyansky <maxk@qti.qualcomm.com>
> > > > Cc: Saeed Mahameed <saeedm@mellanox.com>
> > > > Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > > > Cc: intel-wired-lan@lists.osuosl.org
> > > > Cc: Yisen Zhuang <yisen.zhuang@huawei.com>
> > > > Cc: Salil Mehta <salil.mehta@huawei.com>
> > > > Cc: Michael Chan <michael.chan@broadcom.com>
> > > > Cc: Igor Russkikh <igor.russkikh@aquantia.com>
> > > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ...
> > > Also please add C based test for skb-less flow_dissector.
> > > Current test_flow_dissector.sh doesn't seem to cover it.
> > It doesn't look like we can exercise skb-less flow dissector from
> > test_flow_dissector.sh; we need to trigger some driver code, which is
> > hard when we send the packets on the localhost in
> > test_flow_dissector.sh.
> >
> > To test skb-less dissector I convert BPF_PROG_TEST_RUN to always use skb-less
> > mode. test_flow_dissector.sh tests skb-mode, prog_tests/flow_dissector.c
> > tests skb-less mode.
>
> I saw that but I'm afraid it's not enough.
> tun_get_user() is calling it, so it should be possible to test
> skb-less mode via tun.
Spent some time today looking into how to exercise this path in the tun
driver: doing writev() with IFF_NAPI_FRAGS IFF_TAP device would trigger
eth_get_headlen, but it looks like there is no way to do a test with
pass/no-pass result around that.
The problem is - we don't actually do anything with the result of
eth_get_headlen, there is only a sanity check for "headlen >
skb_headlen(skb)" which can't trigger for BPF flow dissector; we
carefully clamp thoff/nhoff and should not return offset outside the
input buffer.
By reading git history it looks like this call to eth_get_headlen was
added there to only make it possible for tools like syzbot to fuzz flow
dissector. That's why we don't care about the result, we just do that
simple sanity check. The main goal is to trigger some problem
(loop/warning) in the flow dissector code.
tl;dr - no mater which bpf flow dissector is attached to the namespace,
it would not change behavior of the tun device; even empty 'return
false' program would not alter it.
Let me know if you had something different in mind; because so far I
don't see how to do a test around that. Changing that "headlen >
skb_headlen(skb)" check into something meaningful also doesn't seem possible.
I thought about checking the result of eth_get_headlen against
skb_transport_offset(), but at that point transport offset of the skb
is not yet set (napi_gro_frags and gro layer later does that) :-(
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next v5 5/6] net: pass net argument to the eth_get_headlen
2019-04-19 23:29 ` Stanislav Fomichev
@ 2019-04-19 23:37 ` Alexei Starovoitov
2019-04-19 23:47 ` Stanislav Fomichev
0 siblings, 1 reply; 14+ messages in thread
From: Alexei Starovoitov @ 2019-04-19 23:37 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Stanislav Fomichev, netdev, bpf, davem, ast, daniel,
simon.horman, willemb, peterpenkov96, Maxim Krasnyansky,
Saeed Mahameed, Jeff Kirsher, intel-wired-lan, Yisen Zhuang,
Salil Mehta, Michael Chan, Igor Russkikh
On Fri, Apr 19, 2019 at 04:29:44PM -0700, Stanislav Fomichev wrote:
> On 04/18, Alexei Starovoitov wrote:
> > On Thu, Apr 18, 2019 at 05:43:50PM -0700, Stanislav Fomichev wrote:
> > > On 04/18, Alexei Starovoitov wrote:
> > > > On Mon, Apr 15, 2019 at 10:38:00AM -0700, Stanislav Fomichev wrote:
> > > > > Update all users eth_get_headlen to pass network namespace
> > > > > and pass it down to the flow dissector. This commit is a noop
> > > > > until administrator inserts BPF flow dissector program.
> > > > >
> > > > > Cc: Maxim Krasnyansky <maxk@qti.qualcomm.com>
> > > > > Cc: Saeed Mahameed <saeedm@mellanox.com>
> > > > > Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > > > > Cc: intel-wired-lan@lists.osuosl.org
> > > > > Cc: Yisen Zhuang <yisen.zhuang@huawei.com>
> > > > > Cc: Salil Mehta <salil.mehta@huawei.com>
> > > > > Cc: Michael Chan <michael.chan@broadcom.com>
> > > > > Cc: Igor Russkikh <igor.russkikh@aquantia.com>
> > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ...
> > > > Also please add C based test for skb-less flow_dissector.
> > > > Current test_flow_dissector.sh doesn't seem to cover it.
> > > It doesn't look like we can exercise skb-less flow dissector from
> > > test_flow_dissector.sh; we need to trigger some driver code, which is
> > > hard when we send the packets on the localhost in
> > > test_flow_dissector.sh.
> > >
> > > To test skb-less dissector I convert BPF_PROG_TEST_RUN to always use skb-less
> > > mode. test_flow_dissector.sh tests skb-mode, prog_tests/flow_dissector.c
> > > tests skb-less mode.
> >
> > I saw that but I'm afraid it's not enough.
> > tun_get_user() is calling it, so it should be possible to test
> > skb-less mode via tun.
> Spent some time today looking into how to exercise this path in the tun
> driver: doing writev() with IFF_NAPI_FRAGS IFF_TAP device would trigger
> eth_get_headlen, but it looks like there is no way to do a test with
> pass/no-pass result around that.
>
> The problem is - we don't actually do anything with the result of
> eth_get_headlen, there is only a sanity check for "headlen >
> skb_headlen(skb)" which can't trigger for BPF flow dissector; we
> carefully clamp thoff/nhoff and should not return offset outside the
> input buffer.
>
> By reading git history it looks like this call to eth_get_headlen was
> added there to only make it possible for tools like syzbot to fuzz flow
> dissector. That's why we don't care about the result, we just do that
> simple sanity check. The main goal is to trigger some problem
> (loop/warning) in the flow dissector code.
>
> tl;dr - no mater which bpf flow dissector is attached to the namespace,
> it would not change behavior of the tun device; even empty 'return
> false' program would not alter it.
sure, but the program will run and the test can validate that the program
saw valid packet, parsed it correctly and returned correct dissection.
The results can be stored in a map and validated by the test.
iirc you were saying that you'll have one program doing dissection
for with-skb and skb-less cases.
I think it's important to have such program in selftests and being
run continuously for both cases.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next v5 5/6] net: pass net argument to the eth_get_headlen
2019-04-19 23:37 ` Alexei Starovoitov
@ 2019-04-19 23:47 ` Stanislav Fomichev
2019-04-19 23:50 ` Alexei Starovoitov
0 siblings, 1 reply; 14+ messages in thread
From: Stanislav Fomichev @ 2019-04-19 23:47 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Stanislav Fomichev, netdev, bpf, davem, ast, daniel,
simon.horman, willemb, peterpenkov96, Maxim Krasnyansky,
Saeed Mahameed, Jeff Kirsher, intel-wired-lan, Yisen Zhuang,
Salil Mehta, Michael Chan, Igor Russkikh
On 04/19, Alexei Starovoitov wrote:
> On Fri, Apr 19, 2019 at 04:29:44PM -0700, Stanislav Fomichev wrote:
> > On 04/18, Alexei Starovoitov wrote:
> > > On Thu, Apr 18, 2019 at 05:43:50PM -0700, Stanislav Fomichev wrote:
> > > > On 04/18, Alexei Starovoitov wrote:
> > > > > On Mon, Apr 15, 2019 at 10:38:00AM -0700, Stanislav Fomichev wrote:
> > > > > > Update all users eth_get_headlen to pass network namespace
> > > > > > and pass it down to the flow dissector. This commit is a noop
> > > > > > until administrator inserts BPF flow dissector program.
> > > > > >
> > > > > > Cc: Maxim Krasnyansky <maxk@qti.qualcomm.com>
> > > > > > Cc: Saeed Mahameed <saeedm@mellanox.com>
> > > > > > Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > > > > > Cc: intel-wired-lan@lists.osuosl.org
> > > > > > Cc: Yisen Zhuang <yisen.zhuang@huawei.com>
> > > > > > Cc: Salil Mehta <salil.mehta@huawei.com>
> > > > > > Cc: Michael Chan <michael.chan@broadcom.com>
> > > > > > Cc: Igor Russkikh <igor.russkikh@aquantia.com>
> > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > ...
> > > > > Also please add C based test for skb-less flow_dissector.
> > > > > Current test_flow_dissector.sh doesn't seem to cover it.
> > > > It doesn't look like we can exercise skb-less flow dissector from
> > > > test_flow_dissector.sh; we need to trigger some driver code, which is
> > > > hard when we send the packets on the localhost in
> > > > test_flow_dissector.sh.
> > > >
> > > > To test skb-less dissector I convert BPF_PROG_TEST_RUN to always use skb-less
> > > > mode. test_flow_dissector.sh tests skb-mode, prog_tests/flow_dissector.c
> > > > tests skb-less mode.
> > >
> > > I saw that but I'm afraid it's not enough.
> > > tun_get_user() is calling it, so it should be possible to test
> > > skb-less mode via tun.
> > Spent some time today looking into how to exercise this path in the tun
> > driver: doing writev() with IFF_NAPI_FRAGS IFF_TAP device would trigger
> > eth_get_headlen, but it looks like there is no way to do a test with
> > pass/no-pass result around that.
> >
> > The problem is - we don't actually do anything with the result of
> > eth_get_headlen, there is only a sanity check for "headlen >
> > skb_headlen(skb)" which can't trigger for BPF flow dissector; we
> > carefully clamp thoff/nhoff and should not return offset outside the
> > input buffer.
> >
> > By reading git history it looks like this call to eth_get_headlen was
> > added there to only make it possible for tools like syzbot to fuzz flow
> > dissector. That's why we don't care about the result, we just do that
> > simple sanity check. The main goal is to trigger some problem
> > (loop/warning) in the flow dissector code.
> >
> > tl;dr - no mater which bpf flow dissector is attached to the namespace,
> > it would not change behavior of the tun device; even empty 'return
> > false' program would not alter it.
>
> sure, but the program will run and the test can validate that the program
> saw valid packet, parsed it correctly and returned correct dissection.
> The results can be stored in a map and validated by the test.
SG, that's doable; that would make bpf_flow.c less generic because it would
have to have this map which would export the last dissection, but that
should be fine, I guess.
(I planned to use bpf_flow.c internally instead of writing another one).
> iirc you were saying that you'll have one program doing dissection
> for with-skb and skb-less cases.
Correct.
> I think it's important to have such program in selftests and being
> run continuously for both cases.
Ok, in this case, I can write a small userspace program that writes some
dummy packet into a tap device (triggers eth_get_headlen) and reads back
and verifies bpf_flow_keys from the shared map.
Correct me if I misunderstood something. Otherwise, I'll get back to you
with a v6+test.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH bpf-next v5 5/6] net: pass net argument to the eth_get_headlen
2019-04-19 23:47 ` Stanislav Fomichev
@ 2019-04-19 23:50 ` Alexei Starovoitov
0 siblings, 0 replies; 14+ messages in thread
From: Alexei Starovoitov @ 2019-04-19 23:50 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Stanislav Fomichev, netdev, bpf, davem, ast, daniel,
simon.horman, willemb, peterpenkov96, Maxim Krasnyansky,
Saeed Mahameed, Jeff Kirsher, intel-wired-lan, Yisen Zhuang,
Salil Mehta, Michael Chan, Igor Russkikh
On Fri, Apr 19, 2019 at 04:47:44PM -0700, Stanislav Fomichev wrote:
> On 04/19, Alexei Starovoitov wrote:
> > On Fri, Apr 19, 2019 at 04:29:44PM -0700, Stanislav Fomichev wrote:
> > > On 04/18, Alexei Starovoitov wrote:
> > > > On Thu, Apr 18, 2019 at 05:43:50PM -0700, Stanislav Fomichev wrote:
> > > > > On 04/18, Alexei Starovoitov wrote:
> > > > > > On Mon, Apr 15, 2019 at 10:38:00AM -0700, Stanislav Fomichev wrote:
> > > > > > > Update all users eth_get_headlen to pass network namespace
> > > > > > > and pass it down to the flow dissector. This commit is a noop
> > > > > > > until administrator inserts BPF flow dissector program.
> > > > > > >
> > > > > > > Cc: Maxim Krasnyansky <maxk@qti.qualcomm.com>
> > > > > > > Cc: Saeed Mahameed <saeedm@mellanox.com>
> > > > > > > Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > > > > > > Cc: intel-wired-lan@lists.osuosl.org
> > > > > > > Cc: Yisen Zhuang <yisen.zhuang@huawei.com>
> > > > > > > Cc: Salil Mehta <salil.mehta@huawei.com>
> > > > > > > Cc: Michael Chan <michael.chan@broadcom.com>
> > > > > > > Cc: Igor Russkikh <igor.russkikh@aquantia.com>
> > > > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > > ...
> > > > > > Also please add C based test for skb-less flow_dissector.
> > > > > > Current test_flow_dissector.sh doesn't seem to cover it.
> > > > > It doesn't look like we can exercise skb-less flow dissector from
> > > > > test_flow_dissector.sh; we need to trigger some driver code, which is
> > > > > hard when we send the packets on the localhost in
> > > > > test_flow_dissector.sh.
> > > > >
> > > > > To test skb-less dissector I convert BPF_PROG_TEST_RUN to always use skb-less
> > > > > mode. test_flow_dissector.sh tests skb-mode, prog_tests/flow_dissector.c
> > > > > tests skb-less mode.
> > > >
> > > > I saw that but I'm afraid it's not enough.
> > > > tun_get_user() is calling it, so it should be possible to test
> > > > skb-less mode via tun.
> > > Spent some time today looking into how to exercise this path in the tun
> > > driver: doing writev() with IFF_NAPI_FRAGS IFF_TAP device would trigger
> > > eth_get_headlen, but it looks like there is no way to do a test with
> > > pass/no-pass result around that.
> > >
> > > The problem is - we don't actually do anything with the result of
> > > eth_get_headlen, there is only a sanity check for "headlen >
> > > skb_headlen(skb)" which can't trigger for BPF flow dissector; we
> > > carefully clamp thoff/nhoff and should not return offset outside the
> > > input buffer.
> > >
> > > By reading git history it looks like this call to eth_get_headlen was
> > > added there to only make it possible for tools like syzbot to fuzz flow
> > > dissector. That's why we don't care about the result, we just do that
> > > simple sanity check. The main goal is to trigger some problem
> > > (loop/warning) in the flow dissector code.
> > >
> > > tl;dr - no mater which bpf flow dissector is attached to the namespace,
> > > it would not change behavior of the tun device; even empty 'return
> > > false' program would not alter it.
> >
> > sure, but the program will run and the test can validate that the program
> > saw valid packet, parsed it correctly and returned correct dissection.
> > The results can be stored in a map and validated by the test.
> SG, that's doable; that would make bpf_flow.c less generic because it would
> have to have this map which would export the last dissection, but that
> should be fine, I guess.
>
> (I planned to use bpf_flow.c internally instead of writing another one).
>
> > iirc you were saying that you'll have one program doing dissection
> > for with-skb and skb-less cases.
> Correct.
>
> > I think it's important to have such program in selftests and being
> > run continuously for both cases.
> Ok, in this case, I can write a small userspace program that writes some
> dummy packet into a tap device (triggers eth_get_headlen) and reads back
> and verifies bpf_flow_keys from the shared map.
that sounds good. there could be two test programs. bpf_flow.c that you
use internally and another one, but please make that other one to test
both with-skb and skb-less paths.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH bpf-next v5 6/6] selftests/bpf: add flow dissector bpf_skb_load_bytes helper test
2019-04-15 17:37 [PATCH bpf-next v5 0/6] net: flow_dissector: trigger BPF hook when called from eth_get_headlen Stanislav Fomichev
` (4 preceding siblings ...)
2019-04-15 17:38 ` [PATCH bpf-next v5 5/6] net: pass net argument to the eth_get_headlen Stanislav Fomichev
@ 2019-04-15 17:38 ` Stanislav Fomichev
5 siblings, 0 replies; 14+ messages in thread
From: Stanislav Fomichev @ 2019-04-15 17:38 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, simon.horman, willemb, peterpenkov96,
Stanislav Fomichev
When flow dissector is called without skb, we want to make sure
bpf_skb_load_bytes invocations return error. Add small test which tries
to read single byte from a packet.
bpf_skb_load_bytes should always fail under BPF_PROG_TEST_RUN because
it was converted to the skb-less mode.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
.../prog_tests/flow_dissector_load_bytes.c | 48 +++++++++++++++++++
1 file changed, 48 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/flow_dissector_load_bytes.c
diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector_load_bytes.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector_load_bytes.c
new file mode 100644
index 000000000000..dc5ef155ec28
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector_load_bytes.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+
+void test_flow_dissector_load_bytes(void)
+{
+ struct bpf_flow_keys flow_keys;
+ __u32 duration = 0, retval, size;
+ struct bpf_insn prog[] = {
+ // BPF_REG_1 - 1st argument: context
+ // BPF_REG_2 - 2nd argument: offset, start at first byte
+ BPF_MOV64_IMM(BPF_REG_2, 0),
+ // BPF_REG_3 - 3rd argument: destination, reserve byte on stack
+ BPF_ALU64_REG(BPF_MOV, BPF_REG_3, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, -1),
+ // BPF_REG_4 - 4th argument: copy one byte
+ BPF_MOV64_IMM(BPF_REG_4, 1),
+ // bpf_skb_load_bytes(ctx, sizeof(pkt_v4), ptr, 1)
+ BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
+ BPF_FUNC_skb_load_bytes),
+ BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 2),
+ // if (ret == 0) return BPF_DROP (2)
+ BPF_MOV64_IMM(BPF_REG_0, BPF_DROP),
+ BPF_EXIT_INSN(),
+ // if (ret != 0) return BPF_OK (0)
+ BPF_MOV64_IMM(BPF_REG_0, BPF_OK),
+ BPF_EXIT_INSN(),
+ };
+ int fd, err;
+
+ /* make sure bpf_skb_load_bytes is not allowed from skb-less context
+ */
+ fd = bpf_load_program(BPF_PROG_TYPE_FLOW_DISSECTOR, prog,
+ ARRAY_SIZE(prog), "GPL", 0, NULL, 0);
+ CHECK(fd < 0,
+ "flow_dissector-bpf_skb_load_bytes-load",
+ "fd %d errno %d\n",
+ fd, errno);
+
+ err = bpf_prog_test_run(fd, 1, &pkt_v4, sizeof(pkt_v4),
+ &flow_keys, &size, &retval, &duration);
+ CHECK(size != sizeof(flow_keys) || err || retval != 1,
+ "flow_dissector-bpf_skb_load_bytes",
+ "err %d errno %d retval %d duration %d size %u/%zu\n",
+ err, errno, retval, duration, size, sizeof(flow_keys));
+
+ if (fd >= -1)
+ close(fd);
+}
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related [flat|nested] 14+ messages in thread