Re: vmd(8): skip inspecting non-udp packets on local ifs
On Sat, May 22, 2021 at 10:20:37AM -0400, Dave Voutila wrote:
> tech@ & krw (since your code in question was imported to vmd),
>
> I found strange behavior running tcpbench(1) to measure the connection
> between a vmd guest and my host, as well as guest-to-guest. In short,
> it's some bogus logic in how vmd tries to intercept dhcp/bootp on local
> interfaces. Diff at the bottom addresses the issue, some background:
>
> Running tcpbench(1) for ~20-30s on my machine, vmd (with -v debug
> logging) barfs a bunch of lines like:
>
> 5 udp packets in 5 too long - dropped
>
> The tcpbench(1) throughput stalls out at that point and reports 0 Mbps
> avg bandwidth measurements.
>
> If anyone wants to reproduce, use an OpenBSD guest and just run:
>
>[host]$ tcpbench -s
> [guest]$ tcpbench -t 180 100.64.x.2
>
> Where 'x' is the appropriate value for your guest's local interface.
>
> reyk@ imported packet.c from dhclient(8), but there's no validation that
> the packet being inspected is an IP/UDP packet vs. IP/TCP, leading to
> bogus logic related to inspecing UDP header attributes. In dhclient(8),
> the decode_udp_ip_header function is used in a place where a bpf capture
> buffer has already made sure it's a UDP packet (see sbin/dhclient/bpf.c).
>
> In addition, there was a lot of stateful counting and checking we just
> don't need in vmd(8), so I've ripped that out as well. It makes no sense
> in this context.
>
> OK?
>
ok mlarkin
>
> Index: packet.c
> ===
> RCS file: /cvs/src/usr.sbin/vmd/packet.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 packet.c
> --- packet.c 19 Apr 2017 15:38:32 - 1.1
> +++ packet.c 22 May 2021 14:15:09 -
> @@ -220,12 +220,6 @@ decode_udp_ip_header(unsigned char *buf,
> unsigned char *data;
> u_int32_t ip_len;
> u_int32_t sum, usum;
> - static unsigned int ip_packets_seen;
> - static unsigned int ip_packets_bad_checksum;
> - static unsigned int udp_packets_seen;
> - static unsigned int udp_packets_bad_checksum;
> - static unsigned int udp_packets_length_checked;
> - static unsigned int udp_packets_length_overflow;
> int len;
>
> /* Assure that an entire IP header is within the buffer. */
> @@ -236,17 +230,11 @@ decode_udp_ip_header(unsigned char *buf,
> return (-1);
>
> ip = (struct ip *)(buf + offset);
> - ip_packets_seen++;
> + if (ip->ip_p != IPPROTO_UDP)
> + return (-1);
>
> /* Check the IP header checksum - it should be zero. */
> if (wrapsum(checksum(buf + offset, ip_len, 0)) != 0) {
> - ip_packets_bad_checksum++;
> - 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",
> - ip_packets_bad_checksum, ip_packets_seen);
> - ip_packets_seen = ip_packets_bad_checksum = 0;
> - }
> return (-1);
> }
>
> @@ -274,7 +262,6 @@ decode_udp_ip_header(unsigned char *buf,
> if (buflen < offset + ip_len + sizeof(*udp))
> 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))
> @@ -286,20 +273,8 @@ decode_udp_ip_header(unsigned char *buf,
>* UDP header and the data. If the UDP checksum field is zero,
>* we're not supposed to do a checksum.
>*/
> - udp_packets_length_checked++;
> len = ntohs(udp->uh_ulen) - sizeof(*udp);
> if ((len < 0) || (len + data > buf + buflen)) {
> - udp_packets_length_overflow++;
> - if (udp_packets_length_checked > 4 &&
> - udp_packets_length_overflow != 0 &&
> - (udp_packets_length_checked /
> - udp_packets_length_overflow) < 2) {
> - log_info("%u udp packets in %u too long - dropped",
> - udp_packets_length_overflow,
> - udp_packets_length_checked);
> - udp_packets_length_overflow =
> - udp_packets_length_checked = 0;
> - }
> return (-1);
> }
> if (len + data != buf + buflen)
> @@ -313,15 +288,7 @@ decode_udp_ip_header(unsigned char *buf,
> 2 * sizeof(ip->ip_src),
> IPPROTO_UDP + (u_int32_t)ntohs(udp->uh_ulen);
>
> - udp_packets_seen++;
> if (usum && usum != sum) {
> - udp_packets_bad_checksum++;
> - 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",
> -
vmd(8): skip inspecting non-udp packets on local ifs
tech@ & krw (since your code in question was imported to vmd),
I found strange behavior running tcpbench(1) to measure the connection
between a vmd guest and my host, as well as guest-to-guest. In short,
it's some bogus logic in how vmd tries to intercept dhcp/bootp on local
interfaces. Diff at the bottom addresses the issue, some background:
Running tcpbench(1) for ~20-30s on my machine, vmd (with -v debug
logging) barfs a bunch of lines like:
5 udp packets in 5 too long - dropped
The tcpbench(1) throughput stalls out at that point and reports 0 Mbps
avg bandwidth measurements.
If anyone wants to reproduce, use an OpenBSD guest and just run:
[host]$ tcpbench -s
[guest]$ tcpbench -t 180 100.64.x.2
Where 'x' is the appropriate value for your guest's local interface.
reyk@ imported packet.c from dhclient(8), but there's no validation that
the packet being inspected is an IP/UDP packet vs. IP/TCP, leading to
bogus logic related to inspecing UDP header attributes. In dhclient(8),
the decode_udp_ip_header function is used in a place where a bpf capture
buffer has already made sure it's a UDP packet (see sbin/dhclient/bpf.c).
In addition, there was a lot of stateful counting and checking we just
don't need in vmd(8), so I've ripped that out as well. It makes no sense
in this context.
OK?
Index: packet.c
===
RCS file: /cvs/src/usr.sbin/vmd/packet.c,v
retrieving revision 1.1
diff -u -p -r1.1 packet.c
--- packet.c19 Apr 2017 15:38:32 - 1.1
+++ packet.c22 May 2021 14:15:09 -
@@ -220,12 +220,6 @@ decode_udp_ip_header(unsigned char *buf,
unsigned char *data;
u_int32_t ip_len;
u_int32_t sum, usum;
- static unsigned int ip_packets_seen;
- static unsigned int ip_packets_bad_checksum;
- static unsigned int udp_packets_seen;
- static unsigned int udp_packets_bad_checksum;
- static unsigned int udp_packets_length_checked;
- static unsigned int udp_packets_length_overflow;
int len;
/* Assure that an entire IP header is within the buffer. */
@@ -236,17 +230,11 @@ decode_udp_ip_header(unsigned char *buf,
return (-1);
ip = (struct ip *)(buf + offset);
- ip_packets_seen++;
+ if (ip->ip_p != IPPROTO_UDP)
+ return (-1);
/* Check the IP header checksum - it should be zero. */
if (wrapsum(checksum(buf + offset, ip_len, 0)) != 0) {
- ip_packets_bad_checksum++;
- 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",
- ip_packets_bad_checksum, ip_packets_seen);
- ip_packets_seen = ip_packets_bad_checksum = 0;
- }
return (-1);
}
@@ -274,7 +262,6 @@ decode_udp_ip_header(unsigned char *buf,
if (buflen < offset + ip_len + sizeof(*udp))
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))
@@ -286,20 +273,8 @@ decode_udp_ip_header(unsigned char *buf,
* UDP header and the data. If the UDP checksum field is zero,
* we're not supposed to do a checksum.
*/
- udp_packets_length_checked++;
len = ntohs(udp->uh_ulen) - sizeof(*udp);
if ((len < 0) || (len + data > buf + buflen)) {
- udp_packets_length_overflow++;
- if (udp_packets_length_checked > 4 &&
- udp_packets_length_overflow != 0 &&
- (udp_packets_length_checked /
- udp_packets_length_overflow) < 2) {
- log_info("%u udp packets in %u too long - dropped",
- udp_packets_length_overflow,
- udp_packets_length_checked);
- udp_packets_length_overflow =
- udp_packets_length_checked = 0;
- }
return (-1);
}
if (len + data != buf + buflen)
@@ -313,15 +288,7 @@ decode_udp_ip_header(unsigned char *buf,
2 * sizeof(ip->ip_src),
IPPROTO_UDP + (u_int32_t)ntohs(udp->uh_ulen);
- udp_packets_seen++;
if (usum && usum != sum) {
- udp_packets_bad_checksum++;
- 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",
- udp_packets_bad_checksum, udp_packets_seen);
- udp_packets_seen = udp_packets_bad_checksum = 0;
- }
return (-1);
