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?
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...
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
