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, };
/**
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))) {
set_bit(BATADV_ORIG_CAPA_HAS_MCAST, &orig->capabilities); /* If mcast support is being switched off or if this is an initialif (initialized) atomic_dec(&bat_priv->mcast.num_disabled);
@@ -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) ||
atomic_inc(&bat_priv->mcast.num_disabled); clear_bit(BATADV_ORIG_CAPA_HAS_MCAST, &orig->capabilities); }!initialized)) {
@@ -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
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
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
b.a.t.m.a.n@lists.open-mesh.org