Hi,
On Tue, Jul 04, 2017 at 02:41:30PM +0300, Kapetanakis Giannis wrote:
> Hi,
>
> Just upgraded a set of my firewalls that also do dhcrelay to -current.
>
> The program stopped working ok. Some dhcp requests where being forwarded some
> not.
>
> tcpdump was showing the request on internal interface but I couldn't see the
> request being forwarded on the external interface.
> For some vlans the relay was working for some not.
>
> I've located the problem to this commit:
> http://marc.info/?l=openbsd-cvs&m=149140326301074&w=2
>
> Reverting back to:
> bpf.c,v 1.17
> packet.c,v 1.13
> dhcpd.h,v 1.22 2017/04/04
>
> everything was ok again.
>
> My setup is (trunk - on one firewall) - Vlans - carp - dhcrelay
> 28 vlans, 28 carps, 18 dhcrelay, 30 bpf devices
>
First of all, please send a proper bug reports to bugs@, not misc.
"It used to work but now it doesn't" is not very helpful.
Could you share your actual configuration or, even better, provide a
simplified way to reproduce your problem? rzalamena, me, and some
other people have tested different setups but you seem to have an
interestingly complex configuration.
The new code has more validation, so it might be that it rightfully or
wrongfully rejects packets that have been accepted before.
Could you try again with the attached diff? It doesn't change
behavior but it adds some chatty logging when a packet is rejected.
Maybe it helps to find the issue.
Reyk
Index: usr.sbin/dhcrelay/bpf.c
===================================================================
RCS file: /cvs/src/usr.sbin/dhcrelay/bpf.c,v
retrieving revision 1.19
diff -u -p -u -p -r1.19 bpf.c
--- usr.sbin/dhcrelay/bpf.c 19 Apr 2017 05:36:12 -0000 1.19
+++ usr.sbin/dhcrelay/bpf.c 4 Jul 2017 16:01:29 -0000
@@ -349,11 +349,17 @@ send_packet(struct interface_info *inter
/* Assemble the headers... */
if ((bufp = assemble_hw_header(buf, sizeof(buf), 0, pc,
- interface->hw_address.htype)) == -1)
+ interface->hw_address.htype)) == -1) {
+ log_warnx("%s:%d: assemble_hw_header failed, len %zu",
+ __func__, __LINE__, len);
goto done;
+ }
if ((bufp = assemble_udp_ip_header(buf, sizeof(buf), bufp, pc,
- (unsigned char *)raw, len)) == -1)
+ (unsigned char *)raw, len)) == -1) {
+ log_warnx("%s:%d: assemble_udp_ip_header failed,"
+ " offset %zd len %zu", __func__, __LINE__, bufp, len);
goto done;
+ }
/* Fire it off */
iov[0].iov_base = (char *)buf;
@@ -447,6 +453,9 @@ receive_packet(struct interface_info *in
* skip this packet.
*/
if (offset < 0) {
+ log_warnx("%s:%d: decode_hw_header failed,"
+ " len %zu", __func__, __LINE__,
+ interface->rbuf_len);
interface->rbuf_offset += hdr.bh_caplen;
continue;
}
@@ -457,6 +466,9 @@ receive_packet(struct interface_info *in
/* If the IP or UDP checksum was bad, skip the packet... */
if (offset < 0) {
+ log_warnx("%s:%d: decode_udp_ip_header failed,"
+ " offset %zd len %zu", __func__, __LINE__,
+ offset, interface->rbuf_len);
interface->rbuf_offset += hdr.bh_caplen;
continue;
}
@@ -470,6 +482,10 @@ receive_packet(struct interface_info *in
* life, though).
*/
if (hdr.bh_caplen > len) {
+ log_warnx("%s:%d: XXX shouldn't happen in real life,"
+ " caplen %u > len %zu", __func__, __LINE__,
+ hdr.bh_caplen, len);
+
interface->rbuf_offset += hdr.bh_caplen;
continue;
}
Index: usr.sbin/dhcrelay/packet.c
===================================================================
RCS file: /cvs/src/usr.sbin/dhcrelay/packet.c,v
retrieving revision 1.14
diff -u -p -u -p -r1.14 packet.c
--- usr.sbin/dhcrelay/packet.c 5 Apr 2017 14:40:56 -0000 1.14
+++ usr.sbin/dhcrelay/packet.c 4 Jul 2017 16:01:29 -0000
@@ -104,8 +104,12 @@ assemble_hw_header(unsigned char *buf, s
switch (intfhtype) {
case HTYPE_ETHER:
- if (buflen < offset + ETHER_HDR_LEN)
+ if (buflen < offset + ETHER_HDR_LEN) {
+ log_warnx("%s:%d: short ether hdr buflen %zu < %zu",
+ __func__, __LINE__,
+ buflen, offset + ETHER_HDR_LEN);
return (-1);
+ }
/* Use the supplied address or let the kernel fill it. */
memcpy(eh.ether_shost, pc->pc_smac, ETHER_ADDR_LEN);
@@ -117,6 +121,8 @@ assemble_hw_header(unsigned char *buf, s
offset += ETHER_HDR_LEN;
break;
default:
+ log_warnx("%s:%d: invalid htype %u", __func__, __LINE__,
+ intfhtype);
return (-1);
}
@@ -130,8 +136,12 @@ assemble_udp_ip_header(unsigned char *bu
struct ip ip;
struct udphdr udp;
- if (buflen < offset + sizeof(ip) + sizeof(udp))
+ if (buflen < offset + sizeof(ip) + sizeof(udp)) {
+ log_warnx("%s:%d: short udp pkt buflen %zu < %zu",
+ __func__, __LINE__,
+ buflen, offset + sizeof(ip) + sizeof(udp));
return (-1);
+ }
ip.ip_v = 4;
ip.ip_hl = 5;
@@ -174,18 +184,24 @@ decode_hw_header(unsigned char *buf, siz
switch (intfhtype) {
case HTYPE_IPSEC_TUNNEL:
- if (buflen < offset + ENC_HDRLEN + sizeof(*ip))
+ if (buflen < offset + ENC_HDRLEN + sizeof(*ip)) {
+ log_warnx("%s:%d", __func__, __LINE__);
return (-1);
+ }
offset += ENC_HDRLEN;
ip_len = (buf[offset] & 0xf) << 2;
- if (buflen < offset + ip_len)
+ if (buflen < offset + ip_len) {
+ log_warnx("%s:%d", __func__, __LINE__);
return (-1);
+ }
ip = (struct ip *)(buf + offset);
/* Encapsulated IP */
- if (ip->ip_p != IPPROTO_IPIP)
+ if (ip->ip_p != IPPROTO_IPIP) {
+ log_warnx("%s:%d", __func__, __LINE__);
return (-1);
+ }
memset(pc->pc_dmac, 0xff, ETHER_ADDR_LEN);
offset += ip_len;
@@ -194,8 +210,12 @@ decode_hw_header(unsigned char *buf, siz
pc->pc_hlen = ETHER_ADDR_LEN;
break;
case HTYPE_ETHER:
- if (buflen < offset + ETHER_HDR_LEN)
+ if (buflen < offset + ETHER_HDR_LEN) {
+ log_warnx("%s:%d: short ether hdr buflen %zu < %zu",
+ __func__, __LINE__,
+ buflen, offset + ETHER_HDR_LEN);
return (-1);
+ }
memcpy(pc->pc_dmac, buf + offset, ETHER_ADDR_LEN);
memcpy(pc->pc_smac, buf + offset + ETHER_ADDR_LEN,
@@ -206,6 +226,8 @@ decode_hw_header(unsigned char *buf, siz
pc->pc_hlen = ETHER_ADDR_LEN;
break;
default:
+ log_warnx("%s:%d: invalid htype %u", __func__, __LINE__,
+ intfhtype);
return (-1);
}
@@ -230,11 +252,19 @@ decode_udp_ip_header(unsigned char *buf,
int len;
/* Assure that an entire IP header is within the buffer. */
- if (buflen < offset + sizeof(*ip))
+ if (buflen < offset + sizeof(*ip)) {
+ log_warnx("%s:%d: short ip hdr buflen %zu < %zu",
+ __func__, __LINE__,
+ buflen, offset + sizeof(*ip));
return (-1);
+ }
ip_len = (buf[offset] & 0xf) << 2;
- if (buflen < offset + ip_len)
+ if (buflen < offset + ip_len) {
+ log_warnx("%s:%d: short ip pkt buflen %zu < %zu",
+ __func__, __LINE__,
+ buflen, offset + offset + ip_len);
return (-1);
+ }
ip = (struct ip *)(buf + offset);
ip_packets_seen++;
@@ -242,6 +272,9 @@ decode_udp_ip_header(unsigned char *buf,
/* Check the IP header checksum - it should be zero. */
if (wrapsum(checksum(buf + offset, ip_len, 0)) != 0) {
ip_packets_bad_checksum++;
+ log_warnx("%s:%d: %u bad IP checksums seen in %u packets",
+ __func__, __LINE__,
+ ip_packets_bad_checksum, ip_packets_seen);
if (ip_packets_seen > 4 && ip_packets_bad_checksum != 0 &&
(ip_packets_seen / ip_packets_bad_checksum) < 2) {
log_info("%u bad IP checksums seen in %u packets",
@@ -268,18 +301,24 @@ decode_udp_ip_header(unsigned char *buf,
#endif
/* Assure that the entire IP packet is within the buffer. */
- if (buflen < offset + ntohs(ip->ip_len))
+ if (buflen < offset + ntohs(ip->ip_len)) {
+ log_warnx("%s:%d", __func__, __LINE__);
return (-1);
+ }
/* Assure that the UDP header is within the buffer. */
- if (buflen < offset + ip_len + sizeof(*udp))
+ if (buflen < offset + ip_len + sizeof(*udp)) {
+ log_warnx("%s:%d", __func__, __LINE__);
return (-1);
+ }
udp = (struct udphdr *)(buf + offset + ip_len);
udp_packets_seen++;
/* Assure that the entire UDP packet is within the buffer. */
- if (buflen < offset + ip_len + ntohs(udp->uh_ulen))
+ if (buflen < offset + ip_len + ntohs(udp->uh_ulen)) {
+ log_warnx("%s:%d", __func__, __LINE__);
return (-1);
+ }
data = buf + offset + ip_len + sizeof(*udp);
/*
@@ -291,6 +330,10 @@ decode_udp_ip_header(unsigned char *buf,
len = ntohs(udp->uh_ulen) - sizeof(*udp);
if ((len < 0) || (len + data > buf + buflen)) {
udp_packets_length_overflow++;
+ log_warnx("%s:%d: %u udp packets in %u too long - dropped",
+ __func__, __LINE__,
+ udp_packets_length_overflow,
+ udp_packets_length_checked);
if (udp_packets_length_checked > 4 &&
udp_packets_length_overflow != 0 &&
(udp_packets_length_checked /
@@ -317,6 +360,9 @@ decode_udp_ip_header(unsigned char *buf,
udp_packets_seen++;
if (usum && usum != sum) {
udp_packets_bad_checksum++;
+ log_warnx("%s:%d: %u bad udp checksums in %u packets",
+ __func__, __LINE__,
+ udp_packets_bad_checksum, udp_packets_seen);
if (udp_packets_seen > 4 && udp_packets_bad_checksum != 0 &&
(udp_packets_seen / udp_packets_bad_checksum) < 2) {
log_info("%u bad udp checksums in %u packets",
