netdev - Re: [PATCH RFC 1/3] ipv4: make flow functions more grepable by removing flowi4_init

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: <0f3516fc-9108-58a3-88fa-49a80a829acc@stressinduktion.org>
Date:	Thu, 16 Jun 2016 23:39:12 +0200
From:	Hannes Frederic Sowa <hannes@...essinduktion.org>
To:	David Miller <davem@...emloft.net>, dsa@...ulusnetworks.com
Cc:	netdev@...r.kernel.org, ja@....bg
Subject: Re: [PATCH RFC 1/3] ipv4: make flow functions more grepable by
 removing flowi4_init_output

On 16.06.2016 23:18, David Miller wrote:
> From: David Ahern <dsa@...ulusnetworks.com>
> Date: Wed, 15 Jun 2016 14:29:28 -0600
> 
>> On 6/15/16 1:48 PM, David Miller wrote:
>>> But if people don't use the helpers, and initialize flow structures
>>> on their own, yeah that defeats the whole mechanism and things will
>>> seem harder and "unhelpful".
>>
>> That's my point -- the flow struct does not have a consistent
>> initializer. There have been a number of bug patches over the past
>> year like 4cfc86f3dae6 to handle uninitialized elements. My suggestion
>> here is the same as 4cfc86f3dae6 which is to initialize the flow when
>> it is declared.
>>
>> Consider the recent change from Hannes for 38b7097b55b6. fl6 can be
>> initialized when it is declared with a bit of straightforward
>> refactoring -- icmp6_send has an easy split into 2.
>>
>> Seems like that is a more robust long term solution considering the
>> various init use cases.
> 
> The danger with initializers is it puts the burdon all over the tree.
> 
> So now there are many places that need to be audited when a new
> element is added.
> 
> So from my perspective, the bug is that the code in question did
> things by hand rather than using the helper function.
> 
> If everyone used the flow initializer helpers, the compiler would
> tell us every place that needs to be fixed up.
> 
> By doing initializers inline, this process of discovery is more
> difficult and error prone.

I see both sides and acknowledge the fact that we don't generate compile
errors any more when we add fields. OTOH we already have a lot of
places, probably even more, which don't use the initialization helper
functions as they just need to feed in a subset of the flowi4
information (memset and fl4.daddr = <>; and some other attributes is a
common case. Most of the time a new attribute that gets added to flow4i
only decreases the scope of the actual lookup, thus a missing attribute
doesn't necessarily indicate a bug.

In my local branch I adapted the changes from Julian so far. I will
revisit the initialization of flowi4, thanks for all the input so far.

Currently I have difficulties keeping user space behavior the same while
improving the ECN situation, as ToS simply specifies the use of the
second bit. I can't improve the ECN situation in a transparent way.

Bye,
Hannes