[PATCH v3 net-next 1/2] net: introduce skb_postpush_rcsum() helper

[PATCH v3 net-next 1/2] net: introduce skb_postpush_rcsum() helper

[Alexei Starovoitov]

similar to skb_postpull_rcsum() introduce skb_postpush_rcsum() helper

Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---

next patch is also using skb_postpush_rcsum()

 include/linux/skbuff.h |   13 +++++++++++++
 net/core/filter.c      |    4 ++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 36f3f43c0117..b7a313d13d7b 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2586,6 +2586,19 @@ static inline void skb_postpull_rcsum(struct sk_buff *skb,
 		skb->csum = csum_sub(skb->csum, csum_partial(start, len, 0));
 }
 
+/**
+ *	skb_postpush_rcsum - update checksum for skb after push
+ *	@skb: buffer to update
+ *	@start: start of data after push
+ *	@len: length of data pushed
+ */
+static inline void skb_postpush_rcsum(struct sk_buff *skb,
+				      const void *start, unsigned int len)
+{
+	if (skb->ip_summed == CHECKSUM_COMPLETE)
+		skb->csum = csum_add(skb->csum, csum_partial(start, len, 0));
+}
+
 unsigned char *skb_pull_rcsum(struct sk_buff *skb, unsigned int len);
 
 /**
diff --git a/net/core/filter.c b/net/core/filter.c
index b669e75d2b36..e5872704c28b 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1213,8 +1213,8 @@ static u64 bpf_skb_store_bytes(u64 r1, u64 r2, u64 r3, u64 r4, u64 flags)
 		/* skb_store_bits cannot return -EFAULT here */
 		skb_store_bits(skb, offset, ptr, len);
 
-	if (BPF_RECOMPUTE_CSUM(flags) && skb->ip_summed == CHECKSUM_COMPLETE)
-		skb->csum = csum_add(skb->csum, csum_partial(ptr, len, 0));
+	if (BPF_RECOMPUTE_CSUM(flags))
+		skb_postpush_rcsum(skb, ptr, len);
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[Alexei Starovoitov]

TC classifers and actions attached to ingress and egress qdiscs see
inconsistent skb->data. For ingress L2 header is already pulled, whereas
for egress it's present. Introduce an optional flag for ingress qdisc
which if set will cause ingress to push L2 header before calling
into classifiers/actions and pull L2 back afterwards.

The following cls/act assume L2 and now marked as 'needs_l2':
cls_bpf, act_bpf, cls_rsvp, act_csum, act_nat.
The users can use them on ingress qdisc created with 'needs_l2' flag and
on any egress qdisc. The use of them with vanilla ingress is disallowed.

Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
V2->V3: rewrite to use optional flag for ingress qdisc and corresponding
flags for classifers and actions

cls_bpf/act_bpf will always require L2.

cls_rsvp, act_csum, act_nat are doing pskb_may_pull/skb_clone_writable
assuming that skb->data is pointing to L2. Should be fixable.

Everything that calls into ematch is also potentially broken on ingress.
Like em_canid does:
  struct can_frame *cf = (struct can_frame *)skb->data;
and other em-s do:
  case TCF_LAYER_LINK:
    return skb->data;
Should be fixable as well.

skb_share_check() can be costly when taps are used. In my use cases of
tc+bpf that doesn't happen, so I'm not worried at the moment.
If tc+bpf usage changes in the future, we can add 'early_ingress_l2' flag
that will bypass taps and avoid skb_share_check. This early_handle_ing() hook
can be wrapped with static_key as well.

 include/net/act_api.h          |    9 +++++----
 include/net/sch_generic.h      |    3 +++
 include/uapi/linux/pkt_sched.h |   10 ++++++++++
 net/core/dev.c                 |   22 ++++++++++++++++++++--
 net/sched/act_api.c            |   21 +++++++++++++++------
 net/sched/act_bpf.c            |    1 +
 net/sched/act_csum.c           |    2 ++
 net/sched/act_nat.c            |    2 ++
 net/sched/cls_api.c            |   21 +++++++++++++++++++--
 net/sched/cls_bpf.c            |    1 +
 net/sched/cls_rsvp.h           |    2 ++
 net/sched/sch_ingress.c        |   27 +++++++++++++++++++++++++++
 samples/bpf/tcbpf1_kern.c      |    6 ------
 13 files changed, 107 insertions(+), 20 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 3ee4c92afd1b..c52bd98b7dd9 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -95,6 +95,8 @@ struct tc_action_ops {
 			struct nlattr *est, struct tc_action *act, int ovr,
 			int bind);
 	int     (*walk)(struct sk_buff *, struct netlink_callback *, int, struct tc_action *);
+	u32	flags;
+#define TCA_NEEDS_L2	1
 };
 
 int tcf_hash_search(struct tc_action *a, u32 index);
@@ -112,12 +114,11 @@ int tcf_unregister_action(struct tc_action_ops *a);
 int tcf_action_destroy(struct list_head *actions, int bind);
 int tcf_action_exec(struct sk_buff *skb, const struct list_head *actions,
 		    struct tcf_result *res);
-int tcf_action_init(struct net *net, struct nlattr *nla,
-				  struct nlattr *est, char *n, int ovr,
-				  int bind, struct list_head *);
+int tcf_action_init(struct net *net, struct nlattr *nla, struct nlattr *est,
+		    char *n, int ovr, int bind, struct list_head *, bool);
 struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 				    struct nlattr *est, char *n, int ovr,
-				    int bind);
+				    int bind, bool qdisc_has_l2);
 int tcf_action_dump(struct sk_buff *skb, struct list_head *, int, int);
 int tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int, int);
 int tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int, int);
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 6d778efcfdfd..5f0efa556a35 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -61,6 +61,7 @@ struct Qdisc {
 				      */
 #define TCQ_F_WARN_NONWC	(1 << 16)
 #define TCQ_F_CPUSTATS		0x20 /* run using percpu statistics */
+#define TCQ_F_INGRESS_L2	0x40 /* ingress qdisc with L2 header */
 	u32			limit;
 	const struct Qdisc_ops	*ops;
 	struct qdisc_size_table	__rcu *stab;
@@ -228,6 +229,8 @@ struct tcf_proto_ops {
 					struct sk_buff *skb, struct tcmsg*);
 
 	struct module		*owner;
+	u32			flags;
+#define TCF_NEEDS_L2	1
 };
 
 struct tcf_proto {
diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index 534b84710745..1076c56e6458 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -845,4 +845,14 @@ struct tc_pie_xstats {
 	__u32 maxq;             /* maximum queue size */
 	__u32 ecn_mark;         /* packets marked with ecn*/
 };
+
+/* INGRESS section */
+
+enum {
+	TCA_INGRESS_UNSPEC,
+	TCA_INGRESS_NEEDS_L2,
+	__TCA_INGRESS_MAX,
+};
+
+#define TCA_INGRESS_MAX (__TCA_INGRESS_MAX - 1)
 #endif
diff --git a/net/core/dev.c b/net/core/dev.c
index b2775f06c710..957a31ef25ee 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3513,10 +3513,13 @@ EXPORT_SYMBOL_GPL(br_fdb_test_addr_hook);
  * the ingress scheduler, you just can't add policies on ingress.
  *
  */
-static int ing_filter(struct sk_buff *skb, struct netdev_queue *rxq)
+static int ing_filter(struct sk_buff **pskb, struct netdev_queue *rxq)
 {
+	struct sk_buff *skb = *pskb;
 	struct net_device *dev = skb->dev;
+	u32 hard_header_len = dev->hard_header_len;
 	u32 ttl = G_TC_RTTL(skb->tc_verd);
+	u32 needs_l2;
 	int result = TC_ACT_OK;
 	struct Qdisc *q;
 
@@ -3531,10 +3534,25 @@ static int ing_filter(struct sk_buff *skb, struct netdev_queue *rxq)
 
 	q = rcu_dereference(rxq->qdisc);
 	if (q != &noop_qdisc) {
+		needs_l2 = q->flags & TCQ_F_INGRESS_L2;
+		if (needs_l2) {
+			*pskb = skb = skb_share_check(skb, GFP_ATOMIC);
+
+			if (unlikely(!skb))
+				/* original skb was freed by skb_share_check */
+				return TC_ACT_UNSPEC;
+
+			skb_push(skb, hard_header_len);
+			skb_postpush_rcsum(skb, skb->data, hard_header_len);
+		}
+
 		spin_lock(qdisc_lock(q));
 		if (likely(!test_bit(__QDISC_STATE_DEACTIVATED, &q->state)))
 			result = qdisc_enqueue_root(skb, q);
 		spin_unlock(qdisc_lock(q));
+
+		if (needs_l2)
+			skb_pull_rcsum(skb, hard_header_len);
 	}
 
 	return result;
@@ -3554,7 +3572,7 @@ static inline struct sk_buff *handle_ing(struct sk_buff *skb,
 		*pt_prev = NULL;
 	}
 
-	switch (ing_filter(skb, rxq)) {
+	switch (ing_filter(&skb, rxq)) {
 	case TC_ACT_SHOT:
 	case TC_ACT_STOLEN:
 		kfree_skb(skb);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 3d43e4979f27..0fe93a60c59b 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -484,7 +484,7 @@ errout:
 
 struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 				    struct nlattr *est, char *name, int ovr,
-				    int bind)
+				    int bind, bool qdisc_has_l2)
 {
 	struct tc_action *a;
 	struct tc_action_ops *a_o;
@@ -533,6 +533,14 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 		goto err_out;
 	}
 
+	if ((a_o->flags & TCA_NEEDS_L2) && !qdisc_has_l2) {
+		/* actions that require L2 cannot be attached
+		 * to vanilla ingress qdisc
+		 */
+		err = -EINVAL;
+		goto err_mod;
+	}
+
 	err = -ENOMEM;
 	a = kzalloc(sizeof(*a), GFP_KERNEL);
 	if (a == NULL)
@@ -565,9 +573,9 @@ err_out:
 	return ERR_PTR(err);
 }
 
-int tcf_action_init(struct net *net, struct nlattr *nla,
-				  struct nlattr *est, char *name, int ovr,
-				  int bind, struct list_head *actions)
+int tcf_action_init(struct net *net, struct nlattr *nla, struct nlattr *est,
+		    char *name, int ovr, int bind, struct list_head *actions,
+		    bool qdisc_has_l2)
 {
 	struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
 	struct tc_action *act;
@@ -579,7 +587,8 @@ int tcf_action_init(struct net *net, struct nlattr *nla,
 		return err;
 
 	for (i = 1; i <= TCA_ACT_MAX_PRIO && tb[i]; i++) {
-		act = tcf_action_init_1(net, tb[i], est, name, ovr, bind);
+		act = tcf_action_init_1(net, tb[i], est, name, ovr, bind,
+					qdisc_has_l2);
 		if (IS_ERR(act)) {
 			err = PTR_ERR(act);
 			goto err;
@@ -931,7 +940,7 @@ tcf_action_add(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 	int ret = 0;
 	LIST_HEAD(actions);
 
-	ret = tcf_action_init(net, nla, NULL, NULL, ovr, 0, &actions);
+	ret = tcf_action_init(net, nla, NULL, NULL, ovr, 0, &actions, true);
 	if (ret)
 		goto done;
 
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index 4d2cede17468..8f89d97dfa4a 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -339,6 +339,7 @@ static struct tc_action_ops act_bpf_ops __read_mostly = {
 	.dump		=	tcf_bpf_dump,
 	.cleanup	=	tcf_bpf_cleanup,
 	.init		=	tcf_bpf_init,
+	.flags		=	TCA_NEEDS_L2,
 };
 
 static int __init bpf_init_module(void)
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 4cd5cf1aedf8..887033fbdfa6 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -565,6 +565,8 @@ static struct tc_action_ops act_csum_ops = {
 	.act		= tcf_csum,
 	.dump		= tcf_csum_dump,
 	.init		= tcf_csum_init,
+	/* todo: fix pskb_may_pull in tcf_csum to not assume L2 */
+	.flags		= TCA_NEEDS_L2,
 };
 
 MODULE_DESCRIPTION("Checksum updating actions");
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 270a030d5fd0..de83aa016302 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -287,6 +287,8 @@ static struct tc_action_ops act_nat_ops = {
 	.act		=	tcf_nat,
 	.dump		=	tcf_nat_dump,
 	.init		=	tcf_nat_init,
+	/* todo: fix pskb_may_pull in tcf_nat to not assume L2 */
+	.flags		=	TCA_NEEDS_L2,
 };
 
 MODULE_DESCRIPTION("Stateless NAT actions");
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 8b0470e418dc..ae1b1b769136 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -257,6 +257,17 @@ replay:
 			kfree(tp);
 			goto errout;
 		}
+		if ((tp_ops->flags & TCF_NEEDS_L2) &&
+		    (q->flags & TCQ_F_INGRESS) &&
+		    !(q->flags & TCQ_F_INGRESS_L2)) {
+			/* classifiers that need L2 header cannot be
+			 * attached to vanilla ingress qdisc
+			 */
+			err = -EINVAL;
+			module_put(tp_ops->owner);
+			kfree(tp);
+			goto errout;
+		}
 		tp->ops = tp_ops;
 		tp->protocol = protocol;
 		tp->prio = nprio ? :
@@ -517,12 +528,17 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
 #ifdef CONFIG_NET_CLS_ACT
 	{
 		struct tc_action *act;
+		bool qdisc_has_l2 = true;
+
+		if ((tp->q->flags & TCQ_F_INGRESS) &&
+		    !(tp->q->flags & TCQ_F_INGRESS_L2))
+			qdisc_has_l2 = false;
 
 		INIT_LIST_HEAD(&exts->actions);
 		if (exts->police && tb[exts->police]) {
 			act = tcf_action_init_1(net, tb[exts->police], rate_tlv,
 						"police", ovr,
-						TCA_ACT_BIND);
+						TCA_ACT_BIND, qdisc_has_l2);
 			if (IS_ERR(act))
 				return PTR_ERR(act);
 
@@ -532,7 +548,8 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
 			int err;
 			err = tcf_action_init(net, tb[exts->action], rate_tlv,
 					      NULL, ovr,
-					      TCA_ACT_BIND, &exts->actions);
+					      TCA_ACT_BIND, &exts->actions,
+					      qdisc_has_l2);
 			if (err)
 				return err;
 		}
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 5c4171c5d2bd..b99e677e2800 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -476,6 +476,7 @@ static struct tcf_proto_ops cls_bpf_ops __read_mostly = {
 	.delete		=	cls_bpf_delete,
 	.walk		=	cls_bpf_walk,
 	.dump		=	cls_bpf_dump,
+	.flags		=	TCF_NEEDS_L2,
 };
 
 static int __init cls_bpf_init_mod(void)
diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
index 02fa82792dab..9f1ff1e19f20 100644
--- a/net/sched/cls_rsvp.h
+++ b/net/sched/cls_rsvp.h
@@ -716,6 +716,8 @@ static struct tcf_proto_ops RSVP_OPS __read_mostly = {
 	.walk		=	rsvp_walk,
 	.dump		=	rsvp_dump,
 	.owner		=	THIS_MODULE,
+	/* todo: fix pskb_network_may_pull in rsvp_classify to not assume L2 */
+	.flags		=	TCF_NEEDS_L2,
 };
 
 static int __init init_rsvp(void)
diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
index eb5b8445fef9..431ae6ffd142 100644
--- a/net/sched/sch_ingress.c
+++ b/net/sched/sch_ingress.c
@@ -109,6 +109,32 @@ nla_put_failure:
 	return -1;
 }
 
+static const struct nla_policy ingress_policy[TCA_INGRESS_MAX + 1] = {
+	[TCA_INGRESS_NEEDS_L2]	= { .type = NLA_U32 },
+};
+
+/* NEEDS_L2 flag can only be set at init time */
+static int ingress_init(struct Qdisc *sch, struct nlattr *opt)
+{
+	struct nlattr *tb[TCA_INGRESS_MAX + 1];
+	int err;
+
+	if (!opt)
+		return 0;
+
+	err = nla_parse_nested(tb, TCA_INGRESS_MAX, opt, ingress_policy);
+	if (err < 0)
+		return err;
+
+	if (tb[TCA_INGRESS_NEEDS_L2]) {
+		if (nla_get_u32(tb[TCA_INGRESS_NEEDS_L2]))
+			sch->flags |= TCQ_F_INGRESS_L2;
+		else
+			sch->flags &= ~TCQ_F_INGRESS_L2;
+	}
+	return 0;
+}
+
 static const struct Qdisc_class_ops ingress_class_ops = {
 	.leaf		=	ingress_leaf,
 	.get		=	ingress_get,
@@ -123,6 +149,7 @@ static struct Qdisc_ops ingress_qdisc_ops __read_mostly = {
 	.cl_ops		=	&ingress_class_ops,
 	.id		=	"ingress",
 	.priv_size	=	sizeof(struct ingress_qdisc_data),
+	.init		=	ingress_init,
 	.enqueue	=	ingress_enqueue,
 	.destroy	=	ingress_destroy,
 	.dump		=	ingress_dump,
diff --git a/samples/bpf/tcbpf1_kern.c b/samples/bpf/tcbpf1_kern.c
index 7cf3f42a6e39..76cdaab63058 100644
--- a/samples/bpf/tcbpf1_kern.c
+++ b/samples/bpf/tcbpf1_kern.c
@@ -14,12 +14,6 @@ static inline void set_dst_mac(struct __sk_buff *skb, char *mac)
 	bpf_skb_store_bytes(skb, 0, mac, ETH_ALEN, 1);
 }
 
-/* use 1 below for ingress qdisc and 0 for egress */
-#if 0
-#undef ETH_HLEN
-#define ETH_HLEN 0
-#endif
-
 #define IP_CSUM_OFF (ETH_HLEN + offsetof(struct iphdr, check))
 #define TOS_OFF (ETH_HLEN + offsetof(struct iphdr, tos))
 
-- 
1.7.9.5

Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[David Miller]

From: Alexei Starovoitov <ast@plumgrid.com>
Date: Wed,  8 Apr 2015 16:26:15 -0700

> index 4cd5cf1aedf8..887033fbdfa6 100644
> --- a/net/sched/act_csum.c
> +++ b/net/sched/act_csum.c
> @@ -565,6 +565,8 @@ static struct tc_action_ops act_csum_ops = {
>  	.act		= tcf_csum,
>  	.dump		= tcf_csum_dump,
>  	.init		= tcf_csum_init,
> +	/* todo: fix pskb_may_pull in tcf_csum to not assume L2 */
> +	.flags		= TCA_NEEDS_L2,
>  };
>  
>  MODULE_DESCRIPTION("Checksum updating actions");


All this module really cares about is where skb_network_header() is
for data accesses.  And a simple offset would allow it to calculate
the pskb_may_pull() length argument correctly.

And it would therefore work without all of these expensive SKB share
check calls etc.

I really don't like your approach, and I'd rather you propagate the
offset into the BPF programs you generate.  I bet you can do it in
an efficient way, and just haven't put in the effort to discover
how.

Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[Alexei Starovoitov]

On 4/8/15 7:44 PM, David Miller wrote:
> From: Alexei Starovoitov <ast@plumgrid.com>
> Date: Wed,  8 Apr 2015 16:26:15 -0700
>
>> index 4cd5cf1aedf8..887033fbdfa6 100644
>> --- a/net/sched/act_csum.c
>> +++ b/net/sched/act_csum.c
>> @@ -565,6 +565,8 @@ static struct tc_action_ops act_csum_ops = {
>>   	.act		= tcf_csum,
>>   	.dump		= tcf_csum_dump,
>>   	.init		= tcf_csum_init,
>> +	/* todo: fix pskb_may_pull in tcf_csum to not assume L2 */
>> +	.flags		= TCA_NEEDS_L2,
>>   };
>>
>>   MODULE_DESCRIPTION("Checksum updating actions");
>
>
> All this module really cares about is where skb_network_header() is
> for data accesses.  And a simple offset would allow it to calculate
> the pskb_may_pull() length argument correctly.
>
> And it would therefore work without all of these expensive SKB share
> check calls etc.

yes. act_csum can be fixed. I mentioned that in the commit log.
I can drop markings for anything other than cls_bpf/act_bpf if you like?

> I really don't like your approach, and I'd rather you propagate the
> offset into the BPF programs you generate.  I bet you can do it in
> an efficient way, and just haven't put in the effort to discover
> how.

I'm sure there is a way to propagate the offset into the programs.
It's not about efficiency of programs, but about consistency.
Programs should know nothing about kernel. Sending network offset
into them is exposing this very specific kernel behavior.
As soon as we do that all programs need to be written with this
offset in mind, so they can work with ingress and egress. It would
also break sockex1_kern.c, sockex2_kern.c and all other bpf programs.
That's not acceptable to me.
As I said I'd rather disable them attaching to ingress than force
all program authors to always use network_offset.
Offset 0 points to L2 was always fundamental assumption of BPF.
Both in classic and now carried over into extended.
We cannot change that.
We can cheat and say 'bpf for ingress' has to use new rules, but
that's just wrong. Imagine documenting it everywhere and all
confusion it will cause.

I still don't understand why you don't like it.
bridge is doing skb_share_check and that's acceptable there.
What's wrong with doing it for ingress here under flag?
Will 'early_ingress_l2' hook and flag be better?
Like adding:
skb = early_handle_ing(skb, &pt_prev, &ret, orig_dev);
right before taps in __netif_receive_skb_core ?
This way we can safely push/pull without share check.

Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[David Miller]

From: Alexei Starovoitov <ast@plumgrid.com>
Date: Wed, 08 Apr 2015 20:05:13 -0700

> I'm sure there is a way to propagate the offset into the programs.
> It's not about efficiency of programs, but about consistency.
> Programs should know nothing about kernel. Sending network offset
> into them is exposing this very specific kernel behavior.

It can be performed by the data access helpers the JIT'd programs
have to invoke anyways.

Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[Alexei Starovoitov]

On 4/8/15 8:14 PM, David Miller wrote:
> From: Alexei Starovoitov <ast@plumgrid.com>
> Date: Wed, 08 Apr 2015 20:05:13 -0700
>
>> I'm sure there is a way to propagate the offset into the programs.
>> It's not about efficiency of programs, but about consistency.
>> Programs should know nothing about kernel. Sending network offset
>> into them is exposing this very specific kernel behavior.
>
> It can be performed by the data access helpers the JIT'd programs
> have to invoke anyways.

hmm, not sure what you mean.
Let's take specific line from sockex1_kern.c:
int index = load_byte(skb, ETH_HLEN + offsetof(struct iphdr, protocol));
this C code is compiled into
R0 = LD_ABS_B 23
(instruction with fixed offset)
which is being interpreted as:
skb_header_pointer(skb, 23, 1, buffer);
and similar by JITs which are using doing
r0 = *(char *)(skb->data + 23)
in this case.

Are you proposing to change semantics of LD_ABS instruction to use
skb->head + skb->mac_header instead of skb->data in interpreter
and in all JITs?
Performance wise it will be ok, since JITs can cache that pointer.
But that will be huge and very risky change.
I'm not sure yet whether all programs will keep working afterwards.
Is it really worth taking so much risk vs push/pull of L2?
If you say, let's take the risk, sure, I can try hacking all the bits
and see what the cost.

Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[Alexei Starovoitov]

On 4/8/15 8:39 PM, Alexei Starovoitov wrote:
> On 4/8/15 8:14 PM, David Miller wrote:
>> From: Alexei Starovoitov <ast@plumgrid.com>
>> Date: Wed, 08 Apr 2015 20:05:13 -0700
>>
>>> I'm sure there is a way to propagate the offset into the programs.
>>> It's not about efficiency of programs, but about consistency.
>>> Programs should know nothing about kernel. Sending network offset
>>> into them is exposing this very specific kernel behavior.
>>
>> It can be performed by the data access helpers the JIT'd programs
>> have to invoke anyways.
>
> hmm, not sure what you mean.
> Let's take specific line from sockex1_kern.c:
> int index = load_byte(skb, ETH_HLEN + offsetof(struct iphdr, protocol));
> this C code is compiled into
> R0 = LD_ABS_B 23
> (instruction with fixed offset)
> which is being interpreted as:
> skb_header_pointer(skb, 23, 1, buffer);
> and similar by JITs which are using doing
> r0 = *(char *)(skb->data + 23)
> in this case.
>
> Are you proposing to change semantics of LD_ABS instruction to use
> skb->head + skb->mac_header instead of skb->data in interpreter
> and in all JITs?
> Performance wise it will be ok, since JITs can cache that pointer.
> But that will be huge and very risky change.
> I'm not sure yet whether all programs will keep working afterwards.
> Is it really worth taking so much risk vs push/pull of L2?
> If you say, let's take the risk, sure, I can try hacking all the bits
> and see what the cost.

have to correct myself.
we cannot change the meaning of ld_abs, since for dgram sockets
offset is actually not pointing to L2.
af_packet is doing:
  if (sk->sk_type != SOCK_DGRAM)
       skb_push(skb, skb->data - skb_mac_header(skb));
  res = run_filter(skb, sk, snaplen);
so not everything assumes L2. raw socket taps, ppp, team, cls_bpf
certainly want to see L2.
I still hate to see ingress and egress cls/act programs to be different.
af_packet also does:
skb = skb_share_check(skb, GFP_ATOMIC);

so Dave, would you consider early_ingress_l2() hook that doesn't need
to do skb_share_check ?
If not, the only option I have is to introduce a set of helpers
that can read from packet where offset is always starts at L2.
Then we can say that cls/act_bpf should never use ld_abs/ld_ind
instructions if they want to run on both ingress and egress and
should use these new read helpers instead.

Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[David Miller]

From: Alexei Starovoitov <ast@plumgrid.com>
Date: Wed, 08 Apr 2015 22:20:47 -0700

> so Dave, would you consider early_ingress_l2() hook that doesn't need
> to do skb_share_check ?

Let me think about it some more.

Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[Daniel Borkmann]

On 04/09/2015 07:20 AM, Alexei Starovoitov wrote:
...
> we cannot change the meaning of ld_abs, since for dgram sockets
> offset is actually not pointing to L2.
> af_packet is doing:
>   if (sk->sk_type != SOCK_DGRAM)
>        skb_push(skb, skb->data - skb_mac_header(skb));
>   res = run_filter(skb, sk, snaplen);
> so not everything assumes L2.

It would be not just that e.g. sock_queue_rcv_skb() users
have differing offset assumptions as well when they call
from their protocols into sk_filter().

Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[Eric Dumazet]

On Thu, 2015-04-09 at 17:15 +0200, Daniel Borkmann wrote:
> On 04/09/2015 07:20 AM, Alexei Starovoitov wrote:
> ...
> > we cannot change the meaning of ld_abs, since for dgram sockets
> > offset is actually not pointing to L2.
> > af_packet is doing:
> >   if (sk->sk_type != SOCK_DGRAM)
> >        skb_push(skb, skb->data - skb_mac_header(skb));
> >   res = run_filter(skb, sk, snaplen);
> > so not everything assumes L2.
> 
> It would be not just that e.g. sock_queue_rcv_skb() users
> have differing offset assumptions as well when they call
> from their protocols into sk_filter().

It seems that passing an extra parameter
could help those users, and would remove the requirement
of skb_clone() from dev_queue_xmit_nit()

skb_clone() would only be done after run_filter()

Maybe we'll finally have a fast packet capture, instead of simply
focusing on the filter part, which is tiny.

Lets take this discussion as the incentive to finally get this done
right.

Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[Alexei Starovoitov]

On 4/9/15 8:36 AM, Eric Dumazet wrote:
>
> Maybe we'll finally have a fast packet capture, instead of simply
> focusing on the filter part, which is tiny.

My plan for that was to use early_ingress_l2() hook before taps.
which will push L2 and call into the program _without_ skb_share_check.
We'll use it on the main interface where all traffic is seen.
The program will parse the packet the way it likes and redirect
the interesting packets to either different physical interface or
another tap/veth where user space will capture it. Then for 99%
of traffic on the main interface we don't pay any extra cost and if
external physical device is used for capturing then it's even cheaper.
On tracing side we were planing to use existing perf ring buffer infra
to pass some data to user space. The same helpers can be used to pass
info about the packet when user space doesn't need to see the whole
packet. In such case the bpf program will extract interesting headers
from the packet, store them into ring buffer and copy to user space
which is extremely fast.

Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[Jamal Hadi Salim]

On 04/08/15 23:05, Alexei Starovoitov wrote:
[..]
> As I said I'd rather disable them attaching to ingress than force
> all program authors to always use network_offset.
> Offset 0 points to L2 was always fundamental assumption of BPF.

The issue is not really the ingress qdisc here - rather the ingress
qdisc tries to cope with _how things work_.
The challenge is *some netdevs* (not all) strip off the link
layer _before_ the ingres qdisc sees them. You have to recognize
those devices and only speacial case with them. Your assumptions of
blindly pushing/pulling will break  in some cases (take a look at
mirred).

Having said that:
why could you not use the AT bits to identify whether you are
being invoked at ingress in the ebpf cls/ac wrapper? IOW:
have bpf cls/act be responsible for that knowledge.
Your changes penalize everyone else because of this assumption
bpf makes. We have always tried to be sensitive to perfomance.
People like Eric D. complain about a single if statement in this
code path and you are adding a bunch more unconditionally.
If all i wanted to do was police or account (a good number of use
cases) suddenly this gets more costly.

cheers,
jamal

Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[Jamal Hadi Salim]

On 04/09/15 06:49, Jamal Hadi Salim wrote:
> On 04/08/15 23:05, Alexei Starovoitov wrote:
> [..]
>> As I said I'd rather disable them attaching to ingress than force
>> all program authors to always use network_offset.
>> Offset 0 points to L2 was always fundamental assumption of BPF.
>
> The issue is not really the ingress qdisc here - rather the ingress
> qdisc tries to cope with _how things work_.
> The challenge is *some netdevs* (not all) strip off the link
> layer _before_ the ingres qdisc sees them. You have to recognize
> those devices and only speacial case with them. Your assumptions of
> blindly pushing/pulling will break  in some cases (take a look at
> mirred).
>

To be precise look at:
tcf_mirred_init() and how ok_push based on dev->type is decided.
So we do this in the slow path and make a simple compare in the
fast path on whether push the l2 header.

cheers,
jamal

Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[Daniel Borkmann]

On 04/09/2015 12:49 PM, Jamal Hadi Salim wrote:
...
> Your changes penalize everyone else because of this assumption
> bpf makes. We have always tried to be sensitive to perfomance.

That includes also BPF, right? ;) I mean you'd need to push extra
unneeded per-packet instructions down into the interpreter and
JITs that neither the output path needs in case of {cls,act}_bpf,
and generally other users working on skbs such as team driver, all
possible kind of sockets with filters attached, xt_bpf, etc, etc
just to accommodate for the ingress use-case. I mean I understand
your concern, but making BPF cls/act responsible for that knowledge
has it's downsides just as well.

Moreover, we'd enforce user space to start programming with
unintuitive negative offsets when accessing mac layer, and cls_bpf
at least, since it's around for some time, would need to start
differentiating between classic and native eBPF to keep compat
with old BPF programs for the output path. That's pretty messy. :/

Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[Jamal Hadi Salim]

On 04/09/15 11:38, Daniel Borkmann wrote:
> On 04/09/2015 12:49 PM, Jamal Hadi Salim wrote:
> ...
>> Your changes penalize everyone else because of this assumption
>> bpf makes. We have always tried to be sensitive to perfomance.
>
> That includes also BPF, right? ;)

Yes, of course - we are trying to reach some conclusion i hope. I
have no qualms on ebpf;  but I have concerns for other users of tc.
If the whole world is suddenly going to shift to ebpf then it is
easy to make a call. I doubt that will _ever_ happen.
As a new kid on the block ebpf needs to provide a strong case for
making such changes which affect everyone else.
It is not a simple "fix" that Alexei posted that will suffice to
make everything on ingress appear at offset 0. I am afraid, a lot
more intrusion will be needed (and we have avoided it all these
years).

> I mean you'd need to push extra
> unneeded per-packet instructions down into the interpreter and
> JITs that neither the output path needs in case of {cls,act}_bpf,
> and generally other users working on skbs such as team driver, all
> possible kind of sockets with filters attached, xt_bpf, etc, etc
> just to accommodate for the ingress use-case. I mean I understand
> your concern, but making BPF cls/act responsible for that knowledge
> has it's downsides just as well.
>

The main downside is usability for someone who wants to write code
that is inserted in the kernel. They have to know whether their code
will run on ingress or egress and the type of device etc.
The AT_XXX provides that signal and dev->type fills in the other gap.
Such coders i hope will have enough knowledge. It is close to
someone writing netfilter hooks needing to know that something is
at post/pre-routing etc

> Moreover, we'd enforce user space to start programming with
> unintuitive negative offsets when accessing mac layer, and cls_bpf
> at least, since it's around for some time, would need to start
> differentiating between classic and native eBPF to keep compat
> with old BPF programs for the output path. That's pretty messy. :/


I agree that user facing usability issues need to be addressed but that
is not the same as someone writing ebpf code.
The  negative offset presentation to the user does look ugly. You can
teach tc for the case of u32 to hide that from the user.

cheers,
jamal

Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[Alexei Starovoitov]

On 4/9/15 3:49 AM, Jamal Hadi Salim wrote:
> those devices and only speacial case with them. Your assumptions of
> blindly pushing/pulling will break  in some cases (take a look at
> mirred).

you underestimated how much time I've spent studying mirred ;)
In particular my v2 patch fixes two bugs in it:
- it was forwarding packets with L2 present to tunnel devices if used
   with egress qdisc
- it wasn't updating skb->csum with ingress qdisc

In v3 since 'needs_l2' is an optional flag, I didn't touch mirred
and left its bugs for the future patches.

> Your changes penalize everyone else because of this assumption
> bpf makes. We have always tried to be sensitive to perfomance.

hmm. penalize everyone? it's an optional flag.
If ingress_l2 is used with cls_bpf plus some other classifier,
sure, the other classifier is paying potential cost skb_share_check
if taps are active. The chances of having more than a couple classifiers
on a single device are very slim, since they're called sequentially
and killing performance regardless.
In most of the use cases I'm after, cls_bpf will be the only classifier
attached. In some cases there may be few others, but cls_bpf will be
the first and the heaviest one. So it makes sense to optimize for it.

> code path and you are adding a bunch more unconditionally.

not true at all. Looks like you misread the v3 patch completely.
It's an optional flag.

Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[Jamal Hadi Salim]

On 04/09/15 13:03, Alexei Starovoitov wrote:
> On 4/9/15 3:49 AM, Jamal Hadi Salim wrote:
>> those devices and only speacial case with them. Your assumptions of
>> blindly pushing/pulling will break  in some cases (take a look at
>> mirred).
>
> you underestimated how much time I've spent studying mirred ;)
> In particular my v2 patch fixes two bugs in it:
> - it was forwarding packets with L2 present to tunnel devices if used
>    with egress qdisc
> - it wasn't updating skb->csum with ingress qdisc
>
> In v3 since 'needs_l2' is an optional flag, I didn't touch mirred
> and left its bugs for the future patches.
>

Alexei,  when you do stuff like based on some policy as you suggest:
+	skb_push(skb, hard_header_len);
+	skb_postpush_rcsum(skb, skb->data, hard_header_len);

That doesnt give you L2 at offset 0 for _some devices_ which is what you
say you need. In some devices it adds a second L2 header.
This is why i keep pointing you to mirred. That is just how
the system works today.

To your "bugs" comments:
- It is reasonable but borderline grey area intepretation of policy
intent to refer additional L2 header a "bug". It is clearly
undesirable to send additional L2 headers (depending on the tunnel
device) and up  to now i havent thought of it as a "bug". But I would
concede that point. In my testing I thought that was a feature. This
is different from the ingress  side where it simply wont work if
you have a missing L2 header.
- updating csum; earlier i said i was conflicted it being a
useful "feature".
You are repeating again that it is a bug. It is not.
This action is intended to  mirror or redirect packets, period.
Unix philosophy do small things and do them well.
Adding checksum recomputation maybe useful if some preceding action
required it. But calling lack of checksum recomputation a bug is a
big stretch.


>
> hmm. penalize everyone? it's an optional flag.

You added at minimal two if conditions that execute on a per-packet
level. Eric D. has been hounding me for just having one.

> If ingress_l2 is used with cls_bpf plus some other classifier,
> sure, the other classifier is paying potential cost skb_share_check
> if taps are active. The chances of having more than a couple classifiers
> on a single device are very slim, since they're called sequentially
> and killing performance regardless.

In my world it is not uncommon.

> In most of the use cases I'm after, cls_bpf will be the only classifier
> attached.

And i am pointing there are other use cases.

>In some cases there may be few others, but cls_bpf will be
> the first and the heaviest one. So it makes sense to optimize for it.
>

I am sorry - but i remain unconvinced.

>> code path and you are adding a bunch more unconditionally.
>
> not true at all. Looks like you misread the v3 patch completely.
> It's an optional flag.

+		needs_l2 = q->flags & TCQ_F_INGRESS_L2;
+		if (needs_l2) {
..
....
+		if (needs_l2)

That is executed for _every_ packet going after ingress qdisc.

cheers,
jamal

Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[Alexei Starovoitov]

On 4/10/15 5:48 AM, Jamal Hadi Salim wrote:
>
> That doesnt give you L2 at offset 0 for _some devices_ which is what you
> say you need. In some devices it adds a second L2 header.

ok. fair point. will add a check to make sure such ingress_l2 qdisc
doesn't get applied to devices without header_ops.
Same check is done by af_packet.

>
> To your "bugs" comments:
> - updating csum; earlier i said i was conflicted it being a
> useful "feature".
> You are repeating again that it is a bug. It is not.
> This action is intended to  mirror or redirect packets, period.

without updating skb->csum act_mirred is breaking csum for
checksum_complete devices. If these redirected skbs are eventually
seen by stack, the stack will drop them. I cannot use such
'feature', so I would need to find a way to do mirred without fixing
act_mirred. Fine.

> +        needs_l2 = q->flags & TCQ_F_INGRESS_L2;
> +        if (needs_l2) {
> ..
> ....
> +        if (needs_l2)
>
> That is executed for _every_ packet going after ingress qdisc.

and there is hugely expensive spin_lock/unlock around ingress.
Comparing to lock cmpxchg the cost of branch is a noise.
But fine, I'll find a way to optimize the whole thing without
adding extra branches.

> They have to know whether their code
> will run on ingress or egress and the type of device etc.
> The AT_XXX provides that signal and dev->type fills in the other gap.

True. The program authors may want to know whether the packet is seen
on ingress or egress and take different actions inside the program,
but forcing them to _always_ parse the packet differently because of
it is not acceptable.

Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[Jamal Hadi Salim]

On 04/10/15 18:35, Alexei Starovoitov wrote:

>> To your "bugs" comments:
>> - updating csum; earlier i said i was conflicted it being a
>> useful "feature".
>> You are repeating again that it is a bug. It is not.
>> This action is intended to  mirror or redirect packets, period.
>
> without updating skb->csum act_mirred is breaking csum for
> checksum_complete devices.

Ok, could this then be checked for in dev features flags and
only then recomputed?


> True. The program authors may want to know whether the packet is seen
> on ingress or egress and take different actions inside the program,
> but forcing them to _always_ parse the packet differently because of
> it is not acceptable.


Yes, I see your point here as reasonable dilema. But you could add an
API call the user always make that resets and unsets the header?
It would be a single branch failure for egress/stack source.


cheers,
jamal

Re: [PATCH v3 net-next 2/2] tc: add 'needs_l2' flag to ingress qdisc

[Alexei Starovoitov]

On 4/13/15 7:28 AM, Jamal Hadi Salim wrote:
> On 04/10/15 18:35, Alexei Starovoitov wrote:
>
>>> To your "bugs" comments:
>>> - updating csum; earlier i said i was conflicted it being a
>>> useful "feature".
>>> You are repeating again that it is a bug. It is not.
>>> This action is intended to  mirror or redirect packets, period.
>>
>> without updating skb->csum act_mirred is breaking csum for
>> checksum_complete devices.
>
> Ok, could this then be checked for in dev features flags and
> only then recomputed?

that's what my v2 patch did :)
if ip_summed == checksum_complete -> recompute skb->csum

> Yes, I see your point here as reasonable dilema. But you could add an
> API call the user always make that resets and unsets the header?
> It would be a single branch failure for egress/stack source.

cannot do that from the program. skb->data and headerlen are already
cached by JITs and skb pointer itself is in some bpf registers,
so push/pull/skb_share_check is _not_ possible when program is running.
It can only be done before program starts. That's exactly what all my
patches are doing.