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.

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

Reply via email to