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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <09bf24f3-7080-341d-a332-a2042e7fc34b@mojatatu.com>
Date:   Sat, 14 Jan 2017 10:39:24 -0500
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, jiri@...lanox.com,
        paulb@...lanox.com, john.fastabend@...il.com,
        simon.horman@...ronome.com, mrv@...atatu.com, hadarh@...lanox.com,
        ogerlitz@...lanox.com, roid@...lanox.com, xiyou.wangcong@...il.com,
        daniel@...earbox.net, Jamal Hadi Salim <hadi@...atatu.com>
Subject: Re: [PATCH net-next 1/1] net sched actions: Add support for user
 cookies

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

>> .. 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.


>>
>> .. 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.

>> 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.

>> +};
>> +
>> 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