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] [day] [month] [year] [list]

Date:   Sat, 14 Jan 2017 12:18:11 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Jiri Pirko <jiri@...nulli.us>, Jamal Hadi Salim <jhs@...atatu.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, jiri@...lanox.com,
        paulb@...lanox.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 07:49 AM, Jiri Pirko wrote:
> 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.
> 

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