* [patch net-next 2/2] sched: introduce vlan action
2014-11-11 10:13 [patch net-next 1/2] net: move vlan pop/push functions into common code Jiri Pirko
@ 2014-11-11 10:13 ` Jiri Pirko
2014-11-11 12:34 ` Jamal Hadi Salim
` (2 more replies)
2014-11-11 13:06 ` [patch net-next 1/2] net: move vlan pop/push functions into common code Eric Dumazet
2014-11-11 17:24 ` Pravin Shelar
2 siblings, 3 replies; 17+ messages in thread
From: Jiri Pirko @ 2014-11-11 10:13 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, pshelar, therbert, edumazet, willemb, dborkman, mst,
fw, Paul.Durrant, tgraf
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [patch net-next 2/2] sched: introduce vlan action
2014-11-11 10:13 ` [patch net-next 2/2] sched: introduce vlan action Jiri Pirko
@ 2014-11-11 12:34 ` Jamal Hadi Salim
2014-11-11 22:33 ` Cong Wang
2014-11-11 23:18 ` Cong Wang
2 siblings, 0 replies; 17+ messages in thread
From: Jamal Hadi Salim @ 2014-11-11 12:34 UTC (permalink / raw)
To: Jiri Pirko, netdev
Cc: davem, pshelar, therbert, edumazet, willemb, dborkman, mst, fw,
Paul.Durrant, tgraf
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [patch net-next 2/2] sched: introduce vlan action
2014-11-11 10:13 ` [patch net-next 2/2] sched: introduce vlan action Jiri Pirko
2014-11-11 12:34 ` Jamal Hadi Salim
@ 2014-11-11 22:33 ` Cong Wang
2014-11-12 12:34 ` Jiri Pirko
2014-11-11 23:18 ` Cong Wang
2 siblings, 1 reply; 17+ messages in thread
From: Cong Wang @ 2014-11-11 22:33 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, David Miller, Jamal Hadi Salim, Pravin B Shelar,
Tom Herbert, Eric Dumazet, willemb, Daniel Borkmann, mst,
Florian Westphal, Paul.Durrant, Thomas Graf
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() ?
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [patch net-next 2/2] sched: introduce vlan action
2014-11-11 22:33 ` Cong Wang
@ 2014-11-12 12:34 ` Jiri Pirko
2014-11-12 13:03 ` Jamal Hadi Salim
0 siblings, 1 reply; 17+ messages in thread
From: Jiri Pirko @ 2014-11-12 12:34 UTC (permalink / raw)
To: Cong Wang
Cc: netdev, David Miller, Jamal Hadi Salim, Pravin B Shelar,
Tom Herbert, Eric Dumazet, willemb, Daniel Borkmann, mst,
Florian Westphal, Paul.Durrant, Thomas Graf
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() ?
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [patch net-next 2/2] sched: introduce vlan action
2014-11-12 12:34 ` Jiri Pirko
@ 2014-11-12 13:03 ` Jamal Hadi Salim
2014-11-12 13:06 ` Jamal Hadi Salim
0 siblings, 1 reply; 17+ messages in thread
From: Jamal Hadi Salim @ 2014-11-12 13:03 UTC (permalink / raw)
To: Jiri Pirko, Cong Wang
Cc: netdev, David Miller, Pravin B Shelar, Tom Herbert, Eric Dumazet,
willemb, Daniel Borkmann, mst, Florian Westphal, Paul.Durrant,
Thomas Graf
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [patch net-next 2/2] sched: introduce vlan action
2014-11-12 13:03 ` Jamal Hadi Salim
@ 2014-11-12 13:06 ` Jamal Hadi Salim
0 siblings, 0 replies; 17+ messages in thread
From: Jamal Hadi Salim @ 2014-11-12 13:06 UTC (permalink / raw)
To: Jiri Pirko, Cong Wang
Cc: netdev, David Miller, Pravin B Shelar, Tom Herbert, Eric Dumazet,
willemb, Daniel Borkmann, mst, Florian Westphal, Paul.Durrant,
Thomas Graf
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [patch net-next 2/2] sched: introduce vlan action
2014-11-11 10:13 ` [patch net-next 2/2] sched: introduce vlan action Jiri Pirko
2014-11-11 12:34 ` Jamal Hadi Salim
2014-11-11 22:33 ` Cong Wang
@ 2014-11-11 23:18 ` Cong Wang
2014-11-12 7:47 ` Jiri Pirko
2 siblings, 1 reply; 17+ messages in thread
From: Cong Wang @ 2014-11-11 23:18 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, David Miller, Jamal Hadi Salim, Pravin B Shelar,
Tom Herbert, Eric Dumazet, willemb, Daniel Borkmann, mst,
Florian Westphal, Paul.Durrant, Thomas Graf
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [patch net-next 2/2] sched: introduce vlan action
2014-11-11 23:18 ` Cong Wang
@ 2014-11-12 7:47 ` Jiri Pirko
2014-11-12 12:27 ` Jamal Hadi Salim
0 siblings, 1 reply; 17+ messages in thread
From: Jiri Pirko @ 2014-11-12 7:47 UTC (permalink / raw)
To: Cong Wang
Cc: netdev, David Miller, Jamal Hadi Salim, Pravin B Shelar,
Tom Herbert, Eric Dumazet, willemb, Daniel Borkmann, mst,
Florian Westphal, Paul.Durrant, Thomas Graf
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [patch net-next 2/2] sched: introduce vlan action
2014-11-12 7:47 ` Jiri Pirko
@ 2014-11-12 12:27 ` Jamal Hadi Salim
2014-11-13 17:06 ` Cong Wang
0 siblings, 1 reply; 17+ messages in thread
From: Jamal Hadi Salim @ 2014-11-12 12:27 UTC (permalink / raw)
To: Jiri Pirko, Cong Wang
Cc: netdev, David Miller, Pravin B Shelar, Tom Herbert, Eric Dumazet,
willemb, Daniel Borkmann, mst, Florian Westphal, Paul.Durrant,
Thomas Graf
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [patch net-next 2/2] sched: introduce vlan action
2014-11-12 12:27 ` Jamal Hadi Salim
@ 2014-11-13 17:06 ` Cong Wang
0 siblings, 0 replies; 17+ messages in thread
From: Cong Wang @ 2014-11-13 17:06 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Jiri Pirko, netdev, David Miller, Pravin B Shelar, Tom Herbert,
Eric Dumazet, Willem de Bruijn, Daniel Borkmann, mst,
Florian Westphal, Paul.Durrant, Thomas Graf
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [patch net-next 1/2] net: move vlan pop/push functions into common code
2014-11-11 10:13 [patch net-next 1/2] net: move vlan pop/push functions into common code Jiri Pirko
2014-11-11 10:13 ` [patch net-next 2/2] sched: introduce vlan action Jiri Pirko
@ 2014-11-11 13:06 ` Eric Dumazet
2014-11-11 15:00 ` Jiri Pirko
2014-11-11 17:24 ` Pravin Shelar
2 siblings, 1 reply; 17+ messages in thread
From: Eric Dumazet @ 2014-11-11 13:06 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, jhs, pshelar, therbert, edumazet, willemb,
dborkman, mst, fw, Paul.Durrant, tgraf
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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [patch net-next 1/2] net: move vlan pop/push functions into common code
2014-11-11 13:06 ` [patch net-next 1/2] net: move vlan pop/push functions into common code Eric Dumazet
@ 2014-11-11 15:00 ` Jiri Pirko
0 siblings, 0 replies; 17+ messages in thread
From: Jiri Pirko @ 2014-11-11 15:00 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, davem, jhs, pshelar, therbert, edumazet, willemb,
dborkman, mst, fw, Paul.Durrant, tgraf
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.
>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [patch net-next 1/2] net: move vlan pop/push functions into common code
2014-11-11 10:13 [patch net-next 1/2] net: move vlan pop/push functions into common code Jiri Pirko
2014-11-11 10:13 ` [patch net-next 2/2] sched: introduce vlan action Jiri Pirko
2014-11-11 13:06 ` [patch net-next 1/2] net: move vlan pop/push functions into common code Eric Dumazet
@ 2014-11-11 17:24 ` Pravin Shelar
2014-11-12 11:59 ` Jiri Pirko
2 siblings, 1 reply; 17+ messages in thread
From: Pravin Shelar @ 2014-11-11 17:24 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, David Miller, Jamal Hadi Salim, Tom Herbert, Eric Dumazet,
willemb, Daniel Borkmann, mst, fw, Paul.Durrant, Thomas Graf
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [patch net-next 1/2] net: move vlan pop/push functions into common code
2014-11-11 17:24 ` Pravin Shelar
@ 2014-11-12 11:59 ` Jiri Pirko
2014-11-12 19:11 ` David Miller
2014-11-12 19:20 ` Pravin Shelar
0 siblings, 2 replies; 17+ messages in thread
From: Jiri Pirko @ 2014-11-12 11:59 UTC (permalink / raw)
To: Pravin Shelar
Cc: netdev, David Miller, Jamal Hadi Salim, Tom Herbert, Eric Dumazet,
willemb, Daniel Borkmann, mst, fw, Paul.Durrant, Thomas Graf
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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [patch net-next 1/2] net: move vlan pop/push functions into common code
2014-11-12 11:59 ` Jiri Pirko
@ 2014-11-12 19:11 ` David Miller
2014-11-12 19:20 ` Pravin Shelar
1 sibling, 0 replies; 17+ messages in thread
From: David Miller @ 2014-11-12 19:11 UTC (permalink / raw)
To: jiri
Cc: pshelar, netdev, jhs, therbert, edumazet, willemb, dborkman, mst,
fw, Paul.Durrant, tgraf
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [patch net-next 1/2] net: move vlan pop/push functions into common code
2014-11-12 11:59 ` Jiri Pirko
2014-11-12 19:11 ` David Miller
@ 2014-11-12 19:20 ` Pravin Shelar
1 sibling, 0 replies; 17+ messages in thread
From: Pravin Shelar @ 2014-11-12 19:20 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, David Miller, Jamal Hadi Salim, Tom Herbert, Eric Dumazet,
Willem de Bruijn, Daniel Borkmann, mst, fw, Paul.Durrant,
Thomas Graf
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.
^ permalink raw reply [flat|nested] 17+ messages in thread