netdev - Re: [Patch net-next] net_sched: only create filter chains for new filters/actions

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:   Fri, 26 May 2017 10:54:43 -0400 (EDT)
From:   David Miller <davem@...emloft.net>
To:     jiri@...nulli.us
Cc:     xiyou.wangcong@...il.com, netdev@...r.kernel.org, jhs@...atatu.com,
        jiri@...lanox.com
Subject: Re: [Patch net-next] net_sched: only create filter chains for new
 filters/actions

From: Jiri Pirko <jiri@...nulli.us>
Date: Fri, 26 May 2017 07:53:52 +0200

> Thu, May 25, 2017 at 06:14:56PM CEST, davem@...emloft.net wrote:
>>From: Cong Wang <xiyou.wangcong@...il.com>
>>Date: Tue, 23 May 2017 09:42:37 -0700
>>
>>> tcf_chain_get() always creates a new filter chain if not found
>>> in existing ones. This is totally unnecessary when we get or
>>> delete filters, new chain should be only created for new filters
>>> (or new actions).
>>> 
>>> Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
>>> Cc: Jamal Hadi Salim <jhs@...atatu.com>
>>> Cc: Jiri Pirko <jiri@...lanox.com>
>>> Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
>>
>>Indeed, get and delete requests should not create new objects, ever.
>>
>>I have pretty much no idea why Jiri is making such a big fuss about
>>this change, to be quite honest. :-)
> 
> Because it makes already hard to read code even worse, for *no* benefit.
> That's why.

Jiri, if you say the same thing 100 times, it doesn't help anyone
understand your arguments any better.

Creating new objects when a GET or a DEL is requested is flat out
wrong.

And Cong is fixing that.

And I also didn't find the boolean logic hard to understand at all.

It is in fact a very common pattern to pass a "create" boolean into
lookup functions, to tell them whether to create a new object on
lookup failure or not.  And then also to control that boolean via
what kind of netlink request we are processing.

So you tell me what's so bad about his change given the above?

Give me details and real facts, like I just did, rather than vague
statements about "benefit" and "hard to read".

Thank you.