[PATCH net-next 1/1] net_sched: Introduce skbmod action

From: Jamal Hadi Salim <jhs@mojatatu.com>

This action is intended to be an upgrade from a usability perspective
from pedit (as well as operational debugability).
Compare this:

sudo tc filter add dev $ETH parent 1: protocol ip prio 10 \
u32 match ip protocol 1 0xff flowid 1:2 \
action pedit munge offset -14 u8 set 0x02 \
    munge offset -13 u8 set 0x15 \
    munge offset -12 u8 set 0x15 \
    munge offset -11 u8 set 0x15 \
    munge offset -10 u16 set 0x1515 \
    pipe

to:

sudo tc filter add dev $ETH parent 1: protocol ip prio 10 \
u32 match ip protocol 1 0xff flowid 1:2 \
action skbmod dmac 02:15:15:15:15:15

Or worse, try to debug a policy with destination mac, source mac and
etherype. Then make that a hundred rules and you'll get my point.

In the future common use cases on pedit can be migrated to this action
(as an example different fields in ip v4/6, transports like tcp/udp/sctp
etc). For this first cut, this allows modifying basic ethernet header.

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 include/net/tc_act/tc_skbmod.h        |  35 +++++
 include/uapi/linux/tc_act/tc_skbmod.h |  47 +++++++
 net/sched/Kconfig                     |  11 ++
 net/sched/Makefile                    |   1 +
 net/sched/act_skbmod.c                | 245 ++++++++++++++++++++++++++++++++++
 5 files changed, 339 insertions(+)
 create mode 100644 include/net/tc_act/tc_skbmod.h
 create mode 100644 include/uapi/linux/tc_act/tc_skbmod.h
 create mode 100644 net/sched/act_skbmod.c

diff --git a/include/net/tc_act/tc_skbmod.h b/include/net/tc_act/tc_skbmod.h
new file mode 100644
index 0000000..661a107
--- /dev/null
+++ b/include/net/tc_act/tc_skbmod.h

@@ -0,0 +1,35 @@
+/*
+ * Copyright (c) 2016, Jamal Hadi Salim
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ * Author: Jamal Hadi Salim <jhs@mojatatu.com>
+ */
+
+#ifndef __NET_TC_SKBMOD_H
+#define __NET_TC_SKBMOD_H
+
+#include <net/act_api.h>
+#include <linux/tc_act/tc_skbmod.h>
+
+struct tcf_skbmod {
+	struct tcf_common	common;
+	u32	flags;
+	u8	eth_dst[ETH_ALEN];
+	u16	eth_type;
+	u8	eth_src[ETH_ALEN];
+};
+#define to_skbmod(a) \
+	container_of(a->priv, struct tcf_skbmod, common)
+
+#endif /* __NET_TC_SKBMOD_H */

diff --git a/include/uapi/linux/tc_act/tc_skbmod.h b/include/uapi/linux/tc_act/tc_skbmod.h
new file mode 100644
index 0000000..c268b94
--- /dev/null
+++ b/include/uapi/linux/tc_act/tc_skbmod.h

@@ -0,0 +1,47 @@
+/*
+ * Copyright (c) 2016, Jamal Hadi Salim
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * Author: Jamal Hadi Salim
+ */
+
+#ifndef __LINUX_TC_SKBMOD_H
+#define __LINUX_TC_SKBMOD_H
+
+#include <linux/pkt_cls.h>
+
+#define TCA_ACT_SKBMOD 15
+
+#define SKBMOD_F_DMAC	0x1
+#define SKBMOD_F_SMAC	0x2
+#define SKBMOD_F_ETYPE	0x4
+
+struct tc_skbmod {
+	tc_gen;
+};
+
+enum {
+	TCA_SKBMOD_UNSPEC,
+	TCA_SKBMOD_TM,
+	TCA_SKBMOD_PARMS,
+	TCA_SKBMOD_DMAC,
+	TCA_SKBMOD_SMAC,
+	TCA_SKBMOD_ETYPE,
+	TCA_SKBMOD_PAD,
+	__TCA_SKBMOD_MAX
+};
+#define TCA_SKBMOD_MAX (__TCA_SKBMOD_MAX - 1)
+
+#endif

diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index b148302..ca0386c 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig

@@ -739,6 +739,17 @@ config NET_ACT_CONNMARK
 	  To compile this code as a module, choose M here: the
 	  module will be called act_connmark.
 
+config NET_ACT_SKBMOD
+        tristate "skb data modification action"
+        depends on NET_CLS_ACT
+        ---help---
+	  Say Y here to allow modification of skb data
+
+	  If unsure, say N.
+
+	  To compile this code as a module, choose M here: the
+	  module will be called act_skbmod.
+
 config NET_ACT_IFE
         tristate "Inter-FE action based on IETF ForCES InterFE LFB"
         depends on NET_CLS_ACT

diff --git a/net/sched/Makefile b/net/sched/Makefile
index 84bddb3..edead66 100644
--- a/net/sched/Makefile
+++ b/net/sched/Makefile

@@ -19,6 +19,7 @@ obj-$(CONFIG_NET_ACT_CSUM)	+= act_csum.o
 obj-$(CONFIG_NET_ACT_VLAN)	+= act_vlan.o
 obj-$(CONFIG_NET_ACT_BPF)	+= act_bpf.o
 obj-$(CONFIG_NET_ACT_CONNMARK)	+= act_connmark.o
+obj-$(CONFIG_NET_ACT_SKBMOD)	+= act_skbmod.o
 obj-$(CONFIG_NET_ACT_IFE)	+= act_ife.o
 obj-$(CONFIG_NET_IFE_SKBMARK)	+= act_meta_mark.o
 obj-$(CONFIG_NET_IFE_SKBPRIO)	+= act_meta_skbprio.o

diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
new file mode 100644
index 0000000..b8e88091
--- /dev/null
+++ b/net/sched/act_skbmod.c

@@ -0,0 +1,245 @@
+/*
+ * copyright Jamal Hadi Salim (2016)
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ * Author: Jamal Hadi Salim <jhs@mojatatu.com>
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/skbuff.h>
+#include <linux/rtnetlink.h>
+#include <net/netlink.h>
+#include <net/pkt_sched.h>
+
+#include <linux/tc_act/tc_skbmod.h>
+#include <net/tc_act/tc_skbmod.h>
+
+#define SKBMOD_TAB_MASK     15
+
+static int skbmod_net_id;
+
+static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,
+			  struct tcf_result *res)
+{
+	struct tcf_skbmod *d = a->priv;
+
+	spin_lock(&d->tcf_lock);
+	tcf_lastuse_update(&d->tcf_tm);
+	bstats_update(&d->tcf_bstats, skb);
+
+	if (d->flags & SKBMOD_F_DMAC)
+		ether_addr_copy(eth_hdr(skb)->h_dest, d->eth_dst);
+	if (d->flags & SKBMOD_F_SMAC)
+		ether_addr_copy(eth_hdr(skb)->h_source, d->eth_src);
+	if (d->flags & SKBMOD_F_ETYPE)
+		eth_hdr(skb)->h_proto = d->eth_type;
+
+	spin_unlock(&d->tcf_lock);
+	return d->tcf_action;
+}
+
+static const struct nla_policy skbmod_policy[TCA_SKBMOD_MAX + 1] = {
+	[TCA_SKBMOD_PARMS]		= { .len = sizeof(struct tc_skbmod) },
+	[TCA_SKBMOD_DMAC]		= { .len = ETH_ALEN },
+	[TCA_SKBMOD_SMAC]		= { .len = ETH_ALEN },
+	[TCA_SKBMOD_ETYPE]		= { .type = NLA_U16 },
+};
+
+static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
+			    struct nlattr *est, struct tc_action *a,
+			    int ovr, int bind)
+{
+	struct tc_action_net *tn = net_generic(net, skbmod_net_id);
+	struct nlattr *tb[TCA_SKBMOD_MAX + 1];
+	struct tc_skbmod *parm;
+	struct tcf_skbmod *d;
+	u32 flags = 0;
+	u8 *daddr = NULL;
+	u8 *saddr = NULL;
+	u16 eth_type = 0;
+
+	bool exists = false;
+	int ret = 0, err;
+
+	if (nla == NULL)
+		return -EINVAL;
+
+	err = nla_parse_nested(tb, TCA_SKBMOD_MAX, nla, skbmod_policy);
+	if (err < 0)
+		return err;
+
+	if (tb[TCA_SKBMOD_PARMS] == NULL)
+		return -EINVAL;
+
+	/*Allow zero valued mac */
+	if (tb[TCA_SKBMOD_DMAC]) {
+		daddr = nla_data(tb[TCA_SKBMOD_DMAC]);
+		flags |= SKBMOD_F_DMAC;
+	}
+
+	if (tb[TCA_SKBMOD_SMAC]) {
+		saddr = nla_data(tb[TCA_SKBMOD_SMAC]);
+		flags |= SKBMOD_F_SMAC;
+	}
+
+	if (tb[TCA_SKBMOD_ETYPE]) {
+		eth_type = nla_get_u16(tb[TCA_SKBMOD_ETYPE]);
+		flags |= SKBMOD_F_ETYPE;
+	}
+
+	if (!flags) {
+		return -EINVAL;
+	}
+
+	parm = nla_data(tb[TCA_SKBMOD_PARMS]);
+
+	exists = tcf_hash_check(tn, parm->index, a, bind);
+	if (exists && bind)
+		return 0;
+
+	if (!exists) {
+		ret = tcf_hash_create(tn, parm->index, est, a,
+				      sizeof(*d), bind, false);
+		if (ret)
+			return ret;
+
+		d = to_skbmod(a);
+		ret = ACT_P_CREATED;
+	} else {
+		d = to_skbmod(a);
+		tcf_hash_release(a, bind);
+		if (!ovr)
+			return -EEXIST;
+	}
+
+	spin_lock_bh(&d->tcf_lock);
+
+	d->flags = flags;
+	if (flags & SKBMOD_F_DMAC)
+		ether_addr_copy(d->eth_dst, daddr);
+	if (flags & SKBMOD_F_SMAC)
+		ether_addr_copy(d->eth_src, saddr);
+	if (flags & SKBMOD_F_ETYPE)
+		d->eth_type = htons(eth_type);
+
+	d->tcf_action = parm->action;
+
+	spin_unlock_bh(&d->tcf_lock);
+
+	if (ret == ACT_P_CREATED)
+		tcf_hash_insert(tn, a);
+	return ret;
+}
+
+static int tcf_skbmod_dump(struct sk_buff *skb, struct tc_action *a,
+			    int bind, int ref)
+{
+	unsigned char *b = skb_tail_pointer(skb);
+	struct tcf_skbmod *d = a->priv;
+	struct tc_skbmod opt = {
+		.index   = d->tcf_index,
+		.refcnt  = d->tcf_refcnt - ref,
+		.bindcnt = d->tcf_bindcnt - bind,
+		.action  = d->tcf_action,
+	};
+	struct tcf_t t;
+
+	if (nla_put(skb, TCA_SKBMOD_PARMS, sizeof(opt), &opt))
+		goto nla_put_failure;
+	if ((d->flags & SKBMOD_F_DMAC) &&
+	    nla_put(skb, TCA_SKBMOD_DMAC, ETH_ALEN, d->eth_dst))
+		goto nla_put_failure;
+	if ((d->flags & SKBMOD_F_SMAC) &&
+	    nla_put(skb, TCA_SKBMOD_SMAC, ETH_ALEN, d->eth_src))
+		goto nla_put_failure;
+	if ((d->flags & SKBMOD_F_ETYPE) &&
+	    nla_put_u16(skb, TCA_SKBMOD_ETYPE, ntohs(d->eth_type)))
+		goto nla_put_failure;
+
+	tcf_tm_dump(&t, &d->tcf_tm);
+	if (nla_put_64bit(skb, TCA_SKBMOD_TM, sizeof(t), &t, TCA_SKBMOD_PAD))
+		goto nla_put_failure;
+	return skb->len;
+
+nla_put_failure:
+	nlmsg_trim(skb, b);
+	return -1;
+}
+
+static int tcf_skbmod_walker(struct net *net, struct sk_buff *skb,
+			      struct netlink_callback *cb, int type,
+			      struct tc_action *a)
+{
+	struct tc_action_net *tn = net_generic(net, skbmod_net_id);
+
+	return tcf_generic_walker(tn, skb, cb, type, a);
+}
+
+static int tcf_skbmod_search(struct net *net, struct tc_action *a, u32 index)
+{
+	struct tc_action_net *tn = net_generic(net, skbmod_net_id);
+
+	return tcf_hash_search(tn, a, index);
+}
+
+static struct tc_action_ops act_skbmod_ops = {
+	.kind		=	"skbmod",
+	.type		=	TCA_ACT_SKBMOD,
+	.owner		=	THIS_MODULE,
+	.act		=	tcf_skbmod_run,
+	.dump		=	tcf_skbmod_dump,
+	.init		=	tcf_skbmod_init,
+	.walk		=	tcf_skbmod_walker,
+	.lookup		=	tcf_skbmod_search,
+};
+
+static __net_init int skbmod_init_net(struct net *net)
+{
+	struct tc_action_net *tn = net_generic(net, skbmod_net_id);
+
+	return tc_action_net_init(tn, &act_skbmod_ops, SKBMOD_TAB_MASK);
+}
+
+static void __net_exit skbmod_exit_net(struct net *net)
+{
+	struct tc_action_net *tn = net_generic(net, skbmod_net_id);
+
+	tc_action_net_exit(tn);
+}
+
+static struct pernet_operations skbmod_net_ops = {
+	.init = skbmod_init_net,
+	.exit = skbmod_exit_net,
+	.id   = &skbmod_net_id,
+	.size = sizeof(struct tc_action_net),
+};
+
+MODULE_AUTHOR("Jamal Hadi Salim, <jhs@mojatatu.com>");
+MODULE_DESCRIPTION("SKB data mod-ing");
+MODULE_LICENSE("GPL");
+
+static int __init skbmod_init_module(void)
+{
+	return tcf_register_action(&act_skbmod_ops, &skbmod_net_ops);
+}
+
+static void __exit skbmod_cleanup_module(void)
+{
+	tcf_unregister_action(&act_skbmod_ops, &skbmod_net_ops);
+}
+
+module_init(skbmod_init_module);
+module_exit(skbmod_cleanup_module);

-- 
1.9.1

On Sun, Jul 17, 2016 at 04:41:24AM -0400, Jamal Hadi Salim wrote:

From: Jamal Hadi Salim <jhs@mojatatu.com>

This action is intended to be an upgrade from a usability perspective
from pedit (as well as operational debugability).
Compare this:

sudo tc filter add dev $ETH parent 1: protocol ip prio 10 \
u32 match ip protocol 1 0xff flowid 1:2 \
action pedit munge offset -14 u8 set 0x02 \
    munge offset -13 u8 set 0x15 \
    munge offset -12 u8 set 0x15 \
    munge offset -11 u8 set 0x15 \
    munge offset -10 u16 set 0x1515 \
    pipe

to:

sudo tc filter add dev $ETH parent 1: protocol ip prio 10 \
u32 match ip protocol 1 0xff flowid 1:2 \
action skbmod dmac 02:15:15:15:15:15

Or worse, try to debug a policy with destination mac, source mac and
etherype. Then make that a hundred rules and you'll get my point.

In the future common use cases on pedit can be migrated to this action
(as an example different fields in ip v4/6, transports like tcp/udp/sctp
etc). For this first cut, this allows modifying basic ethernet header.

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 include/net/tc_act/tc_skbmod.h        |  35 +++++
 include/uapi/linux/tc_act/tc_skbmod.h |  47 +++++++
 net/sched/Kconfig                     |  11 ++
 net/sched/Makefile                    |   1 +
 net/sched/act_skbmod.c                | 245 ++++++++++++++++++++++++++++++++++
 5 files changed, 339 insertions(+)
 create mode 100644 include/net/tc_act/tc_skbmod.h
 create mode 100644 include/uapi/linux/tc_act/tc_skbmod.h
 create mode 100644 net/sched/act_skbmod.c

I agree with Cong's point that this new action adds 339 lines of kernel
code without adding any new functionality.
It is suppose to solve debugging issues, but it's hard to understand
from commit log.
I can imagine new tc command 'action skbmod dmac 02:15:15:15:15:15' that
uses kernel pedit action undercover, so from user space point of view
the same effect can be achieved by extending iproute2.
What is the reason to add this action to the kernel?
By debug you mean to dump kernel action list and multiple pedits
are hard to decipher? Anything else I'm missing?

On 16-07-18 12:19 AM, Alexei Starovoitov wrote:

On Sun, Jul 17, 2016 at 04:41:24AM -0400, Jamal Hadi Salim wrote:

quoted

From: Jamal Hadi Salim <jhs@mojatatu.com>

This action is intended to be an upgrade from a usability perspective
from pedit (as well as operational debugability).
Compare this:

sudo tc filter add dev $ETH parent 1: protocol ip prio 10 \
u32 match ip protocol 1 0xff flowid 1:2 \
action pedit munge offset -14 u8 set 0x02 \
     munge offset -13 u8 set 0x15 \
     munge offset -12 u8 set 0x15 \
     munge offset -11 u8 set 0x15 \
     munge offset -10 u16 set 0x1515 \
     pipe

to:

sudo tc filter add dev $ETH parent 1: protocol ip prio 10 \
u32 match ip protocol 1 0xff flowid 1:2 \
action skbmod dmac 02:15:15:15:15:15

Or worse, try to debug a policy with destination mac, source mac and
etherype. Then make that a hundred rules and you'll get my point.

In the future common use cases on pedit can be migrated to this action
(as an example different fields in ip v4/6, transports like tcp/udp/sctp
etc). For this first cut, this allows modifying basic ethernet header.

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
  include/net/tc_act/tc_skbmod.h        |  35 +++++
  include/uapi/linux/tc_act/tc_skbmod.h |  47 +++++++
  net/sched/Kconfig                     |  11 ++
  net/sched/Makefile                    |   1 +
  net/sched/act_skbmod.c                | 245 ++++++++++++++++++++++++++++++++++
  5 files changed, 339 insertions(+)
  create mode 100644 include/net/tc_act/tc_skbmod.h
  create mode 100644 include/uapi/linux/tc_act/tc_skbmod.h
  create mode 100644 net/sched/act_skbmod.c

I agree with Cong's point that this new action adds 339 lines of kernel
code without adding any new functionality.

Come on Alexei.
You cant be serious saying that with all the ebpf code being added now
to the driver level.

It is suppose to solve debugging issues, but it's hard to understand
from commit log.

It is. We have deployed hundreds of actions which do pedit. They
get harder to debug as the number goes up.

I can imagine new tc command 'action skbmod dmac 02:15:15:15:15:15' that
uses kernel pedit action undercover, so from user space point of view
the same effect can be achieved by extending iproute2.

Of course - and i pointed to Cong it has been tried before. I should
know because i wrote that code (just look at the pedit code in
iproute2 for udp/icmp/ip type overlays).
It gets tiresome at some point - Ive reached that point. I dont just
sit here and whip features off my ass because it feels great to add 300
lines to the kernel today. We have real need for this.
And as i keep looking closer i see that ebtables has this feature
already. And someone else pointed that openvswitch has it. So precedence
exists.

cheers,
jamal

What is the reason to add this action to the kernel?
By debug you mean to dump kernel action list and multiple pedits
are hard to decipher? Anything else I'm missing?

On 07/18/2016 08:51 AM, Jamal Hadi Salim wrote:

On 16-07-18 12:19 AM, Alexei Starovoitov wrote:

quoted

On Sun, Jul 17, 2016 at 04:41:24AM -0400, Jamal Hadi Salim wrote:

quoted

From: Jamal Hadi Salim <jhs@mojatatu.com>

[...]

quoted

I can imagine new tc command 'action skbmod dmac 02:15:15:15:15:15' that
uses kernel pedit action undercover, so from user space point of view
the same effect can be achieved by extending iproute2.

Of course - and i pointed to Cong it has been tried before. I should
know because i wrote that code (just look at the pedit code in
iproute2 for udp/icmp/ip type overlays).

Looking at that just out of curiosity on how complex it could look
for src/dst mac, is it actually functional in iproute2 upstream tree?

All I see is that pedit can look up 3rd party modules via get_pedit_kind(),
so it will pick p_%s.so, if built as such, and there's code for p_ip,
p_tcp, p_udp, p_icmp. But then, for example, all I see in p_udp.c is
since initial iproute2 import in 2005, apart from some cleanups by
Stephen:

static int
parse_udp(int *argc_p, char ***argv_p, struct tc_pedit_sel *sel, struct tc_pedit_key *tkey)
{
	int res = -1;
	return res;
}

struct m_pedit_util p_pedit_udp = {
	NULL,
	"udp",
	parse_udp,
};

Same for tcp, icmp, ipv6 bits code ... :/ Is it still planned to eventually
complete these? I agree that from a usability PoV, it might be nice to
have some kind of 'pretty printer' for it besides the existing config
parser there (e.g. when we know that a loaded instance was done with a
high-level module, we could annotate that for retrieval on dump or such).

Thanks,
Daniel

On 07/18/16 at 11:44am, Daniel Borkmann wrote:

Same for tcp, icmp, ipv6 bits code ... :/ Is it still planned to eventually
complete these? I agree that from a usability PoV, it might be nice to
have some kind of 'pretty printer' for it besides the existing config
parser there (e.g. when we know that a loaded instance was done with a
high-level module, we could annotate that for retrieval on dump or such).

Right. I was at the same point as Jamal and it is nasty to try and
reverse engineer the dumps without any further hints. I assume that's
what he is referring to with difficulties.

Looking back, I would simply calculate a SHA hash over the original
filter in text form, pass the hash together with the tc filter and then
associate the hash with the filter text stored in user space. This
would not only benefit pedit but also u32 and possibly others.

It also has the advantage that extending the kernel once now will allow
to add additional higher level abstractions later on without requiring
users to rebase their kernels.

On 16-07-18 05:44 AM, Daniel Borkmann wrote:

On 07/18/2016 08:51 AM, Jamal Hadi Salim wrote:

quoted

On 16-07-18 12:19 AM, Alexei Starovoitov wrote:

Looking at that just out of curiosity on how complex it could look
for src/dst mac, is it actually functional in iproute2 upstream tree?

No it is a bunch of bash script wrapping we did on top of pedit (which
should be no different than adding it to m_pedit as an extension).
We started there then decided that this feature is mostly used with
mirred all the time - so modified  mirred.

All I see is that pedit can look up 3rd party modules via get_pedit_kind(),
so it will pick p_%s.so, if built as such, and there's code for p_ip,
p_tcp, p_udp, p_icmp. But then, for example, all I see in p_udp.c is
since initial iproute2 import in 2005, apart from some cleanups by
Stephen:

Yes, IP and tcp should be fine. Others were place holders.
Note, there is even no need to change pedit - you could write a bash
script as we did.

static int
parse_udp(int *argc_p, char ***argv_p, struct tc_pedit_sel *sel, struct
tc_pedit_key *tkey)
{
     int res = -1;
     return res;
}

struct m_pedit_util p_pedit_udp = {
     NULL,
     "udp",
     parse_udp,
};

Same for tcp, icmp, ipv6 bits code ... :/ Is it still planned to eventually
complete these?

someone else could run with it; at the moment i think this was ok at
small scale but it hasnt worked well in a larger scale. When you write
other apps (other than tc) to use these APIs parsing all the 32 bit
chunks is more cumbersome then getting a struct which gives me precise
info.

I agree that from a usability PoV, it might be nice to

have some kind of 'pretty printer' for it besides the existing config
parser there (e.g. when we know that a loaded instance was done with a
high-level module, we could annotate that for retrieval on dump or such).

If i could tag the structure with something the kernel then returns to
me when i dump, I could add nice pretty printers (same arguement applies
to u32).
But that doesnt solve the programmability issue as being a good cause.

cheers,
jamal

On 16-07-18 06:07 AM, Thomas Graf wrote:

Right. I was at the same point as Jamal and it is nasty to try and
reverse engineer the dumps without any further hints. I assume that's
what he is referring to with difficulties.

That +: if you get me a field which says "dstmac" i dont have to go
and start aggregating 32bit chunks to create a 48bit MAC (or worse
look at the offset and figure where they come from).

Looking back, I would simply calculate a SHA hash over the original
filter in text form, pass the hash together with the tc filter and then
associate the hash with the filter text stored in user space. This
would not only benefit pedit but also u32 and possibly others.

A "cookie" tag that is sent to the kernel would serve the purpose.
We would need to standardize what the meaning of such a cookie is.

It also has the advantage that extending the kernel once now will allow
to add additional higher level abstractions later on without requiring
users to rebase their kernels.

Yes - and btw it works well with hardware offloads. It is a good free
form "assembler" level. But i dont think it serves well when we have
known often-used cases such as these.
It is like doing tcp over raw sockets - at some point it becomes more
efficient and more usable to just introduce tcp sockets. Thats all I
am asking for.

cheers,
jamal

On Mon, Jul 18, 2016 at 3:26 AM, Jamal Hadi Salim [off-list ref] wrote:

On 16-07-18 06:07 AM, Thomas Graf wrote:

quoted

Right. I was at the same point as Jamal and it is nasty to try and
reverse engineer the dumps without any further hints. I assume that's
what he is referring to with difficulties.

That +: if you get me a field which says "dstmac" i dont have to go
and start aggregating 32bit chunks to create a 48bit MAC (or worse
look at the offset and figure where they come from).

Been there, done that. I debugged u32 filter dumps too, I understand
your concern.

But this still looks like solvable to me, although it is definitely not easy,
probably even harder than the parsing logic. I will try to write some
code for iproute2 to see how far I can go.

Adding a new type of action/filter in kernel just for convenience seems
overkill. At very least I think we should just extend the existing ones,
for example, allowing pedit to accept and dump DST_MAC etc. Does this
sound good to you?

Thanks.

On 16-07-18 01:38 PM, Cong Wang wrote:

On Mon, Jul 18, 2016 at 3:26 AM, Jamal Hadi Salim [off-list ref] wrote:

quoted

On 16-07-18 06:07 AM, Thomas Graf wrote:

quoted

Right. I was at the same point as Jamal and it is nasty to try and
reverse engineer the dumps without any further hints. I assume that's
what he is referring to with difficulties.

That +: if you get me a field which says "dstmac" i dont have to go
and start aggregating 32bit chunks to create a 48bit MAC (or worse
look at the offset and figure where they come from).

Been there, done that. I debugged u32 filter dumps too, I understand
your concern.

But this still looks like solvable to me, although it is definitely not easy,
probably even harder than the parsing logic. I will try to write some
code for iproute2 to see how far I can go.

I am sure that will be useful for someone - just not me.

Adding a new type of action/filter in kernel just for convenience seems
overkill. At very least I think we should just extend the existing ones,
for example, allowing pedit to accept and dump DST_MAC etc. Does this
sound good to you?

Not for me Cong - but i have no doubt it would improve pedit's
usability. Besides that is fixing one app (tc).
What i am trying to do is a common use case. It is an
optimization that has become necessary; your effort could be parallel
to this. Look at how much time we are spending arm chair lawyering this;
please go ahead and do that with iproute2 and let this patch go in. Like
I said this comes from deployment experience and need.

cheers,
jamal

On 07/18/2016 12:08 PM, Jamal Hadi Salim wrote:

On 16-07-18 05:44 AM, Daniel Borkmann wrote:

quoted

On 07/18/2016 08:51 AM, Jamal Hadi Salim wrote:

quoted

On 16-07-18 12:19 AM, Alexei Starovoitov wrote:

quoted

Looking at that just out of curiosity on how complex it could look
for src/dst mac, is it actually functional in iproute2 upstream tree?

No it is a bunch of bash script wrapping we did on top of pedit (which
should be no different than adding it to m_pedit as an extension).
We started there then decided that this feature is mostly used with
mirred all the time - so modified  mirred.

quoted

All I see is that pedit can look up 3rd party modules via get_pedit_kind(),
so it will pick p_%s.so, if built as such, and there's code for p_ip,
p_tcp, p_udp, p_icmp. But then, for example, all I see in p_udp.c is
since initial iproute2 import in 2005, apart from some cleanups by
Stephen:

Yes, IP and tcp should be fine. Others were place holders.

Well, kind of, also TCP is a place holder here:

static int
parse_tcp(int *argc_p, char ***argv_p, struct tc_pedit_sel *sel, struct tc_pedit_key *tkey)
{
	int res = -1;
	return res;
}
struct m_pedit_util p_pedit_tcp = {
	NULL,
	"tcp",
	parse_tcp,
};

Ohh well, and the IP module implements couple of ICMP stuff and source/dest port
editing ... so only some parts of the IPv4 bits are usable it seems.

quoted

Same for tcp, icmp, ipv6 bits code ... :/ Is it still planned to eventually
complete these?

someone else could run with it; at the moment i think this was ok at
small scale but it hasnt worked well in a larger scale. When you write

I believe no-one really bothered to seriously use these p_* modules in
large (maybe even small) scale given their state. Maybe it would have been
different if this functionality was more complete from the beginning (analog
to u32 cls). True that script wrappers might do the same, but would have more
work once you'd add some 'pretty printer' to it.

other apps (other than tc) to use these APIs parsing all the 32 bit
chunks is more cumbersome then getting a struct which gives me precise
info.

True, the 32 bit chunks are more generic and as such you need to put more
effort in user space to handle them, but at the same time gain more flexibility
w/o having to have a module for each and every proto. But apart from this,
neither pedit nor tcf_skbmod_run() here handle checksum complete, so you'll
potentially get false positives wrt csum corruption and drops as a result
when using either of the two.

[...]

If i could tag the structure with something the kernel then returns to
me when i dump, I could add nice pretty printers (same arguement applies
to u32).
But that doesnt solve the programmability issue as being a good cause.

cheers,
jamal

On 16-07-19 09:21 AM, Daniel Borkmann wrote:

True, the 32 bit chunks are more generic and as such you need to put more
effort in user space to handle them, but at the same time gain more
flexibility
w/o having to have a module for each and every proto.

I dont see anything wrong with using pedit as a first step; even
if you did what Cong said he would do _i wont use it_ given the choice
against skbmod. I think we are going in circles now in this discussion.

You probably didnt mean to say module per protocol above since we only
have one action module [no different than what ebtables or openvswitch
does. It may have more justifiable extensions in the future].

But apart from this,
neither pedit nor tcf_skbmod_run() here handle checksum complete, so you'll
potentially get false positives wrt csum corruption and drops as a result
when using either of the two.

pedit maybe tricky. Any suggestions?
On tcf_skbmod_run, mostly ignorance: while doing only ethernet updates;
is it still needed to do the checksum complete?

cheers,
jamal

On 07/19/2016 03:56 PM, Jamal Hadi Salim wrote:
[...]

quoted

But apart from this,
neither pedit nor tcf_skbmod_run() here handle checksum complete, so you'll
potentially get false positives wrt csum corruption and drops as a result
when using either of the two.

pedit maybe tricky. Any suggestions?
On tcf_skbmod_run, mostly ignorance: while doing only ethernet updates;
is it still needed to do the checksum complete?

Well, what Cong recently fixed with mirred was related to mac header ...

You probably need skb_postpull_rcsum(), skb_postpush_rcsum() pair.

Also, what about skb_try_make_writable()?

On Tue, Jul 19, 2016 at 8:03 AM, Daniel Borkmann [off-list ref] wrote:

On 07/19/2016 03:56 PM, Jamal Hadi Salim wrote:
[...]

quoted

quoted

But apart from this,
neither pedit nor tcf_skbmod_run() here handle checksum complete, so
you'll
potentially get false positives wrt csum corruption and drops as a result
when using either of the two.

pedit maybe tricky. Any suggestions?
On tcf_skbmod_run, mostly ignorance: while doing only ethernet updates;
is it still needed to do the checksum complete?

Well, what Cong recently fixed with mirred was related to mac header ...

You probably need skb_postpull_rcsum(), skb_postpush_rcsum() pair.

Also, what about skb_try_make_writable()?

I don't think so. 1) checksum is supposed to be done by csum action
rather than pedit (or skbmod if it matters), 2) csum action currently
already handles that correctly for both egress and ingress: 2a)
CHECKSUM_COMPLETE is meaningless on egress; 2b) it forces
CHECKSUM_COMPLETE to be CHECKSUM_NONE on ingress
and it is correctly respected.

On 07/19/2016 08:04 PM, Cong Wang wrote:

On Tue, Jul 19, 2016 at 8:03 AM, Daniel Borkmann [off-list ref] wrote:

quoted

On 07/19/2016 03:56 PM, Jamal Hadi Salim wrote:
[...]

quoted

quoted

But apart from this,
neither pedit nor tcf_skbmod_run() here handle checksum complete, so
you'll
potentially get false positives wrt csum corruption and drops as a result
when using either of the two.

pedit maybe tricky. Any suggestions?
On tcf_skbmod_run, mostly ignorance: while doing only ethernet updates;
is it still needed to do the checksum complete?

Well, what Cong recently fixed with mirred was related to mac header ...

You probably need skb_postpull_rcsum(), skb_postpush_rcsum() pair.

Also, what about skb_try_make_writable()?

I don't think so. 1) checksum is supposed to be done by csum action
rather than pedit (or skbmod if it matters), 2) csum action currently
already handles that correctly for both egress and ingress: 2a)
CHECKSUM_COMPLETE is meaningless on egress; 2b) it forces
CHECKSUM_COMPLETE to be CHECKSUM_NONE on ingress
and it is correctly respected.

Ahh, right, so it redoes entire csums when worst-case changing one byte
only, since it cannot know what other actions like pedit did previously.
But it also means for your l2 mac addr changes to force a CHECKSUM_COMPLETE
into CHECKSUM_NONE with csum action that you need to call into one of the
l3/l4 helpers as far as I see.

On 16-07-19 08:23 PM, Daniel Borkmann wrote:

On 07/19/2016 08:04 PM, Cong Wang wrote:

quoted

On Tue, Jul 19, 2016 at 8:03 AM, Daniel Borkmann
[off-list ref] wrote:

quoted

On 07/19/2016 03:56 PM, Jamal Hadi Salim wrote:
[...]

[..]

quoted

I don't think so. 1) checksum is supposed to be done by csum action
rather than pedit (or skbmod if it matters), 2) csum action currently
already handles that correctly for both egress and ingress: 2a)
CHECKSUM_COMPLETE is meaningless on egress; 2b) it forces
CHECKSUM_COMPLETE to be CHECKSUM_NONE on ingress
and it is correctly respected.

Ahh, right, so it redoes entire csums when worst-case changing one byte
only, since it cannot know what other actions like pedit did previously.

 >

But it also means for your l2 mac addr changes to force a CHECKSUM_COMPLETE
into CHECKSUM_NONE with csum action that you need to call into one of the
l3/l4 helpers as far as I see.

Forgot about csum which would work with pedit - didnt quiet parse what
you are saying above though:
does changing MAC address require changing to CHECKSUM_NONE? If yes,
then seems like i need to send a patch for act_ife as well to make in
the spirit of mirred?

Having said all that:
This discussion now has gone past the original patch submission.
I really want to see this patch go in and dont want to get Dave to
review all these side comments - for this reason I changed the subject.

cheers,
jamal

On 07/21/2016 09:27 AM, Jamal Hadi Salim wrote:
[...]

Forgot about csum which would work with pedit - didnt quiet parse what
you are saying above though:
does changing MAC address require changing to CHECKSUM_NONE? If yes,
then seems like i need to send a patch for act_ife as well to make in
the spirit of mirred?

Hmm, seems fine actually up to ETH_HLEN. In cases like vlan or mpls
modifications there's certainly a correction, but when going into rx
path this is pulled out already of the calculation afaik (see f.e.
dev_forward_skb()). So when mirred pushes this back in via skb_push_rcsum()
and dev_forward_skb() back out, it should be okay if I see this correctly.
But don't you still something like need skb_try_make_writable()?