> Hi Lorenzo,
Hi Mark,
thx for the review.
>
> I think this can be done more simply. In northd, the logical flow we create
> for the put_dhcp_opts() action has the following match conditions:
>
> ip4.src == {OFFER_IP, 0.0.0.0} && ip4.dst == {SERVER_IP, 255.255.255.255}
>
> Since the flow already filters based on destination IP address, it should
> mean that pinctrl_handle_put_dhcp_opts() will only be called if the
> destination IP address is the configured OVN server IP, or if it is
> broadcast.
>
> Therefore, checking for an IP match in pinctrl_handle_put_dhcp_opts() is
> unnecessary. The "case DHCP_MSG_DISCOVER:" section simply doesn't need to
> check in_flow->nw_dst at all. We can just set the msg_type to DHCP_MSG_OFFER
> and break.
>
> What do you think?
I think you are right, we already filter based on ip.dst == server_ip in the
logical flow match, so we can just get rid of the check in
pinctrl_handle_put_dhcp_opts(). I will fix it in v2.
>
> Thanks,
> Mark Michelson
>
> PS One final item: Why is the test altered so that the new test case is at
> the beginning? This causes the sequence numbers of all subsequent test_dhcp
> calls to get bumped by one. Would it be easier to add the new test case to
> the end, and therefore not have to update so much of the test?
it is to keep the DHCP_DISCOVER tests together, but I can put it at the end.
Regards,
Lorenzo
>
> On 9/5/24 05:43, Lorenzo Bianconi wrote:
> > 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]>
> > ---
> > controller/pinctrl.c | 48 ++++++++++++++++++++++++++++++++++-----
> > tests/ovn.at | 53 ++++++++++++++++++++++++++++----------------
> > 2 files changed, 76 insertions(+), 25 deletions(-)
> >
> > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > index c86b4f940..d3096c2d3 100644
> > --- a/controller/pinctrl.c
> > +++ b/controller/pinctrl.c
> > @@ -2596,14 +2596,50 @@ pinctrl_handle_put_dhcp_opts(
> > uint8_t msg_type = 0;
> > switch (dhcp_opts.dhcp_msg_type) {
> > - case DHCP_MSG_DISCOVER:
> > + 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;
> > + if (in_flow->nw_dst == htonl(INADDR_BROADCAST)) {
> > + break;
> > }
> > - break;
> > +
> > + const char *ptr = reply_dhcp_opts_ptr->data;
> > + const char *ptr_end = ptr + reply_dhcp_opts_ptr->size;
> > + ovs_be32 server_id = htonl(INADDR_BROADCAST);
> > +
> > + while (ptr < ptr_end) {
> > + struct dhcp_opt_header *in_dhcp_opt =
> > + (struct dhcp_opt_header *) ptr;
> > +
> > + /* 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.
> > + */
> > + if (in_dhcp_opt->code == OVN_DHCP_OPT_CODE_SERVER_ID) {
> > + server_id =
> > get_unaligned_be32(DHCP_OPT_PAYLOAD(in_dhcp_opt));
> > + break;
> > + }
> > +
> > + ptr += sizeof *in_dhcp_opt;
> > + if (ptr > ptr_end) {
> > + break;
> > + }
> > + ptr += in_dhcp_opt->len;
> > + if (ptr > ptr_end) {
> > + break;
> > + }
> > + }
> > +
> > + if (in_flow->nw_dst == server_id) {
> > + break;
> > + }
> > +
> > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > + VLOG_WARN_RL(&rl, "DHCP DISCOVER invalid address: "IP_FMT,
> > + IP_ARGS(in_flow->nw_dst));
> > + goto exit;
> > + }
> > case DHCP_MSG_REQUEST: {
> > msg_type = DHCP_MSG_ACK;
> > if (dhcp_opts.request_ip != *offer_ip) {
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index acf18c4e0..f21249b3e 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -7047,6 +7047,21 @@ compare_dhcp_packets 1
> > # ----------------------------------------------------------------------
> > +# Send unicast DHCPDISCOVER.
> > +reset_pcap_file hv1-vif1 hv1/vif1
> > +reset_pcap_file hv1-vif2 hv1/vif2
> > +rm -f 1.expected
> > +rm -f 2.expected
> > +offer_ip=`ip_to_hex 10 0 0 4`
> > +server_ip=`ip_to_hex 10 0 0 1`
> > +ciaddr=`ip_to_hex 0 0 0 0`
> > +request_ip=0
> > +expected_dhcp_opts=330400000e100104ffffff0003040a00000136040a000001
> > +test_dhcp 2 1 f00000000001 01 0 $ciaddr $offer_ip $request_ip 0 1
> > $offer_ip $server_ip ff1000000001 $server_ip 02 $expected_dhcp_opts
> > +compare_dhcp_packets 1
> > +
> > +# ----------------------------------------------------------------------
> > +
> > # ovs-ofctl also resumes the packets and this causes other ports to
> > receive
> > # the DHCP request packet. So reset the pcap files so that its easier to
> > test.
> > reset_pcap_file hv1-vif1 hv1/vif1
> > @@ -7061,7 +7076,7 @@ server_ip=`ip_to_hex 10 0 0 1`
> > ciaddr=`ip_to_hex 0 0 0 0`
> > request_ip=$offer_ip
> > expected_dhcp_opts=330400000e100104ffffff0003040a00000136040a000001
> > -test_dhcp 2 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 0
> > ff1000000001 $server_ip 05 $expected_dhcp_opts
> > +test_dhcp 3 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 0
> > ff1000000001 $server_ip 05 $expected_dhcp_opts
> > compare_dhcp_packets 2
> > # ----------------------------------------------------------------------
> > @@ -7078,7 +7093,7 @@ server_ip=`ip_to_hex 10 0 0 1`
> > ciaddr=`ip_to_hex 0 0 0 0`
> > request_ip=`ip_to_hex 10 0 0 7`
> > expected_dhcp_opts=""
> > -test_dhcp 3 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 0
> > ff1000000001 $server_ip 06 $expected_dhcp_opts
> > +test_dhcp 4 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 0
> > ff1000000001 $server_ip 06 $expected_dhcp_opts
> > compare_dhcp_packets 2
> > # ----------------------------------------------------------------------
> > @@ -7094,7 +7109,7 @@ rm -f 2.expected
> > ciaddr=`ip_to_hex 0 0 0 0`
> > offer_ip=0
> > request_ip=0
> > -test_dhcp 4 2 f00000000002 09 0 $ciaddr $offer_ip $request_ip 0 0 1 1
> > +test_dhcp 5 2 f00000000002 09 0 $ciaddr $offer_ip $request_ip 0 0 1 1
> > # NXT_RESUMEs should be 4.
> > # vif1-tx.pcap should have received the DHCPv4 (invalid) request packet
> > @@ -7117,7 +7132,7 @@ request_ip=0
> > src_ip=$offer_ip
> > dst_ip=$server_ip
> > expected_dhcp_opts=330400000e100104ffffff0003040a00000136040a000001
> > -test_dhcp 5 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 1 $src_ip
> > $dst_ip ff1000000001 $server_ip 05 $expected_dhcp_opts
> > +test_dhcp 6 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 1 $src_ip
> > $dst_ip ff1000000001 $server_ip 05 $expected_dhcp_opts
> > compare_dhcp_packets 2
> > # ----------------------------------------------------------------------
> > @@ -7136,7 +7151,7 @@ request_ip=0
> > src_ip=$offer_ip
> > dst_ip=`ip_to_hex 255 255 255 255`
> > expected_dhcp_opts=330400000e100104ffffff0003040a00000136040a000001
> > -test_dhcp 6 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 1 $src_ip
> > $dst_ip ff1000000001 $server_ip 05 $expected_dhcp_opts
> > +test_dhcp 7 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 1 $src_ip
> > $dst_ip ff1000000001 $server_ip 05 $expected_dhcp_opts
> > compare_dhcp_packets 2
> > # ----------------------------------------------------------------------
> > @@ -7155,7 +7170,7 @@ request_ip=0
> > src_ip=$offer_ip
> > dst_ip=`ip_to_hex 255 255 255 255`
> > expected_dhcp_opts=""
> > -test_dhcp 7 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 1 $src_ip
> > $dst_ip ff1000000001 $server_ip 06 $expected_dhcp_opts
> > +test_dhcp 8 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 1 $src_ip
> > $dst_ip ff1000000001 $server_ip 06 $expected_dhcp_opts
> > compare_dhcp_packets 2
> > # ----------------------------------------------------------------------
> > @@ -7174,7 +7189,7 @@ request_ip=0
> > src_ip=$offer_ip
> > dst_ip=`ip_to_hex 255 255 255 255`
> > expected_dhcp_opts=""
> > -test_dhcp 8 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 1 $src_ip
> > $dst_ip ff1000000001 $server_ip 06 $expected_dhcp_opts
> > +test_dhcp 9 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 1 $src_ip
> > $dst_ip ff1000000001 $server_ip 06 $expected_dhcp_opts
> > compare_dhcp_packets 2
> > # ----------------------------------------------------------------------
> > @@ -7189,7 +7204,7 @@ rm -f 2.expected
> > ciaddr=`ip_to_hex 0 0 0 0`
> > src_ip=`ip_to_hex 10 0 0 6`
> > dst_ip=`ip_to_hex 10 0 0 4`
> > -test_dhcp --no-resume 9 2 f00000000002 03 0 $ciaddr 0 0 0 1 $src_ip
> > $dst_ip 1
> > +test_dhcp --no-resume 10 2 f00000000002 03 0 $ciaddr 0 0 0 1 $src_ip
> > $dst_ip 1
> > # vif1-tx.pcap should have received the DHCPv4 request packet
> > OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected])
> > @@ -7207,7 +7222,7 @@ server_ip=`ip_to_hex 10 0 0 1`
> > ciaddr=`ip_to_hex 0 0 0 0`
> > request_ip=0
> > expected_dhcp_opts=330400000e100104ffffff0003040a00000136040a000001
> > -test_dhcp 10 1 f00000000001 01 1 $ciaddr $offer_ip $request_ip 0 0
> > ff1000000001 $server_ip 02 $expected_dhcp_opts
> > +test_dhcp 11 1 f00000000001 01 1 $ciaddr $offer_ip $request_ip 0 0
> > ff1000000001 $server_ip 02 $expected_dhcp_opts
> > compare_dhcp_packets 1
> > # ----------------------------------------------------------------------
> > @@ -7223,7 +7238,7 @@ server_ip=`ip_to_hex 10 0 0 1`
> > ciaddr=`ip_to_hex 10 0 0 6`
> > request_ip=0
> > expected_dhcp_opts=0
> > -test_dhcp 11 2 f00000000002 07 0 $ciaddr $offer_ip $request_ip 0 0
> > ff1000000001
> > +test_dhcp 12 2 f00000000002 07 0 $ciaddr $offer_ip $request_ip 0 0
> > ff1000000001
> > # There is no reply for this. Check for the INFO log in ovn-controller.log
> > OVS_WAIT_UNTIL(
> > @@ -7250,7 +7265,7 @@ dst_ip=$server_ip
> > # In the expected_dhcp_opts we should not see 330400000e10 which is
> > # dhcp lease time option and 0104ffffff00 which is subnet mask option.
> > expected_dhcp_opts=03040a00000136040a000001
> > -test_dhcp 12 2 f00000000002 08 0 $ciaddr $offer_ip $request_ip 0 1 $src_ip
> > $dst_ip ff1000000001 $server_ip 05 $expected_dhcp_opts
> > +test_dhcp 13 2 f00000000002 08 0 $ciaddr $offer_ip $request_ip 0 1 $src_ip
> > $dst_ip ff1000000001 $server_ip 05 $expected_dhcp_opts
> > compare_dhcp_packets 2
> > # Now add the dhcp option T1 to the dhcp options.
> > @@ -7275,7 +7290,7 @@ dst_ip=$server_ip
> > # In the expected_dhcp_opts we should not see 330400000e10 which is
> > # dhcp lease time option.
> >
> > expected_dhcp_opts=3a0400000fa0330400000e100104ffffff0003040a00000136040a000001
> > -test_dhcp 13 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 1 $src_ip
> > $dst_ip ff1000000001 $server_ip 05 $expected_dhcp_opts
> > +test_dhcp 14 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 1 $src_ip
> > $dst_ip ff1000000001 $server_ip 05 $expected_dhcp_opts
> > compare_dhcp_packets 2
> > # ----------------------------------------------------------------------
> > @@ -7295,7 +7310,7 @@ dst_ip=$server_ip
> > # In the expected_dhcp_opts we should not see 330400000e10 which is
> > # dhcp lease time option and 0104ffffff00 which is subnet mask option.
> > expected_dhcp_opts=03040a00000136040a000001
> > -test_dhcp 14 2 f00000000002 08 0 $ciaddr $offer_ip $request_ip 0 1 $src_ip
> > $dst_ip ff1000000001 $server_ip 05 $expected_dhcp_opts
> > +test_dhcp 15 2 f00000000002 08 0 $ciaddr $offer_ip $request_ip 0 1 $src_ip
> > $dst_ip ff1000000001 $server_ip 05 $expected_dhcp_opts
> > compare_dhcp_packets 2
> > # ----------------------------------------------------------------------
> > @@ -7321,7 +7336,7 @@ server_ip=`ip_to_hex 10 0 0 1`
> > ciaddr=`ip_to_hex 0 0 0 0`
> > request_ip=$offer_ip
> >
> > expected_dhcp_opts=330400000e100104ffffff0003040a00000136040a00000142040a0a0a0a
> > -test_dhcp 15 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 0
> > ff1000000001 $server_ip 05 $expected_dhcp_opts
> > +test_dhcp 16 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 0
> > ff1000000001 $server_ip 05 $expected_dhcp_opts
> > compare_dhcp_packets 2
> > # ----------------------------------------------------------------------
> > @@ -7347,7 +7362,7 @@ server_ip=`ip_to_hex 10 0 0 1`
> > ciaddr=`ip_to_hex 0 0 0 0`
> > request_ip=$offer_ip
> >
> > expected_dhcp_opts=330400000e100104ffffff0003040a00000136040a0000014210746573745f746674705f736572766572
> > -test_dhcp 16 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 0
> > ff1000000001 $server_ip 05 $expected_dhcp_opts
> > +test_dhcp 17 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 0
> > ff1000000001 $server_ip 05 $expected_dhcp_opts
> > compare_dhcp_packets 2
> > # ----------------------------------------------------------------------
> > @@ -7373,7 +7388,7 @@ server_ip=`ip_to_hex 10 0 0 1`
> > ciaddr=`ip_to_hex 0 0 0 0`
> > request_ip=$offer_ip
> >
> > expected_dhcp_opts=771305746573743103636f6d00057465737432c006330400000e100104ffffff0003040a00000136040a000001
> > -test_dhcp 17 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 0
> > ff1000000001 $server_ip 05 $expected_dhcp_opts
> > +test_dhcp 18 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 0
> > ff1000000001 $server_ip 05 $expected_dhcp_opts
> > compare_dhcp_packets 2
> > # ----------------------------------------------------------------------
> > @@ -7384,7 +7399,7 @@ server_ip=`ip_to_hex 10 0 0 1`
> > ciaddr=`ip_to_hex 0 0 0 0`
> > request_ip=0
> > expected_dhcp_opts=""
> > -test_dhcp 18 1 f00000000001 04 0 $ciaddr $offer_ip $request_ip 0 0
> > ff1000000001 $server_ip 02 $expected_dhcp_opts
> > +test_dhcp 19 1 f00000000001 04 0 $ciaddr $offer_ip $request_ip 0 0
> > ff1000000001 $server_ip 02 $expected_dhcp_opts
> > OVS_WAIT_UNTIL(
> > [test 1 -le $(grep -F -i -c 'DHCPDECLINE from f0:00:00:00:00:01,
> > 10.0.0.4 duplicated' hv1/ovn-controller.log)
> > ])
> > @@ -7413,7 +7428,7 @@ ciaddr=`ip_to_hex 0 0 0 0`
> > request_ip=0
> > boofile=4308626f6f7466696c65
> >
> > expected_dhcp_opts=${boofile}330400000e100104ffffff0003040a00000136040a000001
> > -test_dhcp 19 1 f00000000001 01 0 $ciaddr $offer_ip $request_ip 1 0
> > ff1000000001 $server_ip 02 $expected_dhcp_opts
> > +test_dhcp 20 1 f00000000001 01 0 $ciaddr $offer_ip $request_ip 1 0
> > ff1000000001 $server_ip 02 $expected_dhcp_opts
> > compare_dhcp_packets 1
> > # Test that ovn-controller pinctrl thread handles dhcp requests when it
> > @@ -7460,7 +7475,7 @@ ciaddr=`ip_to_hex 0 0 0 0`
> > request_ip=0
> > boofile=4308626f6f7466696c65
> >
> > expected_dhcp_opts=${boofile}330400000e100104ffffff0003040a00000136040a000001
> > -test_dhcp 20 1 f00000000001 01 0 $ciaddr $offer_ip $request_ip 1 0
> > ff1000000001 $server_ip 02 $expected_dhcp_opts
> > +test_dhcp 21 1 f00000000001 01 0 $ciaddr $offer_ip $request_ip 1 0
> > ff1000000001 $server_ip 02 $expected_dhcp_opts
> > compare_dhcp_packets 1
> > as hv1
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
