[ovs-dev] [PATCH net v2] openvswitch: always update flow key after nat

2022-03-18 Thread Aaron Conole 
During NAT, a tuple collision may occur.  When this happens, openvswitch
will make a second pass through NAT which will perform additional packet
modification.  This will update the skb data, but not the flow key that
OVS uses.  This means that future flow lookups, and packet matches will
have incorrect data.  This has been supported since
5d50aa83e2c8 ("openvswitch: support asymmetric conntrack").

That commit failed to properly update the sw_flow_key attributes, since
it only called the ovs_ct_nat_update_key once, rather than each time
ovs_ct_nat_execute was called.  As these two operations are linked, the
ovs_ct_nat_execute() function should always make sure that the
sw_flow_key is updated after a successful call through NAT infrastructure.

Fixes: 5d50aa83e2c8 ("openvswitch: support asymmetric conntrack")
Cc: Dumitru Ceara 
Cc: Numan Siddique 
Signed-off-by: Aaron Conole 
---
v1->v2: removed forward decl., moved the ovs_nat_update_key function
made sure it compiles with NF_NAT disabled and enabled

 net/openvswitch/conntrack.c | 118 ++--
 1 file changed, 59 insertions(+), 59 deletions(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index c07afff57dd3..4a947c13c813 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -734,6 +734,57 @@ static bool skb_nfct_cached(struct net *net,
 }
 
 #if IS_ENABLED(CONFIG_NF_NAT)
+static void ovs_nat_update_key(struct sw_flow_key *key,
+  const struct sk_buff *skb,
+  enum nf_nat_manip_type maniptype)
+{
+   if (maniptype == NF_NAT_MANIP_SRC) {
+   __be16 src;
+
+   key->ct_state |= OVS_CS_F_SRC_NAT;
+   if (key->eth.type == htons(ETH_P_IP))
+   key->ipv4.addr.src = ip_hdr(skb)->saddr;
+   else if (key->eth.type == htons(ETH_P_IPV6))
+   memcpy(&key->ipv6.addr.src, &ipv6_hdr(skb)->saddr,
+  sizeof(key->ipv6.addr.src));
+   else
+   return;
+
+   if (key->ip.proto == IPPROTO_UDP)
+   src = udp_hdr(skb)->source;
+   else if (key->ip.proto == IPPROTO_TCP)
+   src = tcp_hdr(skb)->source;
+   else if (key->ip.proto == IPPROTO_SCTP)
+   src = sctp_hdr(skb)->source;
+   else
+   return;
+
+   key->tp.src = src;
+   } else {
+   __be16 dst;
+
+   key->ct_state |= OVS_CS_F_DST_NAT;
+   if (key->eth.type == htons(ETH_P_IP))
+   key->ipv4.addr.dst = ip_hdr(skb)->daddr;
+   else if (key->eth.type == htons(ETH_P_IPV6))
+   memcpy(&key->ipv6.addr.dst, &ipv6_hdr(skb)->daddr,
+  sizeof(key->ipv6.addr.dst));
+   else
+   return;
+
+   if (key->ip.proto == IPPROTO_UDP)
+   dst = udp_hdr(skb)->dest;
+   else if (key->ip.proto == IPPROTO_TCP)
+   dst = tcp_hdr(skb)->dest;
+   else if (key->ip.proto == IPPROTO_SCTP)
+   dst = sctp_hdr(skb)->dest;
+   else
+   return;
+
+   key->tp.dst = dst;
+   }
+}
+
 /* Modelled after nf_nat_ipv[46]_fn().
  * range is only used for new, uninitialized NAT state.
  * Returns either NF_ACCEPT or NF_DROP.
@@ -741,7 +792,7 @@ static bool skb_nfct_cached(struct net *net,
 static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
  enum ip_conntrack_info ctinfo,
  const struct nf_nat_range2 *range,
- enum nf_nat_manip_type maniptype)
+ enum nf_nat_manip_type maniptype, struct 
sw_flow_key *key)
 {
int hooknum, nh_off, err = NF_ACCEPT;
 
@@ -813,58 +864,11 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct 
nf_conn *ct,
 push:
skb_push_rcsum(skb, nh_off);
 
-   return err;
-}
-
-static void ovs_nat_update_key(struct sw_flow_key *key,
-  const struct sk_buff *skb,
-  enum nf_nat_manip_type maniptype)
-{
-   if (maniptype == NF_NAT_MANIP_SRC) {
-   __be16 src;
-
-   key->ct_state |= OVS_CS_F_SRC_NAT;
-   if (key->eth.type == htons(ETH_P_IP))
-   key->ipv4.addr.src = ip_hdr(skb)->saddr;
-   else if (key->eth.type == htons(ETH_P_IPV6))
-   memcpy(&key->ipv6.addr.src, &ipv6_hdr(skb)->saddr,
-  sizeof(key->ipv6.addr.src));
-   else
-   return;
-
-   if (key->ip.proto == IPPROTO_UDP)
-   src = udp_hdr(skb)->source;
-   else if (key->ip.proto == IP

Re: [ovs-dev] [PATCH net v2] openvswitch: always update flow key after nat

2022-03-18 Thread Eelco Chaudron 



On 18 Mar 2022, at 13:43, Aaron Conole wrote:

> During NAT, a tuple collision may occur.  When this happens, openvswitch
> will make a second pass through NAT which will perform additional packet
> modification.  This will update the skb data, but not the flow key that
> OVS uses.  This means that future flow lookups, and packet matches will
> have incorrect data.  This has been supported since
> 5d50aa83e2c8 ("openvswitch: support asymmetric conntrack").
>
> That commit failed to properly update the sw_flow_key attributes, since
> it only called the ovs_ct_nat_update_key once, rather than each time
> ovs_ct_nat_execute was called.  As these two operations are linked, the
> ovs_ct_nat_execute() function should always make sure that the
> sw_flow_key is updated after a successful call through NAT infrastructure.
>
> Fixes: 5d50aa83e2c8 ("openvswitch: support asymmetric conntrack")
> Cc: Dumitru Ceara 
> Cc: Numan Siddique 
> Signed-off-by: Aaron Conole 

You were right about the diff, it really looks messy and I had to apply it to 
review it :)

The patch looks fine to me!!

Acked-by: Eelco Chaudron 

> ---
> v1->v2: removed forward decl., moved the ovs_nat_update_key function
> made sure it compiles with NF_NAT disabled and enabled
>
>  net/openvswitch/conntrack.c | 118 ++--
>  1 file changed, 59 insertions(+), 59 deletions(-)
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index c07afff57dd3..4a947c13c813 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -734,6 +734,57 @@ static bool skb_nfct_cached(struct net *net,
>  }
>
>  #if IS_ENABLED(CONFIG_NF_NAT)
> +static void ovs_nat_update_key(struct sw_flow_key *key,
> +const struct sk_buff *skb,
> +enum nf_nat_manip_type maniptype)
> +{
> + if (maniptype == NF_NAT_MANIP_SRC) {
> + __be16 src;
> +
> + key->ct_state |= OVS_CS_F_SRC_NAT;
> + if (key->eth.type == htons(ETH_P_IP))
> + key->ipv4.addr.src = ip_hdr(skb)->saddr;
> + else if (key->eth.type == htons(ETH_P_IPV6))
> + memcpy(&key->ipv6.addr.src, &ipv6_hdr(skb)->saddr,
> +sizeof(key->ipv6.addr.src));
> + else
> + return;
> +
> + if (key->ip.proto == IPPROTO_UDP)
> + src = udp_hdr(skb)->source;
> + else if (key->ip.proto == IPPROTO_TCP)
> + src = tcp_hdr(skb)->source;
> + else if (key->ip.proto == IPPROTO_SCTP)
> + src = sctp_hdr(skb)->source;
> + else
> + return;
> +
> + key->tp.src = src;
> + } else {
> + __be16 dst;
> +
> + key->ct_state |= OVS_CS_F_DST_NAT;
> + if (key->eth.type == htons(ETH_P_IP))
> + key->ipv4.addr.dst = ip_hdr(skb)->daddr;
> + else if (key->eth.type == htons(ETH_P_IPV6))
> + memcpy(&key->ipv6.addr.dst, &ipv6_hdr(skb)->daddr,
> +sizeof(key->ipv6.addr.dst));
> + else
> + return;
> +
> + if (key->ip.proto == IPPROTO_UDP)
> + dst = udp_hdr(skb)->dest;
> + else if (key->ip.proto == IPPROTO_TCP)
> + dst = tcp_hdr(skb)->dest;
> + else if (key->ip.proto == IPPROTO_SCTP)
> + dst = sctp_hdr(skb)->dest;
> + else
> + return;
> +
> + key->tp.dst = dst;
> + }
> +}
> +
>  /* Modelled after nf_nat_ipv[46]_fn().
>   * range is only used for new, uninitialized NAT state.
>   * Returns either NF_ACCEPT or NF_DROP.
> @@ -741,7 +792,7 @@ static bool skb_nfct_cached(struct net *net,
>  static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
> enum ip_conntrack_info ctinfo,
> const struct nf_nat_range2 *range,
> -   enum nf_nat_manip_type maniptype)
> +   enum nf_nat_manip_type maniptype, struct 
> sw_flow_key *key)
>  {
>   int hooknum, nh_off, err = NF_ACCEPT;
>
> @@ -813,58 +864,11 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, 
> struct nf_conn *ct,
>  push:
>   skb_push_rcsum(skb, nh_off);
>
> - return err;
> -}
> -
> -static void ovs_nat_update_key(struct sw_flow_key *key,
> -const struct sk_buff *skb,
> -enum nf_nat_manip_type maniptype)
> -{
> - if (maniptype == NF_NAT_MANIP_SRC) {
> - __be16 src;
> -
> - key->ct_state |= OVS_CS_F_SRC_NAT;
> - if (key->eth.type == htons(ETH_P_IP))
> - key->ipv4.addr.src = ip_hdr(skb)->saddr;
> - else if (key->eth.type == htons(ETH_P_IPV6))
> - mem

Re: [ovs-dev] [PATCH net v2] openvswitch: always update flow key after nat

2022-03-21 Thread patchwork-bot+netdevbpf 
Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski :

On Fri, 18 Mar 2022 08:43:19 -0400 you wrote:
> During NAT, a tuple collision may occur.  When this happens, openvswitch
> will make a second pass through NAT which will perform additional packet
> modification.  This will update the skb data, but not the flow key that
> OVS uses.  This means that future flow lookups, and packet matches will
> have incorrect data.  This has been supported since
> 5d50aa83e2c8 ("openvswitch: support asymmetric conntrack").
> 
> [...]

Here is the summary with links:
  - [net,v2] openvswitch: always update flow key after nat
https://git.kernel.org/netdev/net/c/60b44ca6bd75

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev