[PATCH net-next 1/1] net sched actions: Add support for user cookies

From: Jamal Hadi Salim <redacted>

Introduce optional 128-bit action cookie.
Like all other cookie schemes in the networking world (eg in protocols
like http or existing kernel fib protocol field, etc) the idea is to save
user state that when retrieved serves as a correlator. The kernel
_should not_ intepret it.  The user can store whatever they wish in the
128 bits.

Sample exercise(using two 64bit values to represent the 128 bits):

.. create an accept action with cookie 0xA:0xa0a0a0a0a0a0a0
sudo $TC actions add action ok index 1 cookie 0xA 0xa0a0a0a0a0a0a0

.. dump all gact actions..
sudo $TC -s actions ls action gact

	action order 0: gact action pass
	 random type none pass val 0
	 index 1 ref 2 bind 1 installed 1221 sec used 27 sec
 	Action statistics:
	Sent 373248 bytes 5056 pkt (dropped 0, overlimits 0 requeues 0)
	backlog 0b 0p requeues 0
	 cookie(0000000a:00000000:a0a0a0a0:00a0a0a0)

.. bind the accept action to a filter..
sudo $TC filter add dev lo parent ffff: protocol ip prio 1 \
u32 match ip dst 127.0.0.1/32 flowid 1:1 action gact index 1

... send some traffic..
$ ping 127.0.0.1 -c 3
PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data.
64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.020 ms
64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.027 ms
64 bytes from 127.0.0.1: icmp_seq=3 ttl=64 time=0.038 ms

--- 127.0.0.1 ping statistics ---

3 packets transmitted, 3 received, 0% packet loss, time 2109ms
rtt min/avg/max/mdev = 0.020/0.028/0.038/0.008 ms 1

... show some stats
$ sudo $TC -s actions get  action gact index 1

	action order 1: gact action pass
	 random type none pass val 0
	 index 1 ref 3 bind 1 installed 2182 sec used 1 sec
 	Action statistics:
	Sent 700344 bytes 9486 pkt (dropped 0, overlimits 0 requeues 0)
	backlog 0b 0p requeues 0

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 include/net/act_api.h        |  7 +++++++
 include/uapi/linux/pkt_cls.h |  7 +++++++
 net/sched/act_api.c          | 27 +++++++++++++++++++++++++--
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 1d71644..b948db9 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h

@@ -20,6 +20,12 @@ struct tcf_hashinfo {
 
 struct tc_action_ops;
 
+union act_cookie {
+	u16 ck16[8];
+	u32 ck32[4];
+	u64 ck64[2];
+};
+
 struct tc_action {
 	const struct tc_action_ops	*ops;
 	__u32				type; /* for backward compat(TCA_OLD_COMPAT) */

@@ -41,6 +47,7 @@ struct tc_action {
 	struct rcu_head			tcfa_rcu;
 	struct gnet_stats_basic_cpu __percpu *cpu_bstats;
 	struct gnet_stats_queue __percpu *cpu_qstats;
+	union act_cookie	*ck;
 };
 #define tcf_head	common.tcfa_head
 #define tcf_index	common.tcfa_index

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 1e5e1dd..6379af3 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h

@@ -4,6 +4,12 @@
 #include <linux/types.h>
 #include <linux/pkt_sched.h>
 
+union u_act_cookie {
+	__u16 ck16[8];
+	__u32 ck32[4];
+	__u64 ck64[2];
+};
+
 /* Action attributes */
 enum {
 	TCA_ACT_UNSPEC,

@@ -12,6 +18,7 @@ enum {
 	TCA_ACT_INDEX,
 	TCA_ACT_STATS,
 	TCA_ACT_PAD,
+	TCA_ACT_COOKIE,
 	__TCA_ACT_MAX
 };
 

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index f04715a..85e77181 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c

@@ -33,6 +33,7 @@ static void free_tcf(struct rcu_head *head)
 
 	free_percpu(p->cpu_bstats);
 	free_percpu(p->cpu_qstats);
+	kfree(p->ck);
 	kfree(p);
 }
 

@@ -464,8 +465,8 @@ int tcf_action_destroy(struct list_head *actions, int bind)
 	return a->ops->dump(skb, a, bind, ref);
 }
 
-int
-tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
+int tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind,
+		      int ref)
 {
 	int err = -EINVAL;
 	unsigned char *b = skb_tail_pointer(skb);

@@ -475,6 +476,12 @@ int tcf_action_destroy(struct list_head *actions, int bind)
 		goto nla_put_failure;
 	if (tcf_action_copy_stats(skb, a, 0))
 		goto nla_put_failure;
+	if (a->ck) {
+		if (nla_put(skb, TCA_ACT_COOKIE, sizeof(union act_cookie),
+			    a->ck))
+			goto nla_put_failure;
+	}
+
 	nest = nla_nest_start(skb, TCA_OPTIONS);
 	if (nest == NULL)
 		goto nla_put_failure;

@@ -575,6 +582,22 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 	if (err < 0)
 		goto err_mod;
 
+	if (tb[TCA_ACT_COOKIE]) {
+		if (nla_len(tb[TCA_ACT_COOKIE]) != sizeof(union act_cookie)) {
+			err = -EINVAL;
+			goto err_mod;
+		}
+
+		a->ck = kzalloc(sizeof(union act_cookie), GFP_KERNEL);
+		if (unlikely(!a->ck)) {
+			err = -ENOMEM;
+			goto err_mod;
+		}
+
+		memcpy((void *)a->ck, nla_data(tb[TCA_ACT_COOKIE]),
+		       sizeof(union act_cookie));
+	}
+
 	/* module count goes up only when brand new policy is created
 	 * if it exists and is only bound to in a_o->init() then
 	 * ACT_P_CREATED is not returned (a zero is).

-- 
1.9.1

Sat, Jan 14, 2017 at 03:59:18PM CET, jhs@mojatatu.com wrote:

From: Jamal Hadi Salim <redacted>

Introduce optional 128-bit action cookie.
Like all other cookie schemes in the networking world (eg in protocols
like http or existing kernel fib protocol field, etc) the idea is to save
user state that when retrieved serves as a correlator. The kernel
_should not_ intepret it.  The user can store whatever they wish in the
128 bits.

Sample exercise(using two 64bit values to represent the 128 bits):

.. create an accept action with cookie 0xA:0xa0a0a0a0a0a0a0
sudo $TC actions add action ok index 1 cookie 0xA 0xa0a0a0a0a0a0a0

2x 64bit values? Why can't this have variable length, according to what
user needs:

sudo $TC actions add action ok index 1 cookie a0
sudo $TC actions add action ok index 1 cookie a01122
sudo $TC actions add action ok index 1 cookie a01122334455
sudo $TC actions add action ok index 1 cookie a01122334455aabbccddeeff

.. dump all gact actions..
sudo $TC -s actions ls action gact

action order 0: gact action pass
 random type none pass val 0
 index 1 ref 2 bind 1 installed 1221 sec used 27 sec
	Action statistics:
Sent 373248 bytes 5056 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
 cookie(0000000a:00000000:a0a0a0a0:00a0a0a0)

Input is 2x64 and dump is 4x32? That is confusing. With my suggested
example, this would be:

	 cookie a0
	 cookie a01122
	 cookie a01122334455
	 cookie a01122334455aabbccddeeff

Since this should be never interpreted by kernel, I don't understand why
this union is needed. Why just don't pass a char array?

Also, whatever format this is, could we make is shared with cls cookie?

Again, the same struct? I don't understand why twice.

On 17-01-14 10:22 AM, Jiri Pirko wrote:

quoted

.. create an accept action with cookie 0xA:0xa0a0a0a0a0a0a0
sudo $TC actions add action ok index 1 cookie 0xA 0xa0a0a0a0a0a0a0

2x 64bit values? Why can't this have variable length, according to what
user needs:

You can intepret it however you wish. It is 128 bits. You can make it
2x64, 4x32, 8x16, 16x8

sudo $TC actions add action ok index 1 cookie a0
sudo $TC actions add action ok index 1 cookie a01122
sudo $TC actions add action ok index 1 cookie a01122334455
sudo $TC actions add action ok index 1 cookie a01122334455aabbccddeeff

Sure you can do that too..
I will add add 16 8b fields to the union.

quoted

.. dump all gact actions..
sudo $TC -s actions ls action gact

	action order 0: gact action pass
	 random type none pass val 0
	 index 1 ref 2 bind 1 installed 1221 sec used 27 sec
	Action statistics:
	Sent 373248 bytes 5056 pkt (dropped 0, overlimits 0 requeues 0)
	backlog 0b 0p requeues 0
	 cookie(0000000a:00000000:a0a0a0a0:00a0a0a0)

Input is 2x64 and dump is 4x32? That is confusing. With my suggested
example, this would be:

	 cookie a0
	 cookie a01122
	 cookie a01122334455
	 cookie a01122334455aabbccddeeff

Your suggestion is more sensible for a user space cli tool like tc.
I will add a uchar cku8[16] field and make changes to iproute2.

quoted

struct tc_action_ops;

+union act_cookie {
+	u16 ck16[8];
+	u32 ck32[4];
+	u64 ck64[2];

Since this should be never interpreted by kernel, I don't understand why
this union is needed. Why just don't pass a char array?

programmatic usability.

Also, whatever format this is, could we make is shared with cls cookie?

The structure could be shared (and because it is in pkt_cls.h
that makes it easier). But the TLVs are domain specific. We need another
one for classifiers.

quoted

+};
+
struct tc_action {
	const struct tc_action_ops	*ops;
	__u32				type; /* for backward compat(TCA_OLD_COMPAT) */

@@ -41,6 +47,7 @@ struct tc_action {

	struct rcu_head			tcfa_rcu;
	struct gnet_stats_basic_cpu __percpu *cpu_bstats;
	struct gnet_stats_queue __percpu *cpu_qstats;
+	union act_cookie	*ck;
};
#define tcf_head	common.tcfa_head
#define tcf_index	common.tcfa_index

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 1e5e1dd..6379af3 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h

@@ -4,6 +4,12 @@

#include <linux/types.h>
#include <linux/pkt_sched.h>

+union u_act_cookie {
+	__u16 ck16[8];
+	__u32 ck32[4];
+	__u64 ck64[2];
+};

Again, the same struct? I don't understand why twice.

Just old habits.
user vs kernel api? Standard action approach one says
__u32 other says u32; hanging off the user variant to kernel
didnt feel right.

cheers,
jamal

Sat, Jan 14, 2017 at 04:39:24PM CET, jhs@mojatatu.com wrote:

On 17-01-14 10:22 AM, Jiri Pirko wrote:

quoted

quoted

.. create an accept action with cookie 0xA:0xa0a0a0a0a0a0a0
sudo $TC actions add action ok index 1 cookie 0xA 0xa0a0a0a0a0a0a0

2x 64bit values? Why can't this have variable length, according to what
user needs:

You can intepret it however you wish. It is 128 bits. You can make it
2x64, 4x32, 8x16, 16x8

quoted

sudo $TC actions add action ok index 1 cookie a0
sudo $TC actions add action ok index 1 cookie a01122
sudo $TC actions add action ok index 1 cookie a01122334455
sudo $TC actions add action ok index 1 cookie a01122334455aabbccddeeff

Sure you can do that too..
I will add add 16 8b fields to the union.

quoted

quoted

.. dump all gact actions..
sudo $TC -s actions ls action gact

	action order 0: gact action pass
	 random type none pass val 0
	 index 1 ref 2 bind 1 installed 1221 sec used 27 sec
	Action statistics:
	Sent 373248 bytes 5056 pkt (dropped 0, overlimits 0 requeues 0)
	backlog 0b 0p requeues 0
	 cookie(0000000a:00000000:a0a0a0a0:00a0a0a0)

Input is 2x64 and dump is 4x32? That is confusing. With my suggested
example, this would be:

	 cookie a0
	 cookie a01122
	 cookie a01122334455
	 cookie a01122334455aabbccddeeff

Your suggestion is more sensible for a user space cli tool like tc.
I will add a uchar cku8[16] field and make changes to iproute2.

quoted

quoted

struct tc_action_ops;

+union act_cookie {
+	u16 ck16[8];
+	u32 ck32[4];
+	u64 ck64[2];

Since this should be never interpreted by kernel, I don't understand why
this union is needed. Why just don't pass a char array?

programmatic usability.

I don't see why. In userspace you can map whatever struct you need to the
mem with chararray. It's totally up to you as an app developer. There is
no need to make that part of kernel api. Really.

quoted

Also, whatever format this is, could we make is shared with cls cookie?

The structure could be shared (and because it is in pkt_cls.h
that makes it easier). But the TLVs are domain specific. We need another
one for classifiers.

quoted

quoted

+};
+
struct tc_action {
	const struct tc_action_ops	*ops;
	__u32				type; /* for backward compat(TCA_OLD_COMPAT) */

@@ -41,6 +47,7 @@ struct tc_action {

	struct rcu_head			tcfa_rcu;
	struct gnet_stats_basic_cpu __percpu *cpu_bstats;
	struct gnet_stats_queue __percpu *cpu_qstats;
+	union act_cookie	*ck;
};
#define tcf_head	common.tcfa_head
#define tcf_index	common.tcfa_index

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 1e5e1dd..6379af3 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h

@@ -4,6 +4,12 @@

#include <linux/types.h>
#include <linux/pkt_sched.h>

+union u_act_cookie {
+	__u16 ck16[8];
+	__u32 ck32[4];
+	__u64 ck64[2];
+};

Again, the same struct? I don't understand why twice.

Just old habits.
user vs kernel api? Standard action approach one says
__u32 other says u32; hanging off the user variant to kernel
didnt feel right.

Just have it in uapi and use it from within kernel. But did you see what
I suggested in the other thread (regarding IFLA_PHYS_PORT_ID and
IFLA_PHYS_SWITCH_ID)? If you do it what way, you don't need no struct.

On 17-01-14 07:49 AM, Jiri Pirko wrote:

Sat, Jan 14, 2017 at 04:39:24PM CET, jhs@mojatatu.com wrote:

quoted

On 17-01-14 10:22 AM, Jiri Pirko wrote:

quoted

quoted

.. create an accept action with cookie 0xA:0xa0a0a0a0a0a0a0
sudo $TC actions add action ok index 1 cookie 0xA 0xa0a0a0a0a0a0a0

2x 64bit values? Why can't this have variable length, according to what
user needs:

You can intepret it however you wish. It is 128 bits. You can make it
2x64, 4x32, 8x16, 16x8

quoted

sudo $TC actions add action ok index 1 cookie a0
sudo $TC actions add action ok index 1 cookie a01122
sudo $TC actions add action ok index 1 cookie a01122334455
sudo $TC actions add action ok index 1 cookie a01122334455aabbccddeeff

Sure you can do that too..
I will add add 16 8b fields to the union.

quoted

quoted

.. dump all gact actions..
sudo $TC -s actions ls action gact

	action order 0: gact action pass
	 random type none pass val 0
	 index 1 ref 2 bind 1 installed 1221 sec used 27 sec
	Action statistics:
	Sent 373248 bytes 5056 pkt (dropped 0, overlimits 0 requeues 0)
	backlog 0b 0p requeues 0
	 cookie(0000000a:00000000:a0a0a0a0:00a0a0a0)

Input is 2x64 and dump is 4x32? That is confusing. With my suggested
example, this would be:

	 cookie a0
	 cookie a01122
	 cookie a01122334455
	 cookie a01122334455aabbccddeeff

Your suggestion is more sensible for a user space cli tool like tc.
I will add a uchar cku8[16] field and make changes to iproute2.

quoted

quoted

struct tc_action_ops;

+union act_cookie {
+	u16 ck16[8];
+	u32 ck32[4];
+	u64 ck64[2];

Since this should be never interpreted by kernel, I don't understand why
this union is needed. Why just don't pass a char array?

programmatic usability.

I don't see why. In userspace you can map whatever struct you need to the
mem with chararray. It's totally up to you as an app developer. There is
no need to make that part of kernel api. Really.

quoted

quoted

Also, whatever format this is, could we make is shared with cls cookie?

The structure could be shared (and because it is in pkt_cls.h
that makes it easier). But the TLVs are domain specific. We need another
one for classifiers.

quoted

quoted

+};
+
struct tc_action {
	const struct tc_action_ops	*ops;
	__u32				type; /* for backward compat(TCA_OLD_COMPAT) */

@@ -41,6 +47,7 @@ struct tc_action {

	struct rcu_head			tcfa_rcu;
	struct gnet_stats_basic_cpu __percpu *cpu_bstats;
	struct gnet_stats_queue __percpu *cpu_qstats;
+	union act_cookie	*ck;
};
#define tcf_head	common.tcfa_head
#define tcf_index	common.tcfa_index

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 1e5e1dd..6379af3 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h

@@ -4,6 +4,12 @@

#include <linux/types.h>
#include <linux/pkt_sched.h>

+union u_act_cookie {
+	__u16 ck16[8];
+	__u32 ck32[4];
+	__u64 ck64[2];
+};

Again, the same struct? I don't understand why twice.

Just old habits.
user vs kernel api? Standard action approach one says
__u32 other says u32; hanging off the user variant to kernel
didnt feel right.

Just have it in uapi and use it from within kernel. But did you see what
I suggested in the other thread (regarding IFLA_PHYS_PORT_ID and
IFLA_PHYS_SWITCH_ID)? If you do it what way, you don't need no struct.

+1 on using something like MAX_PHYS_ITEM_ID_LEN seems much nicer to me
and easy to extend later as Jiri notes.