Hi Ales Thanks for the patch The patch itself looks good to me, but I find the commit message difficult to read.
With that fixed, Acked-by: Xavier Simonart <[email protected]> On Thu, Nov 13, 2025 at 8:50 AM Ales Musil via dev <[email protected]> wrote: > Do not use ckeck_pkt_larger action on ARP packets. There are two main > reasons why that should be the case. The ARP packet has defined size, > even if the check_pkt_larger, and it wouldn't make sense to use, > something smaller than 68 bytes, for the check_pkt_larger, as every > other traffic would be rejected too. The other reason is that ARP > doesn't carry any IP header, so we cannot reply with ICMP anyway > in of the check_pkt_larger being true for the ARP packet. > Would something like this not be easier to read ? Do not use "ckeck_pkt_larger" action on ARP packets. There are two main reasons for this: - The ARP packet has a defined size. It would not make sense to use a size smaller than 68 bytes for the check_pkt_larger action, as all other traffic would be rejected as well. - ARP does not carry an IP header. Therefore, we cannot reply with an ICMP message, even if the check_pkt_larger condition was true for the ARP packet. WDYT? > Reported-at: https://issues.redhat.com/browse/FDP-1909 > Signed-off-by: Ales Musil <[email protected]> > --- > northd/northd.c | 19 ++++++++++++------- > tests/ovn-northd.at | 25 ++++++++++++++----------- > 2 files changed, 26 insertions(+), 18 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index cdf12ec86..e978b66c2 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -13853,17 +13853,22 @@ build_gateway_mtu_flow(struct lflow_table > *lflows, struct ovn_port *op, > hint, lflow_ref); > > if (gw_mtu > 0) { > + ds_clear(actions); > + ds_put_format_valist(actions, extra_actions_fmt, > + extra_actions_args); > + ds_put_cstr(match, " && (arp"); > + > const char *gw_mtu_bypass = smap_get(&op->nbrp->options, > "gateway_mtu_bypass"); > if (gw_mtu_bypass) { > - ds_clear(actions); > - ds_put_format_valist(actions, extra_actions_fmt, > - extra_actions_args); > - ds_put_format(match, " && (%s)", gw_mtu_bypass); > - ovn_lflow_add_with_hint(lflows, op->od, stage, prio_high, > - ds_cstr(match), ds_cstr(actions), > - hint, lflow_ref); > + ds_put_format(match, " || %s", gw_mtu_bypass); > } > + > + ds_put_char(match, ')'); > + > + ovn_lflow_add_with_hint(lflows, op->od, stage, prio_high, > + ds_cstr(match), ds_cstr(actions), > + hint, lflow_ref); > } > va_end(extra_actions_args); > } > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 448bc66ae..0eeb2fea8 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -6724,6 +6724,7 @@ fi > AT_CHECK_UNQUOTED([grep -e "chk_pkt_len" -e "lr_in_larger_pkts" lr0flows > | ovn_strip_lflows], [0], [dnl > table=??(lr_in_chk_pkt_len ), priority=0 , match=(1), action=(next;) > table=??(lr_in_chk_pkt_len ), priority=50 , match=(outport == > "lr0-public"), action=(reg9[[1]] = check_pkt_larger(1514); $ct_save_action;) > + table=??(lr_in_chk_pkt_len ), priority=55 , match=(outport == > "lr0-public" && (arp)), action=($ct_save_action;) > table=??(lr_in_larger_pkts ), priority=0 , match=(1), action=(next;) > table=??(lr_in_larger_pkts ), priority=150 , match=(inport == > "lr0-sw0" && outport == "lr0-public" && ip4 && reg9[[1]] && reg9[[0]] == > 0), action=(icmp4_error {reg9[[0]] = 1; reg9[[1]] = 0; eth.dst = > 00:00:00:00:ff:01; ip4.dst = ip4.src; ip4.src = 10.0.0.1; ip.ttl = 255; > icmp4.type = 3; /* Destination Unreachable. */ icmp4.code = 4; /* Frag > Needed and DF was Set. */ icmp4.frag_mtu = 1500; next(pipeline=ingress, > table=??); };) > table=??(lr_in_larger_pkts ), priority=150 , match=(inport == > "lr0-sw0" && outport == "lr0-public" && ip6 && reg9[[1]] && reg9[[0]] == > 0), action=(icmp6_error {reg9[[0]] = 1; reg9[[1]] = 0; eth.dst = > 00:00:00:00:ff:01; ip6.dst = ip6.src; ip6.src = fe80::200:ff:fe00:ff01; > ip.ttl = 255; icmp6.type = 2; /* Packet Too Big. */ icmp6.code = 0; > icmp6.frag_mtu = 1500; next(pipeline=ingress, table=??); };) > @@ -6763,6 +6764,7 @@ AT_CAPTURE_FILE([lr0flows]) > AT_CHECK_UNQUOTED([grep -e "chk_pkt_len" -e "lr_in_larger_pkts" lr0flows > | ovn_strip_lflows], [0], [dnl > table=??(lr_in_chk_pkt_len ), priority=0 , match=(1), action=(next;) > table=??(lr_in_chk_pkt_len ), priority=50 , match=(outport == > "lr0-public"), action=(reg9[[1]] = check_pkt_larger(1514); $ct_save_action;) > + table=??(lr_in_chk_pkt_len ), priority=55 , match=(outport == > "lr0-public" && (arp)), action=($ct_save_action;) > table=??(lr_in_larger_pkts ), priority=0 , match=(1), action=(next;) > table=??(lr_in_larger_pkts ), priority=150 , match=(inport == > "lr0-sw0" && outport == "lr0-public" && ip4 && reg9[[1]] && reg9[[0]] == > 0), action=(icmp4_error {reg9[[0]] = 1; reg9[[1]] = 0; eth.dst = > 00:00:00:00:ff:01; ip4.dst = ip4.src; ip4.src = 10.0.0.1; ip.ttl = 255; > icmp4.type = 3; /* Destination Unreachable. */ icmp4.code = 4; /* Frag > Needed and DF was Set. */ icmp4.frag_mtu = 1500; next(pipeline=ingress, > table=??); };) > table=??(lr_in_larger_pkts ), priority=150 , match=(inport == > "lr0-sw0" && outport == "lr0-public" && ip6 && reg9[[1]] && reg9[[0]] == > 0), action=(icmp6_error {reg9[[0]] = 1; reg9[[1]] = 0; eth.dst = > 00:00:00:00:ff:01; ip6.dst = ip6.src; ip6.src = fe80::200:ff:fe00:ff01; > ip.ttl = 255; icmp6.type = 2; /* Packet Too Big. */ icmp6.code = 0; > icmp6.frag_mtu = 1500; next(pipeline=ingress, table=??); };) > @@ -6799,7 +6801,7 @@ AT_CAPTURE_FILE([lr0flows]) > AT_CHECK_UNQUOTED([grep -e "chk_pkt_len" -e "lr_in_larger_pkts" lr0flows > | ovn_strip_lflows], [0], [dnl > table=??(lr_in_chk_pkt_len ), priority=0 , match=(1), action=(next;) > table=??(lr_in_chk_pkt_len ), priority=50 , match=(outport == > "lr0-public"), action=(reg9[[1]] = check_pkt_larger(1514); $ct_save_action;) > - table=??(lr_in_chk_pkt_len ), priority=55 , match=(outport == > "lr0-public" && (tcp)), action=($ct_save_action;) > + table=??(lr_in_chk_pkt_len ), priority=55 , match=(outport == > "lr0-public" && (arp || tcp)), action=($ct_save_action;) > table=??(lr_in_larger_pkts ), priority=0 , match=(1), action=(next;) > table=??(lr_in_larger_pkts ), priority=150 , match=(inport == > "lr0-sw0" && outport == "lr0-public" && ip4 && reg9[[1]] && reg9[[0]] == > 0), action=(icmp4_error {reg9[[0]] = 1; reg9[[1]] = 0; eth.dst = > 00:00:00:00:ff:01; ip4.dst = ip4.src; ip4.src = 10.0.0.1; ip.ttl = 255; > icmp4.type = 3; /* Destination Unreachable. */ icmp4.code = 4; /* Frag > Needed and DF was Set. */ icmp4.frag_mtu = 1500; next(pipeline=ingress, > table=??); };) > table=??(lr_in_larger_pkts ), priority=150 , match=(inport == > "lr0-sw0" && outport == "lr0-public" && ip6 && reg9[[1]] && reg9[[0]] == > 0), action=(icmp6_error {reg9[[0]] = 1; reg9[[1]] = 0; eth.dst = > 00:00:00:00:ff:01; ip6.dst = ip6.src; ip6.src = fe80::200:ff:fe00:ff01; > ip.ttl = 255; icmp6.type = 2; /* Packet Too Big. */ icmp6.code = 0; > icmp6.frag_mtu = 1500; next(pipeline=ingress, table=??); };) > @@ -6814,8 +6816,8 @@ AT_CHECK_UNQUOTED([grep -e "chk_pkt_len" -e > "lr_in_larger_pkts" lr0flows | ovn_s > AT_CHECK([grep "lr_in_admission" lr0flows | grep -e "check_pkt_larger" -e > "tcp" | ovn_strip_lflows], [0], [dnl > table=??(lr_in_admission ), priority=50 , match=(eth.dst == > 00:00:20:20:12:13 && inport == "lr0-public"), action=(reg9[[1]] = > check_pkt_larger(1514); xreg0[[0..47]] = 00:00:20:20:12:13; next;) > table=??(lr_in_admission ), priority=50 , match=(eth.mcast && > inport == "lr0-public"), action=(reg9[[1]] = check_pkt_larger(1514); > xreg0[[0..47]] = 00:00:20:20:12:13; next;) > - table=??(lr_in_admission ), priority=55 , match=(eth.dst == > 00:00:20:20:12:13 && inport == "lr0-public" && (tcp)), > action=(xreg0[[0..47]] = 00:00:20:20:12:13; next;) > - table=??(lr_in_admission ), priority=55 , match=(eth.mcast && > inport == "lr0-public" && (tcp)), action=(xreg0[[0..47]] = > 00:00:20:20:12:13; next;) > + table=??(lr_in_admission ), priority=55 , match=(eth.dst == > 00:00:20:20:12:13 && inport == "lr0-public" && (arp || tcp)), > action=(xreg0[[0..47]] = 00:00:20:20:12:13; next;) > + table=??(lr_in_admission ), priority=55 , match=(eth.mcast && > inport == "lr0-public" && (arp || tcp)), action=(xreg0[[0..47]] = > 00:00:20:20:12:13; next;) > ]) > > # Set gateway_mtu option on lr0-sw0 > @@ -6828,7 +6830,8 @@ AT_CHECK_UNQUOTED([grep -e "chk_pkt_len" -e > "lr_in_larger_pkts" lr0flows | ovn_s > table=??(lr_in_chk_pkt_len ), priority=0 , match=(1), action=(next;) > table=??(lr_in_chk_pkt_len ), priority=50 , match=(outport == > "lr0-public"), action=(reg9[[1]] = check_pkt_larger(1514); $ct_save_action;) > table=??(lr_in_chk_pkt_len ), priority=50 , match=(outport == > "lr0-sw0"), action=(reg9[[1]] = check_pkt_larger(1414); $ct_save_action;) > - table=??(lr_in_chk_pkt_len ), priority=55 , match=(outport == > "lr0-public" && (tcp)), action=($ct_save_action;) > + table=??(lr_in_chk_pkt_len ), priority=55 , match=(outport == > "lr0-public" && (arp || tcp)), action=($ct_save_action;) > + table=??(lr_in_chk_pkt_len ), priority=55 , match=(outport == > "lr0-sw0" && (arp)), action=($ct_save_action;) > table=??(lr_in_larger_pkts ), priority=0 , match=(1), action=(next;) > table=??(lr_in_larger_pkts ), priority=150 , match=(inport == > "lr0-public" && outport == "lr0-sw0" && ip4 && reg9[[1]] && reg9[[0]] == > 0), action=(icmp4_error {reg9[[0]] = 1; reg9[[1]] = 0; eth.dst = > 00:00:20:20:12:13; ip4.dst = ip4.src; ip4.src = 172.168.0.100; ip.ttl = > 255; icmp4.type = 3; /* Destination Unreachable. */ icmp4.code = 4; /* Frag > Needed and DF was Set. */ icmp4.frag_mtu = 1400; next(pipeline=ingress, > table=??); };) > table=??(lr_in_larger_pkts ), priority=150 , match=(inport == > "lr0-public" && outport == "lr0-sw0" && ip6 && reg9[[1]] && reg9[[0]] == > 0), action=(icmp6_error {reg9[[0]] = 1; reg9[[1]] = 0; eth.dst = > 00:00:20:20:12:13; ip6.dst = ip6.src; ip6.src = fe80::200:20ff:fe20:1213; > ip.ttl = 255; icmp6.type = 2; /* Packet Too Big. */ icmp6.code = 0; > icmp6.frag_mtu = 1400; next(pipeline=ingress, table=??); };) > @@ -6878,8 +6881,8 @@ AT_CHECK_UNQUOTED([grep -e "chk_pkt_len" -e > "lr_in_larger_pkts" lr0flows | ovn_s > table=??(lr_in_chk_pkt_len ), priority=0 , match=(1), action=(next;) > table=??(lr_in_chk_pkt_len ), priority=50 , match=(outport == > "lr0-public"), action=(reg9[[1]] = check_pkt_larger(1514); $ct_save_action;) > table=??(lr_in_chk_pkt_len ), priority=50 , match=(outport == > "lr0-sw0"), action=(reg9[[1]] = check_pkt_larger(1414); $ct_save_action;) > - table=??(lr_in_chk_pkt_len ), priority=55 , match=(outport == > "lr0-public" && (tcp)), action=($ct_save_action;) > - table=??(lr_in_chk_pkt_len ), priority=55 , match=(outport == > "lr0-sw0" && (tcp)), action=($ct_save_action;) > + table=??(lr_in_chk_pkt_len ), priority=55 , match=(outport == > "lr0-public" && (arp || tcp)), action=($ct_save_action;) > + table=??(lr_in_chk_pkt_len ), priority=55 , match=(outport == > "lr0-sw0" && (arp || tcp)), action=($ct_save_action;) > table=??(lr_in_larger_pkts ), priority=0 , match=(1), action=(next;) > table=??(lr_in_larger_pkts ), priority=150 , match=(inport == > "lr0-public" && outport == "lr0-sw0" && ip4 && reg9[[1]] && reg9[[0]] == > 0), action=(icmp4_error {reg9[[0]] = 1; reg9[[1]] = 0; eth.dst = > 00:00:20:20:12:13; ip4.dst = ip4.src; ip4.src = 172.168.0.100; ip.ttl = > 255; icmp4.type = 3; /* Destination Unreachable. */ icmp4.code = 4; /* Frag > Needed and DF was Set. */ icmp4.frag_mtu = 1400; next(pipeline=ingress, > table=??); };) > table=??(lr_in_larger_pkts ), priority=150 , match=(inport == > "lr0-public" && outport == "lr0-sw0" && ip6 && reg9[[1]] && reg9[[0]] == > 0), action=(icmp6_error {reg9[[0]] = 1; reg9[[1]] = 0; eth.dst = > 00:00:20:20:12:13; ip6.dst = ip6.src; ip6.src = fe80::200:20ff:fe20:1213; > ip.ttl = 255; icmp6.type = 2; /* Packet Too Big. */ icmp6.code = 0; > icmp6.frag_mtu = 1400; next(pipeline=ingress, table=??); };) > @@ -6904,10 +6907,10 @@ AT_CHECK([grep "lr_in_admission" lr0flows | grep > -e "check_pkt_larger" -e "tcp" > table=??(lr_in_admission ), priority=50 , match=(eth.dst == > 00:00:20:20:12:13 && inport == "lr0-public"), action=(reg9[[1]] = > check_pkt_larger(1514); xreg0[[0..47]] = 00:00:20:20:12:13; next;) > table=??(lr_in_admission ), priority=50 , match=(eth.mcast && > inport == "lr0-public"), action=(reg9[[1]] = check_pkt_larger(1514); > xreg0[[0..47]] = 00:00:20:20:12:13; next;) > table=??(lr_in_admission ), priority=50 , match=(eth.mcast && > inport == "lr0-sw0"), action=(reg9[[1]] = check_pkt_larger(1414); > xreg0[[0..47]] = 00:00:00:00:ff:01; next;) > - table=??(lr_in_admission ), priority=55 , match=(eth.dst == > 00:00:00:00:ff:01 && inport == "lr0-sw0" && (tcp)), action=(xreg0[[0..47]] > = 00:00:00:00:ff:01; next;) > - table=??(lr_in_admission ), priority=55 , match=(eth.dst == > 00:00:20:20:12:13 && inport == "lr0-public" && (tcp)), > action=(xreg0[[0..47]] = 00:00:20:20:12:13; next;) > - table=??(lr_in_admission ), priority=55 , match=(eth.mcast && > inport == "lr0-public" && (tcp)), action=(xreg0[[0..47]] = > 00:00:20:20:12:13; next;) > - table=??(lr_in_admission ), priority=55 , match=(eth.mcast && > inport == "lr0-sw0" && (tcp)), action=(xreg0[[0..47]] = 00:00:00:00:ff:01; > next;) > + table=??(lr_in_admission ), priority=55 , match=(eth.dst == > 00:00:00:00:ff:01 && inport == "lr0-sw0" && (arp || tcp)), > action=(xreg0[[0..47]] = 00:00:00:00:ff:01; next;) > + table=??(lr_in_admission ), priority=55 , match=(eth.dst == > 00:00:20:20:12:13 && inport == "lr0-public" && (arp || tcp)), > action=(xreg0[[0..47]] = 00:00:20:20:12:13; next;) > + table=??(lr_in_admission ), priority=55 , match=(eth.mcast && > inport == "lr0-public" && (arp || tcp)), action=(xreg0[[0..47]] = > 00:00:20:20:12:13; next;) > + table=??(lr_in_admission ), priority=55 , match=(eth.mcast && > inport == "lr0-sw0" && (arp || tcp)), action=(xreg0[[0..47]] = > 00:00:00:00:ff:01; next;) > ]) > > # Clear gateway_mtu option on lr0-public > @@ -6918,7 +6921,7 @@ AT_CAPTURE_FILE([lr0flows]) > AT_CHECK_UNQUOTED([grep -e "chk_pkt_len" -e "lr_in_larger_pkts" lr0flows > | ovn_strip_lflows], [0], [dnl > table=??(lr_in_chk_pkt_len ), priority=0 , match=(1), action=(next;) > table=??(lr_in_chk_pkt_len ), priority=50 , match=(outport == > "lr0-sw0"), action=(reg9[[1]] = check_pkt_larger(1414); $ct_save_action;) > - table=??(lr_in_chk_pkt_len ), priority=55 , match=(outport == > "lr0-sw0" && (tcp)), action=($ct_save_action;) > + table=??(lr_in_chk_pkt_len ), priority=55 , match=(outport == > "lr0-sw0" && (arp || tcp)), action=($ct_save_action;) > table=??(lr_in_larger_pkts ), priority=0 , match=(1), action=(next;) > table=??(lr_in_larger_pkts ), priority=150 , match=(inport == > "lr0-public" && outport == "lr0-sw0" && ip4 && reg9[[1]] && reg9[[0]] == > 0), action=(icmp4_error {reg9[[0]] = 1; reg9[[1]] = 0; eth.dst = > 00:00:20:20:12:13; ip4.dst = ip4.src; ip4.src = 172.168.0.100; ip.ttl = > 255; icmp4.type = 3; /* Destination Unreachable. */ icmp4.code = 4; /* Frag > Needed and DF was Set. */ icmp4.frag_mtu = 1400; next(pipeline=ingress, > table=??); };) > table=??(lr_in_larger_pkts ), priority=150 , match=(inport == > "lr0-public" && outport == "lr0-sw0" && ip6 && reg9[[1]] && reg9[[0]] == > 0), action=(icmp6_error {reg9[[0]] = 1; reg9[[1]] = 0; eth.dst = > 00:00:20:20:12:13; ip6.dst = ip6.src; ip6.src = fe80::200:20ff:fe20:1213; > ip.ttl = 255; icmp6.type = 2; /* Packet Too Big. */ icmp6.code = 0; > icmp6.frag_mtu = 1400; next(pipeline=ingress, table=??); };) > -- > 2.51.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
