netdev - Re: [PATCH net-next] net/sched: cls_flower: Add user specified data

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:   Mon, 16 Jan 2017 10:51:29 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Paul Blakey <paulb@...lanox.com>
Cc:     John Fastabend <john.fastabend@...il.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Jiri Pirko <jiri@...lanox.com>,
        Hadar Hen Zion <hadarh@...lanox.com>,
        Or Gerlitz <ogerlitz@...lanox.com>,
        Roi Dayan <roid@...lanox.com>, Roman Mashak <mrv@...atatu.com>
Subject: Re: [PATCH net-next] net/sched: cls_flower: Add user specified data

Mon, Jan 16, 2017 at 08:54:18AM CET, paulb@...lanox.com wrote:
>
>
>On 15/01/2017 21:08, John Fastabend wrote:
>> On 17-01-15 09:36 AM, Paul Blakey wrote:
>> > 
>> > 
>> > On 08/01/2017 19:12, Jiri Pirko wrote:
>> > > Mon, Jan 02, 2017 at 03:59:49PM CET, jhs@...atatu.com wrote:
>> > > > 
>> > > > We have been using a cookie as well for actions (which we have been
>> > > > using but have been too lazy to submit so far). I am going to port
>> > > > it over to the newer kernels and post it.
>> > > 
>> > > Hard to deal with something we can't look at :)
>> > > 
>> > > 
>> > > > In our case that is intended to be opaque to the kernel i.e kernel
>> > > > never inteprets it; in that case it is similar to the kernel
>> > > > FIB protocol field.
>> > > 
>> > > In case of this patch, kernel also never interprets it. What makes you
>> > > think otherwise. Bot kernel, it is always a binary blob.
>> > > 
>> > > 
>> > > > 
>> > > > In your case - could this cookie have been a class/flowid
>> > > > (a 32 bit)?
>> > > > And would it not make more sense for it the cookie to be
>> > > > generic to all classifiers? i.e why is it specific to flower?
>> > > 
>> > > Correct, makes sense to have it generic for all cls and perhaps also
>> > > acts.
>> > > 
>> > > 
>> > 
>> > Hi all,
>> > I've started implementing a general cookie for all classifiers,
>> > I added the cookie on tcf_proto struct but then I realized that it won't work as
>> > I want. What I want is cookie per internal element (those that are identified by
>> > handles), which we can have many per one tcf_proto:
>> > 
>> > tc filter add dev <DEV> parent ffff: prio 1 cookie 0x123 basic action drop
>> > tc filter add dev <DEV> parent ffff: prio 1 cookie 0x456 "port6" basic action drop
>> > tc filter add dev <DEV> parent ffff: prio 1 cookie 0x777 basic action drop
>> > 
>> > So there is three options to do that:
>> > 1) Implement it in each classifier, using some generic function. (kinda like
>> > stats, where classifiler_dump() calls tcf_exts_dump_stats())
>> > 2) Have a global hash table for cookies [prio,handle]->[cookie]
>> > 3) Have it on flower only for now :)
>> > 
>> > With the first one we will have to change each classifier (or leave it
>> > unsupported as default).
>> > Second is less code to change (instead of changing each classifier), but might
>> > be slower. I'm afraid how it will affect dumping several filters.
>> > Third is this patch.
>> > 
>> > Thanks,
>> > Paul.
>> 
>> Avoid (2) it creates way more problems than its worth like is it locked/not
>> locked, how is it synced, collisions, etc. Seems way overkill.

+1


>> 
>> I like the generic functionality of (1) but unless we see this pop up in
>> different filters I wouldn't require it for now. If you just do it in flower
>> (option 3) when its added to another classifier we can generalize it. As a
>> middle ground creating nice helper routines as needed goes a long way.
>> 
>> .John
>> 
>
>Hi all,
>So is everyone ok with leaving the commit as is, only adding user data
>to flower for now?

I think we should do it in a generic way, for every classifier, right
away. Same as Jamal is doing for actions. I think that first we should
get Jamal's patch merged and then do the same for classifiers.