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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Sat, 14 Jan 2017 16:49:15 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jamal Hadi Salim <jhs@...atatu.com>
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

Sat, Jan 14, 2017 at 04:39:24PM CET, jhs@...atatu.com wrote:
>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.

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.


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

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.