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]

Message-ID: <587BC894.3080909@gmail.com>
Date:   Sun, 15 Jan 2017 11:08:04 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Paul Blakey <paulb@...lanox.com>, Jiri Pirko <jiri@...nulli.us>,
        Jamal Hadi Salim <jhs@...atatu.com>
Cc:     "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

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.

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