On 3/10/22 04:04, [email protected] wrote:
> From: Numan Siddique <[email protected]>
>
Hi Numan,
> Presently, if the inport or outport is a peer port (of router port),
> then we skip sending the packet to conntrack by not setting the
> reg0[0]/reg0[1] bits. But the packet still goes through the
> stages - ls_in_acl_hint, ls_in_acl and ls_in_stateful in the ingress
> pipeline and similarly ls_out_acl_hint, ls_out_acl and ls_out_stateful
> in the egress pipeline of the logical switch.
>
> In these mentioned stages, the logical flows match on the ct states
> (ct.new, ct.est etc) and this can be problematic if the inport or
> outport is peer port. It is more problematic if the packet was
> sent to conntrack and committed in the ingress pipeline. When
> that packet enters the egress pipeline with outport set to the peer
> port, we skip sending the packet to conntrack (which is expected)
> but ct state values carry over from the ingress state. If the ct.new
> flag was set in the ingress pipeline, then the below flows are hit and
> the packet gets committed in the conntrack zone of the inport logical port.
>
> table=4 (ls_out_acl ), priority=1 ,
> match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))),
> action=(reg0[1] = 1; next;)
> table=7 (ls_out_stateful ), priority=100 ,
> match=(reg0[1] == 1 && reg0[13] == 0),
> action=(ct_commit { ct_label.blocked = 0; }; next;)
>
> With this extra commit to the same conntrack zone, sometimes the packet gets
> dropped or doesn't reach the destination. It is not very clear how
> the packet gets misplaced, but avoiding this resolves the issue.
> And OVN ideally should not do this extra commit.
>
> To address this issue, this patch adds the logical flows to skip all
> the conntrack related stages if the inport or outport is peer logical
> port.
>
> table=5 (ls_in_pre_acl ), priority=110 ,
> match=(ip && inport == "sw0-lr0"),
> action=(next(pipeline=ingress,table=18);)
>
> table=0 (ls_out_pre_lb ), priority=110 ,
> match=(ip && outport == "sw0-lr0"),
> action=(next(pipeline=egress,table=8);)
>
This is a bit worrying. We're skipping a lot of stages, some of which
are not stateful at all, e.g., "ls_in_qos_mark", "ls_in_qos_meter".
Also, I think we would be breaking the scenario in which LB happens on
traffic received from a logical router. Simplified setup with a client
connecting to VIP1:
client (IP1) --> LS1 --> LR (VIP1 in the subnet of LS2) --> LS2 (LB:
VIP1->IP1)
Without your patch the hairpin stage on LS2 would have performed the
DNAT and SNAT (for hairpin) properly. I'm not sure that still works
with your patch.
Wouldn't it be safer and simpler to change the 110-priority flows in
stages ls_in/out_pre_acl and ls_in/out_pre_lb to apply action="ct_clear;
next;" instead?
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2060688
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2062431
> Reported-by: Michael Washer <[email protected]>
> Signed-off-by: Numan Siddique <[email protected]>
> ---
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
