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 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 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
>> ---
>> 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(-)
>
>
>
>> 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
>
>
>
>> 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
Re: [PATCH nf-next v9 8/8] openvswitch: Interface with NAT.
> On Mar 10, 2016, at 4:00 AM, Thomas Graf wrote: > > On 03/09/16 at 07:47pm, Joe Stringer wrote: >> On 9 March 2016 at 15:10, Jarno Rajahalme 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. > > That's fine. The code has evolved a lot since. I don't see anything > further than what Joe spotted so feel free to add my > > Acked-by: Thomas Graf Thanks!
Re: [PATCH nf-next v9 8/8] openvswitch: Interface with NAT.
On 03/09/16 at 07:47pm, Joe Stringer wrote: > On 9 March 2016 at 15:10, Jarno Rajahalme 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. That's fine. The code has evolved a lot since. I don't see anything further than what Joe spotted so feel free to add my Acked-by: Thomas Graf
Re: [PATCH nf-next v9 8/8] openvswitch: Interface with NAT.
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 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
> ---
> 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(-)
> 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.
> 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
> 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?
> /* 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.
> u16 family;
> struct md_mark mark;
> struct md_labels labels;
> +#ifdef CONFIG_NF_NAT_NEEDED
> + struct nf_nat_range range; /* Only present for SRC NAT and DST NAT.
> */
> +#endif
> };
(probably not this one, though)
> static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info);
> @@ -137,12 +161,15 @@ static void __ovs_ct_update_key(struct sw_flow_key
> *key, u8 state,
> ovs_ct_get_labels(ct, &key->ct.labels);
> }
>
> -/* Update 'key' based on skb->nfct. If 'post_ct' is true, then OVS has
> - * previously sent the packet to conntrack via the ct action.
> +/* Update 'key' based on skb->nfct. If 'post_ct' is true, then OVS has
> + * previously sent the packet to conntrack via the ct action. If
> + * 'keep_nat_flags' is true, the existing NAT flags retained, else they are
> + * initialized from the connection status.
> */
> static void ovs_ct_update_key(const struct sk_buff *skb,
> const struct ovs_conntrack_info *info,
> - struct sw_flow_key *key, bool post_ct)
> + struct sw_flow_key *key, bool post_ct,
> + bool keep_nat_flags)
> {
> const struct nf_conntrack_zone *zone = &nf_ct_zone_dflt;
> enum ip_conntrack_info ctinfo;
> @@ -160,6 +187,16 @@ static void ovs_ct_update_key(const struct sk_buff *skb,
> */
> if (ct->master)
> state |= OVS_CS_F_RELATED;
> +#ifdef CONFIG_NF_NAT_NEEDED
> + if (keep_nat_flags) {
> + state |= key->ct.state & OVS_CS_F_NAT_MASK;
> + } else {
> + if (ct->status & IPS_SRC_NAT)
> + state |= OVS_CS_F_SRC_NAT;
> + if (ct->stat
[PATCH nf-next v9 8/8] openvswitch: Interface with NAT.
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.
Signed-off-by: Jarno Rajahalme
---
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(-)
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index a27222d..616d047 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -454,6 +454,14 @@ struct ovs_key_ct_labels {
#define OVS_CS_F_REPLY_DIR 0x08 /* Flow is in the reply direction. */
#define OVS_CS_F_INVALID 0x10 /* Could not track connection. */
#define OVS_CS_F_TRACKED 0x20 /* Conntrack has occurred. */
+#define OVS_CS_F_SRC_NAT 0x40 /* Packet's source address/port was
+* mangled by NAT.
+*/
+#define OVS_CS_F_DST_NAT 0x80 /* Packet's destination address/port
+* was mangled by NAT.
+*/
+
+#define OVS_CS_F_NAT_MASK (OVS_CS_F_SRC_NAT | OVS_CS_F_DST_NAT)
/**
* enum ovs_flow_attr - attributes for %OVS_FLOW_* commands.
@@ -632,6 +640,8 @@ struct ovs_action_hash {
* mask. For each bit set in the mask, the corresponding bit in the value is
* copied to the connection tracking label field in the connection.
* @OVS_CT_ATTR_HELPER: variable length string defining conntrack ALG.
+ * @OVS_CT_ATTR_NAT: Nested OVS_NAT_ATTR_* for performing L3 network address
+ * translation (NAT) on the packet.
*/
enum ovs_ct_attr {
OVS_CT_ATTR_UNSPEC,
@@ -641,12 +651,51 @@ enum ovs_ct_attr {
OVS_CT_ATTR_LABELS, /* labels to associate with this connection. */
OVS_CT_ATTR_HELPER, /* netlink helper to assist detection of
related connections. */
+ OVS_CT_ATTR_NAT,/* Nested OVS_NAT_ATTR_* */
__OVS_CT_ATTR_MAX
};
#define OVS_CT_ATTR_MAX (__OVS_CT_ATTR_MAX - 1)
/**
+ * enum ovs_nat_attr - Attributes for %OVS_CT_ATTR_NAT.
+ *
+ * @OVS_NAT_ATTR_SRC: Flag for Source NAT (mangle source address/port).
+ * @OVS_NAT_ATTR_DST: Flag for Destination NAT (mangle destination
+ * address/port). Only one of (@OVS_NAT_ATTR_SRC, @OVS_NAT_ATTR_DST) may be
+ * specified. Effective only for packets for ct_state NEW connections.
+ * Packets of committed connections are mangled by the NAT action according to
+ * the committed NAT type regardless of the flags specified. As a corollary, a
+ * NAT action without a NAT type flag will only mangle packets of committed
+ * connections. The following NAT attributes only apply for NEW
+ * (non-committed) connections, and they may be included only when the CT
+ * action has the @OVS_CT_ATTR_COMMIT flag and either @OVS_NAT_ATTR_SRC or
+ * @OVS_NAT_ATTR_DST is also included.
+ * @OVS_NAT_ATTR_IP_MIN: struct in_addr or struct in6_addr
+ * @OVS_NAT_ATTR_IP_MAX: struct in_addr or struct in6_addr
+ * @OVS_NAT_ATTR_PROTO_MIN: u16 L4 protocol specific lower boundary (port)
+ * @OVS_NAT_ATTR_PROTO_MAX: u16 L4 protocol specific upper boundary (port)
+ * @OVS_NAT_ATTR_PERSISTENT: Flag for persistent IP mapping across reboots
+ * @OVS_NAT_ATTR_PROTO_HASH: Flag for pseudo random L4 port mapping (MD5)
+ * @OVS_NAT_ATTR_PROTO_RANDOM: Flag for fully randomized L4 port mapping
+ */
+enum ovs_nat_attr {
+ OVS_NAT_ATTR_UNSPEC,
+ OVS_NAT_ATTR_SRC,
+ OVS_NAT_ATTR_DST,
+ OVS_NAT_ATTR_IP_MIN,
+ OVS_NAT_ATTR_IP_MAX,
+ OVS_NAT_ATTR_PROTO_MIN,
+ OVS_NAT_ATTR_PROTO_MAX,
+ OVS_NAT_ATTR_PERSISTENT,
+ OVS_NAT_ATTR_PROTO_HASH,
+ OVS_NAT_ATTR_PROTO_RANDOM,
+ __OVS_NAT_ATTR_MAX,
+};
+
+#define OVS_NAT_ATTR_MAX (__OVS_NAT_ATTR_MAX - 1)
+
+/**
* enum ovs_action_attr - Action types.
*
* @OVS_ACTION_ATTR_OUTPUT: Output packet to port.
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_DEFRA
