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: <20170117170501.GE1807@nanopsycho.orion>
Date:   Tue, 17 Jan 2017 18:05:01 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Jamal Hadi Salim <jhs@...atatu.com>, 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
Subject: Re: [PATCH net-next v4 1/2] net sched actions: Add support for user
 cookies

Tue, Jan 17, 2017 at 05:57:58PM CET, daniel@...earbox.net wrote:
>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.

I agree with Daniel. His naming change suggestions make sense.

In fact, Jamal, now I know why there are names like this all over TC :)