netdev - Re: [net-next PATCH v3 3/8] net: sched: add cls

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: <56C5E212.50505@gmail.com>
Date:	Thu, 18 Feb 2016 07:24:02 -0800
From:	John Fastabend <john.fastabend@...il.com>
To:	Jamal Hadi Salim <jhs@...atatu.com>, jiri@...nulli.us,
	amir@...ai.me, davem@...emloft.net
CC:	netdev@...r.kernel.org, jeffrey.t.kirsher@...el.com
Subject: Re: [net-next PATCH v3 3/8] net: sched: add cls_u32 offload hooks
 for netdevs

On 16-02-18 04:14 AM, Jamal Hadi Salim wrote:
> On 16-02-17 06:07 PM, John Fastabend wrote:
>> [...]
>>
> 
>> Actually thinking about this a bit more I wrote this thinking
>> that there existed some hardware that actually cared if it was
>> a new rule or an existing rule. For me it doesn't matter I do
>> the same thing in the new/replace cases I just write into the
>> slot on the hardware table and if it happens to have something
>> in it well its overwritten e.g. "replaced". This works because
>> the cls_u32 layer protects us from doing something unexpected.
>>
> 
> You are describing create-or-update which is a reasonable default
> BUT: counting on the user to specify the htid+bktid+nodeid
> for every filter and knowing what that means is prone to mistakes
> when for example (using your big hammer approach right now) they
> dont specify the handle and the kernel creates one for them.
> 
> IMO, it would be better at this early stage to enforce the correct
> behavior for future generations.
> To follow the netlink semantics which a lot of people are already
> trained to think in.
> 
> Current netlink behavior is supposed to be:
> 
> 1) NEW ==> "Create".
> Ambigous - could mean a)"create if it doesnt exist" or b) "fail if it
> exists otherwise create"
> Unfortunately different parts of the kernel often assume some
> default from either #a or #b.
> 

But this is already handled by the core cls_api.c code. We never
get to u32_change if the flags are not correct.

Look at the block right above the op call into the classifiers
change() code in cls_api.c. Starting at line 287.

> 2) NEW|REPLACE flag ==> "Create if it doesnt exist and replace
> if it exists"
> 
> 3)NEW|EXCLUSIVE ==> "Create if it doesnt exist and fail if it
> exists"
> 
> 4)NEW|APPEND ==> "just fscking create; i dont care if it exists".
> 
> IOW, just add the flag field which is intepreted from whatever
> the user explicitly asks for. And reject say what the hardware
> doesnt support.

I think I agree with what your saying but I'm fairly sure this is
working as you describe.

> I have worked with tcams where we support #3. It is a bit inefficient
> because you have to check if a rule exists first. And i have worked
> in cases where #1 is assumed to mean #2 and at times #4. It is better
> user experience to be explicit.

Agreed, and I think it is :)

> 
> cheers,
> jamal