On Wed, Jul 23, 2025 at 6:23 AM Mark Michelson <mmich...@redhat.com> wrote:
> Hello Han,
>
> If callers of the ct_*() actions now need to do their own ct state
> filtering beforehand, do we really need two separate OF flows that check
> for ct.new and ct.est? Could we not just install a single OF flow that
> matches on ct.trk?
Hi Mark,
I thought the same way, but it turned out that the ct state is a must have
as prerequisite.
Thanks,
Han
>
>
> On 7/16/25 3:07 AM, Han Zhou wrote:
> > The ct_nw_dst(), ct_ip6_dst(), and ct_tp_dst() logical actions are
> > implemented by jumping to physical flows in tables 81, 82, and 83
> > respectively. These physical flows previously only matched on
> > established connections (ct.trk && ct.est), limiting their use to
> > scenarios where connections were already established.
> >
> > Extend the physical flow matches to support both established and new
> > connection states by changing from:
> > match = ct.trk && ct.est && ip4/ip6
> > to:
> > match = ct.trk && (ct.est || ct.new) && ip4/ip6
> >
> > This allows the ct_xxx actions to work with new connections, while
> > maintaining compatibility with existing established connection use
> > cases.
> >
> > The responsibility for matching specific connection states (e.g., ct.est
> > for fragmented packet handling) is moved to the logical flows in
> > northd.c, providing more flexibility in how these actions are used
> > across different scenarios.
> >
> > Signed-off-by: Han Zhou <hz...@ovn.org>
> > ---
> > controller/physical.c | 38 +++++++++++++++++++++++++++++++-------
> > northd/northd.c | 6 ++++--
> > tests/ovn-northd.at | 24 ++++++++++++------------
> > 3 files changed, 47 insertions(+), 21 deletions(-)
> >
> > diff --git a/controller/physical.c b/controller/physical.c
> > index 1cc4173b2b6b..7b9b66ffe7f4 100644
> > --- a/controller/physical.c
> > +++ b/controller/physical.c
> > @@ -3033,24 +3033,35 @@ physical_run(struct physical_ctx *p_ctx,
> > add_default_drop_flow(p_ctx, OFTABLE_LOG_TO_PHY, flow_table);
> >
> > /* Table 81, 82 and 83
> > - * Match on ct.trk and ct.est and store the ct_nw_dst, ct_ip6_dst
> and
> > - * ct_tp_dst in the registers. */
> > - uint32_t ct_state = OVS_CS_F_TRACKED | OVS_CS_F_ESTABLISHED;
> > + * Match on ct.trk and ct.est | ct.new and store the ct_nw_dst,
> ct_ip6_dst
> > + * and ct_tp_dst in the registers. */
> > + uint32_t ct_state_est = OVS_CS_F_TRACKED | OVS_CS_F_ESTABLISHED;
> > + uint32_t ct_state_new = OVS_CS_F_TRACKED | OVS_CS_F_NEW;
> > + struct match match_new = MATCH_CATCHALL_INITIALIZER;
> > match_init_catchall(&match);
> > ofpbuf_clear(&ofpacts);
> >
> > - /* Add the flow:
> > + /* Add the flows:
> > * match = (ct.trk && ct.est), action = (reg8 = ct_tp_dst)
> > * table = 83
> > + * match = (ct.trk && ct.new), action = (reg8 = ct_tp_dst)
> > + * table = 83
> > */
> > - match_set_ct_state_masked(&match, ct_state, ct_state);
> > + match_set_ct_state_masked(&match, ct_state_est, ct_state_est);
> > put_move(MFF_CT_TP_DST, 0, MFF_LOG_CT_ORIG_TP_DST_PORT, 0, 16,
> &ofpacts);
> > ofctrl_add_flow(flow_table, OFTABLE_CT_ORIG_TP_DST_LOAD, 100, 0,
> &match,
> > &ofpacts, hc_uuid);
> >
> > - /* Add the flow:
> > + match_set_ct_state_masked(&match_new, ct_state_new, ct_state_new);
> > + put_move(MFF_CT_TP_DST, 0, MFF_LOG_CT_ORIG_TP_DST_PORT, 0, 16,
> &ofpacts);
> > + ofctrl_add_flow(flow_table, OFTABLE_CT_ORIG_TP_DST_LOAD, 100, 0,
> > + &match_new, &ofpacts, hc_uuid);
> > +
> > + /* Add the flows:
> > * match = (ct.trk && ct.est && ip4), action = (reg4 = ct_nw_dst)
> > * table = 81
> > + * match = (ct.trk && ct.new && ip4), action = (reg4 = ct_nw_dst)
> > + * table = 81
> > */
> > ofpbuf_clear(&ofpacts);
> > match_set_dl_type(&match, htons(ETH_TYPE_IP));
> > @@ -3058,9 +3069,16 @@ physical_run(struct physical_ctx *p_ctx,
> > ofctrl_add_flow(flow_table, OFTABLE_CT_ORIG_NW_DST_LOAD, 100, 0,
> &match,
> > &ofpacts, hc_uuid);
> >
> > - /* Add the flow:
> > + match_set_dl_type(&match_new, htons(ETH_TYPE_IP));
> > + put_move(MFF_CT_NW_DST, 0, MFF_LOG_CT_ORIG_NW_DST_ADDR, 0, 32,
> &ofpacts);
> > + ofctrl_add_flow(flow_table, OFTABLE_CT_ORIG_NW_DST_LOAD, 100, 0,
> > + &match_new, &ofpacts, hc_uuid);
> > +
> > + /* Add the flows:
> > * match = (ct.trk && ct.est && ip6), action = (xxreg0 =
> ct_ip6_dst)
> > * table = 82
> > + * match = (ct.trk && ct.new && ip6), action = (xxreg0 =
> ct_ip6_dst)
> > + * table = 82
> > */
> > ofpbuf_clear(&ofpacts);
> > match_set_dl_type(&match, htons(ETH_TYPE_IPV6));
> > @@ -3069,6 +3087,12 @@ physical_run(struct physical_ctx *p_ctx,
> > ofctrl_add_flow(flow_table, OFTABLE_CT_ORIG_IP6_DST_LOAD, 100, 0,
> &match,
> > &ofpacts, hc_uuid);
> >
> > + match_set_dl_type(&match_new, htons(ETH_TYPE_IPV6));
> > + put_move(MFF_CT_IPV6_DST, 0, MFF_LOG_CT_ORIG_IP6_DST_ADDR, 0,
> > + 128, &ofpacts);
> > + ofctrl_add_flow(flow_table, OFTABLE_CT_ORIG_IP6_DST_LOAD, 100, 0,
> > + &match_new, &ofpacts, hc_uuid);
> > +
> > /* Implement the ct_state_save() logical action. */
> >
> > /* Add the flow:
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 9b21786fad05..b46895bfafbb 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -8731,12 +8731,14 @@ build_lb_hairpin(const struct ls_stateful_record
> *ls_stateful_rec,
> > * We need to find a better way to handle the fragmented
> packets.
> > * */
> > ovn_lflow_add(lflows, od, S_SWITCH_IN_LB, 110,
> > - "ct.trk && !ct.rpl && "REGBIT_IP_FRAG" == 1 &&
> ip4",
> > + "ct.trk && ct.est && !ct.rpl && "REGBIT_IP_FRAG
> > + " == 1 && ip4",
> > REG_LB_IPV4 " = ct_nw_dst(); "
> > REG_LB_PORT " = ct_tp_dst(); next;",
> > lflow_ref);
> > ovn_lflow_add(lflows, od, S_SWITCH_IN_LB, 110,
> > - "ct.trk && !ct.rpl && "REGBIT_IP_FRAG" == 1 &&
> ip6",
> > + "ct.trk && ct.est && !ct.rpl && "REGBIT_IP_FRAG
> > + " == 1 && ip6",
> > REG_LB_IPV6 " = ct_ip6_dst(); "
> > REG_LB_PORT " = ct_tp_dst(); next;",
> > lflow_ref);
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 5e0747fcc816..3a353368a92f 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -4789,8 +4789,8 @@ check_stateful_flows() {
> >
> > AT_CHECK([grep "ls_in_lb " sw0flows | ovn_strip_lflows], [0], [dnl
> > table=??(ls_in_lb ), priority=0 , match=(1),
> action=(next;)
> > - table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> !ct.rpl && reg0[[19]] == 1 && ip4), action=(reg4 = ct_nw_dst();
> reg2[[0..15]] = ct_tp_dst(); next;)
> > - table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> !ct.rpl && reg0[[19]] == 1 && ip6), action=(xxreg1 = ct_ip6_dst();
> reg2[[0..15]] = ct_tp_dst(); next;)
> > + table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> ct.est && !ct.rpl && reg0[[19]] == 1 && ip4), action=(reg4 = ct_nw_dst();
> reg2[[0..15]] = ct_tp_dst(); next;)
> > + table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> ct.est && !ct.rpl && reg0[[19]] == 1 && ip6), action=(xxreg1 =
> ct_ip6_dst(); reg2[[0..15]] = ct_tp_dst(); next;)
> > table=??(ls_in_lb ), priority=120 , match=(ct.new &&
> ip4.dst == 10.0.0.10 && ct_tcp.dst == 80), action=(reg4 = 10.0.0.10;
> reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.4:8080);)
> > table=??(ls_in_lb ), priority=120 , match=(ct.new &&
> ip4.dst == 10.0.0.20 && ct_tcp.dst == 80), action=(reg4 = 10.0.0.20;
> reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.40:8080);)
> > ])
> > @@ -4897,8 +4897,8 @@ check ovn-nbctl --wait=sb ls-lb-add sw0 lb1
> > AT_CHECK([ovn-sbctl dump-flows sw0 | grep "ls_in_lb " |
> ovn_strip_lflows ], [0], [dnl
> > table=??(ls_in_lb ), priority=0 , match=(1),
> action=(next;)
> > table=??(ls_in_lb ), priority=110 , match=(ct.new &&
> ip4.dst == 10.0.0.20), action=(drop;)
> > - table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> !ct.rpl && reg0[[19]] == 1 && ip4), action=(reg4 = ct_nw_dst();
> reg2[[0..15]] = ct_tp_dst(); next;)
> > - table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> !ct.rpl && reg0[[19]] == 1 && ip6), action=(xxreg1 = ct_ip6_dst();
> reg2[[0..15]] = ct_tp_dst(); next;)
> > + table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> ct.est && !ct.rpl && reg0[[19]] == 1 && ip4), action=(reg4 = ct_nw_dst();
> reg2[[0..15]] = ct_tp_dst(); next;)
> > + table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> ct.est && !ct.rpl && reg0[[19]] == 1 && ip6), action=(xxreg1 =
> ct_ip6_dst(); reg2[[0..15]] = ct_tp_dst(); next;)
> > ])
> >
> > AT_CLEANUP
> > @@ -8149,8 +8149,8 @@ AT_CHECK([grep -e "ls_in_acl.*eval" -e
> "ls_in_acl_hint" lsflows | ovn_strip_lflo
> > AT_CHECK([grep -e "ls_in_lb " lsflows | ovn_strip_lflows], [0], [dnl
> > table=??(ls_in_lb ), priority=0 , match=(1),
> action=(next;)
> > table=??(ls_in_lb ), priority=110 , match=(ct.new &&
> ip4.dst == 10.0.0.2), action=(reg4 = 10.0.0.2;
> ct_lb_mark(backends=10.0.0.10);)
> > - table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> !ct.rpl && reg0[[19]] == 1 && ip4), action=(reg4 = ct_nw_dst();
> reg2[[0..15]] = ct_tp_dst(); next;)
> > - table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> !ct.rpl && reg0[[19]] == 1 && ip6), action=(xxreg1 = ct_ip6_dst();
> reg2[[0..15]] = ct_tp_dst(); next;)
> > + table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> ct.est && !ct.rpl && reg0[[19]] == 1 && ip4), action=(reg4 = ct_nw_dst();
> reg2[[0..15]] = ct_tp_dst(); next;)
> > + table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> ct.est && !ct.rpl && reg0[[19]] == 1 && ip6), action=(xxreg1 =
> ct_ip6_dst(); reg2[[0..15]] = ct_tp_dst(); next;)
> > ])
> >
> > AT_CHECK([grep -e "ls_in_stateful" lsflows | ovn_strip_lflows], [0],
> [dnl
> > @@ -8208,8 +8208,8 @@ AT_CHECK([grep -e "ls_in_acl.*eval" -e
> "ls_in_acl_hint" lsflows | ovn_strip_lflo
> > AT_CHECK([grep -e "ls_in_lb " lsflows | ovn_strip_lflows], [0], [dnl
> > table=??(ls_in_lb ), priority=0 , match=(1),
> action=(next;)
> > table=??(ls_in_lb ), priority=110 , match=(ct.new &&
> ip4.dst == 10.0.0.2), action=(reg4 = 10.0.0.2;
> ct_lb_mark(backends=10.0.0.10);)
> > - table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> !ct.rpl && reg0[[19]] == 1 && ip4), action=(reg4 = ct_nw_dst();
> reg2[[0..15]] = ct_tp_dst(); next;)
> > - table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> !ct.rpl && reg0[[19]] == 1 && ip6), action=(xxreg1 = ct_ip6_dst();
> reg2[[0..15]] = ct_tp_dst(); next;)
> > + table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> ct.est && !ct.rpl && reg0[[19]] == 1 && ip4), action=(reg4 = ct_nw_dst();
> reg2[[0..15]] = ct_tp_dst(); next;)
> > + table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> ct.est && !ct.rpl && reg0[[19]] == 1 && ip6), action=(xxreg1 =
> ct_ip6_dst(); reg2[[0..15]] = ct_tp_dst(); next;)
> > ])
> >
> > AT_CHECK([grep -e "ls_in_stateful" lsflows | ovn_strip_lflows], [0],
> [dnl
> > @@ -8267,8 +8267,8 @@ AT_CHECK([grep -e "ls_in_acl.*eval" -e
> "ls_in_acl_hint" lsflows | ovn_strip_lflo
> > AT_CHECK([grep -e "ls_in_lb " lsflows | ovn_strip_lflows], [0], [dnl
> > table=??(ls_in_lb ), priority=0 , match=(1),
> action=(next;)
> > table=??(ls_in_lb ), priority=110 , match=(ct.new &&
> ip4.dst == 10.0.0.2), action=(reg4 = 10.0.0.2;
> ct_lb_mark(backends=10.0.0.10);)
> > - table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> !ct.rpl && reg0[[19]] == 1 && ip4), action=(reg4 = ct_nw_dst();
> reg2[[0..15]] = ct_tp_dst(); next;)
> > - table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> !ct.rpl && reg0[[19]] == 1 && ip6), action=(xxreg1 = ct_ip6_dst();
> reg2[[0..15]] = ct_tp_dst(); next;)
> > + table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> ct.est && !ct.rpl && reg0[[19]] == 1 && ip4), action=(reg4 = ct_nw_dst();
> reg2[[0..15]] = ct_tp_dst(); next;)
> > + table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> ct.est && !ct.rpl && reg0[[19]] == 1 && ip6), action=(xxreg1 =
> ct_ip6_dst(); reg2[[0..15]] = ct_tp_dst(); next;)
> > ])
> >
> > AT_CHECK([grep -e "ls_in_stateful" lsflows | ovn_strip_lflows], [0],
> [dnl
> > @@ -9800,8 +9800,8 @@ AT_CHECK([grep "ls_in_lb_aff_check" S0flows |
> ovn_strip_lflows], [0], [dnl
> > ])
> > AT_CHECK([grep "ls_in_lb " S0flows | ovn_strip_lflows], [0], [dnl
> > table=??(ls_in_lb ), priority=0 , match=(1),
> action=(next;)
> > - table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> !ct.rpl && reg0[[19]] == 1 && ip4), action=(reg4 = ct_nw_dst();
> reg2[[0..15]] = ct_tp_dst(); next;)
> > - table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> !ct.rpl && reg0[[19]] == 1 && ip6), action=(xxreg1 = ct_ip6_dst();
> reg2[[0..15]] = ct_tp_dst(); next;)
> > + table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> ct.est && !ct.rpl && reg0[[19]] == 1 && ip4), action=(reg4 = ct_nw_dst();
> reg2[[0..15]] = ct_tp_dst(); next;)
> > + table=??(ls_in_lb ), priority=110 , match=(ct.trk &&
> ct.est && !ct.rpl && reg0[[19]] == 1 && ip6), action=(xxreg1 =
> ct_ip6_dst(); reg2[[0..15]] = ct_tp_dst(); next;)
> > table=??(ls_in_lb ), priority=120 , match=(ct.new &&
> ip4.dst == 172.16.0.10 && ct_tcp.dst == 80), action=(reg4 = 172.16.0.10;
> reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80);)
> > table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1
> && ct.new && ip4.dst == 172.16.0.10 && reg4 == 10.0.0.2 && reg2[[0..15]]
> == 80), action=(reg4 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=
> 10.0.0.2:80);)
> > table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1
> && ct.new && ip4.dst == 172.16.0.10 && reg4 == 20.0.0.2 && reg2[[0..15]]
> == 80), action=(reg4 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=
> 20.0.0.2:80);)
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
