The fragmentation code was replaced in 9b3eab61754d74a93c9840c296013fe3b4a1b606 ("batman-adv: Receive fragmented packets and merge"). The new code provided a mostly unused parameter skb for the merging function. It is used inside the function to calculate the additionally needed skb tailroom. But instead of increasing its own tailroom, it is only increasing the tailroom of the first queued skb. This is not correct in most situations because the first queued entry can be a different one than the parameter.

An observed problem was:

1. packet with size 104, total_size 1464, fragno 1 was received - packet is queued 2. packet with size 1400, total_size 1464, fragno 0 was received - packet is queued at the end of the list 3. enough data was received and can be given to the merge function (1464 == (1400 - 20) + (104 - 20)) - merge functions gets 1400 byte large packet as skb argument 4. merge function gets first entry in queue (104 byte) - stored as skb_out 5. merge function calculates the required extra tail as total_size - skb->len - pskb_expand_head tail of skb_out with 64 bytes 6. merge function tries to squeeze the extra 1380 bytes from the second queued skb (1400 byte aka skb parameter) in the 64 extra tail bytes of skb_out

Instead calculate the extra required tail bytes for skb_out also using skb_out instead of using the parameter skb. The skb parameter is only used to get the total_size from the last received packet. This is also the total_size used to decide that all fragments were received.

Signed-off-by: Sven Eckelmann sven@narfation.org Reported-by: Philipp Psurek philipp.psurek@gmail.com --- This is a minimized version which doesn't require the patch "[PATCH-maint] batman-adv: Check total_size when reassembling fragments".

It is only compile tested --- fragmentation.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fragmentation.c b/fragmentation.c index f14e54a..af16844 100644 --- a/fragmentation.c +++ b/fragmentation.c @@ -247,7 +247,7 @@ batadv_frag_merge_packets(struct hlist_head *chain, struct sk_buff *skb) kfree(entry);

/* Make room for the rest of the fragments. */ - if (pskb_expand_head(skb_out, 0, size - skb->len, GFP_ATOMIC) < 0) { + if (pskb_expand_head(skb_out, 0, size - skb_out->len, GFP_ATOMIC) < 0) { kfree_skb(skb_out); skb_out = NULL; goto free;

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; };

/**