BIND 10 #2902: Direct DHCPv4 traffic on Linux - Linux Packet Filter (LPF)

[BIND 10 Development]

#2902: Direct DHCPv4 traffic on Linux - Linux Packet Filter (LPF)
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  enhancement   |  stephen
            Priority:  medium        |                       Status:
           Component:  dhcp4         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130523
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => stephen


Comment:

 Replying to [comment:7 stephen]:
 > Reviewed commit 9a49846cb4456b17526c30799df03c9a06c8a318
 >
 > '''src/bin/dhcp4/dhcp4_srv.cc'''[[BR]]
 > Dhcp4Srv::adjustRemoteAddr(): the test for the broadcast flag is
 currently:
 > {{{
 >         bool bcast_flag = (question->getFlags() >> 0xF) ? true : false;
 > }}}
 > I'm not clear why there is a right-shift. As the method returns a
 unit16_t, and as the broadcast flag is defined in
 [http://tools.ietf.org/html/rfc2131#section-2 RFC 2131] as the left-most
 bit, the test would be better done as:
 > {{{
 >         bool bcast_flag = ((question->getFlags() & 0x8000) != 0);
 > }}}
 > (In fact, as bcast_flag is a bool, the comparison is not strictly needed
 as the compiler should convert the numeric value to true/false as
 appropriate.  However I think it does make things clearer.)

 Yes. This is just another way to do the same thing. I agree it is clearer.
 And I agree that the comparison should remain.

 >
 > What about defining a symbol in Pkt4 (called something along the lines
 of FLAG_BROADCAST_MASK) and using it in the code instead of 0x8000?
 Strictly speaking, although the fact that a broadcast flag is important to
 the packet filter code, the numerical value of the bit mask needed to
 obtain the information does not need to be hardcoded anywhere other than
 in Pkt4.

 Defined and used.

 >
 > Dhcp4Srv::adjustRemoteAddr(): the change made here fixes the issue
 raised in #2760.  When you close this ticket, please close that one as
 well.

 Ok. I will close it once this ticket is merged.

 >
 >
 > '''src/bin/dhcp4/dhcp4_srv.h'''[[BR]]
 > Nothing to do with this ticket, but as the file has been modified... The
 general convention for the "public/protected/private" scope statements is
 to be flush against the left margin.  The "protected" statement is: the
 others are not.
 >
 >

 They are now aligned properly.


 > '''src/bin/dhcp4/tests/dhcp4_srv_unittest.cc'''[[BR]]
 > NamedDhcpv4Srv constructor: with the constructor now fully doxygen-
 documented, the "port" argument should be documented as well.

 Added a comment.

 >
 > testDiscoverRequest: (trivial) in the loops to set the source and
 destination addresses, the source HW address loop uses an increment of
 "i++", the destination address loop uses "++i".

 Corrected.

 >
 > testDiscoverRequest: typo - s/interprented/interpreted/

 Corrected.

 >
 > adjustRemoteAddressSelect(): resetting the !IfaceMgr packet filter
 should be done in the test fixture (Dhcpv4SrvTest) destructor.  This
 guarantees it will always be done, regardless of how the tests exits. (As
 it is, the packet filter is changed, an ASSERT_NO_THROW test is done, then
 the packet filter is reset.  The ASSERT_NO_THROW test could fail, which
 would lead to the test exiting without executing the reset code.)

 The reason why it had been put into the test was that I wanted to avoid
 redundant resets for tests that didn't really need it. Your point about
 the ASSERT_ causing the test to abort and thus never reaching the reset
 part of the code convinced me to put reset operation to TearDown function.

 >
 >
 > '''src/lib/dhcp/pkt_filter_lpf.cc'''[[BR]]
 > > Space added. What do you mean by split macro from its body?
 > I mean "MACRO (BODY)" as opposed to "MACRO(BODY)".  However, that's been
 corrected.

 ok.

 >
 > In the BPF program, with the introduction of symbolic constants, the
 part of the comments describing the offset arithmetic (which explained
 what the various numbers were) are now no-longer needed. However, they can
 be left.

 It doesn't hurt to keep them there. The more information the better. So I
 left it as it was.

 >
 >
 > '''!ChangeLog entry'''[[BR]]
 > Suggest a change of wording:
 > {{{
 > 6XX.  [func]*         marcin
 >       b10-dhcp4: Added the ability for the server to respond to a
 directly
 >         connected client which does not yet have an IP address.  On
 Linux, the
 >         server will unicast the       response to the client's hardware
 address and
 >         the 'yiaddr' (the client's new IP address). Sending a response
 to the
 >         unicast address prevents other (not interested) hosts from
 receiving
 >         the server response. This capability is not yet implemented on
 non-Linux
 >         Operating Systems where, in all cases, the server responds to
 the
 >         broadcast address. The logic conforms to section 4.1 of RFC
 2131.
 >       (Trac #2902, git abc)
 > }}}

 Ok.

 > (As an aside, if the client doesn't have an IP address yet, I don't see
 why RFC 2131 mandates that the response also goes to the yiaddr.)

 When client plugs in to the network it doesn't have an IP address on the
 interface. It requests the new address from the server. Once server
 created the lease, it will send the ACK message with the address being
 assigned. This address is stored in the yiaddr field. RFC recommends that
 this response goes to yiaddr because other hosts don't have this address
 and thus will not receive this response. Server could respond to the
 broadcast address (and it is allowed to do so) but in such case, all hosts
 will receive this packet and will have to process it until they find it
 was not directed to them. The host which is interested to get the new
 lease, will listen to the whole traffic coming back from the server and
 will catch the packet sent to him by matching transaction id, client id
 etc. So, from its point of you it doesn't make much difference whether
 server responds to broadcast or to unicast (yiaddr) because it will have
 to decode all packets. The only exception is that the client may not
 support receiving the unicast traffic on the interface which is not
 configured. In such cases, client requests response to broadcast address
 and server should respect this request.

 Does it answer your question? (at least somewhere along the lines?)

-- 
Ticket URL: <http://bind10.isc.org/ticket/2902#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development