query: bnx2 and tg3 don't check tcp and/or ip header length validity?

My question is whether it would be OK to add a simple test, and set it to zero in case of bad values?

In both cases, they get a number that could be negative (in the case of a badly formed header), and mash it into a flag vector of some sort.

No comments/documentation explaining purpose.

===

bnx2.c:

		u32 tcp_opt_len;
(ipv6 variant)
			vlan_tag_flags |= ((tcp_opt_len >> 2) << 8) |
					  TX_BD_FLAGS_SW_FLAGS;
(ipv4 variant)
			if (tcp_opt_len || (iph->ihl > 5)) {
				vlan_tag_flags |= ((iph->ihl - 5) +
						   (tcp_opt_len >> 2)) << 8;
			}

At least in the latter case, it bothers to check the IP header validity....

These are transmit-only, I cannot find where they use them on receive?

===

tg3.c:

		int tcp_opt_len, ip_tcp_len;

			tcp_opt_len = tcp_optlen(skb);
			ip_tcp_len = ip_hdrlen(skb) + sizeof(struct tcphdr);

			iph->check = 0;
			iph->tot_len = htons(mss + ip_tcp_len + tcp_opt_len);
			hdrlen = ip_tcp_len + tcp_opt_len;

...

		if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5717) {
			mss |= (hdrlen & 0xc) << 12;
			if (hdrlen & 0x10)
				base_flags |= 0x00000010;
			base_flags |= (hdrlen & 0x3e0) << 5;
		} else
			mss |= hdrlen << 9;

Likewise, transmit-only. With completely different code later, in a dma bug fix function. But that's the overall picture....

Anybody have any idea what's going on here?

Although both are compiled in my build, I've got no way to test them. I'm just going to do the easy thing and set to zero for now. Somebody that knows the code -- who should have done real error checking -- could actually write better error checking and comments about the purpose of cramming the length of the TCP option field into a tag....

- tcp_opt_len = tcp_optlen(skb);
+ tcp_opt_len = tcp_option_len_th(tcp_hdr(skb));
+ if (tcp_opt_len < 0)
+ tcp_opt_len = 0;

The option length is needed by the hardware to segment a TSO packet into proper MTU-sized packets. You'll get malformed packets if the TSO header is bad. Setting it to zero perhaps can make these bad packets more deterministic, but I don't know for sure.

Malformed packets are unlikely (I'll use unlikely() on the test), but I've seen a lot of unlikely things happen over the years. When I was concourse manager at Interop '91, a bad Portmaster build wouldn't pass packets through one kind of router (3com); but it passed through all the others! Turned out, *most* routers didn't check the IP version and IHL fields. Shocking!

When we were designing IPv6 in '93, we had to use new IEEE numbers, etc. (instead of the IP version and IHL) to distinguish the new version. Otherwise, various printers crashed....

Unless there's a clearly documented check earlier in the code path (and there's nothing documented here), always re-check everything. (Also, never forget cosmic radiation....) Remember, from a driver developer's perspective, the hardware always fails. (And from a hardware viewpoint, the software is always bad.)

This is transmit, and the packets can only come from the Linux TCP stack, not some external entity.

You're being way too anal here, and adding these checks to drivers would be just a lot of rediculious bloat.