On 17 Mar 2022, at 15:01, Aaron Conole wrote:
> 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. 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... _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
