Wireshark · Ethereal-dev: Re: [Ethereal-dev] Optimization: remove the unconditional ip_checksum() in packet-ip?

[Kaul <mykaul@xxxxxxxxx>]

Attached patch.
Comments are welcome + extensive testing, please.

Regards,
Y.

On 9/23/05, Ulf Lamping <ulf.lamping@xxxxxx> wrote:

Kaul wrote:

> >From packet-ip.c:
>   if (tvb_bytes_exist(tvb, offset, hlen)) {
>     ipsum = ip_checksum(tvb_get_ptr(tvb, offset, hlen), hlen);
>     if (tree) {
>       if (ipsum == 0) {
> ...
> I suggest here calculating the checksum only if the tree exists:
> if (tree & tvb_bytes_exist(tvb, offset, hlen) {
>            ipsum = ip_checksum(tvb_get_ptr(tvb, offset, hlen), hlen);
>             if(ipsum == 0) {
> ...
>
> I'm aware that later on ipsum is needed:
>   if (ip_defragment && (iph->ip_off & (IP_MF|IP_OFFSET)) &&
>       tvb_bytes_exist(tvb, offset, pinfo->iplen - pinfo->iphdrlen) &&
>       ipsum == 0) {
>
> but here I suggest changing this to:
>
>   if (ip_defragment && (iph->ip_off & (IP_MF|IP_OFFSET)) &&
>       tvb_bytes_exist(tvb, offset, pinfo->iplen - pinfo->iphdrlen) &&
>       (ip_checksum(tvb_get_ptr(tvb, offset, hlen), hlen)) == 0) {
>
> So, if there's no tree and there's no need for defragmentation, the
> checksum is not calculated at all!
> I'd be happy to supply a small patch that does just that, along with
> other minor optimizations to packet-ip.c, if this seems reasonable to
> the list.
>
> Y.

Sounds reasonable. BTW: What's the intentions for this, a slight
performance improvement?

In addition, you might consider adding a Preference setting (it's not
too difficult) to switch off checksum checking completely, just as we
already have it for TCP.

Regards, ULFL

Index: packet-ip.c
===================================================================
--- packet-ip.c	(revision 15985)
+++ packet-ip.c	(working copy)
@@ -72,6 +72,9 @@
 if the packet in the payload has more than 128 bytes */
 static gboolean favor_icmp_mpls_ext = FALSE;
 
+/* Perform IP checksum */
+static gboolean ip_check_checksum = TRUE;
+
 static int proto_ip = -1;
 static int hf_ip_version = -1;
 static int hf_ip_hdr_len = -1;
@@ -375,9 +378,6 @@
     return;
   }
   switch (pd[offset + 9]) {
-    case IP_PROTO_SCTP:
-      ld->sctp++;
-      break;
     case IP_PROTO_TCP:
       ld->tcp++;
       break;
@@ -388,6 +388,9 @@
     case IP_PROTO_ICMPV6:	/* XXX - separate counters? */
       ld->icmp++;
       break;
+    case IP_PROTO_SCTP:
+      ld->sctp++;
+      break;
     case IP_PROTO_OSPF:
       ld->ospf++;
       break;
@@ -911,13 +914,16 @@
     goto end_of_ip;
   }
 
-  if (tree) {
-    proto_tree_add_uint_format(ip_tree, hf_ip_hdr_len, tvb, offset, 1, hlen,
-	"Header length: %u bytes", hlen);
-  }
-
   iph->ip_tos = tvb_get_guint8(tvb, offset + 1);
+  iph->ip_len = tvb_get_ntohs(tvb, offset + 2);
+  iph->ip_id = tvb_get_ntohs(tvb, offset + 4);
+  iph->ip_off = tvb_get_ntohs(tvb, offset + 6);
+  iph->ip_p = tvb_get_guint8(tvb, offset + 9);
+  iph->ip_sum = tvb_get_ntohs(tvb, offset + 10);
+
   if (tree) {
+	proto_tree_add_uint_format(ip_tree, hf_ip_hdr_len, tvb, offset, 1, hlen,
+	"Header length: %u bytes", hlen);
     if (g_ip_dscp_actif) {
       tf = proto_tree_add_uint_format(ip_tree, hf_ip_dsfield, tvb, offset + 1, 1, iph->ip_tos,
 	   "Differentiated Services Field: 0x%02x (DSCP 0x%02x: %s; ECN: 0x%02x)", iph->ip_tos,
@@ -948,7 +954,6 @@
      inside an ICMP datagram; we need to somehow let the
      dissector we call know that, as it might want to avoid
      doing its checksumming. */
-  iph->ip_len = tvb_get_ntohs(tvb, offset + 2);
 
   /* Adjust the length of this tvbuff to include only the IP datagram. */
   set_actual_length(tvb, iph->ip_len);
@@ -964,15 +969,9 @@
     }
     goto end_of_ip;
   }
-  if (tree)
-    proto_tree_add_uint(ip_tree, hf_ip_len, tvb, offset + 2, 2, iph->ip_len);
-
-  iph->ip_id  = tvb_get_ntohs(tvb, offset + 4);
-  if (tree)
-    proto_tree_add_uint(ip_tree, hf_ip_id, tvb, offset + 4, 2, iph->ip_id);
-
-  iph->ip_off = tvb_get_ntohs(tvb, offset + 6);
   if (tree) {
+	proto_tree_add_uint(ip_tree, hf_ip_len, tvb, offset + 2, 2, iph->ip_len);
+	proto_tree_add_uint(ip_tree, hf_ip_id, tvb, offset + 4, 2, iph->ip_id);
     flags = (iph->ip_off & (IP_RF | IP_DF | IP_MF)) >> 12;
     tf = proto_tree_add_uint(ip_tree, hf_ip_flags, tvb, offset + 6, 1, flags);
     field_tree = proto_item_add_subtree(tf, ett_ip_off);
@@ -984,60 +983,55 @@
 
     proto_tree_add_uint(ip_tree, hf_ip_frag_offset, tvb, offset + 6, 2,
       (iph->ip_off & IP_OFFSET)*8);
-  }
-
-  if (tree)
     proto_tree_add_item(ip_tree, hf_ip_ttl, tvb, offset + 8, 1, FALSE);
-
-  iph->ip_p = tvb_get_guint8(tvb, offset + 9);
-  if (tree) {
     proto_tree_add_uint_format(ip_tree, hf_ip_proto, tvb, offset + 9, 1, iph->ip_p,
 	"Protocol: %s (0x%02x)", ipprotostr(iph->ip_p), iph->ip_p);
   }
 
-  iph->ip_sum = tvb_get_ntohs(tvb, offset + 10);
 
   /*
    * If we have the entire IP header available, check the checksum.
    */
-  if (tvb_bytes_exist(tvb, offset, hlen)) {
-    ipsum = ip_checksum(tvb_get_ptr(tvb, offset, hlen), hlen);
-    if (tree) {
-      if (ipsum == 0) {
-	item = proto_tree_add_uint_format(ip_tree, hf_ip_checksum, tvb, offset + 10, 2, iph->ip_sum,
-              "Header checksum: 0x%04x [correct]", iph->ip_sum);
-	checksum_tree = proto_item_add_subtree(item, ett_ip_checksum);
-	item = proto_tree_add_boolean(checksum_tree, hf_ip_checksum_good, tvb, offset + 10, 2, TRUE);
+
+  if (tree) {
+	if(ip_check_checksum && tvb_bytes_exist(tvb, offset, hlen)) {
+		ipsum = ip_checksum(tvb_get_ptr(tvb, offset, hlen), hlen);
+		if (ipsum == 0) {
+			item = proto_tree_add_uint_format(ip_tree, hf_ip_checksum, tvb, offset + 10, 2, iph->ip_sum,
+					  "Header checksum: 0x%04x [correct]", iph->ip_sum);
+			checksum_tree = proto_item_add_subtree(item, ett_ip_checksum);
+			item = proto_tree_add_boolean(checksum_tree, hf_ip_checksum_good, tvb, offset + 10, 2, TRUE);
+			PROTO_ITEM_SET_GENERATED(item);
+			item = proto_tree_add_boolean(checksum_tree, hf_ip_checksum_bad, tvb, offset + 10, 2, FALSE);
+		} else {
+			item = proto_tree_add_uint_format(ip_tree, hf_ip_checksum, tvb, offset + 10, 2, iph->ip_sum,
+				  "Header checksum: 0x%04x [incorrect, should be 0x%04x]", iph->ip_sum,
+			in_cksum_shouldbe(iph->ip_sum, ipsum));
+			checksum_tree = proto_item_add_subtree(item, ett_ip_checksum);
+			item = proto_tree_add_boolean(checksum_tree, hf_ip_checksum_good, tvb, offset + 10, 2, FALSE);
+			PROTO_ITEM_SET_GENERATED(item);
+			item = proto_tree_add_boolean(checksum_tree, hf_ip_checksum_bad, tvb, offset + 10, 2, TRUE);
+		}
+	} else {
+		item = proto_tree_add_uint(ip_tree, hf_ip_checksum, tvb, offset + 10, 2, iph->ip_sum);
+		checksum_tree = proto_item_add_subtree(item, ett_ip_checksum);
+		item = proto_tree_add_boolean(checksum_tree, hf_ip_checksum_good, tvb, offset + 10, 2, FALSE);
+		PROTO_ITEM_SET_GENERATED(item);
+		item = proto_tree_add_boolean(checksum_tree, hf_ip_checksum_bad, tvb, offset + 10, 2, FALSE);
+	}
 	PROTO_ITEM_SET_GENERATED(item);
-	item = proto_tree_add_boolean(checksum_tree, hf_ip_checksum_bad, tvb, offset + 10, 2, FALSE);
-	PROTO_ITEM_SET_GENERATED(item);
-      } else {
-	item = proto_tree_add_uint_format(ip_tree, hf_ip_checksum, tvb, offset + 10, 2, iph->ip_sum,
-          "Header checksum: 0x%04x [incorrect, should be 0x%04x]", iph->ip_sum,
-	  in_cksum_shouldbe(iph->ip_sum, ipsum));
-	checksum_tree = proto_item_add_subtree(item, ett_ip_checksum);
-	item = proto_tree_add_boolean(checksum_tree, hf_ip_checksum_good, tvb, offset + 10, 2, FALSE);
-	PROTO_ITEM_SET_GENERATED(item);
-	item = proto_tree_add_boolean(checksum_tree, hf_ip_checksum_bad, tvb, offset + 10, 2, TRUE);
-	PROTO_ITEM_SET_GENERATED(item);
-      }
-    }
-  } else {
-    ipsum = 0;
-    if (tree) {
-      item = proto_tree_add_uint(ip_tree, hf_ip_checksum, tvb, offset + 10, 2, iph->ip_sum);
-      checksum_tree = proto_item_add_subtree(item, ett_ip_checksum);
-      item = proto_tree_add_boolean(checksum_tree, hf_ip_checksum_good, tvb, offset + 10, 2, FALSE);
-      PROTO_ITEM_SET_GENERATED(item);
-      item = proto_tree_add_boolean(checksum_tree, hf_ip_checksum_bad, tvb, offset + 10, 2, FALSE);
-      PROTO_ITEM_SET_GENERATED(item);
-    }
   }
+
   src_addr = tvb_get_ptr(tvb, offset + IPH_SRC, 4);
-  src32 = tvb_get_ntohl(tvb, offset + IPH_SRC);
   SET_ADDRESS(&pinfo->net_src, AT_IPv4, 4, src_addr);
   SET_ADDRESS(&pinfo->src, AT_IPv4, 4, src_addr);
   SET_ADDRESS(&iph->ip_src, AT_IPv4, 4, src_addr);
+
+  dst_addr = tvb_get_ptr(tvb, offset + IPH_DST, 4);
+  SET_ADDRESS(&pinfo->net_dst, AT_IPv4, 4, dst_addr);
+  SET_ADDRESS(&pinfo->dst, AT_IPv4, 4, dst_addr);
+  SET_ADDRESS(&iph->ip_dst, AT_IPv4, 4, dst_addr);
+
   if (tree) {
     memcpy(&addr, iph->ip_src.data, 4);
     if (ip_summary_in_tree) {
@@ -1053,14 +1047,7 @@
     item = proto_tree_add_string(ip_tree, hf_ip_host, tvb, offset + 12, 4, get_hostname(addr));
     PROTO_ITEM_SET_GENERATED(item);
     PROTO_ITEM_SET_HIDDEN(item);
-  }
-  dst_addr = tvb_get_ptr(tvb, offset + IPH_DST, 4);
-  dst32 = tvb_get_ntohl(tvb, offset + IPH_DST);
-  SET_ADDRESS(&pinfo->net_dst, AT_IPv4, 4, dst_addr);
-  SET_ADDRESS(&pinfo->dst, AT_IPv4, 4, dst_addr);
-  SET_ADDRESS(&iph->ip_dst, AT_IPv4, 4, dst_addr);
 
-  if (tree) {
     memcpy(&addr, iph->ip_dst.data, 4);
     if (ip_summary_in_tree) {
       proto_item_append_text(ti, ", Dst: %s (%s)",
@@ -1075,9 +1062,7 @@
     item = proto_tree_add_string(ip_tree, hf_ip_host, tvb, offset + 16, 4, get_hostname(addr));
     PROTO_ITEM_SET_GENERATED(item);
     PROTO_ITEM_SET_HIDDEN(item);
-  }
 
-  if (tree) {
     /* Decode IP options, if any. */
     if (hlen > IPH_MIN_LEN) {
       /* There's more than just the fixed-length header.  Decode the
@@ -1108,8 +1093,12 @@
   save_fragmented = pinfo->fragmented;
   if (ip_defragment && (iph->ip_off & (IP_MF|IP_OFFSET)) &&
       tvb_bytes_exist(tvb, offset, pinfo->iplen - pinfo->iphdrlen) &&
-      ipsum == 0) {
-    ipfd_head = fragment_add_check(tvb, offset, pinfo, 
+      ((ip_check_checksum == 0) || 
+      	(tvb_bytes_exist(tvb, offset -hlen, hlen) && ip_checksum(tvb_get_ptr(tvb, offset -hlen, hlen), hlen) == 0))) {
+      
+		src32 = tvb_get_ntohl(tvb, offset + IPH_SRC);
+		dst32 = tvb_get_ntohl(tvb, offset + IPH_DST);
+		ipfd_head = fragment_add_check(tvb, offset, pinfo, 
 			     iph->ip_id ^ src32 ^ dst32,
 			     ip_fragment_table,
 			     ip_reassembled_table,
@@ -2137,6 +2126,10 @@
 	    "Show IP summary in protocol tree",
 	    "Whether the IP summary line should be shown in the protocol tree",
 	    &ip_summary_in_tree);
+	prefs_register_bool_preference(ip_module, "check_checksum" ,
+		  "Validate the IP checksum if possible",
+		  "Whether to validate the IP checksum",
+		  &ip_check_checksum);
 
 	register_dissector("ip", dissect_ip, proto_ip);
 	register_init_routine(ip_defragment_init);