Skip to content

Commit

Permalink
reduce number of DHCP responder flows for LSPs
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@JacobTanenbaum
JacobTanenbaum committed Nov 16, 2023
1 parent 3bf6740 commit 4a18667
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 29 deletions.
10 changes: 10 additions & 0 deletions controller/pinctrl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(0xffffffff)){
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;
Expand All @@ -2048,6 +2053,11 @@ pinctrl_handle_put_dhcp_opts(
IP_ARGS(*offer_ip));
msg_type = DHCP_MSG_NAK;
}
if (in_flow-> nw_src == htonl(0x00000000)) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
VLOG_WARN_RL(&rl, "DHCP REQUEST cannot be broadcast");
goto exit;
}
break;
}
case OVN_DHCP_MSG_RELEASE: {
Expand Down
22 changes: 1 addition & 21 deletions northd/northd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
9 changes: 3 additions & 6 deletions tests/ovn-northd.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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\""
Expand All @@ -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
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions tests/ovn.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand Down Expand Up @@ -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 \
Expand Down

0 comments on commit 4a18667

Please sign in to comment.