Issue #247 review

Haven't really found the bug, but I'm questioning some of the code
anyway. Uploading tweaks.

While computing the nexthdr of a given IPv6 packet, the code was
performing the following assignment:

	Local network header pointer = skb_network_header(skb) + local network header offset

(By "local" I mean the scope of the variable.)

This operation is reused for outer and inner packets, and strongly
requires and assumes that skb_network_offset(skb) is zero:

- For outer packets, it doesn't make sense to add anything to
  skb_network_header(skb) to get a pointer to the network header,
  and "local network header offset" is skb_network_offset(skb).
- For inner packets, "local network header offset" is an offset
  from skb->data, which means that skb_network_header(skb) needs
  to equal skb->data. (Hence, skb_network_offset(skb) = 0.)

Which is supposed to always the case, which is the reason why this
isn't really a bug, unless the kernel itself has another bug to
match.

I thought about putting a validation in place to ensure that the
network header offset is zero for all outer packets, but then I
realized that the whole reason why I was making this so convoluted
was because I didn't want the code to touch skb->data. (Instead
relying on available kernel API.) I noticed that there is at least
one function around (offset_to_ptr()) that does it anyway.
(And I don't think that there is a way around that unless the
packet.c module is completely rewritten.)

So the new code is

	Local network header pointer = skb->data + local network header offset

Which actually expresses the intent much more clearly.

The following are old assumptions that stand valid still:

- The skb_network_offset() offset is relative to skb->data.
- The skb_header_pointer() offset is relative to skb->data.

The following is no longer an assumption and Jool should continue
working fine if the kernel happens to break it:

- skb_network_offset(skb) during a packet's entrance to a Netfilter
  hook is zero.

So that's what this commit comes down to; I removed an potential
future bug opportunity and made the code clearer as a bonus.

mod/common/packet.c

@@ -10,6 +10,10 @@
 #include "nat64/mod/common/icmp_wrapper.h"
 #include "nat64/mod/common/stats.h"
 
+/*
+ * Note: Offsets need to be relative to skb->data because that's how
+ * skb_header_pointer() works.
+ */
 struct pkt_metadata {
 	bool has_frag_hdr;
 	/* Offset is from skb->data. Do not use if has_frag_hdr is false. */
@@ -89,10 +93,47 @@ static int fail_if_shared(struct sk_buff *skb)
 	return 0;
 }
 
+/*
+ * Jool doesn't like making assumptions, but it certainly needs to make a few.
+ * One of them is that the skb_network_offset() offset is meant to be relative
+ * to skb->data. Thing is, this should be part of the contract of the function,
+ * but I don't see it set in stone anywhere. (Not even in the "How skbs work"
+ * guide.)
+ *
+ * Now, it's pretty obvious that this is the case for all such skbuff.h
+ * non-paged offsets at present, but I'm keeping my buttcheeks tight in case
+ * they are meant to be relative to a common pivot rather than a specific one.
+ */
+static int fail_if_broken_offset(struct sk_buff *skb)
+{
+	if (WARN(skb_network_offset(skb) != (skb_network_header(skb) - skb->data),
+			"The packet's network header offset is not relative to skb->data.\n"
+			"Translating this packet would break Jool, so dropping."))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int paranoid_validations(struct sk_buff *skb, size_t basic_hdr_size)
+{
+	int error;
+
+	error = fail_if_shared(skb);
+	if (error)
+		return error;
+	error = fail_if_broken_offset(skb);
+	if (error)
+		return error;
+	if (!pskb_may_pull(skb, basic_hdr_size))
+		return -EINVAL;
+
+	return 0;
+}
+
 /**
  * Walks through @skb's headers, collecting data and adding it to @meta.
  *
- * @hdr6_offset number of bytes between skb_network_header(skb) and the IPv6 header.
+ * @hdr6_offset number of bytes between skb->data and the IPv6 header.
  *
  * BTW: You might want to read summarize_skb4() first, since it's a lot simpler.
  */
@@ -113,7 +154,7 @@ static int summarize_skb6(struct sk_buff *skb, unsigned int hdr6_offset, struct
 	unsigned int offset;
 	bool is_first = true;
 
-	nexthdr = ((struct ipv6hdr *) (skb_network_header(skb) + hdr6_offset))->nexthdr;
+	nexthdr = ((struct ipv6hdr *)(skb->data + hdr6_offset))->nexthdr;
 	offset = hdr6_offset + sizeof(struct ipv6hdr);
 
 	meta->has_frag_hdr = false;
@@ -284,7 +325,7 @@ int pkt_init_ipv6(struct packet *pkt, struct sk_buff *skb)
 	getnstimeofday(&pkt->start_time);
 #endif
 
-	error = fail_if_shared(skb);
+	error = paranoid_validations(skb, sizeof(struct ipv6hdr));
 	if (error)
 		return error;
 
@@ -451,7 +492,7 @@ int pkt_init_ipv4(struct packet *pkt, struct sk_buff *skb)
 	getnstimeofday(&pkt->start_time);
 #endif
 
-	error = fail_if_shared(skb);
+	error = paranoid_validations(skb, sizeof(struct iphdr));
 	if (error)
 		return error;
 

mod/stateful/Kbuild

@@ -1,4 +1,5 @@
-#CC=cgcc
+#CC=cgcc    # More healthy warnings.
+#CC=gcc-4.9 # Needed to cross-compile Jool for an older machine.
 
 ccflags-y := -I$(src)/../../include $(JOOL_FLAGS)
 #ccflags-y += -Wfatal-errors

mod/stateless/Kbuild

@@ -1,4 +1,5 @@
-#CC=cgcc
+#CC=cgcc    # More healthy warnings.
+#CC=gcc-4.9 # Needed to cross-compile Jool for an older machine.
 
 ccflags-y := -I$(src)/../../include $(JOOL_FLAGS)
 #ccflags-y += -Wfatal-errors