[B.A.T.M.A.N.] [PATCH 1/3] batman-adv: Remove extra check in batadv_bit_get_packet

[B.A.T.M.A.N.] [PATCH 1/3] batman-adv: Remove extra check in batadv_bit_get_packet

[Sven Eckelmann]

batadv_bit_get_packet checks for all common situations before it decides that
the new received packet indicates that the host was restarted. This extra
condition check at the end of the function is not necessary because this
condition is always true.

This patch addresses Coverity #712296: Logically dead code

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 bitarray.c |   23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/bitarray.c b/bitarray.c
index aea174c..5453b17 100644
--- a/bitarray.c
+++ b/bitarray.c
@@ -79,20 +79,17 @@ int batadv_bit_get_packet(void *priv, unsigned long *seq_bits,
 	 * or the old packet got delayed somewhere in the network. The
 	 * packet should be dropped without calling this function if the
 	 * seqno window is protected.
+	 *
+	 * seq_num_diff <= -BATADV_TQ_LOCAL_WINDOW_SIZE
+	 * or
+	 * seq_num_diff >= BATADV_EXPECTED_SEQNO_RANGE
 	 */
-	if (seq_num_diff <= -BATADV_TQ_LOCAL_WINDOW_SIZE ||
-	    seq_num_diff >= BATADV_EXPECTED_SEQNO_RANGE) {
+	batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
+		   "Other host probably restarted!\n");
 
-		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
-			   "Other host probably restarted!\n");
+	bitmap_zero(seq_bits, BATADV_TQ_LOCAL_WINDOW_SIZE);
+	if (set_mark)
+		batadv_set_bit(seq_bits, 0);
 
-		bitmap_zero(seq_bits, BATADV_TQ_LOCAL_WINDOW_SIZE);
-		if (set_mark)
-			batadv_set_bit(seq_bits, 0);
-
-		return 1;
-	}
-
-	/* never reached */
-	return 0;
+	return 1;
 }
-- 
1.7.10.4

[B.A.T.M.A.N.] [PATCH 2/3] batman-adv: Check return value of try_module_get

[Sven Eckelmann]

New operations should not be started when they need an increased module
reference counter and try_module_get failed.

This patch addresses Coverity #712284: Unchecked return value

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 debugfs.c     |    4 +++-
 icmp_socket.c |    4 +++-
 main.c        |    4 ++--
 main.h        |    2 +-
 4 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/debugfs.c b/debugfs.c
index 391d4fb..63152be 100644
--- a/debugfs.c
+++ b/debugfs.c
@@ -99,9 +99,11 @@ int batadv_debug_log(struct batadv_priv *bat_priv, const char *fmt, ...)
 
 static int batadv_log_open(struct inode *inode, struct file *file)
 {
+	if (!batadv_inc_module_count())
+		return -EBUSY;
+
 	nonseekable_open(inode, file);
 	file->private_data = inode->i_private;
-	batadv_inc_module_count();
 	return 0;
 }
 
diff --git a/icmp_socket.c b/icmp_socket.c
index bde3cf7..8c8d0b8 100644
--- a/icmp_socket.c
+++ b/icmp_socket.c
@@ -42,6 +42,9 @@ static int batadv_socket_open(struct inode *inode, struct file *file)
 	unsigned int i;
 	struct batadv_socket_client *socket_client;
 
+	if (!batadv_inc_module_count())
+		return -EBUSY;
+
 	nonseekable_open(inode, file);
 
 	socket_client = kmalloc(sizeof(*socket_client), GFP_KERNEL);
@@ -71,7 +74,6 @@ static int batadv_socket_open(struct inode *inode, struct file *file)
 
 	file->private_data = socket_client;
 
-	batadv_inc_module_count();
 	return 0;
 }
 
diff --git a/main.c b/main.c
index b4aa470..b4b5b89 100644
--- a/main.c
+++ b/main.c
@@ -160,9 +160,9 @@ void batadv_mesh_free(struct net_device *soft_iface)
 	atomic_set(&bat_priv->mesh_state, BATADV_MESH_INACTIVE);
 }
 
-void batadv_inc_module_count(void)
+bool batadv_inc_module_count(void)
 {
-	try_module_get(THIS_MODULE);
+	return try_module_get(THIS_MODULE);
 }
 
 void batadv_dec_module_count(void)
diff --git a/main.h b/main.h
index f2227df..ded65b8 100644
--- a/main.h
+++ b/main.h
@@ -152,7 +152,7 @@ extern struct workqueue_struct *batadv_event_workqueue;
 
 int batadv_mesh_init(struct net_device *soft_iface);
 void batadv_mesh_free(struct net_device *soft_iface);
-void batadv_inc_module_count(void);
+bool batadv_inc_module_count(void);
 void batadv_dec_module_count(void);
 int batadv_is_my_mac(const uint8_t *addr);
 int batadv_batman_skb_recv(struct sk_buff *skb, struct net_device *dev,
-- 
1.7.10.4

[B.A.T.M.A.N.] [PATCHv3 2/3] batman-adv: Check return value of try_module_get

[Sven Eckelmann]

New operations should not be started when they need an increased module
reference counter and try_module_get failed.

This patch addresses Coverity #712284: Unchecked return value

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
Removed the wrapper functions

 debugfs.c     |    6 ++++--
 icmp_socket.c |   12 ++++++++----
 main.c        |   10 ----------
 main.h        |    2 --
 4 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/debugfs.c b/debugfs.c
index 391d4fb..bd032bc 100644
--- a/debugfs.c
+++ b/debugfs.c
@@ -99,15 +99,17 @@ int batadv_debug_log(struct batadv_priv *bat_priv, const char *fmt, ...)
 
 static int batadv_log_open(struct inode *inode, struct file *file)
 {
+	if (!try_module_get(THIS_MODULE))
+		return -EBUSY;
+
 	nonseekable_open(inode, file);
 	file->private_data = inode->i_private;
-	batadv_inc_module_count();
 	return 0;
 }
 
 static int batadv_log_release(struct inode *inode, struct file *file)
 {
-	batadv_dec_module_count();
+	module_put(THIS_MODULE);
 	return 0;
 }
 
diff --git a/icmp_socket.c b/icmp_socket.c
index bde3cf7..5874c0e 100644
--- a/icmp_socket.c
+++ b/icmp_socket.c
@@ -42,12 +42,16 @@ static int batadv_socket_open(struct inode *inode, struct file *file)
 	unsigned int i;
 	struct batadv_socket_client *socket_client;
 
+	if (!try_module_get(THIS_MODULE))
+		return -EBUSY;
+
 	nonseekable_open(inode, file);
 
 	socket_client = kmalloc(sizeof(*socket_client), GFP_KERNEL);
-
-	if (!socket_client)
+	if (!socket_client) {
+		module_put(THIS_MODULE);
 		return -ENOMEM;
+	}
 
 	for (i = 0; i < ARRAY_SIZE(batadv_socket_client_hash); i++) {
 		if (!batadv_socket_client_hash[i]) {
@@ -59,6 +63,7 @@ static int batadv_socket_open(struct inode *inode, struct file *file)
 	if (i == ARRAY_SIZE(batadv_socket_client_hash)) {
 		pr_err("Error - can't add another packet client: maximum number of clients reached\n");
 		kfree(socket_client);
+		module_put(THIS_MODULE);
 		return -EXFULL;
 	}
 
@@ -71,7 +76,6 @@ static int batadv_socket_open(struct inode *inode, struct file *file)
 
 	file->private_data = socket_client;
 
-	batadv_inc_module_count();
 	return 0;
 }
 
@@ -96,7 +100,7 @@ static int batadv_socket_release(struct inode *inode, struct file *file)
 	spin_unlock_bh(&socket_client->lock);
 
 	kfree(socket_client);
-	batadv_dec_module_count();
+	module_put(THIS_MODULE);
 
 	return 0;
 }
diff --git a/main.c b/main.c
index b4aa470..80a1d3e 100644
--- a/main.c
+++ b/main.c
@@ -160,16 +160,6 @@ void batadv_mesh_free(struct net_device *soft_iface)
 	atomic_set(&bat_priv->mesh_state, BATADV_MESH_INACTIVE);
 }
 
-void batadv_inc_module_count(void)
-{
-	try_module_get(THIS_MODULE);
-}
-
-void batadv_dec_module_count(void)
-{
-	module_put(THIS_MODULE);
-}
-
 int batadv_is_my_mac(const uint8_t *addr)
 {
 	const struct batadv_hard_iface *hard_iface;
diff --git a/main.h b/main.h
index f2227df..ebb8e98 100644
--- a/main.h
+++ b/main.h
@@ -152,8 +152,6 @@ extern struct workqueue_struct *batadv_event_workqueue;
 
 int batadv_mesh_init(struct net_device *soft_iface);
 void batadv_mesh_free(struct net_device *soft_iface);
-void batadv_inc_module_count(void);
-void batadv_dec_module_count(void);
 int batadv_is_my_mac(const uint8_t *addr);
 int batadv_batman_skb_recv(struct sk_buff *skb, struct net_device *dev,
 			   struct packet_type *ptype,
-- 
1.7.10.4

Re: [B.A.T.M.A.N.] [PATCHv3 2/3] batman-adv: Check return value of try_module_get

[Marek Lindner]

On Monday, August 20, 2012 23:37:26 Sven Eckelmann wrote:
> New operations should not be started when they need an increased module
> reference counter and try_module_get failed.
> 
> This patch addresses Coverity #712284: Unchecked return value
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
> Removed the wrapper functions
> 
>  debugfs.c     |    6 ++++--
>  icmp_socket.c |   12 ++++++++----
>  main.c        |   10 ----------
>  main.h        |    2 --
>  4 files changed, 12 insertions(+), 18 deletions(-)

Applied in revision 2e66c5e.

Thanks,
Marek

[B.A.T.M.A.N.] [PATCH 3/3] batman-adv: Only increase refcounter once for alternate router

[Sven Eckelmann]

The test whether we can use a router for alternating bonding should only be
done once because it is already known that it is still usable and will not be
deleted from the list soon.

This patch addresses Coverity #712285: Unchecked return value

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 routing.c |   23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/routing.c b/routing.c
index 939fc01..cdd81d8 100644
--- a/routing.c
+++ b/routing.c
@@ -549,25 +549,18 @@ batadv_find_ifalter_router(struct batadv_orig_node *primary_orig,
 		if (tmp_neigh_node->if_incoming == recv_if)
 			continue;
 
+		if (router && tmp_neigh_node->tq_avg <= router->tq_avg)
+			continue;
+
 		if (!atomic_inc_not_zero(&tmp_neigh_node->refcount))
 			continue;
 
-		/* if we don't have a router yet
-		 * or this one is better, choose it.
-		 */
-		if ((!router) ||
-		    (tmp_neigh_node->tq_avg > router->tq_avg)) {
-			/* decrement refcount of
-			 * previously selected router
-			 */
-			if (router)
-				batadv_neigh_node_free_ref(router);
+		/* decrement refcount of previously selected router */
+		if (router)
+			batadv_neigh_node_free_ref(router);
 
-			router = tmp_neigh_node;
-			atomic_inc_not_zero(&router->refcount);
-		}
-
-		batadv_neigh_node_free_ref(tmp_neigh_node);
+		/* we found a better router (or at least one valid router) */
+		router = tmp_neigh_node;
 	}
 
 	/* use the first candidate if nothing was found. */
-- 
1.7.10.4

Re: [B.A.T.M.A.N.] [PATCH 3/3] batman-adv: Only increase refcounter once for alternate router

[Marek Lindner]

On Monday, August 20, 2012 10:26:49 Sven Eckelmann wrote:
> The test whether we can use a router for alternating bonding should only be
> done once because it is already known that it is still usable and will not
> be deleted from the list soon.
> 
> This patch addresses Coverity #712285: Unchecked return value
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
>  routing.c |   23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)

Applied in revision 534f242.

Thanks,
Marek

[B.A.T.M.A.N.] [PATCHv2 2/3] batman-adv: Check return value of try_module_get

[Sven Eckelmann]

New operations should not be started when they need an increased module
reference counter and try_module_get failed.

This patch addresses Coverity #712284: Unchecked return value

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
Added missing batadv_dec_module_count to batadv_socket_open

 debugfs.c     |    4 +++-
 icmp_socket.c |   10 +++++++---
 main.c        |    4 ++--
 main.h        |    2 +-
 4 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/debugfs.c b/debugfs.c
index 391d4fb..63152be 100644
--- a/debugfs.c
+++ b/debugfs.c
@@ -99,9 +99,11 @@ int batadv_debug_log(struct batadv_priv *bat_priv, const char *fmt, ...)
 
 static int batadv_log_open(struct inode *inode, struct file *file)
 {
+	if (!batadv_inc_module_count())
+		return -EBUSY;
+
 	nonseekable_open(inode, file);
 	file->private_data = inode->i_private;
-	batadv_inc_module_count();
 	return 0;
 }
 
diff --git a/icmp_socket.c b/icmp_socket.c
index bde3cf7..562bf07 100644
--- a/icmp_socket.c
+++ b/icmp_socket.c
@@ -42,12 +42,16 @@ static int batadv_socket_open(struct inode *inode, struct file *file)
 	unsigned int i;
 	struct batadv_socket_client *socket_client;
 
+	if (!batadv_inc_module_count())
+		return -EBUSY;
+
 	nonseekable_open(inode, file);
 
 	socket_client = kmalloc(sizeof(*socket_client), GFP_KERNEL);
-
-	if (!socket_client)
+	if (!socket_client) {
+		batadv_dec_module_count();
 		return -ENOMEM;
+	}
 
 	for (i = 0; i < ARRAY_SIZE(batadv_socket_client_hash); i++) {
 		if (!batadv_socket_client_hash[i]) {
@@ -59,6 +63,7 @@ static int batadv_socket_open(struct inode *inode, struct file *file)
 	if (i == ARRAY_SIZE(batadv_socket_client_hash)) {
 		pr_err("Error - can't add another packet client: maximum number of clients reached\n");
 		kfree(socket_client);
+		batadv_dec_module_count();
 		return -EXFULL;
 	}
 
@@ -71,7 +76,6 @@ static int batadv_socket_open(struct inode *inode, struct file *file)
 
 	file->private_data = socket_client;
 
-	batadv_inc_module_count();
 	return 0;
 }
 
diff --git a/main.c b/main.c
index b4aa470..b4b5b89 100644
--- a/main.c
+++ b/main.c
@@ -160,9 +160,9 @@ void batadv_mesh_free(struct net_device *soft_iface)
 	atomic_set(&bat_priv->mesh_state, BATADV_MESH_INACTIVE);
 }
 
-void batadv_inc_module_count(void)
+bool batadv_inc_module_count(void)
 {
-	try_module_get(THIS_MODULE);
+	return try_module_get(THIS_MODULE);
 }
 
 void batadv_dec_module_count(void)
diff --git a/main.h b/main.h
index f2227df..ded65b8 100644
--- a/main.h
+++ b/main.h
@@ -152,7 +152,7 @@ extern struct workqueue_struct *batadv_event_workqueue;
 
 int batadv_mesh_init(struct net_device *soft_iface);
 void batadv_mesh_free(struct net_device *soft_iface);
-void batadv_inc_module_count(void);
+bool batadv_inc_module_count(void);
 void batadv_dec_module_count(void);
 int batadv_is_my_mac(const uint8_t *addr);
 int batadv_batman_skb_recv(struct sk_buff *skb, struct net_device *dev,
-- 
1.7.10.4

Re: [B.A.T.M.A.N.] [PATCH 1/3] batman-adv: Remove extra check in batadv_bit_get_packet

[Marek Lindner]

On Monday, August 20, 2012 10:26:47 Sven Eckelmann wrote:
> batadv_bit_get_packet checks for all common situations before it decides
> that the new received packet indicates that the host was restarted. This
> extra condition check at the end of the function is not necessary because
> this condition is always true.
> 
> This patch addresses Coverity #712296: Logically dead code
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
>  bitarray.c |   23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)

Applied in revision d2da57c.

Thanks,
Marek