Re: [ovs-dev] [PATCH ovn] controller: Accept unicast dhcp-discover in pinctrl_handle_put_dhcp_opts().

Mon, 09 Sep 2024 09:37:38 -0700 

Hi Lorenzo,
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?

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?


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






















					Reply via email to