batman-adv: design suggestions

batman-adv: design suggestions

[Vasiliy Kulikov]

Hi folks,

I was reading batman-adv sources and noted:


1) Some incoming packets may cause a storm of error logs, such as at
routing.c:862


	if (icmp_packet->msg_type != ECHO_REQUEST) {
		pr_warning("Warning - can't forward icmp packet from %pM to "
			   "%pM: ttl exceeded\n", icmp_packet->orig,
			   icmp_packet->dst);

  Any flooding bad guy is able to fill our disks with logs.
  This should be logged only at some slow rate (e.g. 5 logs/sec) or as
  pr_debug().


2) It seems to me that NF_HOOK() at hard-interface.c:458 is misused:

    ...
	ret = NF_HOOK(PF_BRIDGE, NF_BR_LOCAL_IN, skb, dev, NULL,
		      batman_skb_recv_finish);
	if (ret != 1)
		goto err_out;

	/* packet should hold at least type and version */
	if (unlikely(skb_headlen(skb) < 2))
		goto err_free;

	/* expect a valid ethernet header here. */
	if (unlikely(skb->mac_len != sizeof(struct ethhdr)
				|| !skb_mac_header(skb)))
		goto err_free;
    ...

    static int batman_skb_recv_finish(struct sk_buff *skb)
    {
        return NF_ACCEPT;
    }

  As I understand, if there is any hook that returns NF_STOLEN, then skb
  is leaked.


Thanks,
Vasiliy.

Re: batman-adv: design suggestions

[Sven Eckelmann]

[-- Attachment #1: Type: Text/Plain, Size: 1410 bytes --]

Vasiliy Kulikov wrote:
> Hi folks,
> 
> I was reading batman-adv sources and noted:

Thanks a lot.


> 1) Some incoming packets may cause a storm of error logs, such as at
> routing.c:862
> 
> 
> 	if (icmp_packet->msg_type != ECHO_REQUEST) {
> 		pr_warning("Warning - can't forward icmp packet from %pM to "
> 			   "%pM: ttl exceeded\n", icmp_packet->orig,
> 			   icmp_packet->dst);
> 
>   Any flooding bad guy is able to fill our disks with logs.
>   This should be logged only at some slow rate (e.g. 5 logs/sec) or as
>   pr_debug().

Correct. So you would prefer pr_warn_ratelimited?


> 2) It seems to me that NF_HOOK() at hard-interface.c:458 is misused:
> 
>     ...
> 	ret = NF_HOOK(PF_BRIDGE, NF_BR_LOCAL_IN, skb, dev, NULL,
> 		      batman_skb_recv_finish);
> 	if (ret != 1)
> 		goto err_out;
> 
> 	/* packet should hold at least type and version */
> 	if (unlikely(skb_headlen(skb) < 2))
> 		goto err_free;
> 
> 	/* expect a valid ethernet header here. */
> 	if (unlikely(skb->mac_len != sizeof(struct ethhdr)
> 
> 				|| !skb_mac_header(skb)))
> 
> 		goto err_free;
>     ...
> 
>     static int batman_skb_recv_finish(struct sk_buff *skb)
>     {
>         return NF_ACCEPT;
>     }
> 
>   As I understand, if there is any hook that returns NF_STOLEN, then skb
>   is leaked.

@Linus Luessing: Could you please check that.

thanks,
	Sven

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

Re: batman-adv: design suggestions

[Vasiliy Kulikov]

On Mon, Aug 09, 2010 at 22:53 +0200, Sven Eckelmann wrote:
> Vasiliy Kulikov wrote:
> > 1) Some incoming packets may cause a storm of error logs, such as at
> > routing.c:862
> > 
> > 
> > 	if (icmp_packet->msg_type != ECHO_REQUEST) {
> > 		pr_warning("Warning - can't forward icmp packet from %pM to "
> > 			   "%pM: ttl exceeded\n", icmp_packet->orig,
> > 			   icmp_packet->dst);
> > 
> >   Any flooding bad guy is able to fill our disks with logs.
> >   This should be logged only at some slow rate (e.g. 5 logs/sec) or as
> >   pr_debug().
> 
> Correct. So you would prefer pr_warn_ratelimited?

As I see in net/, such packets should be silently dropped with
drop_count++. Exceeded TTL is rather common situation and is not critical.
Also such buggy packets should be found out & dropped as fast as possible.
So IMO it should be debug output (if any) that does no overhead at nodebug
compilation.


3) Also there is no handler of online MTU change, at hard_if_event().


Is there any (un)official documentation/RFC/whatever of batman-adv
protocol? I found only expired RFC of batman that is using UDP at 
www.open-mesh.org.


Thanks,
Vasiliy.

Re: batman-adv: design suggestions

[Marek Lindner]

On Thursday 12 August 2010 14:48:13 Vasiliy Kulikov wrote:
> Is there any (un)official documentation/RFC/whatever of batman-adv
> protocol? I found only expired RFC of batman that is using UDP at
> www.open-mesh.org.

Yes, there is an unofficial one:
http://gitorious.org/batman-adv-doc

Note: This document is neither complete nor up to date. Writing proper 
documentation for the routing protocol is on our long ToDo list. 

The UDP version is very similar to the kernel module since we used the user 
space implementation to verify the concept.


Cheers,
Marek

Re: batman-adv: design suggestions

[Vasiliy Kulikov]

On Sat, Aug 14, 2010 at 16:50 +0200, Marek Lindner wrote:
> On Thursday 12 August 2010 14:48:13 Vasiliy Kulikov wrote:
> > Is there any (un)official documentation/RFC/whatever of batman-adv
> > protocol? I found only expired RFC of batman that is using UDP at
> > www.open-mesh.org.
> 
> Yes, there is an unofficial one:
> http://gitorious.org/batman-adv-doc

Thank you!

> Note: This document is neither complete nor up to date. Writing proper 
> documentation for the routing protocol is on our long ToDo list. 

Do you plan to register it as RFC/IEEE/whatever when linux batman-adv
implementation is stabilized? Or you plan to improve it somehow? :)

> 
> The UDP version is very similar to the kernel module since we used the user 
> space implementation to verify the concept.
> 
> 
> Cheers,
> Marek

Re: batman-adv: design suggestions

[Marek Lindner]

On Saturday 14 August 2010 18:19:15 Vasiliy Kulikov wrote:
> > Note: This document is neither complete nor up to date. Writing proper
> > documentation for the routing protocol is on our long ToDo list.
> 
> Do you plan to register it as RFC/IEEE/whatever when linux batman-adv
> implementation is stabilized? Or you plan to improve it somehow? :)

As you could see we already made the first "registration" attempt when we wrote 
the RFC draft. Unfortunately, we neither have the academic background nor the 
lobby to push things through. But help / ideas / etc are welcome. We are still 
very interested in seeing this happen.

We are constantly tweaking / improving the protocol which should not hinder 
such a standardization. 

Cheers,
Marek

Re: batman-adv: design suggestions

[Vasiliy Kulikov]

On Tue, Aug 10, 2010 at 00:34 +0400, Vasiliy Kulikov wrote:
> 2) It seems to me that NF_HOOK() at hard-interface.c:458 is misused:
> 
>     ...
> 	ret = NF_HOOK(PF_BRIDGE, NF_BR_LOCAL_IN, skb, dev, NULL,
> 		      batman_skb_recv_finish);
> 	if (ret != 1)
> 		goto err_out;
> 
> 	/* packet should hold at least type and version */
> 	if (unlikely(skb_headlen(skb) < 2))
> 		goto err_free;
> 
> 	/* expect a valid ethernet header here. */
> 	if (unlikely(skb->mac_len != sizeof(struct ethhdr)
> 				|| !skb_mac_header(skb)))
> 		goto err_free;
>     ...
> 
>     static int batman_skb_recv_finish(struct sk_buff *skb)
>     {
>         return NF_ACCEPT;
>     }
> 
>   As I understand, if there is any hook that returns NF_STOLEN, then skb
>   is leaked.

New ideas ;)

a) Currently processing of tables does not confirm to its names.

 From `man ebtables`:
    ...
    filter  is  the  default  table and contains three built-in
    chains: INPUT (for frames destined for the  bridge itself,
    on  the  level of the MAC destination address), OUTPUT (for
    locally-generated or (b)routed  frames)  and  FORWARD (for
    frames being forwarded by the bridge).
    nat is mostly used to change the mac addresses and contains
    three built-in chains: PREROUTING (for altering  frames as
    soon  as they come in), OUTPUT (for altering locally gener‐
    ated or (b)routed  frames  before  they  are  bridged) and
    POSTROUTING  (for  altering  frames as they are about to go
    out). A small note on the naming of chains  PREROUTING and
    POSTROUTING: it would be more accurate to call them PREFOR‐
    WARDING and POSTFORWARDING, but for all those who come from
    the  iptables  world  to  ebtables it is easier to have the
    same names. Note that you can change the name (-E)  if you
    don't like the default.
    ...

   Second argument to this NF_HOOK() should be NF_BR_PRE_ROUTING as it is not
   know yet whether this packet is for local machine.

  NF_BR_LOCAL_IN should locate in interface_rx just before netif_rx [*] - see below
  NF_BR_LOCAL_OUT in interface_tx before big 'if (is_bcast ...)' [*]
  NF_BR_POST_ROUTING in send_skb_packet instead of current NF_BR_LOCAL_OUT
  NF_BR_FORWARD somewhere in recv_{bat,icmp,unicast,bcast}_packet() if
    packet is being forwarded [*]


b) Why do you use bridge tables at all? This layer does not know
anything about batman layer, only ethernet that is only a tunnel for
batman. So, it is able to hook traffic from concrete prev-hop routers,
but not from original sources of packets. I think it is not enough for
network filter.  
   Also if you want to process [*] cases you have to append fake
ethernet headers before network header as NF_HOOK() would use ethernet
header.

So, I see 2 solutions:
 1) write your own table layer similar to ebtables & userland tool :) It
is costly, but you'll be able to fully filter/hook of batman traffic.
 2) write ebtables module to check batman header fields. It is slightly 
slower, but potentially may do the same as (1).

However, I think it is not so important and may be rated as feature.


c) Maybe it's better to give user an ability to tune some network
parameters? Like ttl, whether answer to icmp, ratelimit of generating
output icmps, etc.


d) Why do you send icmp TTL exceeded for the icmp itself? E.g. in case
of loop or/and small default TTL you'll probably get a storm of icmps.
Exactly in this case IP silently drops TTL exceeded icmps ;)


Hope this information will be usefull.


Thanks,
Vasiliy.

Re: batman-adv: design suggestions

[Sven Eckelmann]

[-- Attachment #1: Type: Text/Plain, Size: 1570 bytes --]

Vasiliy Kulikov wrote:
> On Tue, Aug 10, 2010 at 00:34 +0400, Vasiliy Kulikov wrote:
> > 2) It seems to me that NF_HOOK() at hard-interface.c:458 is misused:
> >     ...
> > 	
> > 	ret = NF_HOOK(PF_BRIDGE, NF_BR_LOCAL_IN, skb, dev, NULL,
> > 	
> > 		      batman_skb_recv_finish);
> > 	
> > 	if (ret != 1)
> > 	
> > 		goto err_out;
> > 	
> > 	/* packet should hold at least type and version */
> > 	if (unlikely(skb_headlen(skb) < 2))
> > 	
> > 		goto err_free;
> > 	
> > 	/* expect a valid ethernet header here. */
> > 	if (unlikely(skb->mac_len != sizeof(struct ethhdr)
> > 	
> > 				|| !skb_mac_header(skb)))
> > 		
> > 		goto err_free;
> > 		
> >     ...
> >     
> >     static int batman_skb_recv_finish(struct sk_buff *skb)
> >     {
> >     
> >         return NF_ACCEPT;
> >     
> >     }
> >   
> >   As I understand, if there is any hook that returns NF_STOLEN, then skb
> >   is leaked.
> 
[...]

> b) Why do you use bridge tables at all? This layer does not know
> anything about batman layer, only ethernet that is only a tunnel for
> batman. So, it is able to hook traffic from concrete prev-hop routers,
> but not from original sources of packets. I think it is not enough for
> network filter.
>    Also if you want to process [*] cases you have to append fake
> ethernet headers before network header as NF_HOOK() would use ethernet
> header.

Because a different person (no one from the actual development team) wanted to 
have it for testing purposes. Maybe we just drop it again.

thanks,
	Sven

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

Re: batman-adv: design suggestions

[Marek Lindner]

On Friday 13 August 2010 20:18:33 Vasiliy Kulikov wrote:
> d) Why do you send icmp TTL exceeded for the icmp itself? E.g. in case
> of loop or/and small default TTL you'll probably get a storm of icmps.
> Exactly in this case IP silently drops TTL exceeded icmps ;)

These layer2 icmp packets are not ordinary icmp packets. We needed to provide 
a mechanism to make the network topology visible to debug tools like ping or 
traceroute which normally "see" no more than one hop as they operate on 
layer3. Hence, batman-adv does not send an icmp packet for each payload TTL 
exceeded but for traceroute only. I recommend reviewing the traceroute code to 
understand how this is supposed to work:
http://www.open-mesh.org/browser/trunk/batctl/traceroute.c

I'd be interested to learn about a problematic scenario in which this 
mechanism breaks.

Regards,
Marek

Re: batman-adv: design suggestions

[Vasiliy Kulikov]

On Sat, Aug 14, 2010 at 16:59 +0200, Marek Lindner wrote:
> On Friday 13 August 2010 20:18:33 Vasiliy Kulikov wrote:
> > d) Why do you send icmp TTL exceeded for the icmp itself? E.g. in case
> > of loop or/and small default TTL you'll probably get a storm of icmps.
> > Exactly in this case IP silently drops TTL exceeded icmps ;)
> 
> These layer2 icmp packets are not ordinary icmp packets.

By the way, it's better to name it smth another (bcmp?) as ICMP = _internet_
control message protocol. Batman is not limited to IP however ;)

> We needed to provide 
> a mechanism to make the network topology visible to debug tools like ping or 
> traceroute which normally "see" no more than one hop as they operate on 
> layer3. Hence, batman-adv does not send an icmp packet for each payload TTL 
> exceeded but for traceroute only.

Ah, dammit! I didn't see this code:

	if (icmp_packet->msg_type != ECHO_REQUEST) {
		pr_warning("Warning - can't forward icmp packet from %pM to "
			   "%pM: ttl exceeded\n", icmp_packet->orig,
			   icmp_packet->dst);
		return NET_RX_DROP;
	}

I thought that any expired icmp spawns TTL exceeded icmp that may spawn
another one, etc.

> I recommend reviewing the traceroute code to 
> understand how this is supposed to work:
> http://www.open-mesh.org/browser/trunk/batctl/traceroute.c

Thanks, I'll look at it.

> 
> I'd be interested to learn about a problematic scenario in which this 
> mechanism breaks.

Now I don't know anyone too ;)

> 
> Regards,
> Marek