netdev - Re: [PATCH net-next v4 1/2] 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]

Message-ID: <587E4D16.8030708@iogearbox.net>
Date:   Tue, 17 Jan 2017 17:57:58 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jamal Hadi Salim <jhs@...atatu.com>, davem@...emloft.net
CC:     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
Subject: Re: [PATCH net-next v4 1/2] net sched actions: Add support for user
 cookies

On 01/17/2017 05:50 PM, Jamal Hadi Salim wrote:
> On 17-01-17 09:16 AM, Daniel Borkmann wrote:
>> On 01/17/2017 12:11 PM, Jamal Hadi Salim wrote:
>>> From: Jamal Hadi Salim <jhs@...atatu.com>
>>>
>>> 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.
>> [...]
>>
>> Since it looks like you need a v5 anyway, few comments below.
>>
>>>   include/net/act_api.h        |  1 +
>>>   include/net/pkt_cls.h        |  8 ++++++++
>>>   include/uapi/linux/pkt_cls.h |  3 +++
>>>   net/sched/act_api.c          | 25 +++++++++++++++++++++++++
>>>   4 files changed, 37 insertions(+)
>>>
>>> diff --git a/include/net/act_api.h b/include/net/act_api.h
>>> index 1d71644..0692458 100644
>>> --- a/include/net/act_api.h
>>> +++ b/include/net/act_api.h
>>> @@ -41,6 +41,7 @@ struct tc_action {
>>>       struct rcu_head            tcfa_rcu;
>>>       struct gnet_stats_basic_cpu __percpu *cpu_bstats;
>>>       struct gnet_stats_queue __percpu *cpu_qstats;
>>> +    struct tc_cookie    *act_ck;
>>
>> Since we know anyway that this is part of struct tc_action, can't
>> you just give this some real/readable name like ...
>>
>>   struct tc_cookie cookie;
>
> Grep-ability.
> I was worried about when the classifier adds its cookie it
> would need to use something like cls_cookie etc.

Given this cookie is just used for correlation in user space anyway
and not processed any further by the kernel, I think we can well
handle these very few spots, so would be better if the code is more
maintainable instead.

Thanks,
Daniel