linux-kernel - Re: [ovs-dev] [PATCH] openvswitch: call only into reachable nf-nat code

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:	Thu, 17 Mar 2016 15:54:34 +1300
From:	Joe Stringer <joe@....org>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	Pablo Neira Ayuso <pablo@...filter.org>,
	ovs dev <dev@...nvswitch.org>, netdev <netdev@...r.kernel.org>,
	linux-kernel@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [ovs-dev] [PATCH] openvswitch: call only into reachable nf-nat code

On 17 March 2016 at 02:56, Arnd Bergmann <arnd@...db.de> wrote:
> On Wednesday 16 March 2016 14:25:36 Pablo Neira Ayuso wrote:
>> Not related with this patch, just a side note/recommendation.
>>
>> I understand this code just got into tree, and that this needs a bit
>> work/iterations but this thing above is ugly, I wonder if there is a
>> better way to avoid this.
>>
>> Probably with some modularization of the openvswitch code this will
>> look better, I mean:
>>
>> 1) adding Kconfig switches to enable conntrack and NAT support to
>>    net/openvswitch/Kconfig.
>>
>> 2) Move the NAT code to the corresponding openvswitch/nat.c file.
>>
>> Just my two cents.
>
> Yes, I think that would be good too. I also found that the driver
> used to look like that but it was changed as part of f88f69dd17f1
> ("openvswitch: Remove conntrack Kconfig option.").

I don't see how it helps to separate the conntrack, nat pieces into
different Kconfig switches unless you're splitting their code
completely out of the openvswitch module.

Prior to f88f69dd17f1, OPENVSWITCH Kconfig option had no dependency on
NF_CONNTRACK. OPENVSWITCH_CONNTRACK was a switch to build those pieces
into the OPENVSWITCH module (ie,
"openvswitch-$(CONFIG_OPENVSWITCH_CONNTRACK) += conntrack.o"). If you
configured NF_CONNTRACK=m, OPENVSWITCH=y, OPENVSWITCH_CONNTRACK=y then
it would break in the same kind of way as the bug that Arnd is
reporting. So, that patch was introduced to shift the dependency up to
the OPENVSWITCH kconfig option. At that point, there was no strong
benefit to keeping the conntrack separately configurable, so that was
removed. Further splitting the code out in some way (eg new modules)
seemed way overkill to tidy up some ugly-looking dependency logic.

Maybe someone with better Kconfig-fu than me can point out a better way.