[RFC] batman-adv: Calculate extra tail size based on queued fragments

Message ID 1417456897.2788.2.camel@gmail.com (mailing list archive)
State RFC, archived
Headers

Commit Message

Philipp Psurek Dec. 1, 2014, 6:01 p.m. UTC
  Hi Martin, hi Sven, hi all

Thank you very much for analysing, describing and hacking on our issue.
I’m sorry to ruin your other thread Sven ;-)

You are asking about our set-up in other threads. To save you from
reading the commands I posted for Martin, let me make a quick draw:

This bug posted here: 
                     _________                                      User
                     |  NAT  |                     |------------|   on
  

Comments

Sven Eckelmann Dec. 1, 2014, 6:36 p.m. UTC | #1
On Monday 01 December 2014 19:01:37 Philipp Psurek wrote:
> I’d like to inform you that I implement the patch posted with this mail
> 18 h ago. It is a mix of the patch Martin gave me earlier and your 1st
> patch from “Calculate extra tail size based on queued fragments”. There
> was no crash, but this means nothing. Now I see, there are many, many
> patches which solves the bug with different approaches. Please tell me
> exactly which one I should test because I don't speak any C.

The mixed patch is a good start and doesn't really have to be changed for your 
test of this specific problem. It misses parts but at least the important 
change is included. The "final" patches which may need to be tested are (and 
these should be merged into batman-adv maint):

https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2014-December/012613.html
(this is already in your patch)

https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2014-December/012620.html
(this is not really your problem but I doubt that this is related to any 
problem you see - but just keep it in mind)

Optional and not relevant for your problem (these should be merged _together_ 
into batman-adv master):

https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2014-December/012614.html
https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2014-December/012615.html
(these are partially included in your patch - unfortunately your patch misses 
the part from 012614. And 012615 requires 012614 to work correctly. So just 
cross your fingers that no one exploits this).

So if you want to have a clean test setup then please include the two non-
optional patches (012613, 012620). It is also ok to just include 012613 
because this one is most likely related to your problem.

The problem in 012620 should only happen when the difference between batman-
adv MTU and device MTU is far too big. A difference of around 1400 bytes. Or 
if the device MTU is smaller than 1400 then the batman-adv MTU has to be 
around twice as big as the device MTU. This doesn't seem to be the case here 
according to your description.

Kind regards,
	Sven
  
Philipp Psurek Dec. 1, 2014, 8:40 p.m. UTC | #2
Thank you Sven and thank you Martin

for explaining and guiding me through this bug, for identifying and
resolving the issue and also Marek, Linus, Antonio and all of you behind
B.A.T.M.A.N.-adv, for your work and your time and your patience solving
problems and making great meshing protocol. I can’t imagine our mesh
community without your software. Thank you very much.

I wish you the best, fun and happy hacking

Philipp
  

Patch

===================> |  VM   | <=================> | Mesh-Nodes |<--AP
                     |_______|                     |____________|   MTU
ipip-Tunnel to our VM   NAT  bat15 over fastd-Tunnel   ^           1500
which is connected to         1:n to meshing nodes     | NC-disabled
       our ISP                                         | bat15-Link to
  MTU 1400 is set to         default fastd MTU 1426    | other nodes
 reproduce the bug of                                  | wrong MTU 1528
    the other VMs                                      |  [from bat14]
 no batman involved in                                 |(will be changed
    this tunnel                                        |  to 1560 with 
                                                       | next firmware)


On the other VMs, I can't analyse the bug, there is a GRE-tunnel to ISPs
routers with fixed MTU of 1400. Over these tunnels there are no batman
packages sent through. Only IPv4 and IPv6. The VM do the NAT to a public
IP.

I’m sorry making you such a mess with the MTU. Nowadays many communitys
do an outsourcing of the gateways to the internet. They provide more
throughput and a coherent batman cloud if there are white spaces without
nodes on the map.

I’d like to inform you that I implement the patch posted with this mail
18 h ago. It is a mix of the patch Martin gave me earlier and your 1st
patch from “Calculate extra tail size based on queued fragments”. There
was no crash, but this means nothing. Now I see, there are many, many
patches which solves the bug with different approaches. Please tell me
exactly which one I should test because I don't speak any C.

Best regards and happy hacking

Philipp

________________________
Freifunk Rheinland e. V.
– Funkzelle Wuppertal –



# batctl -v
batctl 2014.3.0 [batman-adv: 2014.3.0-44-g650251a-dirty]

diff --git a/fragmentation.c b/fragmentation.c
index 362e91a..743d0d3 100644
--- a/fragmentation.c
+++ b/fragmentation.c
@@ -217,6 +217,18 @@  err:
 	return ret;
 }
 
+static inline void batadv_frag_dbg_entry(struct batadv_frag_list_entry *entry)
+{
+	struct sk_buff *skb = entry->skb;
+	struct batadv_frag_packet *packet;
+
+	packet = (struct batadv_frag_packet *)skb->data;
+
+	printk(KERN_DEBUG "  skb->len: %u, skb->tailroom: %u, pkt->pkt_type: %hhu, pkt->version: %hhu, pkt->no: %hhu, pkt->seqno: %hu, pkt->total_size: %hu\n",
+	      skb->len, skb_tailroom(skb), packet->packet_type, packet->version,
+	      packet->no, ntohs(packet->seqno), ntohs(packet->total_size));
+}
+
 /**
  * batadv_frag_merge_packets - merge a chain of fragments
  * @chain: head of chain with fragments
@@ -228,18 +240,14 @@  err:
  * Returns the merged skb or NULL on error.
  */
 static struct sk_buff *
-batadv_frag_merge_packets(struct hlist_head *chain, struct sk_buff *skb)
+batadv_frag_merge_packets(struct hlist_head *chain)
 {
 	struct batadv_frag_packet *packet;
-	struct batadv_frag_list_entry *entry;
+	struct batadv_frag_list_entry *entry, dbg_entry;
+	struct batadv_frag_table_entry *table_entry;
 	struct sk_buff *skb_out = NULL;
-	int size, hdr_size = sizeof(struct batadv_frag_packet);
-
-	/* Make sure incoming skb has non-bogus data. */
-	packet = (struct batadv_frag_packet *)skb->data;
-	size = ntohs(packet->total_size);
-	if (size > batadv_frag_size_limit())
-		goto free;
+	int size, hdr_size = sizeof(struct batadv_frag_packet), i = 0;
+	int extra_tail;
 
 	/* Remove first entry, as this is the destination for the rest of the
 	 * fragments.
@@ -247,13 +255,26 @@  batadv_frag_merge_packets(struct hlist_head *chain, struct sk_buff *skb)
 	entry = hlist_entry(chain->first, struct batadv_frag_list_entry, list);
 	hlist_del(&entry->list);
 	skb_out = entry->skb;
+	memcpy(&dbg_entry, entry, sizeof(dbg_entry));
 	kfree(entry);
 
+//	if (size < skb->len)
+//		goto debug;
+//
+//	if (size < skb_out->len)
+//		goto debug;
+//
+	packet = (struct batadv_frag_packet *)skb_out->data;
+	size = ntohs(packet->total_size);
+
 	/* Make room for the rest of the fragments. */
-	if (pskb_expand_head(skb_out, 0, size - skb->len, GFP_ATOMIC) < 0) {
-		kfree_skb(skb_out);
-		skb_out = NULL;
-		goto free;
+	if (size > skb_out->len) {
+		extra_tail = size - skb_out->len;
+		if (pskb_expand_head(skb_out, 0, extra_tail, GFP_ATOMIC) < 0) {
+			kfree_skb(skb_out);
+			skb_out = NULL;
+			goto free;
+		}
 	}
 
 	/* Move the existing MAC header to just before the payload. (Override
@@ -268,6 +289,11 @@  batadv_frag_merge_packets(struct hlist_head *chain, struct sk_buff *skb)
 	/* Copy the payload of the each fragment into the last skb */
 	hlist_for_each_entry(entry, chain, list) {
 		size = entry->skb->len - hdr_size;
+		i++;
+
+		if (skb_tailroom(skb_out) < size)
+			goto debug;
+
 		memcpy(skb_put(skb_out, size), entry->skb->data + hdr_size,
 		       size);
 	}
@@ -276,6 +302,19 @@  free:
 	/* Locking is not needed, because 'chain' is not part of any orig. */
 	batadv_frag_clear_chain(chain);
 	return skb_out;
+
+debug:
+	table_entry = container_of(chain, struct batadv_frag_table_entry, head);
+	printk(KERN_DEBUG "batadv_frag_merge_packets: i: %i, size: %i, entry->seqno: %hu, entry->size: %hu, entry->total_size: %hu\n",
+	       i, size, table_entry->seqno, table_entry->size,
+	       table_entry->total_size);
+	batadv_frag_dbg_entry(&dbg_entry);
+
+	hlist_for_each_entry(entry, chain, list)
+		batadv_frag_dbg_entry(entry);
+
+	batadv_frag_clear_chain(chain);
+	return NULL;
 }
 
 /**
@@ -304,7 +343,7 @@  bool batadv_frag_skb_buffer(struct sk_buff **skb,
 	if (hlist_empty(&head))
 		goto out;
 
-	skb_out = batadv_frag_merge_packets(&head, *skb);
+	skb_out = batadv_frag_merge_packets(&head);
 	if (!skb_out)
 		goto out_err;
 
diff --git a/types.h b/types.h
index 462a70c..c4d7d24 100644
--- a/types.h
+++ b/types.h
@@ -132,6 +132,7 @@  struct batadv_orig_ifinfo {
  * @timestamp: time (jiffie) of last received fragment
  * @seqno: sequence number of the fragments in the list
  * @size: accumulated size of packets in list
+ * @total_size: expected size of the assembled packet
  */
 struct batadv_frag_table_entry {
 	struct hlist_head head;
@@ -139,6 +140,7 @@  struct batadv_frag_table_entry {
 	unsigned long timestamp;
 	uint16_t seqno;
 	uint16_t size;
+	uint16_t total_size;
 };
 
 /**