[747] in linux-net channel archive
eth_copy_and_sum() comments + patch.
daemon@ATHENA.MIT.EDU (Tom May)
Fri Jul 21 14:48:32 1995
Date: Thu, 20 Jul 1995 19:04:21 -0700
From: ftom@netcom.com (Tom May)
To: linux-net@vger.rutgers.edu
Some notes on eth_copy_and_sum() from 1.3.11. But first my apologies
for being such a micro-optimizer. I do get carried away sometimes.
But I think note #1 at least is fairly worthwhile.
1. Assuming that the source and destination of the copy are 4-byte
aligned (which I think is true), it is best not to break the copy of
non-IP packets into two pieces because the second piece will not be
aligned. (For the IP packet case, csum_partial_copy() can deal with
2-byte alignment so that's not a problem.) The following patch fixes
this.
2. I added a new variable ip_length because gcc (2.7.0) was not
eliminating the common subexpression completely and making some really
funny code as a result. (In general, using inline functions seems to
result in suboptimal register usage on x86.) I know that's sort of a
bad reason, but I think the code also gets slightly easier to read in
the process . . . (now if only dest were allocated in a variable that
would live across the call to csum_partial_copy()).
3. It scares me to trust the value in iph->tot_len so readily.
Suppose it were equal to 10 because we got a broken packet from
somewhere. Then 10 - 20 = -10 which is certainly < length, but if we
handed it to csum_partial_copy() it would be treated as unsigned and
the result would be a disaster. In this routine we are saved by the
fact that ntohs()-sizeof() is unsigned, so the comparison is unsigned,
and since -10 looks like a huge positive unsigned number it will not
be < length. Maybe all packet size variables should be declared as
unsigned? (I recall hearing a while back that NT would crash if it
were sent certain malformed packets. I certainly wouldn't want that
to happen to Linux.)
4. This isn't a comment about eth_copy_and_sum(), but I decided to
toss it in anyway. There seems to be some schizophrenia in eth.c
(and maybe elsewhere) between using ETH_ALEN and using dev->addr_len.
ETH_ALEN is certainly easier for the compiler to deal with. Since
this is ethernet-specific code it shouldn't hurt to use ETH_ALEN
for everything for consistency and compilability.
This patch implements 1 & 2 above. I also expanded the comment so
nobody else would get suckered into thinking they could just set
length = ntohs()-sizeof() and go for it :-)
--- linux/net/ethernet/eth.c.0 Thu Jul 20 15:05:08 1995
+++ linux/net/ethernet/eth.c Thu Jul 20 16:54:09 1995
@@ -239,23 +239,27 @@
{
struct ethhdr *eth;
struct iphdr *iph;
+ int ip_length;
IS_SKB(dest);
eth=(struct ethhdr *)dest->data;
- memcpy(dest->data,src,34); /* ethernet is always >= 60 */
- length-=34;
if(eth->h_proto!=htons(ETH_P_IP))
{
- memcpy(dest->data+34,src+34,length);
+ memcpy(dest->data,src,length);
return;
}
/*
* We have to watch for padded packets. The csum doesn't include the
* padding, and there is no point in copying the padding anyway.
+ * We have to use the smaller of length and ip_length because it
+ * can happen that ip_length > length.
*/
+ memcpy(dest->data,src,34); /* ethernet is always >= 34 */
+ length -= 34;
iph=(struct iphdr*)(src+14); /* 14 = Rx_addr+Tx_addr+type_field */
- if (ntohs(iph->tot_len)-sizeof(struct iphdr) <= length)
- length=ntohs(iph->tot_len)-sizeof(struct iphdr);
+ ip_length = ntohs(iph->tot_len) - sizeof(struct iphdr);
+ if (ip_length <= length)
+ length=ip_length;
dest->csum=csum_partial_copy(src+34,dest->data+34,length,base);
dest->ip_summed=1;
Tom.
