Eelco Chaudron <[email protected]> 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 <[email protected]>
>> Cc: Numan Siddique <[email protected]>
>> Signed-off-by: Aaron Conole <[email protected]>
>> ---
>> 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...
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
