Simon Wunderlich wrote:
Hello Andreas,
thank you very much for your patch. I've tested the patchset in my KVM setup. The fragmentation seems to work fine over one or multiple hops if i use the same MTUs on all hosts, but some corner cases might not bet handled yet.
For example:
Host 1 has one interface with MTU 1500 to host 2. Host 2 has two interfaces, one with MTU 1500 (to host 1) and one interface with MTU 1470 (to host 3), which might be a VPN connection. Host 3 has one interface with MTU 1470 to host 2.
Now i can send packets with size 1470 or 1440 from host 1 to host 3 without problems, but packets with size 1450 fail. The reason for this might be that host 1 sends the packet as is without fragmentation, but host 2 can not forward it because the MTU (1470) is too low, but does not fragment it. The packets therefore get dropped. (note: when i say packets of size xxxx i do: ping -M do -s xxxx)
I've checked some parts of the patch yesterday and would also say that this is a problem. Just to explain it a little bit further how I understood it (I am a little bit confused about your ping example because ping -s 1440 would create a packet with the size
1440 (data) + 8 (icmp header) + 20 (ip header) + 14 (included ethernet header) + 9 (batman-adv unicast header) ====== 1491
Which means that we are over the mtu of 1470 between node2 and node3. Maybe you wanted to say that size xxxx means `ping -M do -s xxxx-28`. But that would also mean that 1470 is not big enough to get splitted in node1.
So to my test scenario:
node1 <- mtu 1500 -> node2
Each soft-interface (bat0) has now a interface_tx function which is called when data must be send over it (directly send and not when data must get routed). When node1 sends a packet which would have more than 1500 bytes (with batman-adv-unicast-header and the included ethernet header) we would split in two unicast-frag-packet with roughly the half size (size / 2 + 18 bytes unicast-frag packet).
So here an example on node1 to node2: * ping -s 1472 (would need mtu of 1523) - two packets (775, 775) * ping -s 1440 (would need mtu of 1491) - one packet (1491) * ping -s 1449 (would need mtu of 1500) - one packet (1500) * ping -s 1450 (would need mtu of 1501) - one packet (1501) !!!!! error !!!! * ping -s 1451 (would need mtu of 1502) - one packet (1502) !!!!! error !!!! * ping -s 1452 (would need mtu of 1503) - one packet (1503) !!!!! error !!!! * ping -s 1453 (would need mtu of 1504) - one packet (1504) !!!!! error !!!! * ping -s 1454 (would need mtu of 1505) - one packet (1505) !!!!! error !!!! * ping -s 1455 (would need mtu of 1506) - two packets (766, 767)
We only accept packets with the the maximum data size of 1500 and 1501 is over that.
It is quite easy to fix, because you misused sizeof in you check. So here some kind of patch for your patch (as hint and not really a patch) :)
Best regards, Sven
diff --git a/batman-adv/fragmentation.c b/batman-adv/fragmentation.c index f36ad0e..1a33eab 100644 --- a/batman-adv/fragmentation.c +++ b/batman-adv/fragmentation.c @@ -74,8 +74,7 @@ void create_frag_buffer(struct list_head *head) struct frag_packet_list_entry *tfp;
for (i = 0; i < FRAG_BUFFER_SIZE; i++) { - tfp = kmalloc(sizeof(struct frag_packet_list_entry), - GFP_ATOMIC); + tfp = kmalloc(sizeof(*tfp), GFP_ATOMIC); tfp->skb = NULL; tfp->seqno = 0; INIT_LIST_HEAD(&tfp->list); diff --git a/batman-adv/soft-interface.c b/batman-adv/soft-interface.c index ccc97c6..8e3cc40 100644 --- a/batman-adv/soft-interface.c +++ b/batman-adv/soft-interface.c @@ -221,7 +221,7 @@ int interface_tx(struct sk_buff *skb, struct net_device *dev) goto dropped;
if (atomic_read(&bat_priv->frag_enabled) && - data_len + sizeof(unicast_packet) > + data_len + sizeof(*unicast_packet) > batman_if->net_dev->mtu) {
hdr_len = sizeof(struct unicast_frag_packet); @@ -247,8 +247,7 @@ int interface_tx(struct sk_buff *skb, struct net_device *dev) batman_if->net_dev->dev_addr, ETH_ALEN); memcpy(ucast_frag1->dest, orig_node->orig, ETH_ALEN);
- memcpy(ucast_frag2, ucast_frag1, - sizeof(struct unicast_frag_packet)); + memcpy(ucast_frag2, ucast_frag1, sizeof(*ucast_frag1));
ucast_frag1->flags |= UNI_FRAG_HEAD; ucast_frag2->flags &= ~UNI_FRAG_HEAD;