BIND 10 #2902: Direct DHCPv4 traffic on Linux - Linux Packet Filter (LPF)
BIND 10 Development
do-not-reply at isc.org
Fri May 17 17:59:15 UTC 2013
#2902: Direct DHCPv4 traffic on Linux - Linux Packet Filter (LPF)
-------------------------------------+-------------------------------------
Reporter: marcin | Owner:
Type: enhancement | marcin
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 stephen):
* owner: stephen => marcin
Comment:
Reviewed commit 9c2f41d0ce4250623b36966eb04698d55eac7509
'''src/bin/dhcp4/dhcp4_srv.cc'''[[BR]]
Dhcp4Srv constructor: appears to be a word missing in the first comment
block inside the function: it currently says "The 'true' value of in the
call".
Dhcp4Srv constructor: a comment should be added to the effect that the
check on whether the source/destination hardware address are specified
before copying them across is because the copy function throws an
exception if the pointer is null. However, what should the logic be when
these two fields are null and the corresponding fields in "answer" are
not? (In other words, should Pkt4::setLocalHWAddr() zero out the
Pkt4::local_hwaddr_ field if passed a null pointer?)
'''src/bin/dhcp4/tests/dhcp4_srv_unittest.cc'''[[BR]]
The comment in the NakedDhcpv4Srv constructor disables raw traffic owing
to lack of root privileges. If root privileges are enabled, is there a
way of running unit tests with raw sockets?
testDiscoverRequest:
* "i*10" should be "i * 10"
* Adding a comment above the block of code that actually creates the
request packet saying that this is what is happening would immediately
make the point of the next few statements clear.
'''src/lib/dhcp/iface_mgr.cc'''[[BR]]
As Iface::closeSockets now requires that !SocketCollection be a container
whose iterators are not evalidated when erase() has been called, an
additional comment to that effect at the point at which !SocketCollection
is defined might avoid problems later on. (For example, if someone decided
to change it to a vector.)
IfaceMgr::setPacketFilter: The comment "There is at least one socket open,
so we have to fail" should be after the "if" statement. (Either that, or
precede the comment with the word "If".)
'''src/lib/dhcp/iface_mgr.h'''[[BR]]
Trivial point: all the comments the Iface class should start with a
capital letter.
IfaceMgr::setPacketFilter declaration: !PktFilterPtr has been defined in
pkt_filter.h and can be used in place of the type
"boost::shared_ptr<!PktFilter>".
'''src/lib/dhcp/iface_mgr_{bsd,linux,sun}.cc'''[[BR]]
IfaceMgr::setMatchingPacketFilter - could use !PktFilterPtr instead of
explicitly using shared_ptr (and shortens the statements enough to allow
the removal of the intermediate variable without loss of clarity).
'''src/lib/dhcp/pkt4.cc'''[[BR]]
setHWAddr()/setLocalHWAddr()/setLocalHWAddr(): these could call a common
function, passing through the member variable as a parameter.
Alternatively, leave it as-is, but expand the exception message to
indicate which hardware address is being set to NULL.
setPacketFilter declaration: !PktFilterPtr has been defined in
pkt_filter.h and can be used in place of the type
"boost::shared_ptr<!PktFilter>" in the argument list.
'''src/lib/dhcp/pkt_filter.h'''[[BR]]
If the forward declaration of "class Iface" deserves a comment, so does
the preceding declaration of "struct !SockInfo".
'''src/lib/dhcp/pkt_filter_inet.cc'''[[BR]]
PktFilterInet::openSocket: should the "if (receive_bcast)" statement in
the #ifdef'd block be changed to "if (receive_bcast &&
iface.flag_broadcast)" in line with similar changes elsewhere?
'''src/lib/dhcp/pkt_filter_lpf.cc'''[[BR]]
Header comments: several typos
s/other then DHCP/other than DHCP/
s/Socket filter program is/The socket filter program is/
s/origins from/originates from/
Check that there is a space around operators in the BPF_ statements.
Also, it's a bit unusual to split the macro BPF_STMT/BPF_JUMP from its
body.
It would help if the header comments gave a reference to a man page or and
a bit more help in interpreting the BPF_STMT and BPF_JUMP statements,
e.g.
{{{
The following structure defines a Berkely Packet Filter program to perform
the filtering. The program operates on Ethernet packets. To help with
interpretation of the program, for the types of Ethernet packets we are
interested in, the header layout is:
6 bytes Destination Ethernet Address
6 bytes Source Ethernet Address
2 bytes Ethernet packet type
20 bytes Fixed part of IP header
variable Variable part of IP header
2 bytes UDP Source port
2 bytes UDP destination port
4 bytes Rest of UDP header
}}}
Then expand the comments a bit. For example:
{{{
// Make sure this is an IP packet: check the half-word (two bytes)
// at offset 12 in the packet (the Ethernet packet type). If it
// is, advance to the next instruction. If not, advance 8
// instructions (which takes execution to the last instruction in
// the sequence: "drop it").
BPF_STMT (BPF_LD + BPF_H + BPF_ABS, 12),
BPF_JUMP (BPF_JMP + BPF_JEQ + BPF_K, ETHERTYPE_IP, 0, 8),
// Make sure it's a UDP packet. The IP protocol is at offset
// 9 in the IP header so, adding the Ethernet packet header size
// of 14 bytes gives an absolute byte offset in the packet of 23.
BPF_STMT (BPF_LD + BPF_B + BPF_ABS, 23),
BPF_JUMP (BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_UDP, 0, 6),
// Make sure this isn't a fragment by checking that the fragment
// offset field in the IP header is zero. This field is the
// least-significant 13 bits in the bytes at offsets 6 and 7 in
// the IP header, so the half-word at offset 20 (6 + size of
// Ethernet header) is loaded and an appropriate mask applied.
BPF_STMT(BPF_LD + BPF_H + BPF_ABS, 20),
BPF_JUMP(BPF_JMP + BPF_JSET + BPF_K, 0x1fff, 4, 0),
// Get the IP header length. This is achieved by the following
// (special) instruction that, given the offset of the start
// of the IP header (offset 14) loads the IP header length.
BPF_STMT (BPF_LDX + BPF_B + BPF_MSH, 14),
// Make sure it's to the right port. The following instruction
// adds the previously extracted IP header length to the given
// offset to locate the correct byte. The given offset of 16
// comprises the length of the Ethernet header (14) plus the offset
// of the UDP destination port (2) within the UDP header.
BPF_STMT (BPF_LD + BPF_H + BPF_IND, 16),
// The following instruction tests against the default DHCP server
port,
// but the action port is actually set in PktFilterLPF::openSocket().
// N.B. The code in that method assumes that this instruction is at
// offset 8 in the program. If this is changed, openSocket() must be
// updated.
BPF_JUMP (BPF_JMP + BPF_JEQ + BPF_K, isc::dhcp::DHCP4_SERVER_PORT, 0,
1),
// If we passed all the tests, ask for the whole packet.
BPF_STMT(BPF_RET + BPF_K, (u_int) - 1),
// Otherwise, drop it.
BPF_STMT(BPF_RET + BPF_K, 0),
}}}
(I've gone on about this a little bit because, having not come across BPF
before, I spend quite a bit of time working it out. As well as reading
and understanding the BPF documentation, checking offsets etc. involved
looking up the format of Ethernet headers, IP headers and UDP headers.
(And finally, it took a little time puzzling over it before I realised why
an offset of 16 was used when extracting the UDP port number.)
PktFilterLPF::openSocket(): would appreciate a comment before the "bind()"
call saying why this is happening (as the method is openSocket() and the
only documentation for it is "open socket") rather than have to look at
the exception text for the case where it has failed.
PktFilterLPF::receive(): read() can return -1 to indicate an error.
However, this case is ignored and treated as if an end of file was
received. (This might be a valid way to treat the error, but it should be
documented.)
PktFilterLPF::receive(): why don't we know where the DHCP data starts? Is
it because we have an Ethernet packet and there is the variable IP header
in it? If so, it should be stated.
PktFilterLPF::send(): pkt->getLocalAddress() returns an IOAddress object.
This has an "operator uint32_t" to cast an IPV4 address to a uint32_t.
Instead of converting the address to text and checking against
"255.255.255.255", it would be faster to use this cast and compare it the
value against UINT32_MAX. (Or create a 'static const
IOAddress("255.255.255.255")' within the method and compare two IOAddress
objects for equality.)
'''src/lib/dhcp/protocol_util.cc'''[[BR]]
decodeEthernetHeader(): rather than put the explicit "14" in the exception
message, ETHERNET_HEADER_LEN should be used as an argument instead.
decodeIpUdpHeader(): if the IP header length is less that five words, this
is a corrupt packet. An exception should be thrown instead of the error
being ignored.
decodeIpUdpHeader(): The comment about skipping the IP header options
should note "Skip IP header options (if any) to the start of the UDP
packet".
decodeIpUdpHeader(): The pointer position is not set to the tail of the
UDP header (which implies the last byte in the header) but to the start of
the UDP payload, the DHCP packet.
writeEthernetHeader(): rather than use the constant 0x0800, can't you use
the symbolic constant ETHERTYPE_IP inmstead?
calcChecksum(): Spaces around "+".
'''src/lib/dhcp/protocol_util.h'''[[BR]]
Having read this after pkt_filter_lpf.cc, use of the symbolic constants
defined here (extended with additional relevant ones) in the BPF macros
may make the filter instructions easier to understand.
Headers for decode{Ethernet,Ip}Header should either say "the pkt object"
or "a Pkt4 object".
decodeEthernetHeader() does not check if the pointer is NULL, but
decodeIpUdpHeader() does check. Why the difference?
calcChecksum(): is there a suitable reference to where the checksum
calculation is defined?
'''src/lib/dhcp/tests/iface_mgr_unittest.cc'''[[BR]]
socketsForRemoteAddress test: will the OS_LINUX test be re-endabled? If
so, under what circumstances?
sendReceive4 test: Remove the commented out line testing that socket2 is
set.
setMatchingPacketFilter test: both the Linux and non-linux versions look
identical to me.
'''src/lib/dhcp/tests/pkt4_unittest.cc'''[[BR]]
hwaddrSrcRemote() test: I think that the second
EXPECT_NO_THROW/EXPECT_TRUE test should use a different hardware address
to the first. (Currently both use dst_hwaddr) Otherwise the test could
pass if the "set" did nothing and the "get" returned the wrong field.
'''!ChangeLog'''[[BR]]
This will need a !ChangeLog entry.
--
Ticket URL: <https://bind10.isc.org/ticket/2902#comment:5>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list