[B.A.T.M.A.N.] Unsigned integer overflow in batadv_iv_ogm_calc_tq

[Sven Eckelmann]

Hi,

just ran my emulation setup [1] and got an integer overflow (undefined 
behavior):



UBSAN: Undefined behaviour in 
/home/sven/tmp/qemu-batman/batman-adv/net/batman-adv/bat_iv_ogm.c:1246:25
signed integer overflow:
8713350 * 255 cannot be represented in type 'int'
CPU: 1 PID: 0 Comm: swapper/1 Tainted: G   O
4.5.0-rc4-next-20160215 #10
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Debian-1.8.2-1 
04/01/2014
 110001980ea7 10e2f8880f23fbe1 88000cc075b0 817f196f
 41b58ab3 824b0b5f 817f1894 88000cc075d8
 88000cc07588 88000cc075a0 00ff 88000cc07398
Call Trace:
   [] dump_stack+0xdb/0x15c
 [] ? _atomic_dec_and_lock+0xc4/0xc4
 [] ubsan_epilogue+0xd/0x8a
 [] handle_overflow+0x211/0x260
 [] ? __ubsan_handle_negate_overflow+0x1b1/0x1b1
 [] ? trace_hardirqs_on+0xd/0x10
 [] ? batadv_neigh_ifinfo_get+0x330/0x330 [batman_adv]
 [] ? batadv_iv_ogm_process_per_outif+0x1380/0x33f0 
[batman_adv]
 [] ? trace_hardirqs_on+0xd/0x10
 [] __ubsan_handle_mul_overflow+0xe/0x17
 [] batadv_iv_ogm_process_per_outif+0x2d64/0x33f0 
[batman_adv]
 [] ? batadv_iv_ogm_process_per_outif+0xf17/0x33f0 
[batman_adv]
 [] batadv_iv_ogm_receive+0x8d4/0x1d20 [batman_adv]
 [] ? batadv_iv_ogm_receive+0x3c2/0x1d20 [batman_adv]
 [] batadv_batman_skb_recv+0x378/0x490 [batman_adv]
 [] ? batadv_skb_set_priority+0x640/0x640 [batman_adv]
 [] __netif_receive_skb_core+0x7b7/0x2a50
 [] ? __skb_csum_offload_chk+0x12a0/0x12a0
 [] __netif_receive_skb+0x55/0x200
 [] netif_receive_skb_internal+0xf9/0x3e0
 [] ? netif_receive_skb_internal+0x94/0x3e0
 [] ? __netif_receive_skb+0x200/0x200
 [] ? dev_gro_receive+0x76e/0x1ca0
 [] ? dev_gro_receive+0x60c/0x1ca0
 [] ? __netdev_alloc_skb+0x1d1/0x350
 [] ? memcpy+0x36/0x40
 [] ? eth_commit_mac_addr_change+0x70/0x70
 [] ? page_to_skb+0x1d4/0x720
 [] napi_gro_receive+0x11a/0x240
 [] virtnet_receive+0xc14/0x26b0
 [] ? try_fill_recv+0x1530/0x1530
 [] ? pvclock_read_flags+0x6d0/0x6d0
 [] ? __list_add+0x3f0/0x3f0
 [] virtnet_poll+0x1d/0x160
 [] net_rx_action+0x6a3/0xca0
 [] ? handle_irq_event+0xcd/0x1a0
 [] ? handle_fasteoi_irq+0x275/0x8f0
 [] ? __do_softirq+0x1d0/0x870
 [] __do_softirq+0x288/0x870
 [] irq_exit+0xe3/0x140
 [] do_IRQ+0x9e/0x200
 [] common_interrupt+0x8c/0x8c
   [] ? native_safe_halt+0x6/0x10
 [] ? trace_hardirqs_on+0xd/0x10
 [] default_idle+0xe/0x20
 [] arch_cpu_idle+0xa/0x10
 [] default_idle_call+0x4f/0x80
 [] cpu_startup_entry+0x2c4/0x540
 [] ? _raw_spin_unlock_irqrestore+0x36/0x60
 [] ? default_idle_call+0x80/0x80
 [] ? clockevents_register_device+0xf8/0x1f0
 [] start_secondary+0x35b/0x4c0
 [] ? set_cpu_sibling_map+0x2fe0/0x2fe0



The code is:

combined_tq = batadv_ogm_packet->tq *
  tq_own *
  tq_asym_penalty *
  tq_iface_penalty;
combined_tq /= BATADV_TQ_MAX_VALUE *
   BATADV_TQ_MAX_VALUE *
   BATADV_TQ_MAX_VALUE;

It is easy to see that

batadv_ogm_packet::tq (u8 255) *
tq_own (u8 255) *
tq_asym_penalty (int 134) *
tq_iface_penalty (int 255)

is outside the range of an signed integer (32 bit). The maximum seen
here is 255 for each entry. So should tq_iface_penalty +
tq_iface_penalty, inv_asym_penalty be changed to unsigned int?

--- a/net/batman-adv/bat_iv_ogm.c
+++ b/net/batman-adv/bat_iv_ogm.c
@@ -1147,9 +1147,10 @@ static int batadv_iv_ogm_calc_tq(struct batadv_orig_node 
*orig_node,
u8 total_count;
u8 orig_eq_count, neigh_rq_count, neigh_rq_inv, tq_own;
unsigned int neigh_rq_inv_cube, neigh_rq_max_cube;
-   int tq_asym_penalty, inv_asym_penalty, if_num, ret = 0;
+   int if_num, ret = 0;
+   unsigned int tq_asym_penalty, inv_asym_penalty;
unsigned int combined_tq;
-   int tq_iface_penalty;
+   unsigned int tq_iface_penalty;
 
/* find corresponding one hop neighbor */
rcu_read_lock();

Kind regards,
Sven



[1] https://www.open-mesh.org/projects/open-mesh/wiki/Emulation_Debug

Re: [B.A.T.M.A.N.] Antwort: Re: [PATCH 1/4] batman-adv: Prevent mutliple ARP replies sent by gateways in bla setups with dat enabled

[Sven Eckelmann]

On Monday 15 February 2016 10:39:58 Andreas Pape wrote:
> Hi Simon
> 
> was this the only patch which cannot be applied? What about the others I
> sent? I am still working on the e-mail client issue 

All patches were destroyed by your email client.

Kind regards,
Sven

signature.asc
Description: This is a digitally signed message part.

[B.A.T.M.A.N.] Antwort: Re: [PATCH 1/4] batman-adv: Prevent mutliple ARP replies sent by gateways in bla setups with dat enabled

[Andreas Pape]

Hi Simon

was this the only patch which cannot be applied? What about the others I
sent? I am still working on the e-mail client issue 

Kind regards,
Andreas

Simon Wunderlich  schrieb am 15.02.2016 09:50:18:

> Von: Simon Wunderlich 
> An: b.a.t.m.a.n@lists.open-mesh.org
> Kopie: Andreas Pape 
> Datum: 15.02.2016 09:50
> Betreff: Re: [B.A.T.M.A.N.] [PATCH 1/4] batman-adv: Prevent mutliple
> ARP replies sent by gateways in bla setups with dat enabled
>
> On Friday 12 February 2016 14:51:32 Andreas Pape wrote:
> > From 2b90abdf53e9ab09d9acfd141c7225de1ae16719 Mon Sep 17 00:00:00 2001
> > From: Andreas Pape 
> > Date: Fri, 12 Feb 2016 10:05:57 +0100
> > Subject: [PATCH 1/4] batman-adv: Prevent mutliple ARP replies sent by
> > gateways in bla setups with dat enabled
> >
> > This patch shall make sure that only the backbone gw which has claimed
the
> > remote
> > destination for the ARP request answers the ARP request directly if
the
> > MAC address
> > is known due to the local DAT table. This prevents multiple ARP
replies in
> > a common
> > backbone if more than one gateway already knows the remote mac
searched
> > for in the
> > ARP request.
>
> This patch looks good in general. I can not apply it though, please
check the
> links that Sven posted how to set up your mail client to send patches.
Also,
> the commit message seems to have too long lines. Usually your git client

> should limit those to ~72 characters per line (I'm not sure about the
actual
> limit)
>
> >
> > Signed-off-by: Andreas Pape 
> > ---
> >  net/batman-adv/bridge_loop_avoidance.c |   58
> > 
> >  net/batman-adv/bridge_loop_avoidance.h |6 +++
> >  net/batman-adv/distributed-arp-table.c |   14 
> >  3 files changed, 78 insertions(+), 0 deletions(-)
> >
> > diff --git a/net/batman-adv/bridge_loop_avoidance.c
> > b/net/batman-adv/bridge_loop_avoidance.c
> > index 0a6c8b8..c70363d 100644
> > --- a/net/batman-adv/bridge_loop_avoidance.c
> > +++ b/net/batman-adv/bridge_loop_avoidance.c
> > @@ -1906,3 +1906,61 @@ out:
> > batadv_hardif_put(primary_if);
> > return 0;
> >  }
> > +
> > +/**
> > + * batadv_check_local_claim
>
> You should put a short description here, like
>
> batadv_check_local_claim - check if the address has been claimed by the
local
> backbone
>
> > + * @bat_priv: the bat priv with all the soft interface information
> > + * @addr: mac address of which the claim status is checked
> > + * @vid: the VLAN ID
> > + *
> > + * batadv_check_local_claim:
>
> Please remove the repetition of the function name
>
> > + * addr is checked if this address is claimed by the local device
itself.
> > + * If the address is not claimed at all, claim it.
> > + * returns true if bla is disabled or the mac is claimed by the
device
> > + * returns false if the device addr is already claimed by another
gateway
> > + */
>
> Should put Return: and then describe the return values. Please checkthe
other
> functions for reference.
>
> kerneldoc is parsed automatically and must therefore be in the right
format.
>
> > +bool batadv_bla_check_local_claim(struct batadv_priv *bat_priv,
uint8_t
> > *addr, unsigned short vid)
> > +{
> > +   struct batadv_bla_claim search_claim;
> > +   struct batadv_bla_claim *claim = NULL;
> > +   struct batadv_hard_iface *primary_if = NULL;
> > +   bool ret = true;
> > +
> > +   if (atomic_read(_priv->bridge_loop_avoidance)) {
>
> You can save an intendation by doing a return here immediately
>
> > +
> > +   primary_if = batadv_primary_if_get_selected(bat_priv);
> > +   if (!primary_if)
> > +   return ret;
>
> I'd prefer a goto here. If we have other stuff to clean up when we
> change this
> function later, we may forget that this is not done because we used
return
> here.
>
> > +
> > +   /* First look if the mac address is claimed */
> > +   ether_addr_copy(search_claim.addr, addr);
> > +   search_claim.vid = vid;
> > +
> > +   claim = batadv_claim_hash_find(bat_priv,
> > + _claim);
> > +
> > +   /* If there is a claim and we are not owner of the
claim,
> > +* return false;
> > +*/
> > +   if (claim) {
> > +   if
(!batadv_compare_eth(claim->backbone_gw->orig,
> > primary_if->net_dev->dev_addr)) {
> > +   ret = false;
> > +   }
>
> braces not needed
>
> > +   } else {
> > +   /* If there is no claim, claim the device */
> > +   batadv_dbg(BATADV_DBG_BLA, bat_priv, "No claim
> > found for %pM. Claim mac for us.\n",
> > +   search_claim.addr);
>
> Maybe put something in the 

Re: [B.A.T.M.A.N.] [PATCH 1/4] batman-adv: Prevent mutliple ARP replies sent by gateways in bla setups with dat enabled

[Sven Eckelmann]

On Monday 15 February 2016 09:50:18 Simon Wunderlich wrote:
> On Friday 12 February 2016 14:51:32 Andreas Pape wrote:
> > From 2b90abdf53e9ab09d9acfd141c7225de1ae16719 Mon Sep 17 00:00:00 2001
> > From: Andreas Pape 
> > Date: Fri, 12 Feb 2016 10:05:57 +0100
> > Subject: [PATCH 1/4] batman-adv: Prevent mutliple ARP replies sent by
> > gateways in bla setups with dat enabled
> > 
> > This patch shall make sure that only the backbone gw which has claimed the
> > remote
> > destination for the ARP request answers the ARP request directly if the
> > MAC address
> > is known due to the local DAT table. This prevents multiple ARP replies in
> > a common
> > backbone if more than one gateway already knows the remote mac searched
> > for in the
> > ARP request.
> 
> This patch looks good in general. I can not apply it though, please check the 
> links that Sven posted how to set up your mail client to send patches. Also, 
> the commit message seems to have too long lines. Usually your git client 
> should limit those to ~72 characters per line (I'm not sure about the actual 
> limit)

See SubmittingPatches:

- The body of the explanation, line wrapped at 75 columns, which will
  be copied to the permanent changelog to describe this patch.

This should also have been displayed when you've checked the patch with
Linux's

 ./scripts/checkpatch.pl --strict 000*.patch

Kind regards,
Sven

signature.asc
Description: This is a digitally signed message part.

Re: [B.A.T.M.A.N.] Antwort: Re: Antwort: Re: Re: Antwort: Re: Looping unicast packets when using BLA

[Simon Wunderlich]

Hi Andreas,

On Friday 12 February 2016 15:07:59 Andreas Pape wrote:
> Simon Wunderlich  schrieb am 12.02.2016 14:04:23:
> > Von: Simon Wunderlich 
> > An: Andreas Pape 
> > Kopie: The list for a Better Approach To Mobile Ad-hoc Networking
> > 
> > Datum: 12.02.2016 14:04
> > Betreff: Re: Antwort: Re: Re: [B.A.T.M.A.N.] Antwort: Re: Looping
> > unicast packets when using BLA
> > 
> > Hi Andreas,
> > 
> > On Friday 12 February 2016 11:40:21 Andreas Pape wrote:
> > > > > [...]
> > > > > using dat in combination with bla:
> > > > > 1.Broadcast ARP requests from the backbone network are handled by
> 
> each
> 
> > > > > gateway, leading to multiple dat adress resoultions in parallel.
> > > > 
> > > > That shouldn't be a problem on its own.
> > > 
> > > I think I wasn't precise enough concerning this point. I meant the
> 
> effect,
> 
> > > that
> > > a broadcast ARP coming from a common backbone reaches all gateways. If
> 
> now
> 
> > > accidentally
> > > several gateways can already answer that request due to dat, then the
> > > current code sends
> > > an arp reply from each gateway being able to answer. This broadcast
> 
> does
> 
> > > not even reach
> > > the mesh if all gateways can answer the request (as far as I have
> > > understood the code).
> > > Therefore broadcast handling in the mesh layer does not solve this
> > > problem.
> > 
> > Yes, we may have multiple gateways answering with an ARP reply. But how
> 
> is
> 
> > this a problem? It is redundant, yes, but its just a unicast sent back.
> 
> I
> 
> > don't see this a s problem yet ...
> 
> I would like to prevent duplicated packets as much as possible, even if
> they are unicast packets normally harmlexs for typical PC hardware. But I
> know of enough small embedded devices (sensors and stuff like that) which
> don't like that.
> 

Thats a good point. In general it could be debated whether we prefer redundant 
replies to no replies at all. But I'd agree to your point, especially since 
having answers from different devices may confuse a switch because it thinks 
there is some mac flapping or worse, having answers from different ports.

>> [...]
> 
> I've just sent the patches. They have the state of my "experiments" last
> year. That means that your latest proposal is not integrated yet.
> I quickly updated my devices in my test setup and it looks good (no
> looping arp requests or multiple replies seen so far).

Thanks a lot! I've reviewed them, we still have some formatting work to do so 
please bear with us with the iterations. Splitting and cleaning them up was 
definitely a great start, this is a very good contribution. :)

Thanks,
 Simon

signature.asc
Description: This is a digitally signed message part.

Re: [B.A.T.M.A.N.] [PATCH 1/4] batman-adv: Prevent mutliple ARP replies sent by gateways in bla setups with dat enabled

[Simon Wunderlich]

On Friday 12 February 2016 14:51:32 Andreas Pape wrote:
> From 2b90abdf53e9ab09d9acfd141c7225de1ae16719 Mon Sep 17 00:00:00 2001
> From: Andreas Pape 
> Date: Fri, 12 Feb 2016 10:05:57 +0100
> Subject: [PATCH 1/4] batman-adv: Prevent mutliple ARP replies sent by
> gateways in bla setups with dat enabled
> 
> This patch shall make sure that only the backbone gw which has claimed the
> remote
> destination for the ARP request answers the ARP request directly if the
> MAC address
> is known due to the local DAT table. This prevents multiple ARP replies in
> a common
> backbone if more than one gateway already knows the remote mac searched
> for in the
> ARP request.

This patch looks good in general. I can not apply it though, please check the 
links that Sven posted how to set up your mail client to send patches. Also, 
the commit message seems to have too long lines. Usually your git client 
should limit those to ~72 characters per line (I'm not sure about the actual 
limit)

> 
> Signed-off-by: Andreas Pape 
> ---
>  net/batman-adv/bridge_loop_avoidance.c |   58
> 
>  net/batman-adv/bridge_loop_avoidance.h |6 +++
>  net/batman-adv/distributed-arp-table.c |   14 
>  3 files changed, 78 insertions(+), 0 deletions(-)
> 
> diff --git a/net/batman-adv/bridge_loop_avoidance.c
> b/net/batman-adv/bridge_loop_avoidance.c
> index 0a6c8b8..c70363d 100644
> --- a/net/batman-adv/bridge_loop_avoidance.c
> +++ b/net/batman-adv/bridge_loop_avoidance.c
> @@ -1906,3 +1906,61 @@ out:
> batadv_hardif_put(primary_if);
> return 0;
>  }
> +
> +/**
> + * batadv_check_local_claim

You should put a short description here, like

batadv_check_local_claim - check if the address has been claimed by the local 
backbone

> + * @bat_priv: the bat priv with all the soft interface information
> + * @addr: mac address of which the claim status is checked
> + * @vid: the VLAN ID
> + *
> + * batadv_check_local_claim:

Please remove the repetition of the function name

> + * addr is checked if this address is claimed by the local device itself.
> + * If the address is not claimed at all, claim it.
> + * returns true if bla is disabled or the mac is claimed by the device
> + * returns false if the device addr is already claimed by another gateway
> + */

Should put Return: and then describe the return values. Please check the other 
functions for reference.

kerneldoc is parsed automatically and must therefore be in the right format.

> +bool batadv_bla_check_local_claim(struct batadv_priv *bat_priv, uint8_t
> *addr, unsigned short vid)
> +{
> +   struct batadv_bla_claim search_claim;
> +   struct batadv_bla_claim *claim = NULL;
> +   struct batadv_hard_iface *primary_if = NULL;
> +   bool ret = true;
> +
> +   if (atomic_read(_priv->bridge_loop_avoidance)) {

You can save an intendation by doing a return here immediately

> +
> +   primary_if = batadv_primary_if_get_selected(bat_priv);
> +   if (!primary_if)
> +   return ret;

I'd prefer a goto here. If we have other stuff to clean up when we change this 
function later, we may forget that this is not done because we used return 
here.

> +
> +   /* First look if the mac address is claimed */
> +   ether_addr_copy(search_claim.addr, addr);
> +   search_claim.vid = vid;
> +
> +   claim = batadv_claim_hash_find(bat_priv,
> + _claim);
> +
> +   /* If there is a claim and we are not owner of the claim,
> +* return false;
> +*/
> +   if (claim) {
> +   if (!batadv_compare_eth(claim->backbone_gw->orig,
> primary_if->net_dev->dev_addr)) {
> +   ret = false;
> +   }

braces not needed

> +   } else {
> +   /* If there is no claim, claim the device */
> +   batadv_dbg(BATADV_DBG_BLA, bat_priv, "No claim
> found for %pM. Claim mac for us.\n",
> +   search_claim.addr);

Maybe put something in the debug code where this was called from? This looks 
like a very generic claim message.

> +
> +   batadv_handle_claim(bat_priv,
> +  primary_if,
> + primary_if->net_dev->dev_addr, addr,
> +  vid);

I wonder if we should rename the function somehow, since actively claiming 
goes beyond "just checking". Maybe handle_local_claim.?

Also, primary_if can go on the line above I guess.

Cheers,
 Simon

signature.asc
Description: This is a digitally signed message part.

Re: [B.A.T.M.A.N.] [PATCH 2/4] batman-adv: Speed up dat by snooping received ip traffic

[Simon Wunderlich]

On Friday 12 February 2016 14:52:34 Andreas Pape wrote:
> From cc88159dcf18f4b8310414d2d71635fad76bf5bb Mon Sep 17 00:00:00 2001
> From: Andreas Pape 
> Date: Fri, 12 Feb 2016 11:03:10 +0100
> Subject: [PATCH 2/4] batman-adv: Speed up dat by snooping received ip
> traffic
> 
> This patch shall speed up dat by snooping all incoming ip traffic instead
> of only relying on ARP handling. This shall especially increase the
> probability
> that a gateway into a backbone network already has a fitting dat entry to
> answer
> incoming arp requests directly coming from the backbone network.
> 
> Signed-off-by: Andreas Pape 
> ---
>  net/batman-adv/distributed-arp-table.c |   18 ++
>  net/batman-adv/distributed-arp-table.h |8 +++-
>  net/batman-adv/soft-interface.c|   21 -
>  3 files changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/net/batman-adv/distributed-arp-table.c
> b/net/batman-adv/distributed-arp-table.c
> index 93893bf..4e64e6c 100644
> --- a/net/batman-adv/distributed-arp-table.c
> +++ b/net/batman-adv/distributed-arp-table.c
> @@ -362,6 +362,24 @@ out:
> batadv_dat_entry_put(dat_entry);
>  }
> 
> +/**
> + * batadv_dat_entry_check - check and update a dat entry
> + * @bat_priv: the bat priv with all the soft interface information
> + * @ip: ipv4 to add/edit
> + * @mac_addr: mac address to assign to the given ipv4
> + * @vid: VLAN identifier
> + *
> + * checks additionally, if dat is enabled. can be called from other
> modules.
> + */
> +void batadv_dat_entry_check(struct batadv_priv *bat_priv, __be32 ip,
> +u8 *mac_addr, unsigned short vid)
> +{
> +   if(!atomic_read(_priv->distributed_arp_table))
> +   return;
> +
> +   batadv_dat_entry_add(bat_priv, ip, mac_addr, vid);
> +}
> +
>  #ifdef CONFIG_BATMAN_ADV_DEBUG
> 
>  /**
> diff --git a/net/batman-adv/distributed-arp-table.h
> b/net/batman-adv/distributed-arp-table.h
> index 813ecea..a2ab16b 100644
> --- a/net/batman-adv/distributed-arp-table.h
> +++ b/net/batman-adv/distributed-arp-table.h
> @@ -80,7 +80,8 @@ batadv_dat_init_own_addr(struct batadv_priv *bat_priv,
>  int batadv_dat_init(struct batadv_priv *bat_priv);
>  void batadv_dat_free(struct batadv_priv *bat_priv);
>  int batadv_dat_cache_seq_print_text(struct seq_file *seq, void *offset);
> -
> +void batadv_dat_entry_check(struct batadv_priv *bat_priv, __be32 ip,
> +u8 *mac_addr, unsigned short vid);
>  /**
>   * batadv_dat_inc_counter - increment the correct DAT packet counter
>   * @bat_priv: the bat priv with all the soft interface information
> @@ -173,6 +174,11 @@ static inline void batadv_dat_inc_counter(struct
> batadv_priv *bat_priv,
>  {
>  }
> 
> +void batadv_dat_entry_check(struct batadv_priv *bat_priv, __be32 ip,
> +u8 *mac_addr, unsigned short vid)
> +{
> +}
> +
>  #endif /* CONFIG_BATMAN_ADV_DAT */
> 
>  #endif /* _NET_BATMAN_ADV_DISTRIBUTED_ARP_TABLE_H_ */
> diff --git a/net/batman-adv/soft-interface.c
> b/net/batman-adv/soft-interface.c
> index 0710379..41d7987 100644
> --- a/net/batman-adv/soft-interface.c
> +++ b/net/batman-adv/soft-interface.c
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -390,6 +391,7 @@ void batadv_interface_rx(struct net_device
> *soft_iface,
> __be16 ethertype = htons(ETH_P_BATMAN);
> struct vlan_ethhdr *vhdr;
> struct ethhdr *ethhdr;
> +   struct iphdr *iphdr;
> unsigned short vid;
> bool is_bcast;
> 
> @@ -412,11 +414,28 @@ void batadv_interface_rx(struct net_device
> *soft_iface,
> ethhdr = eth_hdr(skb);
> 
> switch (ntohs(ethhdr->h_proto)) {
> +   case ETH_P_IP:
> +   iphdr = (struct iphdr *)(skb->data + ETH_HLEN);
> +   /* snoop incoming traffic for dat update using the source
> mac
> +* and source ip to speed up dat.
> +* Question: does this break the fundamental idea of
> dat
> +*/

That is a really good question, although it doesn't belong in the code ;)

@Antonio, CC'ing you since this is more a design question/proposal and you may 
have thought about this yet.

Basically, doing this change means that we will put a lot of IP addresses in 
our cache which are not in our local network - typically, all Internet IP 
addresses along with the gateway backbone. Also these addresses will never be 
requested by ARP and are therefore practically just littering our cache. They 
are purged after 5 minutes so the impact may be reasonable, but still ...

Maybe there is a way to limit the entries to local networks? Also (and in 
general), should we have an upper limit how many entries we store in DAT? 
After applying this patch, doing a subnet ping scan can deplete the RAM in 
small routers I'm afraid. :)

(even now, 

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: fix logic error in batadv_v_ogm_forward

[Sven Eckelmann]

On Friday 12 February 2016 11:35:35 Simon Wunderlich wrote:
> From: Simon Wunderlich 
> 
> My latest restructure attempt of the BATMAN v forward function
> introduced a regression, causing kernel crashes when an OGM is
> forwarded. This patch fixes it.
> 
> Fixes: 30c96bc787 ("batman-adv: move and restructure
> batadv_v_ogm_forward")
> Reported-by: Sven Eckelmann 
> Signed-off-by: Simon Wunderlich 

Thanks for the fast update. It fixes the problem in my setup

Tested-by: Sven Eckelmann 

> ---
>  net/batman-adv/bat_v_ogm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/batman-adv/bat_v_ogm.c b/net/batman-adv/bat_v_ogm.c
> index 1b2399e..4155fa5 100644
> --- a/net/batman-adv/bat_v_ogm.c
> +++ b/net/batman-adv/bat_v_ogm.c
> @@ -309,7 +309,7 @@ static void batadv_v_ogm_forward(struct batadv_priv 
> *bat_priv,
>   u16 tvlv_len;
>  
>   /* only forward for specific interfaces, not for the default one. */
> - if (if_outgoing != BATADV_IF_DEFAULT)
> + if (if_outgoing == BATADV_IF_DEFAULT)
>   goto out;
>  
>   orig_ifinfo = batadv_orig_ifinfo_new(orig_node, if_outgoing);
> 

Re: [B.A.T.M.A.N.] [PATCH 3/4] batman-adv: prevent duplication of ARP replies in BLA setups when DAT does address resolution

[Simon Wunderlich]

On Friday 12 February 2016 14:53:31 Andreas Pape wrote:
> From 2cc1e9eb153d0c2d64cb0fb0747063ba63472925 Mon Sep 17 00:00:00 2001
> From: Andreas Pape 
> Date: Fri, 12 Feb 2016 13:19:25 +0100
> Subject: [PATCH 3/4] batman-adv: prevent duplication of ARP replies in BLA
> setups when DAT does address resolution
> 
> This patch covers the case of a bla setup with enabled dat if none of the
> common gateways of the
> same backbone has already knowledge of the searched ip address and
> therefore has to ask via DAT some
> of the other mesh nodes. A broadcast arp request is coming
> from the backbone and each backbone gateway starts an address resolution
> via other DAT mesh nodes.
> In this case it should be prevented, that multiple answers from DAT
> enabled mesh nodes reach the
> backbone gateways leading to multiple replies in a common backbone again.

I think this approach and its methods as implented are good. However I think 
we should generalize this case and forbid UNICAST/UNICAST4ADDR between 
backbone gateways, as discussed in the other thread. 

What do you think, or does anyone else have opinions?

@Antonio, in what way would be the DHT in DAT be affected? Basically, we would 
exclude some orginators who are on the same backbone.

Cheers,
 Simon

signature.asc
Description: This is a digitally signed message part.

Re: [B.A.T.M.A.N.] [PATCH 4/4] batman-adv: free skb when dropping broadcast packet received from another backbone gw

[Simon Wunderlich]

Hi Andreas,

On Friday 12 February 2016 14:54:15 Andreas Pape wrote:
> From 1cf69fc5b7ffac3193ad8fa4439586c865c5acab Mon Sep 17 00:00:00 2001
> From: Andreas Pape 
> Date: Fri, 12 Feb 2016 14:00:53 +0100
> Subject: [PATCH 4/4] batman-adv: free skb when dropping broadcast packet
> received from another backbone gw
> 
> skb should be freed in batadv_recv_bcast_packet if packet shall be dropped
> due to
> reception from another backbone gateway of the same backbone.
> 
> Signed-off-by: Andreas Pape 
> ---
>  net/batman-adv/routing.c |6 --
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
> index 4dd646a..128ed28 100644
> --- a/net/batman-adv/routing.c
> +++ b/net/batman-adv/routing.c
> @@ -1104,8 +1104,10 @@ int batadv_recv_bcast_packet(struct sk_buff *skb,
> /* don't hand the broadcast up if it is from an originator
>  * from the same backbone.
>  */
> -   if (batadv_bla_is_backbone_gw(skb, orig_node, hdr_size))
> -   goto out;
> +   if (batadv_bla_is_backbone_gw(skb, orig_node, hdr_size)) {
> +   kfree_skb(skb);
> +   goto rx_success;
> +   }

I disagree. In the original code, we return NET_RX_DROP, which makes the 
calling code already free the skb. Check batadv_batman_skb_recv() in main.c. 

Cheers,
Simon


signature.asc
Description: This is a digitally signed message part.