[PATCH] ath10k: unregister spectral before mac

[PATCH] ath10k: unregister spectral before mac

[Simon Wunderlich]

If spectral is unregistered after mac80211, the relayfs file has already
been removed recursively by mac/cfg80211, and spectral tries to remove
the file once more, thus leading to double free problems. Better clean
up spectral before to avoid that problem.

Reported-by: Kalle Valo <kvalo@qca.qualcomm.com>
Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
---
 drivers/net/wireless/ath/ath10k/core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index d3474b4..ba2e87a 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1043,6 +1043,12 @@ void ath10k_core_unregister(struct ath10k *ar)
 	if (!test_bit(ATH10K_FLAG_CORE_REGISTERED, &ar->dev_flags))
 		return;
 
+	/* Stop spectral before unregistering from mac80211 to remove the
+	 * relayfs debugfs file cleanly. Otherwise the parent debugfs tree
+	 * would be already be free'd recursively, leading to a double free.
+	 */
+	ath10k_spectral_destroy(ar);
+
 	/* We must unregister from mac80211 before we stop HTC and HIF.
 	 * Otherwise we will fail to submit commands to FW and mac80211 will be
 	 * unhappy about callback failures. */
@@ -1050,8 +1056,6 @@ void ath10k_core_unregister(struct ath10k *ar)
 
 	ath10k_core_free_firmware_files(ar);
 
-	ath10k_spectral_destroy(ar);
-
 	ath10k_debug_destroy(ar);
 }
 EXPORT_SYMBOL(ath10k_core_unregister);
-- 
2.1.0.rc1


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: unregister spectral before mac

[Kalle Valo]

Simon Wunderlich <sw@simonwunderlich.de> writes:

> If spectral is unregistered after mac80211, the relayfs file has already
> been removed recursively by mac/cfg80211, and spectral tries to remove
> the file once more, thus leading to double free problems. Better clean
> up spectral before to avoid that problem.
>
> Reported-by: Kalle Valo <kvalo@qca.qualcomm.com>
> Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>

Thanks for checking this. But I'm just wondering why even bother to call
relay_close() in the first place if mac80211 recursively removes
everything anyway? We don't remove any of the debugfs files anyway.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: Re: [PATCH] ath10k: unregister spectral before mac

[Simon Wunderlich]

On Thursday 14 August 2014 13:12:13 Kalle Valo wrote:
> Simon Wunderlich <sw@simonwunderlich.de> writes:
> > If spectral is unregistered after mac80211, the relayfs file has already
> > been removed recursively by mac/cfg80211, and spectral tries to remove
> > the file once more, thus leading to double free problems. Better clean
> > up spectral before to avoid that problem.
> > 
> > Reported-by: Kalle Valo <kvalo@qca.qualcomm.com>
> > Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
> 
> Thanks for checking this. But I'm just wondering why even bother to call
> relay_close() in the first place if mac80211 recursively removes
> everything anyway? We don't remove any of the debugfs files anyway.

We need to relay_close() in any case because this function also causes to 
clean up internal buffers of relayfs.

An alternative to this patch could be to instead having 
remove_buf_file_handler() call debugfs_remove() just do nothing and relay on 
the upper layers to (recursively) remove that file - then, the relayfs stuff 
still can get cleaned up. But that's also not very elegant either. If you 
prefer that I can prepare a patch though ....

Cheers,
     Simon

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: unregister spectral before mac

[Kalle Valo]

Simon Wunderlich <sw@simonwunderlich.de> writes:

> On Thursday 14 August 2014 13:12:13 Kalle Valo wrote:
>> Simon Wunderlich <sw@simonwunderlich.de> writes:
>> > If spectral is unregistered after mac80211, the relayfs file has already
>> > been removed recursively by mac/cfg80211, and spectral tries to remove
>> > the file once more, thus leading to double free problems. Better clean
>> > up spectral before to avoid that problem.
>> > 
>> > Reported-by: Kalle Valo <kvalo@qca.qualcomm.com>
>> > Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
>> 
>> Thanks for checking this. But I'm just wondering why even bother to call
>> relay_close() in the first place if mac80211 recursively removes
>> everything anyway? We don't remove any of the debugfs files anyway.
>
> We need to relay_close() in any case because this function also causes to 
> clean up internal buffers of relayfs.
>
> An alternative to this patch could be to instead having 
> remove_buf_file_handler() call debugfs_remove() just do nothing and relay on 
> the upper layers to (recursively) remove that file - then, the relayfs stuff 
> still can get cleaned up. But that's also not very elegant either. If you 
> prefer that I can prepare a patch though ....

Yeah, that's not really any better. I'll apply your original patch.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: unregister spectral before mac

[Kalle Valo]

Simon Wunderlich <sw@simonwunderlich.de> writes:

> If spectral is unregistered after mac80211, the relayfs file has already
> been removed recursively by mac/cfg80211, and spectral tries to remove
> the file once more, thus leading to double free problems. Better clean
> up spectral before to avoid that problem.
>
> Reported-by: Kalle Valo <kvalo@qca.qualcomm.com>
> Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>

Thanks, applied.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k