netdev - Re: [PATCH net-next 1/5] vxlan: implement GPE in L2 mode

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: <20160229112322.4da0f0c0@griffin>
Date:	Mon, 29 Feb 2016 11:23:22 +0100
From:	Jiri Benc <jbenc@...hat.com>
To:	Tom Herbert <tom@...bertland.com>
Cc:	Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 1/5] vxlan: implement GPE in L2 mode

On Sat, 27 Feb 2016 12:54:52 -0800, Tom Herbert wrote:
> Yes, but RCO has not been specified for VXLAN-GPE either

As far as I can see, RCO will just work with VXLAN-GPE. But I have no
problem disallowing them to be set together, if you prefer that.

> so the patch
> does not correctly refuse setting those two together. Inevitably
> though, those and other extensions will defined for VXLAN-GPE and new
> ones for VXLAN. Again, the protocols are fundamentally incompatible,
> so instead of trying to enforce each valid combination at
> configuration

We need to do the checking in either case. If we accepted unsupported
combinations and then just silently ignored them, we'd be in troubles
later when such combination becomes defined/supported. There would be
no way for the userspace tools to detect whether a particular kernel
supports the combination or not.

So, we need to check for supported combination of options during
configuration anyway.

And when we have that, I don't really see the reason for doing that
kind of code duplication that you suggest.

> or performing multiple checks for flavor each time we
> look at a packet, it seems easier to split the parsing with at most
> one check for the protocol variant. For instance in
> vxlan_udp_encap_recv just do:
> 
> if (vs->flags & VXLAN_F_GPE)
>                if (!vxlan_parse_gpe_hdr(&unparsed, skb, vs->flags))
>                        goto drop;
> else
>                if (!vxlan_parse_gpe(&unparsed, skb, vs->flags))
>                        goto drop;

Most of the code of these two functions will be identical. To
consolidate that as much as possible, you'll end up with what I have or
something very similar.

> And then move REMCSUM and GPB and other protocol specific checks to
> the right function.

And when RCO is defined for GPE, we copy the code? Doesn't make sense,
sorry.

If you look at the code in the current net-next (and the code after
this patchset), the extension handling has been made generic and each
extension gets its own handler function, leading to clean separation in
the code. There's no reason to split the vxlan_rcv into two functions
doing the same things but with slightly different calls to extensions.

 Jiri