netdev - Re: [RFC v2 6/6] flow_dissector: Parse batman-adv unicast headers

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:   Wed, 6 Dec 2017 09:10:48 -0800
From:   Tom Herbert <tom@...bertland.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Sven Eckelmann <sven.eckelmann@...nmesh.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Jiri Pirko <jiri@...lanox.com>,
        Eric Dumazet <edumazet@...gle.com>,
        LKML <linux-kernel@...r.kernel.org>,
        b.a.t.m.a.n@...ts.open-mesh.org
Subject: Re: [RFC v2 6/6] flow_dissector: Parse batman-adv unicast headers

On Wed, Dec 6, 2017 at 8:54 AM, Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
> On Wed, Dec 6, 2017 at 5:26 AM, Sven Eckelmann
> <sven.eckelmann@...nmesh.com> wrote:
>> On Dienstag, 5. Dezember 2017 09:19:45 CET Tom Herbert wrote:
>> [...]
>>> Switch statements with cases having many LOC is hard to read and
>>> __skb_flow_dissect is aleady quite convoluted to begin with.
>>>
>>> I suggest putting this in a static function similar to how MPLS and
>>> GRE are handled.
>
> Perhaps it can even be behind a static key depending on whether any
> devices are active, adjusted in batadv_hardif_(en|dis)able_interface.
>
It's aready in a switch statement so static key shouldn't make a
difference. Also, we have made flow dissector operate indendently of
whether to end protocol is enable (for instance GRE is dissected
regarless of whether and GRE tunnels are configured).

Tom

>> Thanks for the feedback.
>>
>> I was not sure whether "inline" or an extra function would be preferred. I've
>> then decided to implement it like most of the other protocols. But since an
>> extra function is the preferred method for handling new protos, I will move it
>> to an extra function.
>>
>> The change can already be found in
>>  https://git.open-mesh.org/linux-merge.git/shortlog/refs/heads/ecsv/flowdissector
>>
>> I also saw that you've introduced in
>> commit 823b96939578 ("flow_dissector: Add control/reporting of encapsulation")
>> a flag to stop dissecting when something encapsulated was detected. It is not
>> 100% clear to me when the  FLOW_DIS_ENCAPSULATION should be set and
>> FLOW_DISSECTOR_F_STOP_AT_ENCAP be checked. Only when there is IP/eth in IP
>> (like in the two examples from the commit message) or also when there is a
>> ethernet header, followed by batman-adv unicast header and again followed by
>> an ethernet header?
>
> Please implement FLOW_DISSECTOR_F_STOP_AT_ENCAP. It may
> be used in more flow dissector paths in the future.
>
> The features are also used by GRE, which can encap Ethernet, for an example
> that is closer to this protocol.