netdev - Re: [PATCH nf-next v9 8/8] openvswitch: Interface with NAT.

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] [day] [month] [year] [list]

Message-Id: <D23E44DD-FA6D-4095-ABC8-CAE8E032BE7A@ovn.org>
Date:	Thu, 10 Mar 2016 10:55:53 -0800
From:	Jarno Rajahalme <jarno@....org>
To:	Joe Stringer <joe@....org>
Cc:	Thomas Graf <tgraf@...g.ch>, netfilter-devel@...r.kernel.org,
	netdev <netdev@...r.kernel.org>, ovs dev <dev@...nvswitch.org>
Subject: Re: [PATCH nf-next v9 8/8] openvswitch: Interface with NAT.

Thanks for the reviews Joe! Comments below.

> On Mar 9, 2016, at 7:47 PM, Joe Stringer <joe@....org> wrote:
> 
> Hi Jarno,
> 
> Thanks for working on this. Mostly just a few style things around #ifdefs below.
> 
> On 9 March 2016 at 15:10, Jarno Rajahalme <jarno@....org> wrote:
>> Extend OVS conntrack interface to cover NAT.  New nested
>> OVS_CT_ATTR_NAT attribute may be used to include NAT with a CT action.
>> A bare OVS_CT_ATTR_NAT only mangles existing and expected connections.
>> If OVS_NAT_ATTR_SRC or OVS_NAT_ATTR_DST is included within the nested
>> attributes, new (non-committed/non-confirmed) connections are mangled
>> according to the rest of the nested attributes.
>> 
>> The corresponding OVS userspace patch series includes test cases (in
>> tests/system-traffic.at) that also serve as example uses.
>> 
>> This work extends on a branch by Thomas Graf at
>> https://github.com/tgraf/ovs/tree/nat.
> 
> Thomas, I guess there was not signoff in these patches so Jarno does
> not have your signoff in this patch.
> 
>> Signed-off-by: Jarno Rajahalme <jarno@....org>
>> ---
>> v9: Fixed module dependencies.
>> 
>> include/uapi/linux/openvswitch.h |  49 ++++
>> net/openvswitch/Kconfig          |   3 +-
>> net/openvswitch/conntrack.c      | 523 +++++++++++++++++++++++++++++++++++++--
>> net/openvswitch/conntrack.h      |   3 +-
>> 4 files changed, 551 insertions(+), 27 deletions(-)
> 
> <snip>
> 
>> diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
>> index cd5fd9d..23471a4 100644
>> --- a/net/openvswitch/Kconfig
>> +++ b/net/openvswitch/Kconfig
>> @@ -6,7 +6,8 @@ config OPENVSWITCH
>>        tristate "Open vSwitch"
>>        depends on INET
>>        depends on !NF_CONNTRACK || \
>> -                  (NF_CONNTRACK && (!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6))
>> +                  (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \
>> +                                (!NF_NAT || NF_NAT)))
> 
> Whitespace.
> 

Fixed.

>>        select LIBCRC32C
>>        select MPLS
>>        select NET_MPLS_GSO
>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>> index 5711f80..6455237 100644
>> --- a/net/openvswitch/conntrack.c
>> +++ b/net/openvswitch/conntrack.c
> 
> <snip>
> 
>> struct ovs_ct_len_tbl {
>> -       size_t maxlen;
>> -       size_t minlen;
>> +       int maxlen;
>> +       int minlen;
>> };
> 
> Are these changed for a specific reason, or just to use INT_MAX rather
> than SIZE_MAX in ovs_ct_len_tbl?
> 

‘maxlen’ and ‘minlen’ are compared against the values returned by nla_len(), which returns an int:

net/netlink.h:static inline int nla_len(const struct nlattr *nla)

so I figured it is better to have these as ints, too.

>> /* Metadata mark for masked write to conntrack mark */
>> @@ -42,15 +52,29 @@ struct md_labels {
>>        struct ovs_key_ct_labels mask;
>> };
>> 
>> +#ifdef CONFIG_NF_NAT_NEEDED
>> +enum ovs_ct_nat {
>> +       OVS_CT_NAT = 1 << 0,     /* NAT for committed connections only. */
>> +       OVS_CT_SRC_NAT = 1 << 1, /* Source NAT for NEW connections. */
>> +       OVS_CT_DST_NAT = 1 << 2, /* Destination NAT for NEW connections. */
>> +};
>> +#endif
> 
> Here...
> 
>> /* Conntrack action context for execution. */
>> struct ovs_conntrack_info {
>>        struct nf_conntrack_helper *helper;
>>        struct nf_conntrack_zone zone;
>>        struct nf_conn *ct;
>>        u8 commit : 1;
>> +#ifdef CONFIG_NF_NAT_NEEDED
>> +       u8 nat : 3;                 /* enum ovs_ct_nat */
>> +#endif
> 
> and here.. I wonder if we can trim more of these #ifdefs, for
> readability and more compiler coverage if the feature is disabled.
> 

Trimmed this and other #ifdefs as you suggested, and it still compiles when NAT is disabled.

Just posted the v10, which I hope will be the final version :-)

  Jarno