Skip to contentSkip to conversation

ath11k: fix a locking bug in ath11k_mac_op_start()

PATCH Superseded Dan Carpenter 3 participants 3 messages Feb 1, 2021 – Feb 2, 2021

Subsystems (2)
Kernel janitors Qualcomm Atheros ATH11K wireless driver

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 ath11k: fix a locking bug in ath11k_mac_op_start()

This error path leads to a Smatch warning:

    drivers/net/wireless/ath/ath11k/mac.c:4269 ath11k_mac_op_start()
    error: double unlocked '&ar->conf_mutex' (orig line 4251)

We're not holding the lock when we do the "goto err;" so it leads to a double unlock. We should hold the lock because the error path sets "ar->state" so the right fix is to take the lock before doing the goto.

Fixes c83c500b55b6 ("ath11k: enable idle power save mode")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/net/wireless/ath/ath11k/mac.c | 1 +
 1 file changed, 1 insertion(+)
drivers/net/wireless/ath/ath11k/mac.c +1 -0
index c1608f64ea95..12e981e9e3d7 100644
@@ -4259,6 +4259,7 @@ static int ath11k_mac_op_start(struct ieee80211_hw *hw)
 						1, pdev->pdev_id);
 		if (ret) {
 			ath11k_err(ab, "failed to enable idle ps: %d\n", ret);
+			mutex_lock(&ar->conf_mutex);
 			goto err;
 		}
 	}
Signature · 1 line
2.29.2
#1 lore raw
Quoted 20 lines · Dan Carpenter

This error path leads to a Smatch warning:

     drivers/net/wireless/ath/ath11k/mac.c:4269 ath11k_mac_op_start()
     error: double unlocked '&ar->conf_mutex' (orig line 4251)

We're not holding the lock when we do the "goto err;" so it leads to a double unlock. We should hold the lock because the error path sets "ar->state" so the right fix is to take the lock before doing the goto.

Fixes c83c500b55b6 ("ath11k: enable idle power save mode")
Signed-off-by Dan Carpenter <[email protected]>
---
  drivers/net/wireless/ath/ath11k/mac.c | 1 +
  1 file changed, 1 insertion(+)
drivers/net/wireless/ath/ath11k/mac.c +0 -0
index c1608f64ea95..12e981e9e3d7 100644

drivers/net/wireless/ath/ath11k/mac.c static int ath11k_mac_op_start(struct ieee80211_hw *hw) · line 4265

Quoted 7 lines
drivers/net/wireless/ath/ath11k/mac.c +1 -0
@@ -4259,6 +4259,7 @@ static int ath11k_mac_op_start(struct ieee80211_hw *hw)
  						1, pdev->pdev_id);
  		if (ret) {
  			ath11k_err(ab, "failed to enable idle ps: %d\n", ret);
+			mutex_lock(&ar->conf_mutex);
  			goto err;
  		}
  	}

It seems moving idle_ps condition in between ath11k_wmi_pdev_lro_cfg() and mutex_unlock()
 is a better way in this case.

Thanks,
Peter

#2 lore raw
Quoted 16 lines · Peter Oh
Quoted 15 lines · Dan Carpenter

This error path leads to a Smatch warning:

     drivers/net/wireless/ath/ath11k/mac.c:4269 ath11k_mac_op_start()
     error: double unlocked '&ar->conf_mutex' (orig line 4251)

We're not holding the lock when we do the "goto err;" so it leads to a double unlock. We should hold the lock because the error path sets "ar->state" so the right fix is to take the lock before doing the goto.

Fixes c83c500b55b6 ("ath11k: enable idle power save mode")
Signed-off-by Dan Carpenter <[email protected]>
---
  drivers/net/wireless/ath/ath11k/mac.c | 1 +
  1 file changed, 1 insertion(+)
Quoted 16 lines

drivers/net/wireless/ath/ath11k/mac.c static int ath11k_mac_op_start(struct ieee80211_hw *hw) · line 4265

Quoted 12 lines
drivers/net/wireless/ath/ath11k/mac.c +1 -0
index c1608f64ea95..12e981e9e3d7 100644
@@ -4259,6 +4259,7 @@ static int ath11k_mac_op_start(struct ieee80211_hw *hw)
  						1, pdev->pdev_id);
  		if (ret) {
  			ath11k_err(ab, "failed to enable idle ps: %d\n", ret);
+			mutex_lock(&ar->conf_mutex);
  			goto err;
  		}
  	}

It seems moving idle_ps condition in between ath11k_wmi_pdev_lro_cfg() and mutex_unlock()
 is a better way in this case.

I agree. In Dan's approach I'm worried about the race when we unlock and lock again, the state can change in that case.

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches