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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170527100230.GB1831@nanopsycho>
Date:   Sat, 27 May 2017 12:02:30 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     David Miller <davem@...emloft.net>
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

Fri, May 26, 2017 at 04:54:43PM CEST, davem@...emloft.net wrote:
>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.
 
Allright. I ack that.


>
>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".

What I don't like is the double "n->nlmsg_type == RTM_NEWTFILTER" check
and return value decusion according to the latter check. The code logic
is split into tcf_chain_get function and its caller. That is
at least odd.

Since you don't like the PTR_ERR approach, I'll try to figure out how to
do this another way.