[ovs-dev] [PATCH ovn] Broadcast DHCPREPLY when BROADCAST flag is set
Ilya Maximets
i.maximets at ovn.org
Wed Mar 4 15:26:57 UTC 2020
On 3/4/20 3:10 PM, Ihar Hrachyshka wrote:
> As per RFC2131, section 4.1:
> A server or relay agent sending or relaying a DHCP message directly
> to a DHCP client (i.e., not to a relay agent specified in the
> 'giaddr' field) SHOULD examine the BROADCAST bit in the 'flags'
> field. If this bit is set to 1, the DHCP message SHOULD be sent as
> an IP broadcast using an IP broadcast address (preferably 0xffffffff)
> as the IP destination address and the link-layer broadcast address as
> the link-layer destination address.
>
> This patch changes destination IP address to 255.255.255.255 when client
> set BROADCAST flag in their DHCPREQUEST. Note: the offered IP address is
> still part of the DHCP payload.
Not a full review. Just a couple of style/patch formatting suggestions.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1801006
>
> Signed-off-by: Ihar Hrachyshka <ihrachys at redhat.com>
> ---
Since this is the second verion of the patch, subject line prefix
should contain v2 tag, ex. [PATCH ovn v2].
BTW, for the next version of this patch I'd suggest using v3 (not v2).
Here under the '---', should be list of changes made betwen versions.
That significantly simplifies review process. For example:
Version 3:
* Fixed ...
* Modified ...
Version 2:
* Fixed line length warning reproted by checkpatch.
Please keep the whole version history in all the patch versions.
> controller/pinctrl.c | 11 +++++
> northd/ovn-northd.c | 7 ++--
> tests/ovn.at | 98 ++++++++++++++++++++++++++++++++------------
> 3 files changed, 85 insertions(+), 31 deletions(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index d06915a65..6197ea325 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -966,6 +966,8 @@ pinctrl_handle_tcp_reset(struct rconn *swconn, const struct flow *ip_flow,
> dp_packet_uninit(&packet);
> }
>
> +#define DHCP_FLAGS_BROADCAST(flags) (((flags) & (1UL << 7)) != 0)
> +
> /* Called with in the pinctrl_handler thread context. */
> static void
> pinctrl_handle_put_dhcp_opts(
> @@ -1190,7 +1192,16 @@ pinctrl_handle_put_dhcp_opts(
>
> udp->udp_len = htons(new_l4_size);
>
> + /* Send a broadcast IP frame when BROADCAST flag is set */
We're typically want comments to be a full sentencies, i.e. start
with capital letter and have a period at the end (missed).
> struct ip_header *out_ip = dp_packet_l3(&pkt_out);
> + uint32_t ip_dst;
> + if DHCP_FLAGS_BROADCAST(dhcp_data->flags) {
Despite the fact that this translates to a valid C code after
the preprocessor run, it should be better it to have valid C code
in a source code :), i.e. have 'if' expression parenthesised.
> + ip_dst = htonl(0xffffffff);
> + } else {
> + ip_dst = *offer_ip;
> + }
> + put_16aligned_be32(&out_ip->ip_dst, ip_dst);
> +
> out_ip->ip_tot_len = htons(pkt_out.l4_ofs - pkt_out.l3_ofs + new_l4_size);
> udp->udp_csum = 0;
> /* Checksum needs to be initialized to zero. */
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 3aba0487d..58dfee617 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -4271,10 +4271,9 @@ build_dhcpv4_action(struct ovn_port *op, ovs_be32 offer_ip,
> ds_put_cstr(options_action, "); next;");
>
> ds_put_format(response_action, "eth.dst = eth.src; eth.src = %s; "
> - "ip4.dst = "IP_FMT"; ip4.src = %s; udp.src = 67; "
> - "udp.dst = 68; outport = inport; flags.loopback = 1; "
> - "output;",
> - server_mac, IP_ARGS(offer_ip), server_ip);
> + "ip4.src = %s; udp.src = 67; udp.dst = 68; "
> + "outport = inport; flags.loopback = 1; output;",
> + server_mac, server_ip);
>
> ds_put_format(ipv4_addr_match,
> "ip4.src == "IP_FMT" && ip4.dst == {%s, 255.255.255.255}",
> diff --git a/tests/ovn.at b/tests/ovn.at
> index cbaa6d4a2..a7842a35e 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -4595,10 +4595,11 @@ sleep 2
> as hv1 ovs-vsctl show
>
> # This shell function sends a DHCP request packet
> -# test_dhcp INPORT SRC_MAC DHCP_TYPE OFFER_IP REQUEST_IP ...
> +# test_dhcp INPORT SRC_MAC DHCP_TYPE BROADCAST CIADDR OFFER_IP REQUEST_IP USE_IP ...
> test_dhcp() {
> - local inport=$1 src_mac=$2 dhcp_type=$3 ciaddr=$4 offer_ip=$5 request_ip=$6 use_ip=$7
> - shift; shift; shift; shift; shift; shift; shift;
> + local inport=$1 src_mac=$2 dhcp_type=$3 broadcast=$4 ciaddr=$5 offer_ip=$6 request_ip=$7 use_ip=$8
> + shift; shift; shift; shift; shift; shift; shift; shift;
> +
> if test $use_ip != 0; then
> src_ip=$1
> dst_ip=$2
> @@ -4607,6 +4608,7 @@ test_dhcp() {
> src_ip=`ip_to_hex 0 0 0 0`
> dst_ip=`ip_to_hex 255 255 255 255`
> fi
> +
> if test $request_ip != 0; then
> ip_len=0120
> udp_len=010b
> @@ -4614,10 +4616,19 @@ test_dhcp() {
> ip_len=011a
> udp_len=0106
> fi
> +
> + if test $broadcast != 0; then
> + flags=8000
> + reply_dst_ip=`ip_to_hex 255 255 255 255`
> + else
> + flags=0000
> + reply_dst_ip=${offer_ip}
> + fi
> +
> local request=ffffffffffff${src_mac}08004510${ip_len}0000000080110000${src_ip}${dst_ip}
> # udp header and dhcp header
> request=${request}00440043${udp_len}0000
> - request=${request}010106006359aa7600000000${ciaddr}000000000000000000000000${src_mac}
> + request=${request}010106006359aa760000${flags}${ciaddr}000000000000000000000000${src_mac}
> # client hardware padding
> request=${request}00000000000000000000
> # server hostname
> @@ -4655,10 +4666,10 @@ test_dhcp() {
> ip_len=$(printf "%x" $ip_len)
> udp_len=$(printf "%x" $udp_len)
> # $ip_len var will be in 3 digits i.e 134. So adding a '0' before $ip_len
> - local reply=${src_mac}${srv_mac}080045100${ip_len}000000008011XXXX${srv_ip}${offer_ip}
> + local reply=${src_mac}${srv_mac}080045100${ip_len}000000008011XXXX${srv_ip}${reply_dst_ip}
> # udp header and dhcp header.
> # $udp_len var will be in 3 digits. So adding a '0' before $udp_len
> - reply=${reply}004300440${udp_len}0000020106006359aa7600000000${ciaddr}
> + reply=${reply}004300440${udp_len}0000020106006359aa760000${flags}${ciaddr}
> # your ip address; 0 for NAK
> if test $dhcp_reply_type = 06; then
> reply=${reply}00000000
> @@ -4729,7 +4740,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 1 f00000000001 01 $ciaddr $offer_ip $request_ip 0 ff1000000001 $server_ip 02 $expected_dhcp_opts
> +test_dhcp 1 f00000000001 01 0 $ciaddr $offer_ip $request_ip 0 ff1000000001 $server_ip 02 $expected_dhcp_opts
>
> # NXT_RESUMEs should be 1.
> OVS_WAIT_UNTIL([test 1 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`])
> @@ -4755,7 +4766,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 f00000000002 03 $ciaddr $offer_ip $request_ip 0 ff1000000001 $server_ip 05 $expected_dhcp_opts
> +test_dhcp 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 ff1000000001 $server_ip 05 $expected_dhcp_opts
>
> # NXT_RESUMEs should be 2.
> OVS_WAIT_UNTIL([test 2 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`])
> @@ -4779,7 +4790,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 2 f00000000002 03 $ciaddr $offer_ip $request_ip 0 ff1000000001 $server_ip 06 $expected_dhcp_opts
> +test_dhcp 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 ff1000000001 $server_ip 06 $expected_dhcp_opts
>
> # NXT_RESUMEs should be 3.
> OVS_WAIT_UNTIL([test 3 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`])
> @@ -4803,7 +4814,7 @@ rm -f 2.expected
> ciaddr=`ip_to_hex 0 0 0 0`
> offer_ip=0
> request_ip=0
> -test_dhcp 2 f00000000002 08 $ciaddr $offer_ip $request_ip 0 1 1
> +test_dhcp 2 f00000000002 08 0 $ciaddr $offer_ip $request_ip 0 1 1
>
> # NXT_RESUMEs should be 4.
> OVS_WAIT_UNTIL([test 4 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`])
> @@ -4820,12 +4831,12 @@ rm -f 2.expected
> # ls2-lp2 (vif4-tx.pcap) should receive the DHCPv4 request packet once.
>
> ciaddr=`ip_to_hex 0 0 0 0`
> -test_dhcp 3 f00000000003 01 $ciaddr 0 0 4 0
> +test_dhcp 3 f00000000003 01 0 $ciaddr 0 0 4 0
>
> # Send DHCPv4 packet on ls2-lp2. "router" DHCPv4 option is not defined for
> # this lport.
> ciaddr=`ip_to_hex 0 0 0 0`
> -test_dhcp 4 f00000000004 01 $ciaddr 0 0 3 0
> +test_dhcp 4 f00000000004 01 0 $ciaddr 0 0 3 0
>
> # NXT_RESUMEs should be 4.
> OVS_WAIT_UNTIL([test 4 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`])
> @@ -4842,7 +4853,7 @@ request_ip=0
> src_ip=$offer_ip
> dst_ip=$server_ip
> expected_dhcp_opts=330400000e100104ffffff0003040a00000136040a000001
> -test_dhcp 2 f00000000002 03 $ciaddr $offer_ip $request_ip 1 $src_ip $dst_ip ff1000000001 $server_ip 05 $expected_dhcp_opts
> +test_dhcp 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 1 $src_ip $dst_ip ff1000000001 $server_ip 05 $expected_dhcp_opts
>
> # NXT_RESUMEs should be 5.
> OVS_WAIT_UNTIL([test 5 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`])
> @@ -4868,7 +4879,7 @@ request_ip=0
> src_ip=$offer_ip
> dst_ip=`ip_to_hex 255 255 255 255`
> expected_dhcp_opts=330400000e100104ffffff0003040a00000136040a000001
> -test_dhcp 2 f00000000002 03 $ciaddr $offer_ip $request_ip 1 $src_ip $dst_ip ff1000000001 $server_ip 05 $expected_dhcp_opts
> +test_dhcp 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 1 $src_ip $dst_ip ff1000000001 $server_ip 05 $expected_dhcp_opts
>
> # NXT_RESUMEs should be 6.
> OVS_WAIT_UNTIL([test 6 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`])
> @@ -4894,7 +4905,7 @@ request_ip=0
> src_ip=$offer_ip
> dst_ip=`ip_to_hex 255 255 255 255`
> expected_dhcp_opts=""
> -test_dhcp 2 f00000000002 03 $ciaddr $offer_ip $request_ip 1 $src_ip $dst_ip ff1000000001 $server_ip 06 $expected_dhcp_opts
> +test_dhcp 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 1 $src_ip $dst_ip ff1000000001 $server_ip 06 $expected_dhcp_opts
>
> # NXT_RESUMEs should be 7.
> OVS_WAIT_UNTIL([test 7 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`])
> @@ -4920,7 +4931,7 @@ request_ip=0
> src_ip=$offer_ip
> dst_ip=`ip_to_hex 255 255 255 255`
> expected_dhcp_opts=""
> -test_dhcp 2 f00000000002 03 $ciaddr $offer_ip $request_ip 1 $src_ip $dst_ip ff1000000001 $server_ip 06 $expected_dhcp_opts
> +test_dhcp 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 1 $src_ip $dst_ip ff1000000001 $server_ip 06 $expected_dhcp_opts
>
> # NXT_RESUMEs should be 8.
> OVS_WAIT_UNTIL([test 8 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`])
> @@ -4942,7 +4953,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 2 f00000000002 03 $ciaddr 0 0 1 $src_ip $dst_ip 1
> +test_dhcp 2 f00000000002 03 0 $ciaddr 0 0 1 $src_ip $dst_ip 1
>
> # NXT_RESUMEs should be 8.
> OVS_WAIT_UNTIL([test 8 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`])
> @@ -4950,6 +4961,29 @@ OVS_WAIT_UNTIL([test 8 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`])
> # vif1-tx.pcap should have received the DHCPv4 request packet
> OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected])
>
> +reset_pcap_file hv1-vif1 hv1/vif1
> +reset_pcap_file hv1-vif2 hv1/vif2
> +rm -f 1.expected
> +rm -f 2.expected
> +
> +# Send DHCPDISCOVER with BROADCAST flag on.
> +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 1 f00000000001 01 1 $ciaddr $offer_ip $request_ip 0 ff1000000001 $server_ip 02 $expected_dhcp_opts
> +
> +# NXT_RESUMEs should be 9.
> +OVS_WAIT_UNTIL([test 9 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`])
> +
> +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif1-tx.pcap > 1.packets
> +cat 1.expected | cut -c -48 > expout
> +AT_CHECK([cat 1.packets | cut -c -48], [0], [expout])
> +# Skipping the IPv4 checksum.
> +cat 1.expected | cut -c 53- > expout
> +AT_CHECK([cat 1.packets | cut -c 53-], [0], [expout])
> +
> OVN_CLEANUP([hv1])
>
> AT_CLEANUP
> @@ -13206,10 +13240,11 @@ as hv1
> ovs-vsctl show
>
> # This shell function sends a DHCP request packet
> -# test_dhcp INPORT SRC_MAC DHCP_TYPE OFFER_IP ...
> +# test_dhcp INPORT SRC_MAC DHCP_TYPE BROADCAST OFFER_IP ...
> test_dhcp() {
> - local inport=$1 src_mac=$2 dhcp_type=$3 offer_ip=$4 use_ip=$5
> - shift; shift; shift; shift; shift;
> + local inport=$1 src_mac=$2 dhcp_type=$3 broadcast=$4 offer_ip=$5 use_ip=$6
> + shift; shift; shift; shift; shift; shift;
> +
> if test $use_ip != 0; then
> src_ip=$1
> dst_ip=$2
> @@ -13218,10 +13253,19 @@ test_dhcp() {
> src_ip=`ip_to_hex 0 0 0 0`
> dst_ip=`ip_to_hex 255 255 255 255`
> fi
> +
> + if test $broadcast != 0; then
> + flags=8000
> + reply_dst_ip=`ip_to_hex 255 255 255 255`
> + else
> + flags=0000
> + reply_dst_ip=${offer_ip}
> + fi
> +
> local request=ffffffffffff${src_mac}0800451001100000000080110000${src_ip}${dst_ip}
> # udp header and dhcp header
> request=${request}0044004300fc0000
> - request=${request}010106006359aa760000000000000000000000000000000000000000${src_mac}
> + request=${request}010106006359aa760000${flags}00000000000000000000000000000000${src_mac}
> # client hardware padding
> request=${request}00000000000000000000
> # server hostname
> @@ -13245,10 +13289,10 @@ test_dhcp() {
> ip_len=$(printf "%x" $ip_len)
> udp_len=$(printf "%x" $udp_len)
> # $ip_len var will be in 3 digits i.e 134. So adding a '0' before $ip_len
> - local reply=${src_mac}${srv_mac}080045100${ip_len}000000008011XXXX${srv_ip}${offer_ip}
> + local reply=${src_mac}${srv_mac}080045100${ip_len}000000008011XXXX${srv_ip}${reply_dst_ip}
> # udp header and dhcp header.
> # $udp_len var will be in 3 digits. So adding a '0' before $udp_len
> - reply=${reply}004300440${udp_len}0000020106006359aa760000000000000000
> + reply=${reply}004300440${udp_len}0000020106006359aa760000${flags}00000000
> # your ip address
> reply=${reply}${offer_ip}
> # next server ip address, relay agent ip address, client mac address
> @@ -13367,7 +13411,7 @@ offer_ip=`ip_to_hex 10 0 0 6`
> server_ip=`ip_to_hex 10 0 0 1`
> server_mac=ff1000000001
> expected_dhcp_opts=330400000e100104ffffff0003040a00000136040a000001
> -test_dhcp 1 f00000000003 01 $offer_ip 0 $server_mac $server_ip \
> +test_dhcp 1 f00000000003 01 0 $offer_ip 0 $server_mac $server_ip \
> $expected_dhcp_opts
>
> # NXT_RESUMEs should be 1 in hv1.
> @@ -13465,7 +13509,7 @@ offer_ip=`ip_to_hex 10 0 0 6`
> server_ip=`ip_to_hex 10 0 0 1`
> server_mac=ff1000000001
> expected_dhcp_opts=330400000e100104ffffff0003040a00000136040a000001
> -test_dhcp 1 f00000000003 01 $offer_ip 0 $server_mac $server_ip \
> +test_dhcp 1 f00000000003 01 0 $offer_ip 0 $server_mac $server_ip \
> $expected_dhcp_opts
>
> # NXT_RESUMEs should be 2 in hv1.
> @@ -13575,7 +13619,7 @@ offer_ip=`ip_to_hex 10 0 0 6`
> server_ip=`ip_to_hex 10 0 0 1`
> server_mac=ff1000000001
> expected_dhcp_opts=330400000e100104ffffff0003040a00000136040a000001
> -test_dhcp 1 f00000000003 01 $offer_ip 0 $server_mac $server_ip \
> +test_dhcp 1 f00000000003 01 0 $offer_ip 0 $server_mac $server_ip \
> $expected_dhcp_opts
>
> # NXT_RESUMEs should be 3 in hv1.
> @@ -13655,7 +13699,7 @@ offer_ip=`ip_to_hex 10 0 0 6`
> server_ip=`ip_to_hex 10 0 0 1`
> server_mac=ff1000000001
> expected_dhcp_opts=330400000e100104ffffff0003040a00000136040a000001
> -test_dhcp 1 f00000000003 01 $offer_ip 0 $server_mac $server_ip \
> +test_dhcp 1 f00000000003 01 0 $offer_ip 0 $server_mac $server_ip \
> $expected_dhcp_opts
>
> # NXT_RESUMEs should be 4 in hv1.
>
More information about the dev
mailing list