> On Mon, Nov 25, 2024 at 10:50 AM Lorenzo Bianconi > <[email protected]> wrote: > > > > On Fri, Nov 24, 2023 at 4:57 PM <[email protected]> wrote: > > > > > From: Jacob Tanenbaum <[email protected]> > > > > > > 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 > > > > > > Submitted-at: https://github.com/ovn-org/ovn/pull/224 > > > Signed-off-by: Jacob Tanenbaum <[email protected]> > > > Acked-by: Dumitru Ceara <[email protected]> > > > --- > > > controller/pinctrl.c | 5 +++++ > > > northd/northd.c | 23 ++--------------------- > > > tests/ovn-northd.at | 9 +++------ > > > tests/ovn.at | 4 ++-- > > > 4 files changed, 12 insertions(+), 29 deletions(-) > > > > > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > > index 62cf4da324..2f2f945b71 100644 > > > --- a/controller/pinctrl.c > > > +++ b/controller/pinctrl.c > > > @@ -2039,6 +2039,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; > > > diff --git a/northd/northd.c b/northd/northd.c > > > index e93d0c8f82..d0fe1929fc 100644 > > > --- a/northd/northd.c > > > +++ b/northd/northd.c > > > @@ -6805,7 +6805,8 @@ 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 +9439,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 66ea495e5d..b0e464e77c 100644 > > > --- a/tests/ovn-northd.at > > > +++ b/tests/ovn-northd.at > > > @@ -4768,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 = "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\"" > > > @@ -4778,8 +4777,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 > > > @@ -4789,8 +4787,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 17ad1fb243..b2d69ecccf 100644 > > > --- a/tests/ovn.at > > > +++ b/tests/ovn.at > > > @@ -19639,7 +19639,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 \ > > > @@ -19891,7 +19891,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 \ > > > -- > > > 2.41.0 > > > > > > _______________________________________________ > > > dev mailing list > > > [email protected] > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > > > Can we please backport this patch to ovn-23.09. This is needed in order to > > backport the following fix: > > > > Done. > > Thanks > Numan
Thx Numan. BTW, it seems to me the commit below is not fully applied now in ovn-23.09 since we now we accept only discover dhcp packets with broadcast address in pinctrl_handle_put_dhcp_opts(). We need to fix it. commit f74b38f774061e7a56c125b45c8761e9ddc43a03 Author: Lorenzo Bianconi <[email protected]> Date: Thu Sep 12 18:40:02 2024 +0200 controller: Accept unicast dhcp-discover in pinctrl_handle_put_dhcp_opts(). According to RFC 2131 section 4.4.4, when the DHCP client knows the address of a DHCP server, in either INIT or REBOOTING state, the client may use that address in the DHCPDISCOVER or DHCPREQUEST rather than the IP broadcast address. Fix pinctrl_handle_put_dhcp_opts implementation according to the RFC 2131. Reported-at: https://issues.redhat.com/browse/FDP-765 Signed-off-by: Lorenzo Bianconi <[email protected]> Acked-by: Mark Michelson <[email protected]> Signed-off-by: Numan Siddique <[email protected]> Regards, Lorenzo > > > commit f74b38f774061e7a56c125b45c8761e9ddc43a03 > > Author: Lorenzo Bianconi <[email protected]> > > Date: Thu Sep 12 18:40:02 2024 +0200 > > > > controller: Accept unicast dhcp-discover in > > pinctrl_handle_put_dhcp_opts(). > > > > According to RFC 2131 section 4.4.4, when the DHCP client knows the > > address of a DHCP server, in either INIT or REBOOTING state, the client > > may use that address in the DHCPDISCOVER or DHCPREQUEST rather than the > > IP broadcast address. Fix pinctrl_handle_put_dhcp_opts implementation > > according to the RFC 2131. > > > > Reported-at: https://issues.redhat.com/browse/FDP-765 > > Signed-off-by: Lorenzo Bianconi <[email protected]> > > Acked-by: Mark Michelson <[email protected]> > > Signed-off-by: Numan Siddique <[email protected]> > > > > More info reported here: https://issues.redhat.com/browse/FDP-765 > > > > Regards, > > Lorenzo > > _______________________________________________ > > 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
