diff mbox

[maint] batman-adv: Fix broken capability checks

Message ID 1435811430-6949-1-git-send-email-linus.luessing@c0d3.blue
State Superseded, archived
Headers show

Commit Message

Linus Lüssing July 2, 2015, 4:30 a.m. UTC
The introduction of set_bit() and clear_bit() calls in batman-adv
wrongly passed bitmasks and not the bit numbers to these functions.
This leads to broken capability checks in the according features.

Fixing this by making the capability enum a non-bitmasked one and by
that passing non-masked values to set_bit()/clear_bit() while explicitly
masking with BIT() for testing capability presence.

Fixes: bfd0fbaef270 ("batman-adv: Make DAT capability changes atomic")
Fixes: 586df9e2537b ("batman-adv: Make NC capability changes atomic")
Fixes: a51fa16ecf3f ("batman-adv: Make TT capability changes atomic")
Fixes: 201a54ba710a ("batman-adv: Make MCAST capability changes atomic")
Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
---
Maybe wait for a desired "Reported-by" name/email in #219.

 distributed-arp-table.c |    2 +-
 multicast.c             |   16 ++++++++--------
 network-coding.c        |    2 +-
 translation-table.c     |    3 ++-
 types.h                 |    8 ++++----
 5 files changed, 16 insertions(+), 15 deletions(-)

Comments

Linus Lüssing July 2, 2015, 8:34 p.m. UTC | #1
Reported-by: Def <def@laposte.net>


On Thu, Jul 02, 2015 at 06:30:30AM +0200, Linus Lüssing wrote:
> The introduction of set_bit() and clear_bit() calls in batman-adv
> wrongly passed bitmasks and not the bit numbers to these functions.
> This leads to broken capability checks in the according features.
> 
> Fixing this by making the capability enum a non-bitmasked one and by
> that passing non-masked values to set_bit()/clear_bit() while explicitly
> masking with BIT() for testing capability presence.
> 
> Fixes: bfd0fbaef270 ("batman-adv: Make DAT capability changes atomic")
> Fixes: 586df9e2537b ("batman-adv: Make NC capability changes atomic")
> Fixes: a51fa16ecf3f ("batman-adv: Make TT capability changes atomic")
> Fixes: 201a54ba710a ("batman-adv: Make MCAST capability changes atomic")
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
> ---
> Maybe wait for a desired "Reported-by" name/email in #219.
> 
>  distributed-arp-table.c |    2 +-
>  multicast.c             |   16 ++++++++--------
>  network-coding.c        |    2 +-
>  translation-table.c     |    3 ++-
>  types.h                 |    8 ++++----
>  5 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/distributed-arp-table.c b/distributed-arp-table.c
> index b2cc19b..c663201 100644
> --- a/distributed-arp-table.c
> +++ b/distributed-arp-table.c
> @@ -422,7 +422,7 @@ static bool batadv_is_orig_node_eligible(struct batadv_dat_candidate *res,
>  	int j;
>  
>  	/* check if orig node candidate is running DAT */
> -	if (!(candidate->capabilities & BATADV_ORIG_CAPA_HAS_DAT))
> +	if (!(candidate->capabilities & BIT(BATADV_ORIG_CAPA_HAS_DAT)))
>  		goto out;
>  
>  	/* Check if this node has already been selected... */
> diff --git a/multicast.c b/multicast.c
> index b75bcc3..5b6a679 100644
> --- a/multicast.c
> +++ b/multicast.c
> @@ -702,22 +702,22 @@ static void batadv_mcast_tvlv_ogm_handler_v1(struct batadv_priv *bat_priv,
>  {
>  	bool orig_mcast_enabled = !(flags & BATADV_TVLV_HANDLER_OGM_CIFNOTFND);
>  	uint8_t mcast_flags = BATADV_NO_FLAGS;
> -	bool orig_initialized;
> +	bool initialized;
>  
>  	if (orig_mcast_enabled && tvlv_value &&
>  	    (tvlv_value_len >= sizeof(mcast_flags)))
>  		mcast_flags = *(uint8_t *)tvlv_value;
>  
>  	spin_lock_bh(&orig->mcast_handler_lock);
> -	orig_initialized = orig->capa_initialized & BATADV_ORIG_CAPA_HAS_MCAST;
> +	initialized = orig->capa_initialized & BIT(BATADV_ORIG_CAPA_HAS_MCAST);
>  
>  	/* If mcast support is turned on decrease the disabled mcast node
>  	 * counter only if we had increased it for this node before. If this
>  	 * is a completely new orig_node no need to decrease the counter.
>  	 */
>  	if (orig_mcast_enabled &&
> -	    !(orig->capabilities & BATADV_ORIG_CAPA_HAS_MCAST)) {
> -		if (orig_initialized)
> +	    !(orig->capabilities & BIT(BATADV_ORIG_CAPA_HAS_MCAST))) {
> +		if (initialized)
>  			atomic_dec(&bat_priv->mcast.num_disabled);
>  		set_bit(BATADV_ORIG_CAPA_HAS_MCAST, &orig->capabilities);
>  	/* If mcast support is being switched off or if this is an initial
> @@ -725,8 +725,8 @@ static void batadv_mcast_tvlv_ogm_handler_v1(struct batadv_priv *bat_priv,
>  	 * node counter.
>  	 */
>  	} else if (!orig_mcast_enabled &&
> -		   (orig->capabilities & BATADV_ORIG_CAPA_HAS_MCAST ||
> -		    !orig_initialized)) {
> +		   (orig->capabilities & BIT(BATADV_ORIG_CAPA_HAS_MCAST) ||
> +		    !initialized)) {
>  		atomic_inc(&bat_priv->mcast.num_disabled);
>  		clear_bit(BATADV_ORIG_CAPA_HAS_MCAST, &orig->capabilities);
>  	}
> @@ -774,8 +774,8 @@ void batadv_mcast_purge_orig(struct batadv_orig_node *orig)
>  
>  	spin_lock_bh(&orig->mcast_handler_lock);
>  
> -	if (!(orig->capabilities & BATADV_ORIG_CAPA_HAS_MCAST) &&
> -	    orig->capa_initialized & BATADV_ORIG_CAPA_HAS_MCAST)
> +	if (!(orig->capabilities & BIT(BATADV_ORIG_CAPA_HAS_MCAST)) &&
> +	    orig->capa_initialized & BIT(BATADV_ORIG_CAPA_HAS_MCAST))
>  		atomic_dec(&bat_priv->mcast.num_disabled);
>  
>  	batadv_mcast_want_unsnoop_update(bat_priv, orig, BATADV_NO_FLAGS);
> diff --git a/network-coding.c b/network-coding.c
> index 3ce493e..3690606 100644
> --- a/network-coding.c
> +++ b/network-coding.c
> @@ -871,7 +871,7 @@ void batadv_nc_update_nc_node(struct batadv_priv *bat_priv,
>  		goto out;
>  
>  	/* check if orig node is network coding enabled */
> -	if (!(orig_node->capabilities & BATADV_ORIG_CAPA_HAS_NC))
> +	if (!(orig_node->capabilities & BIT(BATADV_ORIG_CAPA_HAS_NC)))
>  		goto out;
>  
>  	/* accept ogms from 'good' neighbors and single hop neighbors */
> diff --git a/translation-table.c b/translation-table.c
> index b6c0f52..8de5df4 100644
> --- a/translation-table.c
> +++ b/translation-table.c
> @@ -3319,11 +3319,12 @@ static void batadv_tt_update_orig(struct batadv_priv *bat_priv,
>  {
>  	uint8_t orig_ttvn = (uint8_t)atomic_read(&orig_node->last_ttvn);
>  	struct batadv_tvlv_tt_vlan_data *tt_vlan;
> +	unsigned long capa_initialized = orig_node->capa_initialized;
>  	bool full_table = true;
>  	bool has_tt_init;
>  
>  	tt_vlan = (struct batadv_tvlv_tt_vlan_data *)tt_buff;
> -	has_tt_init = orig_node->capa_initialized & BATADV_ORIG_CAPA_HAS_TT;
> +	has_tt_init = capa_initialized & BIT(BATADV_ORIG_CAPA_HAS_TT);
>  
>  	/* orig table not initialised AND first diff is in the OGM OR the ttvn
>  	 * increased by one -> we can apply the attached changes
> diff --git a/types.h b/types.h
> index 65dc6bf..3baf595 100644
> --- a/types.h
> +++ b/types.h
> @@ -299,10 +299,10 @@ struct batadv_orig_node {
>   *  (= orig node announces a tvlv of type BATADV_TVLV_MCAST)
>   */
>  enum batadv_orig_capabilities {
> -	BATADV_ORIG_CAPA_HAS_DAT = BIT(0),
> -	BATADV_ORIG_CAPA_HAS_NC = BIT(1),
> -	BATADV_ORIG_CAPA_HAS_TT = BIT(2),
> -	BATADV_ORIG_CAPA_HAS_MCAST = BIT(3),
> +	BATADV_ORIG_CAPA_HAS_DAT,
> +	BATADV_ORIG_CAPA_HAS_NC,
> +	BATADV_ORIG_CAPA_HAS_TT,
> +	BATADV_ORIG_CAPA_HAS_MCAST,
>  };
>  
>  /**
> -- 
> 1.7.10.4
>
Marek Lindner July 3, 2015, 11:13 a.m. UTC | #2
On Thursday, July 02, 2015 06:30:30 Linus Lüssing wrote:
> Fixes: bfd0fbaef270 ("batman-adv: Make DAT capability changes atomic")
> Fixes: 586df9e2537b ("batman-adv: Make NC capability changes atomic")
> Fixes: a51fa16ecf3f ("batman-adv: Make TT capability changes atomic")
> Fixes: 201a54ba710a ("batman-adv: Make MCAST capability changes atomic")

Can we please split these patches too ? Antonio needs to merge the patch pairs 
before submitting them.


> @@ -422,7 +422,7 @@ static bool batadv_is_orig_node_eligible(struct
> batadv_dat_candidate *res, int j;
> 
>  	/* check if orig node candidate is running DAT */
> -	if (!(candidate->capabilities & BATADV_ORIG_CAPA_HAS_DAT))
> +	if (!(candidate->capabilities & BIT(BATADV_ORIG_CAPA_HAS_DAT)))
>  		goto out;

How about using test_bit() to be consistent ?


> @@ -299,10 +299,10 @@ struct batadv_orig_node {
>   *  (= orig node announces a tvlv of type BATADV_TVLV_MCAST)
>   */
>  enum batadv_orig_capabilities {
> -	BATADV_ORIG_CAPA_HAS_DAT = BIT(0),
> -	BATADV_ORIG_CAPA_HAS_NC = BIT(1),
> -	BATADV_ORIG_CAPA_HAS_TT = BIT(2),
> -	BATADV_ORIG_CAPA_HAS_MCAST = BIT(3),
> +	BATADV_ORIG_CAPA_HAS_DAT,
> +	BATADV_ORIG_CAPA_HAS_NC,
> +	BATADV_ORIG_CAPA_HAS_TT,
> +	BATADV_ORIG_CAPA_HAS_MCAST,
>  };

Do we need an initialization of the first element or can we rely on the 
compiler to start with 0 ?

Cheers,
Marek
Sven Eckelmann July 3, 2015, 11:40 a.m. UTC | #3
On Friday 03 July 2015 19:13:02 Marek Lindner wrote:
> > @@ -299,10 +299,10 @@ struct batadv_orig_node {
> >   *  (= orig node announces a tvlv of type BATADV_TVLV_MCAST)
> >   */
> >  enum batadv_orig_capabilities {
> > -	BATADV_ORIG_CAPA_HAS_DAT = BIT(0),
> > -	BATADV_ORIG_CAPA_HAS_NC = BIT(1),
> > -	BATADV_ORIG_CAPA_HAS_TT = BIT(2),
> > -	BATADV_ORIG_CAPA_HAS_MCAST = BIT(3),
> > +	BATADV_ORIG_CAPA_HAS_DAT,
> > +	BATADV_ORIG_CAPA_HAS_NC,
> > +	BATADV_ORIG_CAPA_HAS_TT,
> > +	BATADV_ORIG_CAPA_HAS_MCAST,
> >  };
> 
> Do we need an initialization of the first element or can we rely on the 
> compiler to start with 0 ?

Just discussed this with Marek on IRC. We came to the conclusion that ANSI C 
"3.5.2.2 Enumeration specifiers" is enforcing a 0 as first entry (when not 
specified otherwise):

    If the first enumerator has no = , the value of
    its enumeration constant is 0.  Each subsequent enumerator with no =
    defines its enumeration constant as the value of the constant
    expression obtained by adding 1 to the value of the previous
    enumeration constant.

But the code should maybe cleaned up to either use "= 0" everywhere or nowhere 
in the internal representations.

Places where = 0 is used:

 * net/batman-adv/bat_iv_ogm.c:enum batadv_dup_status
 * net/batman-adv/main.h:enum batadv_uev_action (maybe not internal?)
 * net/batman-adv/main.h:enum batadv_uev_type (maybe not internal?)
 * net/batman-adv/types.h:enum batadv_dhcp_recipient

Not used:

 * net/batman-adv/gateway_common.h:enum batadv_gw_modes
 * net/batman-adv/gateway_common.h:enum batadv_bandwidth_units
 * net/batman-adv/hard-interface.h:enum batadv_hard_if_state
 * net/batman-adv/hard-interface.h:enum batadv_hard_if_cleanup
 * net/batman-adv/main.h:enum batadv_mesh_state
 * net/batman-adv/multicast.h:enum batadv_forw_mode
 * net/batman-adv/types.h:enum batadv_counters

I don't count packet.h because this is clearly not internal and the packet 
definitions should not be accidentally changed just because the enum constant 
order is changed.

Kind regards,
	Sven
diff mbox

Patch

diff --git a/distributed-arp-table.c b/distributed-arp-table.c
index b2cc19b..c663201 100644
--- a/distributed-arp-table.c
+++ b/distributed-arp-table.c
@@ -422,7 +422,7 @@  static bool batadv_is_orig_node_eligible(struct batadv_dat_candidate *res,
 	int j;
 
 	/* check if orig node candidate is running DAT */
-	if (!(candidate->capabilities & BATADV_ORIG_CAPA_HAS_DAT))
+	if (!(candidate->capabilities & BIT(BATADV_ORIG_CAPA_HAS_DAT)))
 		goto out;
 
 	/* Check if this node has already been selected... */
diff --git a/multicast.c b/multicast.c
index b75bcc3..5b6a679 100644
--- a/multicast.c
+++ b/multicast.c
@@ -702,22 +702,22 @@  static void batadv_mcast_tvlv_ogm_handler_v1(struct batadv_priv *bat_priv,
 {
 	bool orig_mcast_enabled = !(flags & BATADV_TVLV_HANDLER_OGM_CIFNOTFND);
 	uint8_t mcast_flags = BATADV_NO_FLAGS;
-	bool orig_initialized;
+	bool initialized;
 
 	if (orig_mcast_enabled && tvlv_value &&
 	    (tvlv_value_len >= sizeof(mcast_flags)))
 		mcast_flags = *(uint8_t *)tvlv_value;
 
 	spin_lock_bh(&orig->mcast_handler_lock);
-	orig_initialized = orig->capa_initialized & BATADV_ORIG_CAPA_HAS_MCAST;
+	initialized = orig->capa_initialized & BIT(BATADV_ORIG_CAPA_HAS_MCAST);
 
 	/* If mcast support is turned on decrease the disabled mcast node
 	 * counter only if we had increased it for this node before. If this
 	 * is a completely new orig_node no need to decrease the counter.
 	 */
 	if (orig_mcast_enabled &&
-	    !(orig->capabilities & BATADV_ORIG_CAPA_HAS_MCAST)) {
-		if (orig_initialized)
+	    !(orig->capabilities & BIT(BATADV_ORIG_CAPA_HAS_MCAST))) {
+		if (initialized)
 			atomic_dec(&bat_priv->mcast.num_disabled);
 		set_bit(BATADV_ORIG_CAPA_HAS_MCAST, &orig->capabilities);
 	/* If mcast support is being switched off or if this is an initial
@@ -725,8 +725,8 @@  static void batadv_mcast_tvlv_ogm_handler_v1(struct batadv_priv *bat_priv,
 	 * node counter.
 	 */
 	} else if (!orig_mcast_enabled &&
-		   (orig->capabilities & BATADV_ORIG_CAPA_HAS_MCAST ||
-		    !orig_initialized)) {
+		   (orig->capabilities & BIT(BATADV_ORIG_CAPA_HAS_MCAST) ||
+		    !initialized)) {
 		atomic_inc(&bat_priv->mcast.num_disabled);
 		clear_bit(BATADV_ORIG_CAPA_HAS_MCAST, &orig->capabilities);
 	}
@@ -774,8 +774,8 @@  void batadv_mcast_purge_orig(struct batadv_orig_node *orig)
 
 	spin_lock_bh(&orig->mcast_handler_lock);
 
-	if (!(orig->capabilities & BATADV_ORIG_CAPA_HAS_MCAST) &&
-	    orig->capa_initialized & BATADV_ORIG_CAPA_HAS_MCAST)
+	if (!(orig->capabilities & BIT(BATADV_ORIG_CAPA_HAS_MCAST)) &&
+	    orig->capa_initialized & BIT(BATADV_ORIG_CAPA_HAS_MCAST))
 		atomic_dec(&bat_priv->mcast.num_disabled);
 
 	batadv_mcast_want_unsnoop_update(bat_priv, orig, BATADV_NO_FLAGS);
diff --git a/network-coding.c b/network-coding.c
index 3ce493e..3690606 100644
--- a/network-coding.c
+++ b/network-coding.c
@@ -871,7 +871,7 @@  void batadv_nc_update_nc_node(struct batadv_priv *bat_priv,
 		goto out;
 
 	/* check if orig node is network coding enabled */
-	if (!(orig_node->capabilities & BATADV_ORIG_CAPA_HAS_NC))
+	if (!(orig_node->capabilities & BIT(BATADV_ORIG_CAPA_HAS_NC)))
 		goto out;
 
 	/* accept ogms from 'good' neighbors and single hop neighbors */
diff --git a/translation-table.c b/translation-table.c
index b6c0f52..8de5df4 100644
--- a/translation-table.c
+++ b/translation-table.c
@@ -3319,11 +3319,12 @@  static void batadv_tt_update_orig(struct batadv_priv *bat_priv,
 {
 	uint8_t orig_ttvn = (uint8_t)atomic_read(&orig_node->last_ttvn);
 	struct batadv_tvlv_tt_vlan_data *tt_vlan;
+	unsigned long capa_initialized = orig_node->capa_initialized;
 	bool full_table = true;
 	bool has_tt_init;
 
 	tt_vlan = (struct batadv_tvlv_tt_vlan_data *)tt_buff;
-	has_tt_init = orig_node->capa_initialized & BATADV_ORIG_CAPA_HAS_TT;
+	has_tt_init = capa_initialized & BIT(BATADV_ORIG_CAPA_HAS_TT);
 
 	/* orig table not initialised AND first diff is in the OGM OR the ttvn
 	 * increased by one -> we can apply the attached changes
diff --git a/types.h b/types.h
index 65dc6bf..3baf595 100644
--- a/types.h
+++ b/types.h
@@ -299,10 +299,10 @@  struct batadv_orig_node {
  *  (= orig node announces a tvlv of type BATADV_TVLV_MCAST)
  */
 enum batadv_orig_capabilities {
-	BATADV_ORIG_CAPA_HAS_DAT = BIT(0),
-	BATADV_ORIG_CAPA_HAS_NC = BIT(1),
-	BATADV_ORIG_CAPA_HAS_TT = BIT(2),
-	BATADV_ORIG_CAPA_HAS_MCAST = BIT(3),
+	BATADV_ORIG_CAPA_HAS_DAT,
+	BATADV_ORIG_CAPA_HAS_NC,
+	BATADV_ORIG_CAPA_HAS_TT,
+	BATADV_ORIG_CAPA_HAS_MCAST,
 };
 
 /**