Sven Eckelmann | 29 Jun 18:16 2015

Re: [PATCH v2 4/4] batman-adv: Fix gw_bandwidth calculation on 32 bit systems

On Monday 29 June 2015 11:47:00 Antonio Quartulli wrote:
> 
> On 21/06/15 14:42, Sven Eckelmann wrote:
> > The TVLV for the gw_bandwidth stores everything as u32. But the
> > gw_bandwidth reads the signed long which limits the maximum value to
> > (2 ** 31 - 1) on systems with 4 byte long. Also the input value is always
> > converted from either Mibit/s or Kibit/s to 100Kibit/s. This reduces the
> > values even further when the user sets it via the default unit Kibit/s. It
> > may even cause an integer overflow and end up with a value the user never
> > intended.
> > 
> > Instead read the values as u64, check for possible overflows, do the unit
> > adjustments and then reduce the size to u32.
> > 
> > Signed-off-by: Sven Eckelmann <sven@...>
> > ---
> > v2:
> >  * rebased on current master
> 
> shouldn't this patch be for maint as it is also fixing a potential
> overflow issue?

Only the configuration for users is not correctly stored. For example 
4294967296Mbit would result in 0.1 Mbit instead of showing a failure. This is 
hopefully not the biggest problem.

Just tell me if you want it resubmitted against maint (Linux 4.1.x).

Kind regards,
	Sven
(Continue reading)

Linus Lüssing | 29 Jun 04:52 2015

Re: [PATCH 1/1] batman-adv: Always flood IGMP/MLD reports

On Sun, Jun 28, 2015 at 09:21:34PM +0800, Marek Lindner wrote:
> On Sunday, June 28, 2015 05:59:14 Linus Lüssing wrote:
> > > I am not quite clear on what the patch does. It helps to support a feature
> > > that is yet to come (upcoming bridge integration) or improves the
> > > situation
> > > today ?
> > 
> > The former. It's not fixing or improving anything for the current
> > implementation.
> 
> If this patch isn't doing anything (for now) maybe it should be merged when it 
> becomes useful ?

Hm, good point. The intention was to keep things in small chunks for
easy reviewing. With the unicasted reports approach there were
more lines of code changed and an actual (but broken /
insufficient) change / "improvement". Now it got rather small with
no practical advantage so far. So yes, could be added to the
multicast-bridge patchset.

> 
> 
> > > > +#if IS_ENABLED(CONFIG_IPV6)
> > > > 
> > > >  	case ETH_P_IPV6:
> > > >  		return batadv_mcast_forw_mode_check_ipv6(bat_priv, skb,
> > > >  		
> > > >  							 is_unsnoopable);
> > > > 
> > > > +#endif
(Continue reading)

Linus Lüssing | 28 Jun 05:59 2015

Re: [PATCH 1/1] batman-adv: Always flood IGMP/MLD reports

On Sun, Jun 28, 2015 at 09:37:20AM +0800, Marek Lindner wrote:
> On Sunday, June 21, 2015 15:36:17 Linus Lüssing wrote:
> > With this patch IGMP or MLD reports are always flooded. This is
> > necessary for the upcoming bridge integration:
> > 
> > An IGMPv2/MLDv1 querier does not actively join the multicast group the
> > reports are sent to. Because of this, this would lead to snooping
> > bridges/switches not being able to learn of multicast listeners in the
> > mesh and wrongly shutting down ports for multicast traffic to the mesh,
> > leading to packet loss.
> 
> I am not quite clear on what the patch does. It helps to support a feature 
> that is yet to come (upcoming bridge integration) or improves the situation 
> today ?

The former. It's not fixing or improving anything for the current
implementation.

> 
> 
> >  /**
> >   * batadv_mcast_forw_mode_check - check for optimized forwarding potential
> >  <at>  <at>  -376,9 +434,11  <at>  <at>  static int batadv_mcast_forw_mode_check(struct
> > batadv_priv *bat_priv, case ETH_P_IP:
> >  		return batadv_mcast_forw_mode_check_ipv4(bat_priv, skb,
> >  							 is_unsnoopable);
> > +#if IS_ENABLED(CONFIG_IPV6)
> >  	case ETH_P_IPV6:
> >  		return batadv_mcast_forw_mode_check_ipv6(bat_priv, skb,
> >  							 is_unsnoopable);
(Continue reading)

Sven Eckelmann | 26 Jun 16:45 2015

Missing list checks for *list_add*

Hi,

Simon debugged the refcnt problem and submitted some patches to fix them. I
had a brief look and noticed that there are possible more problems similar to
the *list_del* ones - just with *list_add*. Basically some functions use some
kind of get function, notice that the element does not exist and then create
a new one to add to the list. Only the "list_add" is protected. The result may
be that an element in twice in a list when only a single occurrence is allowed.

The problem I saw is batadv_gw_node_update. It first calls batadv_gw_node_get
to check if an object with this value already exists and then uses
batadv_gw_node_add to add a node (which may already be added between these
two calls). So it has to be made sure that nothing modifies the list between
these two calls).

Similar looking functions are for example:

 * batadv_tvlv_handler_register
 * batadv_nc_get_nc_node
 * batadv_softif_create_vlan
 * batadv_tt_global_orig_entry_add

Just for illustration what I meant with "nothing modifies the list between
these two calls":

--- a/net/batman-adv/gateway_client.c
+++ b/net/batman-adv/gateway_client.c
 <at>  <at>  -30,6 +30,7  <at>  <at> 
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
(Continue reading)

Simon Wunderlich | 26 Jun 13:11 2015
Picon

[PATCHv2] batman-adv: remove obsolete deleted attribute for gateway node

From: Simon Wunderlich <simon@...>

With rcu, the gateway node deleted attribute is not needed anymore. In
fact, it may delay the free of the gateway node and its referenced
structures. Therefore remove it altogether and simplify purging as well.

Signed-off-by: Simon Wunderlich <simon@...>
---
Changes to PATCH:
 * iterator through list to remove the element when deleting to
   avoid possible double list remove
---
 net/batman-adv/gateway_client.c | 52 +++++++++++++++--------------------------
 net/batman-adv/originator.c     |  1 -
 net/batman-adv/types.h          |  2 --
 3 files changed, 19 insertions(+), 36 deletions(-)

diff --git a/net/batman-adv/gateway_client.c b/net/batman-adv/gateway_client.c
index 97f7c44..6af5dab 100644
--- a/net/batman-adv/gateway_client.c
+++ b/net/batman-adv/gateway_client.c
 <at>  <at>  -165,9 +165,6  <at>  <at>  batadv_gw_get_best_gw_node(struct batadv_priv *bat_priv)

 	rcu_read_lock();
 	hlist_for_each_entry_rcu(gw_node, &bat_priv->gw.list, list) {
-		if (gw_node->deleted)
-			continue;
-
 		orig_node = gw_node->orig_node;
 		router = batadv_orig_router_get(orig_node, BATADV_IF_DEFAULT);
(Continue reading)

Simon Wunderlich | 25 Jun 14:00 2015

Re: [PATCH] batman-adv: remove obsolete deleted attribute for gateway node

On Wednesday 24 June 2015 19:00:18 Antonio Quartulli wrote:
> On 24/06/15 14:50, Simon Wunderlich wrote:
> >  <at>  <at>  -537,9 +529,15  <at>  <at>  void batadv_gw_node_update(struct batadv_priv
> > *bat_priv,> 
> >  		/* Note: We don't need a NULL check here, since curr_gw never
> >  		
> >  		 * gets dereferenced.
> >  		 */
> > 
> > +		spin_lock_bh(&bat_priv->gw.list_lock);
> > +		hlist_del_rcu(&gw_node->list);
> > +		spin_unlock_bh(&bat_priv->gw.list_lock);
> > +
> 
> have you verified that we can't get to this point through two threads
> and enter the same problem that Sven and Marek have been discussing and
> fixing in these days: removing an element from a list twice
> 
> If this is possible, you should check that this element is in the list
> before deleting it.

That's an excellent point, I didn't check that. It also appears a little 
complicated, so I'll add some more protection at this point and resend v2.

Thank you!
     Simon

Matthias Schiffer | 24 Jun 20:34 2015
Picon

[RFC] batman-adv: add generic netlink query API to replace debugfs files

This is the first RFC for a new way to query originators and translation
tables, with the intention to make it a replacement for the current debugfs
files. I talked about this with Antonio in IRC 1~2 weeks ago.

debugfs is currently severely broken virtually everywhere in the kernel
where files are dynamically added and removed (see
http://lkml.iu.edu/hypermail/linux/kernel/1506.1/02196.html for some
details). In addition to that, there are general drawbacks to the current
approach:

* As batman-adv uses single_open, the whole content of the originators/
  transglobal files must fit into a single buffer; in large batman-adv
  networks this often fails (as an order-5 allocation or even more would be
  necessary)
* When originators or transglobal aren't just used for debugging, they are
  first converted to text, and then parsed again in userspace by tools like
  alfred/batadv-vis. Sending MAC address lists from the kernel to userspace
  as text makes the buffer size issue even worse.

For all commands that dump tables (originators, translocal, transglobal)
only NLM_F_DUMP queries are allowed, so arbitrary numbers of entries can be
dumped without ever needing a buffer larger than one page.

As the kernel can return to userspace any time during a query and only the
index of the entry to dump next is saved for the next call, this means that
the dump is not necessarily atomic, and it is even possible for entries
that haven't changed inbetween to be missing in the dump or be dumped
twice. This is a general limitation of all netlink APIs; it would be
possible though to add an atomic "revision" counter to each sent entry, so
userspace can at least detect this case and restart the query if atomicity
(Continue reading)

Simon Wunderlich | 24 Jun 14:50 2015
Picon

[PATCH] batman-adv: remove obsolete deleted attribute for gateway node

From: Simon Wunderlich <simon@...>

With rcu, the gateway node deleted attribute is not needed anymore. In
fact, it may delay the free of the gateway node and its referenced
structures. Therefore remove it altogether and simplify purging as well.

Signed-off-by: Simon Wunderlich <simon@...>
---
 net/batman-adv/gateway_client.c | 39 +++++++++------------------------------
 net/batman-adv/originator.c     |  1 -
 net/batman-adv/types.h          |  2 --
 3 files changed, 9 insertions(+), 33 deletions(-)

diff --git a/net/batman-adv/gateway_client.c b/net/batman-adv/gateway_client.c
index 97f7c44..ca436ff 100644
--- a/net/batman-adv/gateway_client.c
+++ b/net/batman-adv/gateway_client.c
 <at>  <at>  -165,9 +165,6  <at>  <at>  batadv_gw_get_best_gw_node(struct batadv_priv *bat_priv)

 	rcu_read_lock();
 	hlist_for_each_entry_rcu(gw_node, &bat_priv->gw.list, list) {
-		if (gw_node->deleted)
-			continue;
-
 		orig_node = gw_node->orig_node;
 		router = batadv_orig_router_get(orig_node, BATADV_IF_DEFAULT);
 		if (!router)
 <at>  <at>  -475,9 +472,6  <at>  <at>  batadv_gw_node_get(struct batadv_priv *bat_priv,
 		if (gw_node_tmp->orig_node != orig_node)
 			continue;
(Continue reading)

Simon Wunderlich | 24 Jun 14:50 2015
Picon

[PATCH maint 1/2] batman-adv: initialize up/down values when adding a gateway

From: Simon Wunderlich <simon@...>

Without this initialization, gateways which actually announce up/down
bandwidth of 0/0 could be added. If these nodes get purged later, the
gw_node structure does not get removed since batadv_gw_node_delete()
updates the gw_node with up/down bandwidth of 0/0, and the updating
function then discards the change and does not free gw_node.

This results in leaking the gw_node structures, which references other
structures: gw_node -> orig_node -> orig_node_ifinfo -> hardif. When
removing the interface later, the open reference on the hardif may cause
hangs with the infamous "unregister_netdevice: waiting for mesh1 to
become free. Usage count = 1" message.

Signed-off-by: Simon Wunderlich <simon@...>
---
 gateway_client.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gateway_client.c b/gateway_client.c
index 3f32357..d8e3ead 100644
--- a/gateway_client.c
+++ b/gateway_client.c
 <at>  <at>  -419,6 +419,8  <at>  <at>  static void batadv_gw_node_add(struct batadv_priv *bat_priv,

 	INIT_HLIST_NODE(&gw_node->list);
 	gw_node->orig_node = orig_node;
+	gw_node->bandwidth_down = ntohl(gateway->bandwidth_down);
+	gw_node->bandwidth_up = ntohl(gateway->bandwidth_up);
 	atomic_set(&gw_node->refcount, 1);
(Continue reading)

Sven Eckelmann | 22 Jun 09:23 2015

[PATCH] batman-adv: Add missing include rculist.h

originator.c is using hlist_for_each_entry_rcu but did not include the
required header for it.

Reported-by: Marek Lindner <mareklindner@...>
Signed-off-by: Sven Eckelmann <sven@...>
---
 net/batman-adv/originator.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/batman-adv/originator.c b/net/batman-adv/originator.c
index 4500e3a..f51860d 100644
--- a/net/batman-adv/originator.c
+++ b/net/batman-adv/originator.c
 <at>  <at>  -26,6 +26,7  <at>  <at> 
 #include <linux/list.h>
 #include <linux/lockdep.h>
 #include <linux/netdevice.h>
+#include <linux/rculist.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
--

-- 
2.1.4

Sven Eckelmann | 22 Jun 09:13 2015

[PATCH-maint] batman-adv: Replace gw_reselect 64 bit div with shift

The 64-bit gw_factor is divided by BATADV_TQ_LOCAL_WINDOW_SIZE ** 2 * 64.
But the rest of the calculation has nothing to do with the tq window size
end therefore the calculation is just (tmp_gw_factor / (64 ** 3)).

The problem with 64 bit div is that it doesn't work on systems without
native 64 bit div support. It has to be emulated using do_div or div_u64.
The change in f63c54bba31d ("batman-adv: Avoid u32 overflow during gateway
select") only compiled on such systems because the compiler converted the
div to a (tmp_gw_factor >> 18). Making this explicit avoids having build
problems in the future when BATADV_TQ_LOCAL_WINDOW_SIZE is changed in such
a way that (BATADV_TQ_LOCAL_WINDOW_SIZE ** 2 * 64) is not a power of two.

Signed-off-by: Sven Eckelmann <sven@...>
---
 gateway_client.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/gateway_client.c b/gateway_client.c
index 3f32357..0f8d06b 100644
--- a/gateway_client.c
+++ b/gateway_client.c
 <at>  <at>  -134,14 +134,10  <at>  <at>  batadv_gw_get_best_gw_node(struct batadv_priv *bat_priv)
 	struct batadv_neigh_ifinfo *router_ifinfo;
 	struct batadv_gw_node *gw_node, *curr_gw = NULL;
 	uint64_t max_gw_factor = 0, tmp_gw_factor = 0;
-	uint32_t gw_divisor;
 	uint8_t max_tq = 0;
 	uint8_t tq_avg;
 	struct batadv_orig_node *orig_node;

(Continue reading)


Gmane