On Fri, Mar 6, 2020 at 7:14 AM Ihar Hrachyshka <ihrac...@redhat.com> 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. > > While the new DHCP response is sent as a broadcast IP frame, it's > handled locally, as any other DHCP reply by the native responder. > Meaning, the reply is sent to the client port that initiated the DHCP > session only. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1801006 > > Signed-off-by: Ihar Hrachyshka <ihrac...@redhat.com>
Thanks. I applied this patch to master and branch-20.03 as it's a bug. Numan > > --- > > Version 5: > * Applied htons to a constant to help compilers optimize > is_dhcp_flags_broadcast. > * Clarified in commit message that the IP frame is still sent to > client only. > Version 4: > * Introduced DHCP_BROADCAST_FLAG macro. > Version 3: > * Converted DHCP_FLAGS_BROADCAST into a static function. > * Used full sentences in comments (added missing dot). > * Negated if-condition to handle least common case in else branch. > * Fixed build with --enable-sparse --enable-Werror. > * Updated ovn-northd documentation to reflect new flow actions. > Version 2: > * Fixed line length warning reported by checkpatch. > --- > controller/pinctrl.c | 15 +++++++ > lib/ovn-l7.h | 2 + > northd/ovn-northd.8.xml | 5 +-- > northd/ovn-northd.c | 7 ++- > tests/ovn.at | 98 +++++++++++++++++++++++++++++------------ > 5 files changed, 93 insertions(+), 34 deletions(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index d06915a65..42813c0b5 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -966,6 +966,12 @@ pinctrl_handle_tcp_reset(struct rconn *swconn, const > struct flow *ip_flow, > dp_packet_uninit(&packet); > } > > +static bool > +is_dhcp_flags_broadcast(ovs_be16 flags) > +{ > + return flags & htons(DHCP_BROADCAST_FLAG); > +} > + > /* Called with in the pinctrl_handler thread context. */ > static void > pinctrl_handle_put_dhcp_opts( > @@ -1190,7 +1196,16 @@ pinctrl_handle_put_dhcp_opts( > > udp->udp_len = htons(new_l4_size); > > + /* Send a broadcast IP frame when BROADCAST flag is set. */ > struct ip_header *out_ip = dp_packet_l3(&pkt_out); > + ovs_be32 ip_dst; > + if (!is_dhcp_flags_broadcast(dhcp_data->flags)) { > + ip_dst = *offer_ip; > + } else { > + ip_dst = htonl(0xffffffff); > + } > + 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/lib/ovn-l7.h b/lib/ovn-l7.h > index f20d86c39..931e6ffcf 100644 > --- a/lib/ovn-l7.h > +++ b/lib/ovn-l7.h > @@ -34,6 +34,8 @@ struct gen_opts_map { > size_t code; > }; > > +#define DHCP_BROADCAST_FLAG 0x8000 > + > #define DHCP_OPTION(NAME, CODE, TYPE) \ > {.name = NAME, .code = CODE, .type = TYPE} > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > index a27dfa951..d37beafff 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -937,7 +937,6 @@ next; > <pre> > eth.dst = eth.src; > eth.src = <var>E</var>; > -ip4.dst = <var>A</var>; > ip4.src = <var>S</var>; > udp.src = 67; > udp.dst = 68; > @@ -948,8 +947,8 @@ output; > > <p> > where <var>E</var> is the server MAC address and <var>S</var> is > the > - server IPv4 address defined in the DHCPv4 options and <var>A</var> > is > - the IPv4 address defined in the logical port's addresses column. > + server IPv4 address defined in the DHCPv4 options. Note that > + <code>ip4.dst</code> field is handled by > <code>put_dhcp_opts</code>. > </p> > > <p> > 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. > -- > 2.24.1 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev