From 12e959354a783fd02b95517cc712d78e8f0abb88 Mon Sep 17 00:00:00 2001 From: Jacob Tanenbaum Date: Fri, 10 Nov 2023 14:34:03 -0500 Subject: [PATCH] reduce number of DHCP responder flows for LSPs currently for every LSP two DHCP responder flows are created. These two flows are almost exactly the same differing only in the match. ex. Unicast flow (for RENEW/REBIND): "match":"inport == \"0e4b7821-b5ae-4bff-9424-d7245c679150\" && eth.src == fa:16:3e:68:5e:c1 && ip4.src == 1.65.5.169 && ip4.dst == {1.65.5.1, 255.255.255.255} && udp.src == 68 && udp.dst == 67" Broadcast flow (for DISCOVER): "match":"inport == \"0e4b7821-b5ae-4bff-9424-d7245c679150\" && eth.src == fa:16:3e:68:5e:c1 && ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && udp.src == 68 && udp.dst == 67" I consolidated the flows to use the conjunctive match (ip4.src == {1.65.5.169, 0.0.0.0} && ip4.dst == {1.65.5.1, 255.255.255.255}) there is potential for a faulty DHCP discovery and DHCP Request packet getting through - added a check in pinctrl.c to check for illegal dhcp discovery packet sending to host - added a check in punctrl.c to check for illegal dhcp request packet to broadcast --- controller/pinctrl.c | 10 ++++++++++ northd/northd.c | 22 +--------------------- tests/ovn-northd.at | 9 +++------ tests/ovn.at | 4 ++-- 4 files changed, 16 insertions(+), 29 deletions(-) diff --git a/controller/pinctrl.c b/controller/pinctrl.c index d88e951a6d..6b634c243a 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -2038,6 +2038,11 @@ pinctrl_handle_put_dhcp_opts( switch (*in_dhcp_msg_type) { case DHCP_MSG_DISCOVER: msg_type = DHCP_MSG_OFFER; + if (in_flow->nw_dst != htonl(INADDR_BROADCAST)) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); + VLOG_WARN_RL(&rl, "DHCP DISCOVER must be Broadcast"); + goto exit; + } break; case DHCP_MSG_REQUEST: { msg_type = DHCP_MSG_ACK; @@ -2048,6 +2053,11 @@ pinctrl_handle_put_dhcp_opts( IP_ARGS(*offer_ip)); msg_type = DHCP_MSG_NAK; } + if (in_flow-> nw_src == htonl(INADDR_ANY)) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); + VLOG_WARN_RL(&rl, "DHCP REQUEST cannot be sent from an unassigned address"); + goto exit; + } break; } case OVN_DHCP_MSG_RELEASE: { diff --git a/northd/northd.c b/northd/northd.c index f8b046d83e..3e0558fa80 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -6805,7 +6805,7 @@ build_dhcpv4_action(struct ovn_port *op, ovs_be32 offer_ip, server_mac, server_ip); ds_put_format(ipv4_addr_match, - "ip4.src == "IP_FMT" && ip4.dst == {%s, 255.255.255.255}", + "(ip4.src == {"IP_FMT", 0.0.0.0} && ip4.dst == {%s, 255.255.255.255})", IP_ARGS(offer_ip), server_ip); smap_destroy(&dhcpv4_options); return true; @@ -9438,27 +9438,7 @@ build_dhcpv4_options_flows(struct ovn_port *op, op, lsp_addrs->ipv4_addrs[j].addr, &options_action, &response_action, &ipv4_addr_match)) { ds_clear(&match); - ds_put_format( - &match, "inport == %s && eth.src == %s && " - "ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && " - "udp.src == 68 && udp.dst == 67", - inport->json_key, lsp_addrs->ea_s); - if (is_external) { - ds_put_format(&match, " && is_chassis_resident(%s)", - op->json_key); - } - - ovn_lflow_add_with_hint__(lflows, op->od, - S_SWITCH_IN_DHCP_OPTIONS, 100, - ds_cstr(&match), - ds_cstr(&options_action), - inport->key, - copp_meter_get(COPP_DHCPV4_OPTS, - op->od->nbs->copp, - meter_groups), - &op->nbsp->dhcpv4_options->header_); - ds_clear(&match); /* Allow ip4.src = OFFER_IP and * ip4.dst = {SERVER_IP, 255.255.255.255} for the below * cases diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 196fe01fbd..ac84b90930 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -4759,8 +4759,7 @@ AT_CAPTURE_FILE([sw0flows]) AT_CHECK([grep -w "ls_in_dhcp_options" sw0flows | sort | sed 's/table=../table=??/'], [0], [dnl table=??(ls_in_dhcp_options ), priority=0 , match=(1), action=(next;) - table=??(ls_in_dhcp_options ), priority=100 , match=(inport == "sw0-port1" && eth.src == 50:54:00:00:00:01 && ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && udp.src == 68 && udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2, hostname = "foo", lease_time = 3600, netmask = 255.255.255.0, router = 10.0.0.1, server_id = 10.0.0.1); next;) - table=??(ls_in_dhcp_options ), priority=100 , match=(inport == "sw0-port1" && eth.src == 50:54:00:00:00:01 && ip4.src == 10.0.0.2 && ip4.dst == {10.0.0.1, 255.255.255.255} && udp.src == 68 && udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2, hostname = "foo", lease_time = 3600, netmask = 255.255.255.0, router = 10.0.0.1, server_id = 10.0.0.1); next;) + table=??(ls_in_dhcp_options ), priority=100 , match=(inport == "sw0-port1" && eth.src == 50:54:00:00:00:01 && (ip4.src == {10.0.0.2, 0.0.0.0} && ip4.dst == {10.0.0.1, 255.255.255.255}) && udp.src == 68 && udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2, hostname = "foo", lease_time = 3600, netmask = 255.255.255.0, router = 10.0.0.1, server_id = 10.0.0.1); next;) ]) check ovn-nbctl --wait=sb lsp-set-options sw0-port1 hostname="\"port1\"" @@ -4769,8 +4768,7 @@ AT_CAPTURE_FILE([sw0flows]) AT_CHECK([grep -w "ls_in_dhcp_options" sw0flows | sort | sed 's/table=../table=??/'], [0], [dnl table=??(ls_in_dhcp_options ), priority=0 , match=(1), action=(next;) - table=??(ls_in_dhcp_options ), priority=100 , match=(inport == "sw0-port1" && eth.src == 50:54:00:00:00:01 && ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && udp.src == 68 && udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2, hostname = "port1", lease_time = 3600, netmask = 255.255.255.0, router = 10.0.0.1, server_id = 10.0.0.1); next;) - table=??(ls_in_dhcp_options ), priority=100 , match=(inport == "sw0-port1" && eth.src == 50:54:00:00:00:01 && ip4.src == 10.0.0.2 && ip4.dst == {10.0.0.1, 255.255.255.255} && udp.src == 68 && udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2, hostname = "port1", lease_time = 3600, netmask = 255.255.255.0, router = 10.0.0.1, server_id = 10.0.0.1); next;) + table=??(ls_in_dhcp_options ), priority=100 , match=(inport == "sw0-port1" && eth.src == 50:54:00:00:00:01 && (ip4.src == {10.0.0.2, 0.0.0.0} && ip4.dst == {10.0.0.1, 255.255.255.255}) && udp.src == 68 && udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2, hostname = "port1", lease_time = 3600, netmask = 255.255.255.0, router = 10.0.0.1, server_id = 10.0.0.1); next;) ]) ovn-nbctl dhcp-options-set-options $CIDR_UUID lease_time=3600 router=10.0.0.1 server_id=10.0.0.1 server_mac=c0:ff:ee:00:00:01 @@ -4780,8 +4778,7 @@ AT_CAPTURE_FILE([sw0flows]) AT_CHECK([grep -w "ls_in_dhcp_options" sw0flows | sort | sed 's/table=../table=??/'], [0], [dnl table=??(ls_in_dhcp_options ), priority=0 , match=(1), action=(next;) - table=??(ls_in_dhcp_options ), priority=100 , match=(inport == "sw0-port1" && eth.src == 50:54:00:00:00:01 && ip4.src == 0.0.0.0 && ip4.dst == 255.255.255.255 && udp.src == 68 && udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2, hostname = "bar", lease_time = 3600, netmask = 255.255.255.0, router = 10.0.0.1, server_id = 10.0.0.1); next;) - table=??(ls_in_dhcp_options ), priority=100 , match=(inport == "sw0-port1" && eth.src == 50:54:00:00:00:01 && ip4.src == 10.0.0.2 && ip4.dst == {10.0.0.1, 255.255.255.255} && udp.src == 68 && udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2, hostname = "bar", lease_time = 3600, netmask = 255.255.255.0, router = 10.0.0.1, server_id = 10.0.0.1); next;) + table=??(ls_in_dhcp_options ), priority=100 , match=(inport == "sw0-port1" && eth.src == 50:54:00:00:00:01 && (ip4.src == {10.0.0.2, 0.0.0.0} && ip4.dst == {10.0.0.1, 255.255.255.255}) && udp.src == 68 && udp.dst == 67), action=(reg0[[3]] = put_dhcp_opts(offerip = 10.0.0.2, hostname = "bar", lease_time = 3600, netmask = 255.255.255.0, router = 10.0.0.1, server_id = 10.0.0.1); next;) ]) AT_CLEANUP diff --git a/tests/ovn.at b/tests/ovn.at index b8c61f87fb..d7aaa816aa 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -19631,7 +19631,7 @@ wait_for_ports_up ls1-lp_ext1 as hv1 ovs-ofctl dump-flows br-int > brintflows AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | \ grep controller | grep "0a.00.00.06" | grep reg14=0x$ln_public_key | \ -wc -l], [0], [3 +wc -l], [0], [1 ]) AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | \ grep controller | grep tp_src=546 | grep \ @@ -19890,7 +19890,7 @@ wait_for_ports_up ls1-lp_ext1 # There should be OF flows for DHCP4/v6 for the ls1-lp_ext1 port in hv2 AT_CHECK([as hv2 ovs-ofctl dump-flows br-int | \ grep controller | grep "0a.00.00.06" | grep reg14=0x$ln_public_key | \ -wc -l], [0], [3 +wc -l], [0], [1 ]) AT_CHECK([as hv2 ovs-ofctl dump-flows br-int | \ grep controller | grep tp_src=546 | grep \