On 04/07/17 19:09, Reyk Floeter wrote:
> 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
Hi and thanks for the answer.
I would have send a better report if I could debug it.
-d does not produce any messages.
Also I'm not sure how to reproduce it. It just happened after the upgrade
and it happens in a random way... Not all users and not on all vlans.
I will try the patch and send a report to @bugs
G
> 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",
>
