On Tue, Nov 18, 2025 at 11:14 AM Xavier Simonart <[email protected]> wrote:
> 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? > Yeah that is better. > 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 >> >> >> Thank you Xavier, I have updated the commit message, merged this into main and backported all the way down to 24.03. Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
