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

[Aaron Conole <aconole@redhat.com>]

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...