[patch net-next 1/2] net: move vlan pop/push functions into common code

[patch net-next 1/2] net: move vlan pop/push functions into common code

[Jiri Pirko]

So it can be used from out of openvswitch code.
Did couple of cosmetic changes on the way, namely variable naming and
adding support for 8021AD proto.

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 include/linux/skbuff.h    |  2 ++
 net/core/skbuff.c         | 86 +++++++++++++++++++++++++++++++++++++++++++++++
 net/openvswitch/actions.c | 76 ++---------------------------------------
 3 files changed, 90 insertions(+), 74 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 103fbe8..3b0445c 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2673,6 +2673,8 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet);
 unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
 struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
 struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
+int skb_vlan_pop(struct sk_buff *skb);
+int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci);
 
 struct skb_checksum_ops {
 	__wsum (*update)(const void *mem, int len, __wsum wsum);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 7001896..75e53d4 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4151,6 +4151,92 @@ err_free:
 }
 EXPORT_SYMBOL(skb_vlan_untag);
 
+/* remove VLAN header from packet and update csum accordingly. */
+static int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci)
+{
+	struct vlan_hdr *vhdr;
+	int err;
+
+	if (!pskb_may_pull(skb, VLAN_ETH_HLEN))
+		return -ENOMEM;
+
+	if (!skb_cloned(skb) || skb_clone_writable(skb, VLAN_ETH_HLEN))
+		return 0;
+
+	err = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
+	if (unlikely(err))
+		return err;
+
+	if (skb->ip_summed == CHECKSUM_COMPLETE)
+		skb->csum = csum_sub(skb->csum, csum_partial(skb->data
+					+ (2 * ETH_ALEN), VLAN_HLEN, 0));
+
+	vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
+	*vlan_tci = ntohs(vhdr->h_vlan_TCI);
+
+	memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
+	__skb_pull(skb, VLAN_HLEN);
+
+	vlan_set_encap_proto(skb, vhdr);
+	skb->mac_header += VLAN_HLEN;
+	if (skb_network_offset(skb) < ETH_HLEN)
+		skb_set_network_header(skb, ETH_HLEN);
+	skb_reset_mac_len(skb);
+
+	return 0;
+}
+
+int skb_vlan_pop(struct sk_buff *skb)
+{
+	u16 vlan_tci;
+	__be16 vlan_proto;
+	int err;
+
+	if (likely(vlan_tx_tag_present(skb))) {
+		skb->vlan_tci = 0;
+	} else {
+		if (unlikely((skb->protocol != htons(ETH_P_8021Q) &&
+			      skb->protocol != htons(ETH_P_8021AD)) ||
+			     skb->len < VLAN_ETH_HLEN))
+			return 0;
+
+		err = __skb_vlan_pop(skb, &vlan_tci);
+		if (err)
+			return err;
+	}
+	/* move next vlan tag to hw accel tag */
+	if (likely((skb->protocol != htons(ETH_P_8021Q) &&
+		    skb->protocol != htons(ETH_P_8021AD)) ||
+		   skb->len < VLAN_ETH_HLEN))
+		return 0;
+
+	vlan_proto = skb->protocol;
+	err = __skb_vlan_pop(skb, &vlan_tci);
+	if (unlikely(err))
+		return err;
+
+	__vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
+	return 0;
+}
+EXPORT_SYMBOL(skb_vlan_pop);
+
+int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci)
+{
+	if (vlan_tx_tag_present(skb)) {
+		/* push down current VLAN tag */
+		if (!__vlan_put_tag(skb, skb->vlan_proto,
+				    vlan_tx_tag_get(skb)))
+			return -ENOMEM;
+
+		if (skb->ip_summed == CHECKSUM_COMPLETE)
+			skb->csum = csum_add(skb->csum, csum_partial(skb->data
+					+ (2 * ETH_ALEN), VLAN_HLEN, 0));
+	}
+	__vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
+	return 0;
+}
+EXPORT_SYMBOL(skb_vlan_push);
+
 /**
  * alloc_skb_with_frags - allocate skb with page frags
  *
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index f7e5891..1b28110 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -206,86 +206,14 @@ static int set_mpls(struct sk_buff *skb, const __be32 *mpls_lse)
 	return 0;
 }
 
-/* remove VLAN header from packet and update csum accordingly. */
-static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
-{
-	struct vlan_hdr *vhdr;
-	int err;
-
-	err = make_writable(skb, VLAN_ETH_HLEN);
-	if (unlikely(err))
-		return err;
-
-	if (skb->ip_summed == CHECKSUM_COMPLETE)
-		skb->csum = csum_sub(skb->csum, csum_partial(skb->data
-					+ (2 * ETH_ALEN), VLAN_HLEN, 0));
-
-	vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
-	*current_tci = vhdr->h_vlan_TCI;
-
-	memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
-	__skb_pull(skb, VLAN_HLEN);
-
-	vlan_set_encap_proto(skb, vhdr);
-	skb->mac_header += VLAN_HLEN;
-
-	if (skb_network_offset(skb) < ETH_HLEN)
-		skb_set_network_header(skb, ETH_HLEN);
-
-	/* Update mac_len for subsequent MPLS actions */
-	skb_reset_mac_len(skb);
-	return 0;
-}
-
 static int pop_vlan(struct sk_buff *skb)
 {
-	__be16 tci;
-	int err;
-
-	if (likely(vlan_tx_tag_present(skb))) {
-		skb->vlan_tci = 0;
-	} else {
-		if (unlikely(skb->protocol != htons(ETH_P_8021Q) ||
-			     skb->len < VLAN_ETH_HLEN))
-			return 0;
-
-		err = __pop_vlan_tci(skb, &tci);
-		if (err)
-			return err;
-	}
-	/* move next vlan tag to hw accel tag */
-	if (likely(skb->protocol != htons(ETH_P_8021Q) ||
-		   skb->len < VLAN_ETH_HLEN))
-		return 0;
-
-	err = __pop_vlan_tci(skb, &tci);
-	if (unlikely(err))
-		return err;
-
-	__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), ntohs(tci));
-	return 0;
+	return skb_vlan_pop(skb);
 }
 
 static int push_vlan(struct sk_buff *skb, const struct ovs_action_push_vlan *vlan)
 {
-	if (unlikely(vlan_tx_tag_present(skb))) {
-		u16 current_tag;
-
-		/* push down current VLAN tag */
-		current_tag = vlan_tx_tag_get(skb);
-
-		if (!__vlan_put_tag(skb, skb->vlan_proto, current_tag))
-			return -ENOMEM;
-		/* Update mac_len for subsequent MPLS actions */
-		skb->mac_len += VLAN_HLEN;
-
-		if (skb->ip_summed == CHECKSUM_COMPLETE)
-			skb->csum = csum_add(skb->csum, csum_partial(skb->data
-					+ (2 * ETH_ALEN), VLAN_HLEN, 0));
-
-	}
-	__vlan_hwaccel_put_tag(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
-	return 0;
+	return skb_vlan_push(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
 }
 
 static int set_eth_addr(struct sk_buff *skb,
-- 
1.9.3

[patch net-next 2/2] sched: introduce vlan action

[Jiri Pirko]

This tc action allows to work with vlan tagged skbs. Two supported
sub-actions are header pop and header push.

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 include/net/tc_act/tc_vlan.h        |  27 +++++
 include/uapi/linux/tc_act/tc_vlan.h |  35 ++++++
 net/sched/Kconfig                   |  11 ++
 net/sched/Makefile                  |   1 +
 net/sched/act_vlan.c                | 207 ++++++++++++++++++++++++++++++++++++
 5 files changed, 281 insertions(+)
 create mode 100644 include/net/tc_act/tc_vlan.h
 create mode 100644 include/uapi/linux/tc_act/tc_vlan.h
 create mode 100644 net/sched/act_vlan.c

diff --git a/include/net/tc_act/tc_vlan.h b/include/net/tc_act/tc_vlan.h
new file mode 100644
index 0000000..c809c1d
--- /dev/null
+++ b/include/net/tc_act/tc_vlan.h
@@ -0,0 +1,27 @@
+/*
+ * Copyright (c) 2014 Jiri Pirko <jiri@resnulli.us>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __NET_TC_VLAN_H
+#define __NET_TC_VLAN_H
+
+#include <net/act_api.h>
+
+#define VLAN_F_POP		0x1
+#define VLAN_F_PUSH		0x2
+
+struct tcf_vlan {
+	struct tcf_common	common;
+	int			tcfv_action;
+	__be16			tcfv_push_vid;
+	__be16			tcfv_push_proto;
+};
+#define to_vlan(a) \
+	container_of(a->priv, struct tcf_vlan, common)
+
+#endif /* __NET_TC_VLAN_H */
diff --git a/include/uapi/linux/tc_act/tc_vlan.h b/include/uapi/linux/tc_act/tc_vlan.h
new file mode 100644
index 0000000..f7b8d44
--- /dev/null
+++ b/include/uapi/linux/tc_act/tc_vlan.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright (c) 2014 Jiri Pirko <jiri@resnulli.us>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_TC_VLAN_H
+#define __LINUX_TC_VLAN_H
+
+#include <linux/pkt_cls.h>
+
+#define TCA_ACT_VLAN 12
+
+#define TCA_VLAN_ACT_POP	1
+#define TCA_VLAN_ACT_PUSH	2
+
+struct tc_vlan {
+	tc_gen;
+	int v_action;
+};
+
+enum {
+	TCA_VLAN_UNSPEC,
+	TCA_VLAN_TM,
+	TCA_VLAN_PARMS,
+	TCA_VLAN_PUSH_VLAN_ID,
+	TCA_VLAN_PUSH_VLAN_PROTOCOL,
+	__TCA_VLAN_MAX,
+};
+#define TCA_VLAN_MAX (__TCA_VLAN_MAX - 1)
+
+#endif
diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index a1a8e29..88618f8 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig
@@ -686,6 +686,17 @@ config NET_ACT_CSUM
 	  To compile this code as a module, choose M here: the
 	  module will be called act_csum.
 
+config NET_ACT_VLAN
+        tristate "Vlan manipulation"
+        depends on NET_CLS_ACT
+        ---help---
+	  Say Y here to push or pop vlan headers.
+
+	  If unsure, say N.
+
+	  To compile this code as a module, choose M here: the
+	  module will be called act_vlan.
+
 config NET_CLS_IND
 	bool "Incoming device classification"
 	depends on NET_CLS_U32 || NET_CLS_FW
diff --git a/net/sched/Makefile b/net/sched/Makefile
index 0a869a1..679f24a 100644
--- a/net/sched/Makefile
+++ b/net/sched/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_NET_ACT_PEDIT)	+= act_pedit.o
 obj-$(CONFIG_NET_ACT_SIMP)	+= act_simple.o
 obj-$(CONFIG_NET_ACT_SKBEDIT)	+= act_skbedit.o
 obj-$(CONFIG_NET_ACT_CSUM)	+= act_csum.o
+obj-$(CONFIG_NET_ACT_VLAN)	+= act_vlan.o
 obj-$(CONFIG_NET_SCH_FIFO)	+= sch_fifo.o
 obj-$(CONFIG_NET_SCH_CBQ)	+= sch_cbq.o
 obj-$(CONFIG_NET_SCH_HTB)	+= sch_htb.o
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
new file mode 100644
index 0000000..013b17d
--- /dev/null
+++ b/net/sched/act_vlan.c
@@ -0,0 +1,207 @@
+/*
+ * Copyright (c) 2014 Jiri Pirko <jiri@resnulli.us>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/skbuff.h>
+#include <linux/rtnetlink.h>
+#include <linux/if_vlan.h>
+#include <net/netlink.h>
+#include <net/pkt_sched.h>
+
+#include <linux/tc_act/tc_vlan.h>
+#include <net/tc_act/tc_vlan.h>
+
+#define VLAN_TAB_MASK     15
+
+static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
+		    struct tcf_result *res)
+{
+	struct tcf_vlan *v = a->priv;
+	int action;
+	int err;
+
+	spin_lock(&v->tcf_lock);
+	v->tcf_tm.lastuse = jiffies;
+	bstats_update(&v->tcf_bstats, skb);
+	action = v->tcf_action;
+
+	switch (v->tcfv_action) {
+	case TCA_VLAN_ACT_POP:
+		err = skb_vlan_pop(skb);
+		if (err)
+			goto drop;
+		break;
+	case TCA_VLAN_ACT_PUSH:
+		err = skb_vlan_push(skb, v->tcfv_push_proto, v->tcfv_push_vid);
+		if (err)
+			goto drop;
+		break;
+	default:
+		BUG();
+	}
+
+	goto unlock;
+
+drop:
+	action = TC_ACT_SHOT;
+	v->tcf_qstats.drops++;
+unlock:
+	spin_unlock(&v->tcf_lock);
+	return action;
+}
+
+static const struct nla_policy vlan_policy[TCA_VLAN_MAX + 1] = {
+	[TCA_VLAN_PARMS]		= { .len = sizeof(struct tc_vlan) },
+	[TCA_VLAN_PUSH_VLAN_ID]		= { .type = NLA_U16 },
+	[TCA_VLAN_PUSH_VLAN_PROTOCOL]	= { .type = NLA_U16 },
+};
+
+static int tcf_vlan_init(struct net *net, struct nlattr *nla,
+			 struct nlattr *est, struct tc_action *a,
+			 int ovr, int bind)
+{
+	struct nlattr *tb[TCA_VLAN_MAX + 1];
+	struct tc_vlan *parm;
+	struct tcf_vlan *v;
+	int action;
+	__be16 push_vid = 0;
+	__be16 push_proto = 0;
+	int ret = 0;
+	int err;
+
+	if (!nla)
+		return -EINVAL;
+
+	err = nla_parse_nested(tb, TCA_VLAN_MAX, nla, vlan_policy);
+	if (err < 0)
+		return err;
+
+	if (!tb[TCA_VLAN_PARMS])
+		return -EINVAL;
+	parm = nla_data(tb[TCA_VLAN_PARMS]);
+	switch (parm->v_action) {
+	case TCA_VLAN_ACT_POP:
+		break;
+	case TCA_VLAN_ACT_PUSH:
+		if (!tb[TCA_VLAN_PUSH_VLAN_ID])
+			return -EINVAL;
+		push_vid = nla_get_u16(tb[TCA_VLAN_PUSH_VLAN_ID]);
+		if (push_vid >= VLAN_VID_MASK)
+			return -ERANGE;
+
+		if (tb[TCA_VLAN_PUSH_VLAN_PROTOCOL]) {
+			push_proto = nla_get_be16(tb[TCA_VLAN_PUSH_VLAN_PROTOCOL]);
+			switch (push_proto) {
+			case htons(ETH_P_8021Q):
+			case htons(ETH_P_8021AD):
+				break;
+			default:
+				return -EPROTONOSUPPORT;
+			}
+		} else {
+			push_proto = htons(ETH_P_8021Q);
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+	action = parm->v_action;
+
+	if (!tcf_hash_check(parm->index, a, bind)) {
+		ret = tcf_hash_create(parm->index, est, a, sizeof(*v), bind);
+		if (ret)
+			return ret;
+
+		ret = ACT_P_CREATED;
+	} else {
+		if (bind)
+			return 0;
+		tcf_hash_release(a, bind);
+		if (!ovr)
+			return -EEXIST;
+	}
+
+	v = to_vlan(a);
+
+	spin_lock_bh(&v->tcf_lock);
+
+	v->tcfv_action = action;
+	v->tcfv_push_vid = push_vid;
+	v->tcfv_push_proto = push_proto;
+
+	v->tcf_action = parm->action;
+
+	spin_unlock_bh(&v->tcf_lock);
+
+	if (ret == ACT_P_CREATED)
+		tcf_hash_insert(a);
+	return ret;
+}
+
+static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a,
+			 int bind, int ref)
+{
+	unsigned char *b = skb_tail_pointer(skb);
+	struct tcf_vlan *v = a->priv;
+	struct tc_vlan opt = {
+		.index    = v->tcf_index,
+		.refcnt   = v->tcf_refcnt - ref,
+		.bindcnt  = v->tcf_bindcnt - bind,
+		.action   = v->tcf_action,
+		.v_action = v->tcfv_action,
+	};
+	struct tcf_t t;
+
+	if (nla_put(skb, TCA_VLAN_PARMS, sizeof(opt), &opt))
+		goto nla_put_failure;
+
+	if (v->tcfv_action == TCA_VLAN_ACT_PUSH &&
+	    nla_put_u16(skb, TCA_VLAN_PUSH_VLAN_ID, v->tcfv_push_vid) &&
+	    nla_put_u16(skb, TCA_VLAN_PUSH_VLAN_PROTOCOL, v->tcfv_push_proto))
+		goto nla_put_failure;
+
+	t.install = jiffies_to_clock_t(jiffies - v->tcf_tm.install);
+	t.lastuse = jiffies_to_clock_t(jiffies - v->tcf_tm.lastuse);
+	t.expires = jiffies_to_clock_t(v->tcf_tm.expires);
+	if (nla_put(skb, TCA_VLAN_TM, sizeof(t), &t))
+		goto nla_put_failure;
+	return skb->len;
+
+nla_put_failure:
+	nlmsg_trim(skb, b);
+	return -1;
+}
+
+static struct tc_action_ops act_vlan_ops = {
+	.kind		=	"vlan",
+	.type		=	TCA_ACT_VLAN,
+	.owner		=	THIS_MODULE,
+	.act		=	tcf_vlan,
+	.dump		=	tcf_vlan_dump,
+	.init		=	tcf_vlan_init,
+};
+
+static int __init vlan_init_module(void)
+{
+	return tcf_register_action(&act_vlan_ops, VLAN_TAB_MASK);
+}
+
+static void __exit vlan_cleanup_module(void)
+{
+	tcf_unregister_action(&act_vlan_ops);
+}
+
+module_init(vlan_init_module);
+module_exit(vlan_cleanup_module);
+
+MODULE_AUTHOR("Jiri Pirko <jiri@resnulli.us>");
+MODULE_DESCRIPTION("vlan manipulation actions");
+MODULE_LICENSE("GPL v2");
-- 
1.9.3

Re: [patch net-next 2/2] sched: introduce vlan action

[Jamal Hadi Salim]

On 11/11/14 05:13, Jiri Pirko wrote:
> This tc action allows to work with vlan tagged skbs. Two supported
> sub-actions are header pop and header push.
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>

Looks good. I am going to test it - but dont see any obvious issues I
from inspection; willing to say:

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

Re: [patch net-next 1/2] net: move vlan pop/push functions into common code

[Eric Dumazet]

On Tue, 2014-11-11 at 11:13 +0100, Jiri Pirko wrote:
> So it can be used from out of openvswitch code.
> Did couple of cosmetic changes on the way, namely variable naming and
> adding support for 8021AD proto.
> 
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
>  include/linux/skbuff.h    |  2 ++
>  net/core/skbuff.c         | 86 +++++++++++++++++++++++++++++++++++++++++++++++
>  net/openvswitch/actions.c | 76 ++---------------------------------------
>  3 files changed, 90 insertions(+), 74 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 103fbe8..3b0445c 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -2673,6 +2673,8 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet);
>  unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
>  struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
>  struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
> +int skb_vlan_pop(struct sk_buff *skb);
> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci);
>  
>  struct skb_checksum_ops {
>  	__wsum (*update)(const void *mem, int len, __wsum wsum);
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 7001896..75e53d4 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4151,6 +4151,92 @@ err_free:
>  }
>  EXPORT_SYMBOL(skb_vlan_untag);
>  
> +/* remove VLAN header from packet and update csum accordingly. */
> +static int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci)
> +{
> +	struct vlan_hdr *vhdr;
> +	int err;
> +



> +	if (!pskb_may_pull(skb, VLAN_ETH_HLEN))
> +		return -ENOMEM;
> +
> +	if (!skb_cloned(skb) || skb_clone_writable(skb, VLAN_ETH_HLEN))
> +		return 0;
> +
> +	err = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
> +	if (unlikely(err))
> +		return err;

All this should be a function of its own (OVS calls this
make_writable()).

Its too bad netfilter has a different skb_make_writable()


> +
> +	if (skb->ip_summed == CHECKSUM_COMPLETE)
> +		skb->csum = csum_sub(skb->csum, csum_partial(skb->data
> +					+ (2 * ETH_ALEN), VLAN_HLEN, 0));

This looks like skb_postpull_rcsum()

BTW, calling csum_partial() for 4 bytes is quite expensive.

Re: [patch net-next 1/2] net: move vlan pop/push functions into common code

[Jiri Pirko]

Tue, Nov 11, 2014 at 02:06:21PM CET, eric.dumazet@gmail.com wrote:
>On Tue, 2014-11-11 at 11:13 +0100, Jiri Pirko wrote:
>> So it can be used from out of openvswitch code.
>> Did couple of cosmetic changes on the way, namely variable naming and
>> adding support for 8021AD proto.
>> 
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>>  include/linux/skbuff.h    |  2 ++
>>  net/core/skbuff.c         | 86 +++++++++++++++++++++++++++++++++++++++++++++++
>>  net/openvswitch/actions.c | 76 ++---------------------------------------
>>  3 files changed, 90 insertions(+), 74 deletions(-)
>> 
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 103fbe8..3b0445c 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -2673,6 +2673,8 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet);
>>  unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
>>  struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
>>  struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
>> +int skb_vlan_pop(struct sk_buff *skb);
>> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci);
>>  
>>  struct skb_checksum_ops {
>>  	__wsum (*update)(const void *mem, int len, __wsum wsum);
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index 7001896..75e53d4 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -4151,6 +4151,92 @@ err_free:
>>  }
>>  EXPORT_SYMBOL(skb_vlan_untag);
>>  
>> +/* remove VLAN header from packet and update csum accordingly. */
>> +static int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci)
>> +{
>> +	struct vlan_hdr *vhdr;
>> +	int err;
>> +
>
>
>
>> +	if (!pskb_may_pull(skb, VLAN_ETH_HLEN))
>> +		return -ENOMEM;
>> +
>> +	if (!skb_cloned(skb) || skb_clone_writable(skb, VLAN_ETH_HLEN))
>> +		return 0;
>> +
>> +	err = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
>> +	if (unlikely(err))
>> +		return err;
>
>All this should be a function of its own (OVS calls this
>make_writable()).
>
>Its too bad netfilter has a different skb_make_writable()

How different is that? Looking at it it seems it is doing the same
thing. Not sure though...

>
>
>> +
>> +	if (skb->ip_summed == CHECKSUM_COMPLETE)
>> +		skb->csum = csum_sub(skb->csum, csum_partial(skb->data
>> +					+ (2 * ETH_ALEN), VLAN_HLEN, 0));
>
>This looks like skb_postpull_rcsum()
>
>BTW, calling csum_partial() for 4 bytes is quite expensive.
>
>
>

Re: [patch net-next 1/2] net: move vlan pop/push functions into common code

[Pravin Shelar]

On Tue, Nov 11, 2014 at 2:13 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> So it can be used from out of openvswitch code.
> Did couple of cosmetic changes on the way, namely variable naming and
> adding support for 8021AD proto.
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
>  include/linux/skbuff.h    |  2 ++
>  net/core/skbuff.c         | 86 +++++++++++++++++++++++++++++++++++++++++++++++
>  net/openvswitch/actions.c | 76 ++---------------------------------------
>  3 files changed, 90 insertions(+), 74 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 103fbe8..3b0445c 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -2673,6 +2673,8 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet);
>  unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
>  struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
>  struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
> +int skb_vlan_pop(struct sk_buff *skb);
> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci);
>
>  struct skb_checksum_ops {
>         __wsum (*update)(const void *mem, int len, __wsum wsum);
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 7001896..75e53d4 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4151,6 +4151,92 @@ err_free:
>  }
>  EXPORT_SYMBOL(skb_vlan_untag);
>
> +/* remove VLAN header from packet and update csum accordingly. */
> +static int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci)
> +{
> +       struct vlan_hdr *vhdr;
> +       int err;
> +
> +       if (!pskb_may_pull(skb, VLAN_ETH_HLEN))
> +               return -ENOMEM;
> +
> +       if (!skb_cloned(skb) || skb_clone_writable(skb, VLAN_ETH_HLEN))
> +               return 0;
> +
> +       err = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
> +       if (unlikely(err))
> +               return err;
> +
> +       if (skb->ip_summed == CHECKSUM_COMPLETE)
> +               skb->csum = csum_sub(skb->csum, csum_partial(skb->data
> +                                       + (2 * ETH_ALEN), VLAN_HLEN, 0));
> +
> +       vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
> +       *vlan_tci = ntohs(vhdr->h_vlan_TCI);
> +
> +       memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
> +       __skb_pull(skb, VLAN_HLEN);
> +
> +       vlan_set_encap_proto(skb, vhdr);
> +       skb->mac_header += VLAN_HLEN;
> +       if (skb_network_offset(skb) < ETH_HLEN)
> +               skb_set_network_header(skb, ETH_HLEN);
> +       skb_reset_mac_len(skb);
> +
> +       return 0;
> +}
> +
> +int skb_vlan_pop(struct sk_buff *skb)
> +{
> +       u16 vlan_tci;
> +       __be16 vlan_proto;
> +       int err;
> +
> +       if (likely(vlan_tx_tag_present(skb))) {
> +               skb->vlan_tci = 0;
> +       } else {
> +               if (unlikely((skb->protocol != htons(ETH_P_8021Q) &&
> +                             skb->protocol != htons(ETH_P_8021AD)) ||
> +                            skb->len < VLAN_ETH_HLEN))
> +                       return 0;
> +
> +               err = __skb_vlan_pop(skb, &vlan_tci);
> +               if (err)
> +                       return err;
> +       }
> +       /* move next vlan tag to hw accel tag */
> +       if (likely((skb->protocol != htons(ETH_P_8021Q) &&
> +                   skb->protocol != htons(ETH_P_8021AD)) ||
> +                  skb->len < VLAN_ETH_HLEN))
> +               return 0;
> +
> +       vlan_proto = skb->protocol;
> +       err = __skb_vlan_pop(skb, &vlan_tci);
> +       if (unlikely(err))
> +               return err;
> +
> +       __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
> +       return 0;
> +}
> +EXPORT_SYMBOL(skb_vlan_pop);
> +
> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci)
> +{
> +       if (vlan_tx_tag_present(skb)) {
> +               /* push down current VLAN tag */
> +               if (!__vlan_put_tag(skb, skb->vlan_proto,
> +                                   vlan_tx_tag_get(skb)))
> +                       return -ENOMEM;
> +
Since you are restructuring this code, can you also change
__vlan_put_tag() to not free skb on error. So that these two new
functions can have common error handling code.

> +               if (skb->ip_summed == CHECKSUM_COMPLETE)
> +                       skb->csum = csum_add(skb->csum, csum_partial(skb->data
> +                                       + (2 * ETH_ALEN), VLAN_HLEN, 0));
> +       }
> +       __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
> +       return 0;
> +}
> +EXPORT_SYMBOL(skb_vlan_push);
> +
>  /**
>   * alloc_skb_with_frags - allocate skb with page frags
>   *
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index f7e5891..1b28110 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -206,86 +206,14 @@ static int set_mpls(struct sk_buff *skb, const __be32 *mpls_lse)
>         return 0;
>  }
>
> -/* remove VLAN header from packet and update csum accordingly. */
> -static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
> -{
> -       struct vlan_hdr *vhdr;
> -       int err;
> -
> -       err = make_writable(skb, VLAN_ETH_HLEN);
> -       if (unlikely(err))
> -               return err;
> -
> -       if (skb->ip_summed == CHECKSUM_COMPLETE)
> -               skb->csum = csum_sub(skb->csum, csum_partial(skb->data
> -                                       + (2 * ETH_ALEN), VLAN_HLEN, 0));
> -
> -       vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
> -       *current_tci = vhdr->h_vlan_TCI;
> -
> -       memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
> -       __skb_pull(skb, VLAN_HLEN);
> -
> -       vlan_set_encap_proto(skb, vhdr);
> -       skb->mac_header += VLAN_HLEN;
> -
> -       if (skb_network_offset(skb) < ETH_HLEN)
> -               skb_set_network_header(skb, ETH_HLEN);
> -
> -       /* Update mac_len for subsequent MPLS actions */
> -       skb_reset_mac_len(skb);
> -       return 0;
> -}
> -
>  static int pop_vlan(struct sk_buff *skb)
>  {
> -       __be16 tci;
> -       int err;
> -
> -       if (likely(vlan_tx_tag_present(skb))) {
> -               skb->vlan_tci = 0;
> -       } else {
> -               if (unlikely(skb->protocol != htons(ETH_P_8021Q) ||
> -                            skb->len < VLAN_ETH_HLEN))
> -                       return 0;
> -
> -               err = __pop_vlan_tci(skb, &tci);
> -               if (err)
> -                       return err;
> -       }
> -       /* move next vlan tag to hw accel tag */
> -       if (likely(skb->protocol != htons(ETH_P_8021Q) ||
> -                  skb->len < VLAN_ETH_HLEN))
> -               return 0;
> -
> -       err = __pop_vlan_tci(skb, &tci);
> -       if (unlikely(err))
> -               return err;
> -
> -       __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), ntohs(tci));
> -       return 0;
> +       return skb_vlan_pop(skb);
>  }
>
>  static int push_vlan(struct sk_buff *skb, const struct ovs_action_push_vlan *vlan)
>  {
> -       if (unlikely(vlan_tx_tag_present(skb))) {
> -               u16 current_tag;
> -
> -               /* push down current VLAN tag */
> -               current_tag = vlan_tx_tag_get(skb);
> -
> -               if (!__vlan_put_tag(skb, skb->vlan_proto, current_tag))
> -                       return -ENOMEM;
> -               /* Update mac_len for subsequent MPLS actions */
> -               skb->mac_len += VLAN_HLEN;
> -
> -               if (skb->ip_summed == CHECKSUM_COMPLETE)
> -                       skb->csum = csum_add(skb->csum, csum_partial(skb->data
> -                                       + (2 * ETH_ALEN), VLAN_HLEN, 0));
> -
> -       }
> -       __vlan_hwaccel_put_tag(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
> -       return 0;
> +       return skb_vlan_push(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
>  }
>
>  static int set_eth_addr(struct sk_buff *skb,
> --
> 1.9.3
>

Re: [patch net-next 2/2] sched: introduce vlan action

[Cong Wang]

On Tue, Nov 11, 2014 at 2:13 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> +static int tcf_vlan_init(struct net *net, struct nlattr *nla,
> +                        struct nlattr *est, struct tc_action *a,
> +                        int ovr, int bind)
> +{
> +       struct nlattr *tb[TCA_VLAN_MAX + 1];
> +       struct tc_vlan *parm;
> +       struct tcf_vlan *v;
> +       int action;
> +       __be16 push_vid = 0;
> +       __be16 push_proto = 0;
> +       int ret = 0;
> +       int err;
> +
> +       if (!nla)
> +               return -EINVAL;
> +
> +       err = nla_parse_nested(tb, TCA_VLAN_MAX, nla, vlan_policy);
> +       if (err < 0)
> +               return err;
> +
> +       if (!tb[TCA_VLAN_PARMS])
> +               return -EINVAL;
> +       parm = nla_data(tb[TCA_VLAN_PARMS]);
> +       switch (parm->v_action) {
> +       case TCA_VLAN_ACT_POP:
> +               break;
> +       case TCA_VLAN_ACT_PUSH:
> +               if (!tb[TCA_VLAN_PUSH_VLAN_ID])
> +                       return -EINVAL;
> +               push_vid = nla_get_u16(tb[TCA_VLAN_PUSH_VLAN_ID]);

nla_get_be16() ?


> +               if (push_vid >= VLAN_VID_MASK)
> +                       return -ERANGE;
> +

htons() ?

[...]

> +static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a,
> +                        int bind, int ref)
> +{
> +       unsigned char *b = skb_tail_pointer(skb);
> +       struct tcf_vlan *v = a->priv;
> +       struct tc_vlan opt = {
> +               .index    = v->tcf_index,
> +               .refcnt   = v->tcf_refcnt - ref,
> +               .bindcnt  = v->tcf_bindcnt - bind,
> +               .action   = v->tcf_action,
> +               .v_action = v->tcfv_action,
> +       };
> +       struct tcf_t t;
> +
> +       if (nla_put(skb, TCA_VLAN_PARMS, sizeof(opt), &opt))
> +               goto nla_put_failure;
> +
> +       if (v->tcfv_action == TCA_VLAN_ACT_PUSH &&
> +           nla_put_u16(skb, TCA_VLAN_PUSH_VLAN_ID, v->tcfv_push_vid) &&
> +           nla_put_u16(skb, TCA_VLAN_PUSH_VLAN_PROTOCOL, v->tcfv_push_proto))
> +               goto nla_put_failure;

nla_put_be16() ?

Re: [patch net-next 2/2] sched: introduce vlan action

[Cong Wang]

On Tue, Nov 11, 2014 at 2:13 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> This tc action allows to work with vlan tagged skbs. Two supported
> sub-actions are header pop and header push.
>

Can we add this to skbedit instead of adding a new action?

I know vlan tag is not exactly the skb metadata, but still seems
fits in skbedit for me.

Re: [patch net-next 2/2] sched: introduce vlan action

[Jiri Pirko]

Wed, Nov 12, 2014 at 12:18:47AM CET, cwang@twopensource.com wrote:
>On Tue, Nov 11, 2014 at 2:13 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> This tc action allows to work with vlan tagged skbs. Two supported
>> sub-actions are header pop and header push.
>>
>
>Can we add this to skbedit instead of adding a new action?
>
>I know vlan tag is not exactly the skb metadata, but still seems
>fits in skbedit for me.

I was thinking about that as well. It seems much clearer to add a new
action.

Re: [patch net-next 1/2] net: move vlan pop/push functions into common code

[Jiri Pirko]

Tue, Nov 11, 2014 at 06:24:15PM CET, pshelar@nicira.com wrote:
>On Tue, Nov 11, 2014 at 2:13 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> So it can be used from out of openvswitch code.
>> Did couple of cosmetic changes on the way, namely variable naming and
>> adding support for 8021AD proto.
>>
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>>  include/linux/skbuff.h    |  2 ++
>>  net/core/skbuff.c         | 86 +++++++++++++++++++++++++++++++++++++++++++++++
>>  net/openvswitch/actions.c | 76 ++---------------------------------------
>>  3 files changed, 90 insertions(+), 74 deletions(-)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 103fbe8..3b0445c 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -2673,6 +2673,8 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet);
>>  unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
>>  struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
>>  struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
>> +int skb_vlan_pop(struct sk_buff *skb);
>> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci);
>>
>>  struct skb_checksum_ops {
>>         __wsum (*update)(const void *mem, int len, __wsum wsum);
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index 7001896..75e53d4 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -4151,6 +4151,92 @@ err_free:
>>  }
>>  EXPORT_SYMBOL(skb_vlan_untag);
>>
>> +/* remove VLAN header from packet and update csum accordingly. */
>> +static int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci)
>> +{
>> +       struct vlan_hdr *vhdr;
>> +       int err;
>> +
>> +       if (!pskb_may_pull(skb, VLAN_ETH_HLEN))
>> +               return -ENOMEM;
>> +
>> +       if (!skb_cloned(skb) || skb_clone_writable(skb, VLAN_ETH_HLEN))
>> +               return 0;
>> +
>> +       err = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
>> +       if (unlikely(err))
>> +               return err;
>> +
>> +       if (skb->ip_summed == CHECKSUM_COMPLETE)
>> +               skb->csum = csum_sub(skb->csum, csum_partial(skb->data
>> +                                       + (2 * ETH_ALEN), VLAN_HLEN, 0));
>> +
>> +       vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
>> +       *vlan_tci = ntohs(vhdr->h_vlan_TCI);
>> +
>> +       memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
>> +       __skb_pull(skb, VLAN_HLEN);
>> +
>> +       vlan_set_encap_proto(skb, vhdr);
>> +       skb->mac_header += VLAN_HLEN;
>> +       if (skb_network_offset(skb) < ETH_HLEN)
>> +               skb_set_network_header(skb, ETH_HLEN);
>> +       skb_reset_mac_len(skb);
>> +
>> +       return 0;
>> +}
>> +
>> +int skb_vlan_pop(struct sk_buff *skb)
>> +{
>> +       u16 vlan_tci;
>> +       __be16 vlan_proto;
>> +       int err;
>> +
>> +       if (likely(vlan_tx_tag_present(skb))) {
>> +               skb->vlan_tci = 0;
>> +       } else {
>> +               if (unlikely((skb->protocol != htons(ETH_P_8021Q) &&
>> +                             skb->protocol != htons(ETH_P_8021AD)) ||
>> +                            skb->len < VLAN_ETH_HLEN))
>> +                       return 0;
>> +
>> +               err = __skb_vlan_pop(skb, &vlan_tci);
>> +               if (err)
>> +                       return err;
>> +       }
>> +       /* move next vlan tag to hw accel tag */
>> +       if (likely((skb->protocol != htons(ETH_P_8021Q) &&
>> +                   skb->protocol != htons(ETH_P_8021AD)) ||
>> +                  skb->len < VLAN_ETH_HLEN))
>> +               return 0;
>> +
>> +       vlan_proto = skb->protocol;
>> +       err = __skb_vlan_pop(skb, &vlan_tci);
>> +       if (unlikely(err))
>> +               return err;
>> +
>> +       __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(skb_vlan_pop);
>> +
>> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci)
>> +{
>> +       if (vlan_tx_tag_present(skb)) {
>> +               /* push down current VLAN tag */
>> +               if (!__vlan_put_tag(skb, skb->vlan_proto,
>> +                                   vlan_tx_tag_get(skb)))
>> +                       return -ENOMEM;
>> +
>Since you are restructuring this code, can you also change
>__vlan_put_tag() to not free skb on error. So that these two new
>functions can have common error handling code.

That is not directly related to this patchset. I believe that should be
changed in separate patch later on.

Re: [patch net-next 2/2] sched: introduce vlan action

[Jamal Hadi Salim]

On 11/12/14 02:47, Jiri Pirko wrote:
> Wed, Nov 12, 2014 at 12:18:47AM CET, cwang@twopensource.com wrote:

>> I know vlan tag is not exactly the skb metadata, but still seems
>> fits in skbedit for me.
>
> I was thinking about that as well. It seems much clearer to add a new
> action.
>

Cong,
I think it is better to have all these "tunneling" activities
as a separate action each. It is cleaner from a usability perspective.
[e.g. it is not hard to express nat action with pedit action or take
checksum out of nat since we have a csum action), but makes sense to
have it separate)].

cheers,
jamal

Re: [patch net-next 2/2] sched: introduce vlan action

[Jiri Pirko]

Tue, Nov 11, 2014 at 11:33:21PM CET, cwang@twopensource.com wrote:
>On Tue, Nov 11, 2014 at 2:13 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> +static int tcf_vlan_init(struct net *net, struct nlattr *nla,
>> +                        struct nlattr *est, struct tc_action *a,
>> +                        int ovr, int bind)
>> +{
>> +       struct nlattr *tb[TCA_VLAN_MAX + 1];
>> +       struct tc_vlan *parm;
>> +       struct tcf_vlan *v;
>> +       int action;
>> +       __be16 push_vid = 0;
>> +       __be16 push_proto = 0;
>> +       int ret = 0;
>> +       int err;
>> +
>> +       if (!nla)
>> +               return -EINVAL;
>> +
>> +       err = nla_parse_nested(tb, TCA_VLAN_MAX, nla, vlan_policy);
>> +       if (err < 0)
>> +               return err;
>> +
>> +       if (!tb[TCA_VLAN_PARMS])
>> +               return -EINVAL;
>> +       parm = nla_data(tb[TCA_VLAN_PARMS]);
>> +       switch (parm->v_action) {
>> +       case TCA_VLAN_ACT_POP:
>> +               break;
>> +       case TCA_VLAN_ACT_PUSH:
>> +               if (!tb[TCA_VLAN_PUSH_VLAN_ID])
>> +                       return -EINVAL;
>> +               push_vid = nla_get_u16(tb[TCA_VLAN_PUSH_VLAN_ID]);
>
>nla_get_be16() ?

No. I made this the same as it is done in net/8021q/vlan_netlink.c

>
>
>> +               if (push_vid >= VLAN_VID_MASK)
>> +                       return -ERANGE;
>> +
>
>htons() ?
>
>[...]
>
>> +static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a,
>> +                        int bind, int ref)
>> +{
>> +       unsigned char *b = skb_tail_pointer(skb);
>> +       struct tcf_vlan *v = a->priv;
>> +       struct tc_vlan opt = {
>> +               .index    = v->tcf_index,
>> +               .refcnt   = v->tcf_refcnt - ref,
>> +               .bindcnt  = v->tcf_bindcnt - bind,
>> +               .action   = v->tcf_action,
>> +               .v_action = v->tcfv_action,
>> +       };
>> +       struct tcf_t t;
>> +
>> +       if (nla_put(skb, TCA_VLAN_PARMS, sizeof(opt), &opt))
>> +               goto nla_put_failure;
>> +
>> +       if (v->tcfv_action == TCA_VLAN_ACT_PUSH &&
>> +           nla_put_u16(skb, TCA_VLAN_PUSH_VLAN_ID, v->tcfv_push_vid) &&
>> +           nla_put_u16(skb, TCA_VLAN_PUSH_VLAN_PROTOCOL, v->tcfv_push_proto))
>> +               goto nla_put_failure;
>
>nla_put_be16() ?

Re: [patch net-next 2/2] sched: introduce vlan action

[Jamal Hadi Salim]

On 11/12/14 07:34, Jiri Pirko wrote:
> Tue, Nov 11, 2014 at 11:33:21PM CET, cwang@twopensource.com wrote:
>> On Tue, Nov 11, 2014 at 2:13 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> +static int tcf_vlan_init(struct net *net, struct nlattr *nla,
>>> +                        struct nlattr *est, struct tc_action *a,
>>> +                        int ovr, int bind)
>>> +{
>>> +       struct nlattr *tb[TCA_VLAN_MAX + 1];
>>> +       struct tc_vlan *parm;
>>> +       struct tcf_vlan *v;
>>> +       int action;
>>> +       __be16 push_vid = 0;
>>> +       __be16 push_proto = 0;
>>> +       int ret = 0;
>>> +       int err;
>>> +
>>> +       if (!nla)
>>> +               return -EINVAL;
>>> +
>>> +       err = nla_parse_nested(tb, TCA_VLAN_MAX, nla, vlan_policy);
>>> +       if (err < 0)
>>> +               return err;
>>> +
>>> +       if (!tb[TCA_VLAN_PARMS])
>>> +               return -EINVAL;
>>> +       parm = nla_data(tb[TCA_VLAN_PARMS]);
>>> +       switch (parm->v_action) {
>>> +       case TCA_VLAN_ACT_POP:
>>> +               break;
>>> +       case TCA_VLAN_ACT_PUSH:
>>> +               if (!tb[TCA_VLAN_PUSH_VLAN_ID])
>>> +                       return -EINVAL;
>>> +               push_vid = nla_get_u16(tb[TCA_VLAN_PUSH_VLAN_ID]);
>>
>> nla_get_be16() ?
>
> No. I made this the same as it is done in net/8021q/vlan_netlink.c
>

It just happens that user space passes it to you in BE already.
So this works out because skb_vlan_push expects that to be in BE
as in:
int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci)



cheers,
jamal

Re: [patch net-next 2/2] sched: introduce vlan action

[Jamal Hadi Salim]

On 11/12/14 08:03, Jamal Hadi Salim wrote:
> On 11/12/14 07:34, Jiri Pirko wrote:

> It just happens that user space passes it to you in BE already.
> So this works out because skb_vlan_push expects that to be in BE
> as in:
> int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci)
>

Sorry, meant proto is in BE but vlan is in host order..
(grr.. need to test).

cheers,
jamal

Re: [patch net-next 1/2] net: move vlan pop/push functions into common code

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 12 Nov 2014 12:59:33 +0100

> That is not directly related to this patchset. I believe that should be
> changed in separate patch later on.

+1, but this has been a long standing issue and a large source of bugs.

Re: [patch net-next 1/2] net: move vlan pop/push functions into common code

[Pravin Shelar]

On Wed, Nov 12, 2014 at 3:59 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Tue, Nov 11, 2014 at 06:24:15PM CET, pshelar@nicira.com wrote:
>>On Tue, Nov 11, 2014 at 2:13 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> So it can be used from out of openvswitch code.
>>> Did couple of cosmetic changes on the way, namely variable naming and
>>> adding support for 8021AD proto.
>>>
>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>> ---
>>>  include/linux/skbuff.h    |  2 ++
>>>  net/core/skbuff.c         | 86 +++++++++++++++++++++++++++++++++++++++++++++++
>>>  net/openvswitch/actions.c | 76 ++---------------------------------------
>>>  3 files changed, 90 insertions(+), 74 deletions(-)
>>>
>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>> index 103fbe8..3b0445c 100644
>>> --- a/include/linux/skbuff.h
>>> +++ b/include/linux/skbuff.h
>>> @@ -2673,6 +2673,8 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet);
>>>  unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
>>>  struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
>>>  struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
>>> +int skb_vlan_pop(struct sk_buff *skb);
>>> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci);
>>>
>>>  struct skb_checksum_ops {
>>>         __wsum (*update)(const void *mem, int len, __wsum wsum);
>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>> index 7001896..75e53d4 100644
>>> --- a/net/core/skbuff.c
>>> +++ b/net/core/skbuff.c
>>> @@ -4151,6 +4151,92 @@ err_free:
>>>  }
>>>  EXPORT_SYMBOL(skb_vlan_untag);
>>>
>>> +/* remove VLAN header from packet and update csum accordingly. */
>>> +static int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci)
>>> +{
>>> +       struct vlan_hdr *vhdr;
>>> +       int err;
>>> +
>>> +       if (!pskb_may_pull(skb, VLAN_ETH_HLEN))
>>> +               return -ENOMEM;
>>> +
>>> +       if (!skb_cloned(skb) || skb_clone_writable(skb, VLAN_ETH_HLEN))
>>> +               return 0;
>>> +
>>> +       err = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
>>> +       if (unlikely(err))
>>> +               return err;
>>> +
>>> +       if (skb->ip_summed == CHECKSUM_COMPLETE)
>>> +               skb->csum = csum_sub(skb->csum, csum_partial(skb->data
>>> +                                       + (2 * ETH_ALEN), VLAN_HLEN, 0));
>>> +
>>> +       vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
>>> +       *vlan_tci = ntohs(vhdr->h_vlan_TCI);
>>> +
>>> +       memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
>>> +       __skb_pull(skb, VLAN_HLEN);
>>> +
>>> +       vlan_set_encap_proto(skb, vhdr);
>>> +       skb->mac_header += VLAN_HLEN;
>>> +       if (skb_network_offset(skb) < ETH_HLEN)
>>> +               skb_set_network_header(skb, ETH_HLEN);
>>> +       skb_reset_mac_len(skb);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +int skb_vlan_pop(struct sk_buff *skb)
>>> +{
>>> +       u16 vlan_tci;
>>> +       __be16 vlan_proto;
>>> +       int err;
>>> +
>>> +       if (likely(vlan_tx_tag_present(skb))) {
>>> +               skb->vlan_tci = 0;
>>> +       } else {
>>> +               if (unlikely((skb->protocol != htons(ETH_P_8021Q) &&
>>> +                             skb->protocol != htons(ETH_P_8021AD)) ||
>>> +                            skb->len < VLAN_ETH_HLEN))
>>> +                       return 0;
>>> +
>>> +               err = __skb_vlan_pop(skb, &vlan_tci);
>>> +               if (err)
>>> +                       return err;
>>> +       }
>>> +       /* move next vlan tag to hw accel tag */
>>> +       if (likely((skb->protocol != htons(ETH_P_8021Q) &&
>>> +                   skb->protocol != htons(ETH_P_8021AD)) ||
>>> +                  skb->len < VLAN_ETH_HLEN))
>>> +               return 0;
>>> +
>>> +       vlan_proto = skb->protocol;
>>> +       err = __skb_vlan_pop(skb, &vlan_tci);
>>> +       if (unlikely(err))
>>> +               return err;
>>> +
>>> +       __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
>>> +       return 0;
>>> +}
>>> +EXPORT_SYMBOL(skb_vlan_pop);
>>> +
>>> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci)
>>> +{
>>> +       if (vlan_tx_tag_present(skb)) {
>>> +               /* push down current VLAN tag */
>>> +               if (!__vlan_put_tag(skb, skb->vlan_proto,
>>> +                                   vlan_tx_tag_get(skb)))
>>> +                       return -ENOMEM;
>>> +
>>Since you are restructuring this code, can you also change
>>__vlan_put_tag() to not free skb on error. So that these two new
>>functions can have common error handling code.
>
> That is not directly related to this patchset. I believe that should be
> changed in separate patch later on.

If you are not going to change this then I am not sure how tcf_vlan()
(that is introduced in next patch) can have common error handling
code.

Re: [patch net-next 2/2] sched: introduce vlan action

[Cong Wang]

On Wed, Nov 12, 2014 at 4:27 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> Cong,
> I think it is better to have all these "tunneling" activities
> as a separate action each. It is cleaner from a usability perspective.
> [e.g. it is not hard to express nat action with pedit action or take
> checksum out of nat since we have a csum action), but makes sense to
> have it separate)].
>

Sounds good.