linux-kernel - Re: [PATCH v2] carl9170: Remove redundant protection check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1679858.MgakDZE2Vr@debian64>
Date:	Fri, 22 Aug 2014 22:38:08 +0200
From:	Christian Lamparter <chunkeey@...glemail.com>
To:	Andreea-Cristina Bernat <bernat.ada@...il.com>
Cc:	linville@...driver.com, linux-wireless@...r.kernel.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	paulmck@...ux.vnet.ibm.com
Subject: Re: [PATCH v2] carl9170: Remove redundant protection check

On Friday, August 22, 2014 10:14:31 PM Andreea-Cristina Bernat wrote:
> The carl9170_op_ampdu_action() function is used only by the mac80211
> framework. Since the mac80211 already takes care of checks and 
> properly serializing calls to the driver's function there is no
> need for the driver to do the same thing.
> 
> Signed-off-by: Andreea-Cristina Bernat <bernat.ada@...il.com>
> ---
> Changes in v2:
>  - Change subject line from
>    "carl9170: Replace rcu_dereference() with rcu_access_pointer()"
>    to
>    "carl9170: Remove redundant protection check"
>  - Update the commit message according to the modifications
>  - Delete the lines of interest at the suggestion and explanations of
>    Christian Lamparter <chunkeey@...glemail.com>
> 
>  drivers/net/wireless/ath/carl9170/main.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c
> index 12018ff..6758b9a 100644
> --- a/drivers/net/wireless/ath/carl9170/main.c
> +++ b/drivers/net/wireless/ath/carl9170/main.c
> @@ -1430,12 +1430,6 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
>  		if (!sta_info->ht_sta)
>  			return -EOPNOTSUPP;
>  
> -		rcu_read_lock();
> -		if (rcu_access_pointer(sta_info->agg[tid])) {
> -			rcu_read_unlock();
> -			return -EBUSY;
> -		}
> -
>  		tid_info = kzalloc(sizeof(struct carl9170_sta_tid),
>  				   GFP_ATOMIC);
>  		if (!tid_info) {
> 

sparse [0] hit a bug when testing the patch:

drivers/net/wireless/ath/carl9170/main.c:1440:17: 
  warning: context imbalance in 'carl9170_op_ampdu_action' - unexpected unlock

This warning is caused by the remaining, stray rcu_read protection
in the code below (Sorry! Guess RCU needed a bit more explanation
in the previous post. If you are looking for *pointers*, there are
excellent resources available in Documentation/RCU/ [1]).

I've attached a full patch (see below) with all changes so far and
tested if the device/driver still behaves ;-). This patch applies 
cleanly on top of wireless-testing.

@John,
can you please take it?

---
From: Andreea-Cristina Bernat <bernat.ada@...il.com>

The carl9170_op_ampdu_action() function is used only by the mac80211
framework. Since the mac80211 already takes care of checks and 
properly serializing calls to the driver's function there is no
need for the driver to do the same thing.
 
Signed-off-by: Andreea-Cristina Bernat <bernat.ada@...il.com>
[chunkeey@...glemail.com: remove two stray rcu_read_unlock()]
Signed-off-by: Christian Lamparter <chunkeey@...glemail.com>
---
diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c
index f8ded84..ef5b6dc 100644
--- a/drivers/net/wireless/ath/carl9170/main.c
+++ b/drivers/net/wireless/ath/carl9170/main.c
@@ -1430,18 +1430,10 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
 		if (!sta_info->ht_sta)
 			return -EOPNOTSUPP;
 
-		rcu_read_lock();
-		if (rcu_dereference(sta_info->agg[tid])) {
-			rcu_read_unlock();
-			return -EBUSY;
-		}
-
 		tid_info = kzalloc(sizeof(struct carl9170_sta_tid),
 				   GFP_ATOMIC);
-		if (!tid_info) {
-			rcu_read_unlock();
+		if (!tid_info)
 			return -ENOMEM;
-		}
 
 		tid_info->hsn = tid_info->bsn = tid_info->snx = (*ssn);
 		tid_info->state = CARL9170_TID_STATE_PROGRESS;
@@ -1460,7 +1452,6 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
 		list_add_tail_rcu(&tid_info->list, &ar->tx_ampdu_list);
 		rcu_assign_pointer(sta_info->agg[tid], tid_info);
 		spin_unlock_bh(&ar->tx_ampdu_list_lock);
-		rcu_read_unlock();
 
 		ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid);
 		break;
---

Regards
Christian

[0] <http://kernelnewbies.org/Sparse>
[1] <https://www.kernel.org/doc/Documentation/RCU/whatisRCU.txt>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/