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: <20170526.105443.1489276661727770629.davem@davemloft.net>
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.

Powered by blists - more mailing lists