diff mbox series

[net] openvswitch: always update flow key after NAT

Message ID 20220317121729.2810292-1-aconole@redhat.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net] openvswitch: always update flow key after NAT | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers fail 1 blamed authors not CCed: davem@davemloft.net; 3 maintainers not CCed: davem@davemloft.net kuba@kernel.org pabeni@redhat.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: line length of 88 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Aaron Conole March 17, 2022, 12:17 p.m. UTC 
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
commit 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 <dceara@redhat.com>
Cc: Numan Siddique <nusiddiq@redhat.com>
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 net/openvswitch/conntrack.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

Comments

Eelco Chaudron March 17, 2022, 1:42 p.m. UTC | #1 
On 17 Mar 2022, at 13:17, 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
> commit 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 <dceara@redhat.com>
> Cc: Numan Siddique <nusiddiq@redhat.com>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  net/openvswitch/conntrack.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index c07afff57dd3..461dbb3b7090 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -104,6 +104,10 @@ static bool labels_nonzero(const struct ovs_key_ct_labels *labels);
>
>  static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info);
>
> +static void ovs_nat_update_key(struct sw_flow_key *key,
> +			       const struct sk_buff *skb,
> +			       enum nf_nat_manip_type maniptype);
> +

nit?: Rather than adding this would it be simpler to swap the order of ovs_nat_update_key and ovs_ct_nat_execute?

Also, the function is defined by “#if IS_ENABLED(CONFIG_NF_NAT)” not sure if this will generate warnings if the function is not present?

>  static u16 key_to_nfproto(const struct sw_flow_key *key)
>  {
>  	switch (ntohs(key->eth.type)) {
> @@ -741,7 +745,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,6 +817,10 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
>  push:
>  	skb_push_rcsum(skb, nh_off);
>
> +	/* Update the flow key if NAT successful. */
> +	if (err == NF_ACCEPT)
> +		ovs_nat_update_key(key, skb, maniptype);
> +
>  	return err;
>  }
>
> @@ -906,7 +914,7 @@ static int ovs_ct_nat(struct net *net, struct sw_flow_key *key,
>  	} else {
>  		return NF_ACCEPT; /* Connection is not NATed. */
>  	}
> -	err = ovs_ct_nat_execute(skb, ct, ctinfo, &info->range, maniptype);
> +	err = ovs_ct_nat_execute(skb, ct, ctinfo, &info->range, maniptype, key);
>
>  	if (err == NF_ACCEPT && ct->status & IPS_DST_NAT) {
>  		if (ct->status & IPS_SRC_NAT) {
> @@ -916,17 +924,13 @@ static int ovs_ct_nat(struct net *net, struct sw_flow_key *key,
>  				maniptype = NF_NAT_MANIP_SRC;
>
>  			err = ovs_ct_nat_execute(skb, ct, ctinfo, &info->range,
> -						 maniptype);
> +						 maniptype, key);
>  		} else if (CTINFO2DIR(ctinfo) == IP_CT_DIR_ORIGINAL) {
>  			err = ovs_ct_nat_execute(skb, ct, ctinfo, NULL,
> -						 NF_NAT_MANIP_SRC);
> +						 NF_NAT_MANIP_SRC, key);
>  		}
>  	}
>
> -	/* Mark NAT done if successful and update the flow key. */
> -	if (err == NF_ACCEPT)
> -		ovs_nat_update_key(key, skb, maniptype);
> -
>  	return err;
>  }
>  #else /* !CONFIG_NF_NAT */

The rest of the patch looks fine to me...
Aaron Conole March 17, 2022, 2:01 p.m. UTC | #2 
Eelco Chaudron <echaudro@redhat.com> writes:

> On 17 Mar 2022, at 13:17, 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
>> commit 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 <dceara@redhat.com>
>> Cc: Numan Siddique <nusiddiq@redhat.com>
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>>  net/openvswitch/conntrack.c | 20 ++++++++++++--------
>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>> index c07afff57dd3..461dbb3b7090 100644
>> --- a/net/openvswitch/conntrack.c
>> +++ b/net/openvswitch/conntrack.c
>> @@ -104,6 +104,10 @@ static bool labels_nonzero(const struct ovs_key_ct_labels *labels);
>>
>>  static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info);
>>
>> +static void ovs_nat_update_key(struct sw_flow_key *key,
>> +			       const struct sk_buff *skb,
>> +			       enum nf_nat_manip_type maniptype);
>> +
>
> nit?: Rather than adding this would it be simpler to swap the order of
> ovs_nat_update_key and ovs_ct_nat_execute?

I thought it looked messier that way.

> Also, the function is defined by “#if IS_ENABLED(CONFIG_NF_NAT)” not
> sure if this will generate warnings if the function is not present?

Good catch, yes it will.  I will submit a v2 with this guard in place if
you think the foward decl is okay to keep.

>>  static u16 key_to_nfproto(const struct sw_flow_key *key)
>>  {
>>  	switch (ntohs(key->eth.type)) {
>> @@ -741,7 +745,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,6 +817,10 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
>>  push:
>>  	skb_push_rcsum(skb, nh_off);
>>
>> +	/* Update the flow key if NAT successful. */
>> +	if (err == NF_ACCEPT)
>> +		ovs_nat_update_key(key, skb, maniptype);
>> +
>>  	return err;
>>  }
>>
>> @@ -906,7 +914,7 @@ static int ovs_ct_nat(struct net *net, struct sw_flow_key *key,
>>  	} else {
>>  		return NF_ACCEPT; /* Connection is not NATed. */
>>  	}
>> -	err = ovs_ct_nat_execute(skb, ct, ctinfo, &info->range, maniptype);
>> +	err = ovs_ct_nat_execute(skb, ct, ctinfo, &info->range, maniptype, key);
>>
>>  	if (err == NF_ACCEPT && ct->status & IPS_DST_NAT) {
>>  		if (ct->status & IPS_SRC_NAT) {
>> @@ -916,17 +924,13 @@ static int ovs_ct_nat(struct net *net, struct sw_flow_key *key,
>>  				maniptype = NF_NAT_MANIP_SRC;
>>
>>  			err = ovs_ct_nat_execute(skb, ct, ctinfo, &info->range,
>> -						 maniptype);
>> +						 maniptype, key);
>>  		} else if (CTINFO2DIR(ctinfo) == IP_CT_DIR_ORIGINAL) {
>>  			err = ovs_ct_nat_execute(skb, ct, ctinfo, NULL,
>> -						 NF_NAT_MANIP_SRC);
>> +						 NF_NAT_MANIP_SRC, key);
>>  		}
>>  	}
>>
>> -	/* Mark NAT done if successful and update the flow key. */
>> -	if (err == NF_ACCEPT)
>> -		ovs_nat_update_key(key, skb, maniptype);
>> -
>>  	return err;
>>  }
>>  #else /* !CONFIG_NF_NAT */
>
> The rest of the patch looks fine to me...
Eelco Chaudron March 17, 2022, 2:30 p.m. UTC | #3 
On 17 Mar 2022, at 15:01, Aaron Conole wrote:

> Eelco Chaudron <echaudro@redhat.com> writes:
>
>> On 17 Mar 2022, at 13:17, 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
>>> commit 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 <dceara@redhat.com>
>>> Cc: Numan Siddique <nusiddiq@redhat.com>
>>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>>> ---
>>>  net/openvswitch/conntrack.c | 20 ++++++++++++--------
>>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>>> index c07afff57dd3..461dbb3b7090 100644
>>> --- a/net/openvswitch/conntrack.c
>>> +++ b/net/openvswitch/conntrack.c
>>> @@ -104,6 +104,10 @@ static bool labels_nonzero(const struct ovs_key_ct_labels *labels);
>>>
>>>  static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info);
>>>
>>> +static void ovs_nat_update_key(struct sw_flow_key *key,
>>> +			       const struct sk_buff *skb,
>>> +			       enum nf_nat_manip_type maniptype);
>>> +
>>
>> nit?: Rather than adding this would it be simpler to swap the order of
>> ovs_nat_update_key and ovs_ct_nat_execute?
>
> I thought it looked messier that way.
>
>> Also, the function is defined by “#if IS_ENABLED(CONFIG_NF_NAT)” not
>> sure if this will generate warnings if the function is not present?
>
> Good catch, yes it will.  I will submit a v2 with this guard in place if
> you think the foward decl is okay to keep.

I’m fine with either way, but I personally prefer the re-order as the code looks cleaner (but the patch doesn't ;).

>>>  static u16 key_to_nfproto(const struct sw_flow_key *key)
>>>  {
>>>  	switch (ntohs(key->eth.type)) {
>>> @@ -741,7 +745,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,6 +817,10 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
>>>  push:
>>>  	skb_push_rcsum(skb, nh_off);
>>>
>>> +	/* Update the flow key if NAT successful. */
>>> +	if (err == NF_ACCEPT)
>>> +		ovs_nat_update_key(key, skb, maniptype);
>>> +
>>>  	return err;
>>>  }
>>>
>>> @@ -906,7 +914,7 @@ static int ovs_ct_nat(struct net *net, struct sw_flow_key *key,
>>>  	} else {
>>>  		return NF_ACCEPT; /* Connection is not NATed. */
>>>  	}
>>> -	err = ovs_ct_nat_execute(skb, ct, ctinfo, &info->range, maniptype);
>>> +	err = ovs_ct_nat_execute(skb, ct, ctinfo, &info->range, maniptype, key);
>>>
>>>  	if (err == NF_ACCEPT && ct->status & IPS_DST_NAT) {
>>>  		if (ct->status & IPS_SRC_NAT) {
>>> @@ -916,17 +924,13 @@ static int ovs_ct_nat(struct net *net, struct sw_flow_key *key,
>>>  				maniptype = NF_NAT_MANIP_SRC;
>>>
>>>  			err = ovs_ct_nat_execute(skb, ct, ctinfo, &info->range,
>>> -						 maniptype);
>>> +						 maniptype, key);
>>>  		} else if (CTINFO2DIR(ctinfo) == IP_CT_DIR_ORIGINAL) {
>>>  			err = ovs_ct_nat_execute(skb, ct, ctinfo, NULL,
>>> -						 NF_NAT_MANIP_SRC);
>>> +						 NF_NAT_MANIP_SRC, key);
>>>  		}
>>>  	}
>>>
>>> -	/* Mark NAT done if successful and update the flow key. */
>>> -	if (err == NF_ACCEPT)
>>> -		ovs_nat_update_key(key, skb, maniptype);
>>> -
>>>  	return err;
>>>  }
>>>  #else /* !CONFIG_NF_NAT */
>>
>> The rest of the patch looks fine to me...
diff mbox series

Patch

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index c07afff57dd3..461dbb3b7090 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -104,6 +104,10 @@  static bool labels_nonzero(const struct ovs_key_ct_labels *labels);
 
 static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info);
 
+static void ovs_nat_update_key(struct sw_flow_key *key,
+			       const struct sk_buff *skb,
+			       enum nf_nat_manip_type maniptype);
+
 static u16 key_to_nfproto(const struct sw_flow_key *key)
 {
 	switch (ntohs(key->eth.type)) {
@@ -741,7 +745,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,6 +817,10 @@  static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
 push:
 	skb_push_rcsum(skb, nh_off);
 
+	/* Update the flow key if NAT successful. */
+	if (err == NF_ACCEPT)
+		ovs_nat_update_key(key, skb, maniptype);
+
 	return err;
 }
 
@@ -906,7 +914,7 @@  static int ovs_ct_nat(struct net *net, struct sw_flow_key *key,
 	} else {
 		return NF_ACCEPT; /* Connection is not NATed. */
 	}
-	err = ovs_ct_nat_execute(skb, ct, ctinfo, &info->range, maniptype);
+	err = ovs_ct_nat_execute(skb, ct, ctinfo, &info->range, maniptype, key);
 
 	if (err == NF_ACCEPT && ct->status & IPS_DST_NAT) {
 		if (ct->status & IPS_SRC_NAT) {
@@ -916,17 +924,13 @@  static int ovs_ct_nat(struct net *net, struct sw_flow_key *key,
 				maniptype = NF_NAT_MANIP_SRC;
 
 			err = ovs_ct_nat_execute(skb, ct, ctinfo, &info->range,
-						 maniptype);
+						 maniptype, key);
 		} else if (CTINFO2DIR(ctinfo) == IP_CT_DIR_ORIGINAL) {
 			err = ovs_ct_nat_execute(skb, ct, ctinfo, NULL,
-						 NF_NAT_MANIP_SRC);
+						 NF_NAT_MANIP_SRC, key);
 		}
 	}
 
-	/* Mark NAT done if successful and update the flow key. */
-	if (err == NF_ACCEPT)
-		ovs_nat_update_key(key, skb, maniptype);
-
 	return err;
 }
 #else /* !CONFIG_NF_NAT */