diff mbox

[1/7] mac80211: fix possible memory leak on AP CSA failure

Message ID 1390227670-19030-2-git-send-email-michal.kazior@tieto.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Michal Kazior Jan. 20, 2014, 2:21 p.m. UTC
If CSA for AP interface failed and the interface
was not stopped afterwards another CSA request
would leak sdata->u.ap.next_beacon.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 net/mac80211/cfg.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

Comments

Johannes Berg Jan. 21, 2014, 2:55 p.m. UTC | #1
On Mon, 2014-01-20 at 15:21 +0100, Michal Kazior wrote:
> If CSA for AP interface failed and the interface
> was not stopped afterwards another CSA request
> would leak sdata->u.ap.next_beacon.

>  void ieee80211_csa_finish(struct ieee80211_vif *vif)
>  {
>  	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
> @@ -3019,15 +3034,9 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
>  	sdata->vif.csa_active = false;
>  	switch (sdata->vif.type) {
>  	case NL80211_IFTYPE_AP:
> -		err = ieee80211_assign_beacon(sdata, sdata->u.ap.next_beacon);
> +		err = ieee80211_ap_finish_csa(sdata);
>  		if (err < 0)
>  			return;
> -
> -		changed |= err;

This looks a bit like somebody had intended to batch the
ieee80211_bss_info_change_notify() calls, which would probably be a good
thing. You're breaking them apart even further - maybe we should
actually batch them instead by moving ieee80211_bss_info_change_notify()
after the switch()?

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Kazior Jan. 22, 2014, 6:54 a.m. UTC | #2
On 21 January 2014 15:55, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2014-01-20 at 15:21 +0100, Michal Kazior wrote:
>> If CSA for AP interface failed and the interface
>> was not stopped afterwards another CSA request
>> would leak sdata->u.ap.next_beacon.
>
>>  void ieee80211_csa_finish(struct ieee80211_vif *vif)
>>  {
>>       struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
>> @@ -3019,15 +3034,9 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
>>       sdata->vif.csa_active = false;
>>       switch (sdata->vif.type) {
>>       case NL80211_IFTYPE_AP:
>> -             err = ieee80211_assign_beacon(sdata, sdata->u.ap.next_beacon);
>> +             err = ieee80211_ap_finish_csa(sdata);
>>               if (err < 0)
>>                       return;
>> -
>> -             changed |= err;
>
> This looks a bit like somebody had intended to batch the
> ieee80211_bss_info_change_notify() calls, which would probably be a good
> thing. You're breaking them apart even further - maybe we should
> actually batch them instead by moving ieee80211_bss_info_change_notify()
> after the switch()?

Sounds good. I'll fix it.


Micha?
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 65dac7f..62bf6c4 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2988,6 +2988,21 @@  cfg80211_beacon_dup(struct cfg80211_beacon_data *beacon)
 	return new_beacon;
 }
 
+static int ieee80211_ap_finish_csa(struct ieee80211_sub_if_data *sdata)
+{
+	int err;
+
+	err = ieee80211_assign_beacon(sdata, sdata->u.ap.next_beacon);
+	kfree(sdata->u.ap.next_beacon);
+	sdata->u.ap.next_beacon = NULL;
+
+	if (err < 0)
+		return err;
+
+	ieee80211_bss_info_change_notify(sdata, err);
+	return 0;
+}
+
 void ieee80211_csa_finish(struct ieee80211_vif *vif)
 {
 	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
@@ -3019,15 +3034,9 @@  static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 	sdata->vif.csa_active = false;
 	switch (sdata->vif.type) {
 	case NL80211_IFTYPE_AP:
-		err = ieee80211_assign_beacon(sdata, sdata->u.ap.next_beacon);
+		err = ieee80211_ap_finish_csa(sdata);
 		if (err < 0)
 			return;
-
-		changed |= err;
-		kfree(sdata->u.ap.next_beacon);
-		sdata->u.ap.next_beacon = NULL;
-
-		ieee80211_bss_info_change_notify(sdata, err);
 		break;
 	case NL80211_IFTYPE_ADHOC:
 		ieee80211_ibss_finish_csa(sdata);