[B.A.T.M.A.N.] Strange sequence numbers

[B.A.T.M.A.N.] Strange sequence numbers

[Kevin Steen]

I've been lurking on this list for a few weeks, but finally tried
compiling and running the batman-adv module from the svn sources.

It seems to work fine between my two test machines so far, but I noticed
some weird sequence numbers when doing a dump with 'batctl td'. In the
sequence below, I had 'rover' running and active on rover's eth0, then I
activated the eth1 interface on hilltop for a few seconds, then
de-activated it. 

It seems like when rover re-sends the OGM from hilltop, it gets the
sequence number wrong (1->256, 2->512, 3->768). Is this my lack of
understanding of how it should work, a bug in batctl's display or a bug
in batman-adv?

rover is kernel 2.6.31-20-generic (Ubuntu karmic),
hilltop is kernel 2.6.26-2-686 (Debian lenny)

$ ./batctl td eth1
23:37:22.022554 BAT rover-eth0: OGM via neigh rover-eth0, seqno 481, tq 255, ttl 50, v 9, flags [..F], length 42
23:37:23.002547 BAT rover-eth0: OGM via neigh rover-eth0, seqno 482, tq 255, ttl 50, v 9, flags [..F], length 42
23:37:23.982469 BAT rover-eth0: OGM via neigh rover-eth0, seqno 483, tq 255, ttl 50, v 9, flags [..F], length 42
23:37:24.962417 BAT rover-eth0: OGM via neigh rover-eth0, seqno 484, tq 255, ttl 50, v 9, flags [..F], length 42
23:37:25.962320 BAT rover-eth0: OGM via neigh rover-eth0, seqno 485, tq 255, ttl 50, v 9, flags [..F], length 42
23:37:26.943490 BAT rover-eth0: OGM via neigh rover-eth0, seqno 486, tq   0, ttl 49, v 9, flags [D..], length 42
23:37:27.041437 BAT rover-eth0: OGM via neigh hilltop-eth1, seqno 486, tq   0, ttl 49, v 9, flags [D..], length 28
23:37:27.893422 BAT hilltop-eth1: OGM via neigh hilltop-eth1, seqno 1, tq 255, ttl 50, v 9, flags [..F], length 22
23:37:27.922233 BAT rover-eth0: OGM via neigh rover-eth0, seqno 487, tq   0, ttl 49, v 9, flags [D..], length 42
23:37:27.978267 BAT hilltop-eth1: OGM via neigh rover-eth0, seqno 256, tq  10, ttl 49, v 9, flags [D..], length 42
23:37:28.025425 BAT rover-eth0: OGM via neigh hilltop-eth1, seqno 487, tq   0, ttl 49, v 9, flags [D..], length 28
23:37:28.873428 BAT hilltop-eth1: OGM via neigh hilltop-eth1, seqno 2, tq 255, ttl 50, v 9, flags [..F], length 28
23:37:28.902203 BAT rover-eth0: OGM via neigh rover-eth0, seqno 488, tq  10, ttl 49, v 9, flags [D..], length 42
23:37:28.958188 BAT hilltop-eth1: OGM via neigh rover-eth0, seqno 512, tq  21, ttl 49, v 9, flags [D..], length 42
23:37:29.005443 BAT rover-eth0: OGM via neigh hilltop-eth1, seqno 488, tq  10, ttl 49, v 9, flags [D..], length 28
23:37:29.873428 BAT hilltop-eth1: OGM via neigh hilltop-eth1, seqno 3, tq 255, ttl 50, v 9, flags [..F], length 28
23:37:29.902152 BAT rover-eth0: OGM via neigh rover-eth0, seqno 489, tq  22, ttl 49, v 9, flags [D..], length 42
23:37:29.970180 BAT hilltop-eth1: OGM via neigh rover-eth0, seqno 768, tq  30, ttl 49, v 9, flags [D..], length 42
23:37:30.009433 BAT rover-eth0: OGM via neigh hilltop-eth1, seqno 489, tq  22, ttl 49, v 9, flags [D..], length 28
23:37:30.882142 BAT rover-eth0: OGM via neigh rover-eth0, seqno 490, tq 255, ttl 50, v 9, flags [..F], length 42
23:37:31.882087 BAT rover-eth0: OGM via neigh rover-eth0, seqno 491, tq 255, ttl 50, v 9, flags [..F], length 42
23:37:32.862046 BAT rover-eth0: OGM via neigh rover-eth0, seqno 492, tq 255, ttl 50, v 9, flags [..F], length 42
23:37:33.861989 BAT rover-eth0: OGM via neigh rover-eth0, seqno 493, tq 255, ttl 50, v 9, flags [..F], length 42
23:37:34.861946 BAT rover-eth0: OGM via neigh rover-eth0, seqno 494, tq 255, ttl 50, v 9, flags [..F], length 42

-Kevin

Re: [B.A.T.M.A.N.] Strange sequence numbers

[Kevin Steen]

Further information: the strange behaviour seems to stop once the
sequence number reaches 1024.

-Kevin

Re: [B.A.T.M.A.N.] Strange sequence numbers

[Sven Eckelmann]

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

On Sun, Mar 14, 2010 at 12:35:50AM +0000, Kevin Steen wrote:
> I've been lurking on this list for a few weeks, but finally tried
> compiling and running the batman-adv module from the svn sources.
> 
> It seems to work fine between my two test machines so far, but I noticed
> some weird sequence numbers when doing a dump with 'batctl td'. In the
> sequence below, I had 'rover' running and active on rover's eth0, then I
> activated the eth1 interface on hilltop for a few seconds, then
> de-activated it. 
> 
> It seems like when rover re-sends the OGM from hilltop, it gets the
> sequence number wrong (1->256, 2->512, 3->768). Is this my lack of
> understanding of how it should work, a bug in batctl's display or a bug
> in batman-adv?
> 
> rover is kernel 2.6.31-20-generic (Ubuntu karmic),
> hilltop is kernel 2.6.26-2-686 (Debian lenny)
> 
> $ ./batctl td eth1
> 23:37:22.022554 BAT rover-eth0: OGM via neigh rover-eth0, seqno 481, tq 255, ttl 50, v 9, flags [..F], length 42
> 23:37:23.002547 BAT rover-eth0: OGM via neigh rover-eth0, seqno 482, tq 255, ttl 50, v 9, flags [..F], length 42
> 23:37:23.982469 BAT rover-eth0: OGM via neigh rover-eth0, seqno 483, tq 255, ttl 50, v 9, flags [..F], length 42
> 23:37:24.962417 BAT rover-eth0: OGM via neigh rover-eth0, seqno 484, tq 255, ttl 50, v 9, flags [..F], length 42
> 23:37:25.962320 BAT rover-eth0: OGM via neigh rover-eth0, seqno 485, tq 255, ttl 50, v 9, flags [..F], length 42
> 23:37:26.943490 BAT rover-eth0: OGM via neigh rover-eth0, seqno 486, tq   0, ttl 49, v 9, flags [D..], length 42
> 23:37:27.041437 BAT rover-eth0: OGM via neigh hilltop-eth1, seqno 486, tq   0, ttl 49, v 9, flags [D..], length 28
> 23:37:27.893422 BAT hilltop-eth1: OGM via neigh hilltop-eth1, seqno 1, tq 255, ttl 50, v 9, flags [..F], length 22
> 23:37:27.922233 BAT rover-eth0: OGM via neigh rover-eth0, seqno 487, tq   0, ttl 49, v 9, flags [D..], length 42

Thanks for testing the code. I doubt that this is a bug in the batman-adv code.
Can you please make a capture of the raw packet data using tcpdump and/or
wireshark? I think that this is a batctl bug, but I don't have time to check it
right now. You can do the comparison by yourself using the wireshark-batman-adv
dissector linked in the open-mesh.net wiki.

Best regards,
	Sven

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

Re: [B.A.T.M.A.N.] Strange sequence numbers

[Sven Eckelmann]

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

On Sun, Mar 14, 2010 at 01:04:10PM +0100, Sven Eckelmann wrote:
> Thanks for testing the code. I doubt that this is a bug in the batman-adv code.
> Can you please make a capture of the raw packet data using tcpdump and/or
> wireshark? I think that this is a batctl bug, but I don't have time to check it
> right now. You can do the comparison by yourself using the wireshark-batman-adv
> dissector linked in the open-mesh.net wiki.

Just checked the output of a uml tcpdump versus a tcpdump of the corresponding
tap device on the host. The result on the host ist correct and the output from
the uml has the two bytes from the sequence number exchanged. So it seems that
the skb is changed without cloning the data which is changed.

The problem appears when data is resend by the other node and we retreive them.
It doesn't seem to happen when we receive data from the other node with the
originator also being the other node. This is not the data the network really
sees - only tcpdump will see it.

So it is real a problem with batman-adv and not batctl (the code of batctl
isn't that complex that it could create such a problem).

Best regards,
	Sven

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

[B.A.T.M.A.N.] [PATCH] batman-adv: Clone shared bat packets before modifying them

[Sven Eckelmann]

"tcpdump" and "batctl td" will receive packets with a wrong sequence
number on systems with a different endianess than network byte order.
This happens due to the reordering of bytes in the function which
handles aggregated bat packets. The function which receives the bat
packets must ensure that these buffers aren't shared with anything else
before that function tries to write into it. Otherwise it has to copy
the buffers so it is save again to change them.

Signed-off-by: Sven Eckelmann <sven.eckelmann@gmx.de>
---
 batman-adv-kernelland/routing.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/batman-adv-kernelland/routing.c b/batman-adv-kernelland/routing.c
index 8610b22..0051259 100644
--- a/batman-adv-kernelland/routing.c
+++ b/batman-adv-kernelland/routing.c
@@ -680,6 +680,7 @@ int recv_bat_packet(struct sk_buff *skb,
 {
 	struct ethhdr *ethhdr;
 	unsigned long flags;
+	struct sk_buff *skb_old;
 
 	/* drop packet if it has not necessary minimum size */
 	if (skb_headlen(skb) < sizeof(struct batman_packet))
@@ -695,12 +696,19 @@ int recv_bat_packet(struct sk_buff *skb,
 	if (is_bcast(ethhdr->h_source))
 		return NET_RX_DROP;
 
-	spin_lock_irqsave(&orig_hash_lock, flags);
 	/* TODO: we use headlen instead of "length", because
 	 * only this data is paged in. */
-	/* TODO: is another skb_copy needed here? there will be
-	 * written on the data, but nobody (?) should further use
-	 * this data */
+
+	/* create a copy of the skb, if needed, to modify it. */
+	if (!skb_clone_writable(skb, skb_headlen(skb))) {
+		skb_old = skb;
+		skb = skb_copy(skb, GFP_ATOMIC);
+		if (!skb)
+			return NET_RX_DROP;
+		kfree_skb(skb_old);
+	}
+
+	spin_lock_irqsave(&orig_hash_lock, flags);
 	receive_aggr_bat_packet(ethhdr,
 				skb->data,
 				skb_headlen(skb),
-- 
1.7.0

[B.A.T.M.A.N.] [PATCHv2] batman-adv: Clone shared bat packets before modifying them

[Sven Eckelmann]

"tcpdump" and "batctl td" will receive packets with a wrong sequence
number on systems with a different endianess than network byte order.
This happens due to the reordering of bytes in the function which
handles aggregated bat packets. The function which receives the bat
packets must ensure that these buffers aren't shared with anything else
before that function tries to write into it. Otherwise it has to copy
the buffers so it is save again to change them.

Reported-by: Kevin Steen <batman@kevinsteen.net>
Signed-off-by: Sven Eckelmann <sven.eckelmann@gmx.de>
---
 batman-adv-kernelland/routing.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/batman-adv-kernelland/routing.c b/batman-adv-kernelland/routing.c
index 8610b22..0051259 100644
--- a/batman-adv-kernelland/routing.c
+++ b/batman-adv-kernelland/routing.c
@@ -680,6 +680,7 @@ int recv_bat_packet(struct sk_buff *skb,
 {
 	struct ethhdr *ethhdr;
 	unsigned long flags;
+	struct sk_buff *skb_old;
 
 	/* drop packet if it has not necessary minimum size */
 	if (skb_headlen(skb) < sizeof(struct batman_packet))
@@ -695,12 +696,19 @@ int recv_bat_packet(struct sk_buff *skb,
 	if (is_bcast(ethhdr->h_source))
 		return NET_RX_DROP;
 
-	spin_lock_irqsave(&orig_hash_lock, flags);
 	/* TODO: we use headlen instead of "length", because
 	 * only this data is paged in. */
-	/* TODO: is another skb_copy needed here? there will be
-	 * written on the data, but nobody (?) should further use
-	 * this data */
+
+	/* create a copy of the skb, if needed, to modify it. */
+	if (!skb_clone_writable(skb, skb_headlen(skb))) {
+		skb_old = skb;
+		skb = skb_copy(skb, GFP_ATOMIC);
+		if (!skb)
+			return NET_RX_DROP;
+		kfree_skb(skb_old);
+	}
+
+	spin_lock_irqsave(&orig_hash_lock, flags);
 	receive_aggr_bat_packet(ethhdr,
 				skb->data,
 				skb_headlen(skb),
-- 
1.7.0

Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: Clone shared bat packets before modifying them

[Marek Lindner]

On Monday 15 March 2010 01:15:55 Sven Eckelmann wrote:
> "tcpdump" and "batctl td" will receive packets with a wrong sequence
> number on systems with a different endianess than network byte order.
> This happens due to the reordering of bytes in the function which
> handles aggregated bat packets. The function which receives the bat
> packets must ensure that these buffers aren't shared with anything else
> before that function tries to write into it. Otherwise it has to copy
> the buffers so it is save again to change them.

Applied in rev 1598.

Thanks for this quick fix!

Cheers,
Marek

Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: Clone shared bat packets before modifying them

[Kevin Steen]

Works correctly on my systems now - thanks.

-Kevin