[B.A.T.M.A.N.] [PATCH 1/2] batman-adv: fix aggregation timing bug

[B.A.T.M.A.N.] [PATCH 1/2] batman-adv: fix aggregation timing bug

[Linus Lüssing]

From: Marek Lindner <lindner_marek@yahoo.de>

batman-adv aggregates routing packets to reduce the number of packets in
the air. Every outgoing packet is compared with other packets in the
buffer to determine whether it can be aggregated or not. Packets sent
at a lower interval can be held back longer to maximize the aggregation.
Due to insufficient checking batman-adv held back all packets for a
certain time depending on its own lowest interval rate which slowed
down all other nodes.

Reported-by: Linus Luessing <linus.luessing@web.de>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
 aggregation.c |   12 ++++++++++++
 send.c        |   10 +---------
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/batman-adv-kernelland/aggregation.c b/batman-adv-kernelland/aggregation.c
index a829814..0635b5a 100644
--- a/batman-adv-kernelland/aggregation.c
+++ b/batman-adv-kernelland/aggregation.c
@@ -52,6 +52,8 @@ static bool can_aggregate_with(struct batman_packet *new_batman_packet,
 	 */
 
 	if (time_before(send_time, forw_packet->send_time) &&
+	    time_after_eq(send_time + msecs_to_jiffies(MAX_AGGREGATION_MS),
+					forw_packet->send_time) &&
 	    (aggregated_bytes <= MAX_AGGREGATION_BYTES)) {
 
 		/**
@@ -196,6 +198,16 @@ void add_bat_packet_to_list(unsigned char *packet_buff, int packet_len,
 	if (forw_packet_aggr == NULL) {
 		/* the following section can run without the lock */
 		spin_unlock_irqrestore(&forw_bat_list_lock, flags);
+
+		/**
+		 * if we could not aggregate this packet with one of the others
+		 * we hold it back for a while, so that it might be aggregated
+		 * later on
+		 */
+		if ((!own_packet) &&
+		    (atomic_read(&bat_priv->aggregation_enabled)))
+			send_time += msecs_to_jiffies(MAX_AGGREGATION_MS);
+
 		new_aggregated_packet(packet_buff, packet_len,
 				      send_time, direct_link,
 				      if_incoming, own_packet);
diff --git a/batman-adv-kernelland/send.c b/batman-adv-kernelland/send.c
index 1c82f43..4320855 100644
--- a/batman-adv-kernelland/send.c
+++ b/batman-adv-kernelland/send.c
@@ -49,15 +49,7 @@ static unsigned long own_send_time(void)
 /* when do we schedule a forwarded packet to be sent */
 static unsigned long forward_send_time(struct bat_priv *bat_priv)
 {
-	unsigned long send_time = jiffies; /* Starting now plus... */
-
-	if (atomic_read(&bat_priv->aggregation_enabled))
-		send_time += (((MAX_AGGREGATION_MS - (JITTER/2) +
-				(random32() % JITTER)) * HZ) / 1000);
-	else
-		send_time += (((random32() % (JITTER/2)) * HZ) / 1000);
-
-	return send_time;
+	return jiffies + (((random32() % (JITTER/2)) * HZ) / 1000);
 }
 
 /* send out an already prepared packet to the given address via the
-- 
1.7.0

[B.A.T.M.A.N.] [PATCH 2/2] batman-adv: Fix aggregation direct-link bug

[Linus Lüssing]

So far, neighbour's secondary interface OGMs can involuntarily
piggyback on primary interface OGMs that arrived on the same secondary
interface before. Secondary interface OGMs should NEVER leave their
direct neighbour broadcast domain! This patch ensures that secondary
interface OGMs can only be aggregated to other secondary interface OGMs.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 aggregation.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/batman-adv-kernelland/aggregation.c b/batman-adv-kernelland/aggregation.c
index 0635b5a..567c31a 100644
--- a/batman-adv-kernelland/aggregation.c
+++ b/batman-adv-kernelland/aggregation.c
@@ -81,9 +81,15 @@ static bool can_aggregate_with(struct batman_packet *new_batman_packet,
 		 * interface only - we still can aggregate */
 		if ((directlink) &&
 		    (new_batman_packet->ttl == 1) &&
-		    (forw_packet->if_incoming == if_incoming))
+		    (forw_packet->if_incoming == if_incoming) &&
+
+		    /* packets from direct neighbors or
+		     * own secondary interface packets
+		     * (= secondary interface packets in general) */
+		    (batman_packet->flags & DIRECTLINK ||
+		     (forw_packet->own &&
+		      forw_packet->if_incoming->if_num != 0)))
 			return true;
-
 	}
 
 	return false;
-- 
1.7.0

Re: [B.A.T.M.A.N.] [PATCH 2/2] batman-adv: Fix aggregation direct-link bug

[Linus Lüssing]

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

And there are two more things I'm wondering about:
- Why should primary interface OGMs from a different originator
  which already travelled over 49 hops not be aggregated to other
  primary interface OGMs?
- Why should secondary interface OGMs from neighbours not be
  aggregated on our own, neighboring secondary interface OGMs?
As far as I can tell, those two combinations are not being
aggregated at the moment. Is there a reason why they shouldn't?

Cheers, Linus

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCH 2/2] batman-adv: Fix aggregation direct-link bug

[Marek Lindner]

Hi,

thanks for your patch and for testing mine. I applied both of them.

> And there are two more things I'm wondering about:
> - Why should primary interface OGMs from a different originator
>   which already travelled over 49 hops not be aggregated to other
>   primary interface OGMs?
> - Why should secondary interface OGMs from neighbours not be
>   aggregated on our own, neighboring secondary interface OGMs?
> As far as I can tell, those two combinations are not being
> aggregated at the moment. Is there a reason why they shouldn't?

Well, well - we happily apply more of your patches.  ;-)

Cheers,
Marek