[PATCH ] mac80211: check return value of dev_alloc_skb() in ieee80211_sta_join_ibss().

[PATCH ] mac80211: check return value of dev_alloc_skb() in ieee80211_sta_join_ibss().

[Rami Rosen]

[-- Attachment #1: Type: text/plain, Size: 201 bytes --]

This patch adds a check on the return value of dev_alloc_skb() in
ieee80211_sta_join_ibss()
in net/mac80211/mlme.c.

The patch is to wireless-next-2.6.


Signed-off-by: Rami Rosen <ramirose@gmail.com>

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 3765 bytes --]

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 49f86fa..2dd53ef 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -1414,62 +1414,67 @@ static int ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
 
 	/* Build IBSS probe response */
 	skb = dev_alloc_skb(local->hw.extra_tx_headroom + 400);
-	if (skb) {
-		skb_reserve(skb, local->hw.extra_tx_headroom);
+	if (!skb) {
+		printk(KERN_DEBUG "%s: failed to allocate buffer for probe "
+		       "response\n", sdata->dev->name);
+		return -1;
+	}
 
-		mgmt = (struct ieee80211_mgmt *)
-			skb_put(skb, 24 + sizeof(mgmt->u.beacon));
-		memset(mgmt, 0, 24 + sizeof(mgmt->u.beacon));
-		mgmt->frame_control = cpu_to_le16(IEEE80211_FTYPE_MGMT |
-						  IEEE80211_STYPE_PROBE_RESP);
-		memset(mgmt->da, 0xff, ETH_ALEN);
-		memcpy(mgmt->sa, sdata->dev->dev_addr, ETH_ALEN);
-		memcpy(mgmt->bssid, ifsta->bssid, ETH_ALEN);
-		mgmt->u.beacon.beacon_int =
-			cpu_to_le16(local->hw.conf.beacon_int);
-		mgmt->u.beacon.timestamp = cpu_to_le64(bss->timestamp);
-		mgmt->u.beacon.capab_info = cpu_to_le16(bss->capability);
-
-		pos = skb_put(skb, 2 + ifsta->ssid_len);
-		*pos++ = WLAN_EID_SSID;
-		*pos++ = ifsta->ssid_len;
-		memcpy(pos, ifsta->ssid, ifsta->ssid_len);
-
-		rates = bss->supp_rates_len;
-		if (rates > 8)
-			rates = 8;
-		pos = skb_put(skb, 2 + rates);
-		*pos++ = WLAN_EID_SUPP_RATES;
-		*pos++ = rates;
-		memcpy(pos, bss->supp_rates, rates);
+	skb_reserve(skb, local->hw.extra_tx_headroom);
 
-		if (bss->band == IEEE80211_BAND_2GHZ) {
-			pos = skb_put(skb, 2 + 1);
-			*pos++ = WLAN_EID_DS_PARAMS;
-			*pos++ = 1;
-			*pos++ = ieee80211_frequency_to_channel(bss->freq);
-		}
+	mgmt = (struct ieee80211_mgmt *)
+		skb_put(skb, 24 + sizeof(mgmt->u.beacon));
+	memset(mgmt, 0, 24 + sizeof(mgmt->u.beacon));
+	mgmt->frame_control = cpu_to_le16(IEEE80211_FTYPE_MGMT |
+						IEEE80211_STYPE_PROBE_RESP);
+	memset(mgmt->da, 0xff, ETH_ALEN);
+	memcpy(mgmt->sa, sdata->dev->dev_addr, ETH_ALEN);
+	memcpy(mgmt->bssid, ifsta->bssid, ETH_ALEN);
+	mgmt->u.beacon.beacon_int =
+		cpu_to_le16(local->hw.conf.beacon_int);
+	mgmt->u.beacon.timestamp = cpu_to_le64(bss->timestamp);
+	mgmt->u.beacon.capab_info = cpu_to_le16(bss->capability);
 
-		pos = skb_put(skb, 2 + 2);
-		*pos++ = WLAN_EID_IBSS_PARAMS;
-		*pos++ = 2;
-		/* FIX: set ATIM window based on scan results */
-		*pos++ = 0;
-		*pos++ = 0;
+	pos = skb_put(skb, 2 + ifsta->ssid_len);
+	*pos++ = WLAN_EID_SSID;
+	*pos++ = ifsta->ssid_len;
+	memcpy(pos, ifsta->ssid, ifsta->ssid_len);
 
-		if (bss->supp_rates_len > 8) {
-			rates = bss->supp_rates_len - 8;
-			pos = skb_put(skb, 2 + rates);
-			*pos++ = WLAN_EID_EXT_SUPP_RATES;
-			*pos++ = rates;
-			memcpy(pos, &bss->supp_rates[8], rates);
-		}
+	rates = bss->supp_rates_len;
+	if (rates > 8)
+		rates = 8;
+	pos = skb_put(skb, 2 + rates);
+	*pos++ = WLAN_EID_SUPP_RATES;
+	*pos++ = rates;
+	memcpy(pos, bss->supp_rates, rates);
 
-		ifsta->probe_resp = skb;
+	if (bss->band == IEEE80211_BAND_2GHZ) {
+		pos = skb_put(skb, 2 + 1);
+		*pos++ = WLAN_EID_DS_PARAMS;
+		*pos++ = 1;
+		*pos++ = ieee80211_frequency_to_channel(bss->freq);
+	}
+
+	pos = skb_put(skb, 2 + 2);
+	*pos++ = WLAN_EID_IBSS_PARAMS;
+	*pos++ = 2;
+	/* FIX: set ATIM window based on scan results */
+	*pos++ = 0;
+	*pos++ = 0;
 
-		ieee80211_if_config(sdata, IEEE80211_IFCC_BEACON);
+	if (bss->supp_rates_len > 8) {
+		rates = bss->supp_rates_len - 8;
+		pos = skb_put(skb, 2 + rates);
+		*pos++ = WLAN_EID_EXT_SUPP_RATES;
+		*pos++ = rates;
+		memcpy(pos, &bss->supp_rates[8], rates);
 	}
 
+	ifsta->probe_resp = skb;
+
+	ieee80211_if_config(sdata, IEEE80211_IFCC_BEACON);
+ 
+
 	rates = 0;
 	sband = local->hw.wiphy->bands[local->hw.conf.channel->band];
 	for (i = 0; i < bss->supp_rates_len; i++) {

Re: [PATCH ] mac80211: check return value of dev_alloc_skb() in ieee80211_sta_join_ibss().

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 316 bytes --]

On Wed, 2008-10-22 at 09:00 +0200, Rami Rosen wrote:
> This patch adds a check on the return value of dev_alloc_skb() in
> ieee80211_sta_join_ibss()
> in net/mac80211/mlme.c.

What's wrong with the current code?


> +               return -1;

don't return -1, return meaningful error values.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH ] mac80211: check return value of dev_alloc_skb() in ieee80211_sta_join_ibss().

[Rami Rosen]

Hello,

Do we want to continue as usual if dev_alloc_skb() in theis method fails?

Regards,
Rami

On Wed, Oct 22, 2008 at 9:36 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2008-10-22 at 09:00 +0200, Rami Rosen wrote:
>> This patch adds a check on the return value of dev_alloc_skb() in
>> ieee80211_sta_join_ibss()
>> in net/mac80211/mlme.c.
>
> What's wrong with the current code?
>
>
>> +               return -1;
>
> don't return -1, return meaningful error values.
>
> johannes
>

Re: [PATCH ] mac80211: check return value of dev_alloc_skb() in ieee80211_sta_join_ibss().

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 306 bytes --]

On Wed, 2008-10-22 at 09:42 +0200, Rami Rosen wrote:
> Hello,
> 
> Do we want to continue as usual if dev_alloc_skb() in theis method fails?

well if anything then we shouldn't do half the stuff and then abort, so
the allocation/check should be moved to the beginning of the function...

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH ] mac80211: check return value of dev_alloc_skb() in ieee80211_sta_join_ibss().

[Jouni Malinen]

On Wed, Oct 22, 2008 at 09:42:37AM +0200, Rami Rosen wrote:

> Do we want to continue as usual if dev_alloc_skb() in theis method fails?

I think we should continue and as such, I would rather not see the patch
that proposes to abort here go in. The allocated skb is not used at this
point anyway; it is only used as an optimization to prepare a ProbeRsp
frame for possible future use. This could be done at the time when
ProbeReq is received (and we sent the last Beacon), i.e., only when
needed. Or if the optimization of generating this only once is
considered desirable, that place could try to allocate a new skb if the
one here failed (or do it on first need and cache the result).

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH ] mac80211: check return value of dev_alloc_skb() in ieee80211_sta_join_ibss().

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1053 bytes --]

On Wed, 2008-10-22 at 11:11 +0300, Jouni Malinen wrote:
> On Wed, Oct 22, 2008 at 09:42:37AM +0200, Rami Rosen wrote:
> 
> > Do we want to continue as usual if dev_alloc_skb() in theis method fails?
> 
> I think we should continue and as such, I would rather not see the patch
> that proposes to abort here go in. The allocated skb is not used at this
> point anyway; it is only used as an optimization to prepare a ProbeRsp
> frame for possible future use. This could be done at the time when
> ProbeReq is received (and we sent the last Beacon), i.e., only when
> needed. Or if the optimization of generating this only once is
> considered desirable, that place could try to allocate a new skb if the
> one here failed (or do it on first need and cache the result).

Good point. We currently don't try again, but we could put this into a
new function and call it from when we need it. Then again, we'll need it
right away, the driver will probably call _get_beacon from
ieee80211_if_config(sdata, IEEE80211_IFCC_BEACON);

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH ] mac80211: check return value of dev_alloc_skb() in ieee80211_sta_join_ibss().

[Jouni Malinen]

On Wed, Oct 22, 2008 at 10:17:33AM +0200, Johannes Berg wrote:

> Good point. We currently don't try again, but we could put this into a
> new function and call it from when we need it. Then again, we'll need it
> right away, the driver will probably call _get_beacon from
> ieee80211_if_config(sdata, IEEE80211_IFCC_BEACON);

Yes.. I forgot about that and only looked at probe_resp references
inside mlme.c. With that in mind, it probably is cleaner to make
ieee80211_sta_join_ibss() fail if it cannot allocate an skb for
ProbeResp. If we are going to be doing some cleanup here, I would
suggest moving the proberesp building into a helper function and just do
skb = ieee80211_sta_build_probe_resp(...); if (!skb) return -ENOMEM; in
ieee80211_sta_join_ibss().

-- 
Jouni Malinen                                            PGP id EFC895FA