Re: [B.A.T.M.A.N.] [RFC] batman-adv: Calculate extra tail size based on queued fragments

[Philipp Psurek <philipp.psurek@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6921 bytes --]

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
===================> |  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;
 };
 
 /**





[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]