From patchwork Fri Jun 26 14:45:01 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sven Eckelmann X-Patchwork-Id: 4538 X-Patchwork-Delegate: mareklindner@neomailbox.ch Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=79.140.41.39; helo=v3-1039.vlinux.de; envelope-from=sven@narfation.org; receiver=b.a.t.m.a.n@lists.open-mesh.org Received: from v3-1039.vlinux.de (narfation.org [79.140.41.39]) by open-mesh.org (Postfix) with ESMTPS id 82EDA601419 for ; Fri, 26 Jun 2015 16:45:06 +0200 (CEST) Received: from bentobox.localnet (x4d05e906.dyn.telefonica.de [77.5.233.6]) by v3-1039.vlinux.de (Postfix) with ESMTPSA id 81BAD1100FE; Fri, 26 Jun 2015 16:45:05 +0200 (CEST) From: Sven Eckelmann To: b.a.t.m.a.n@lists.open-mesh.org Date: Fri, 26 Jun 2015 16:45:01 +0200 Message-ID: <1898798.WP4bC1arXA@bentobox> User-Agent: KMail/4.14.2 (Linux/4.0.0-2-amd64; KDE/4.14.2; x86_64; ; ) MIME-Version: 1.0 Cc: Martin =?ISO-8859-1?Q?Hundeb=F8ll?= , Marek Lindner , Antonio Quartulli Subject: [B.A.T.M.A.N.] Missing list checks for *list_add* X-BeenThere: b.a.t.m.a.n@lists.open-mesh.org X-Mailman-Version: 2.1.15 Precedence: list Reply-To: The list for a Better Approach To Mobile Ad-hoc Networking List-Id: The list for a Better Approach To Mobile Ad-hoc Networking List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 26 Jun 2015 14:45:07 -0000 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": Kind regards, Sven --- a/net/batman-adv/gateway_client.c +++ b/net/batman-adv/gateway_client.c @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -425,6 +426,8 @@ static void batadv_gw_node_add(struct batadv_priv *bat_priv, { struct batadv_gw_node *gw_node; + lockdep_assert_held(&bat_priv->gw.list_lock); + if (gateway->bandwidth_down == 0) return; @@ -441,9 +444,7 @@ static void batadv_gw_node_add(struct batadv_priv *bat_priv, gw_node->orig_node = orig_node; atomic_set(&gw_node->refcount, 1); - spin_lock_bh(&bat_priv->gw.list_lock); hlist_add_head_rcu(&gw_node->list, &bat_priv->gw.list); - spin_unlock_bh(&bat_priv->gw.list_lock); batadv_dbg(BATADV_DBG_BATMAN, bat_priv, "Found new gateway %pM -> gw bandwidth: %u.%u/%u.%u MBit\n", @@ -497,11 +498,14 @@ void batadv_gw_node_update(struct batadv_priv *bat_priv, struct batadv_gw_node *gw_node, *gw_node_tmp, *curr_gw = NULL; struct hlist_node *node_tmp; + spin_lock_bh(&bat_priv->gw.list_lock); gw_node = batadv_gw_node_get(bat_priv, orig_node); if (!gw_node) { batadv_gw_node_add(bat_priv, orig_node, gateway); + spin_unlock_bh(&bat_priv->gw.list_lock); goto out; } + spin_unlock_bh(&bat_priv->gw.list_lock); if ((gw_node->bandwidth_down == ntohl(gateway->bandwidth_down)) && (gw_node->bandwidth_up == ntohl(gateway->bandwidth_up)))