[PATCH] mac80211: Fix scan RX processing oops

[PATCH] mac80211: Fix scan RX processing oops

[Jouni Malinen]

ieee80211_bss_info_update() can return NULL. Verify that this is not the
case before calling ieee802111_rx_bss_put() which would trigger an oops
in interrupt context in atomic_dec_and_lock().

Signed-off-by: Jouni Malinen <jouni.malinen@atheros.com>


Index: wireless-testing/net/mac80211/scan.c
===================================================================
--- wireless-testing.orig/net/mac80211/scan.c
+++ wireless-testing/net/mac80211/scan.c
@@ -388,7 +388,8 @@ ieee80211_scan_rx(struct ieee80211_sub_i
 	bss = ieee80211_bss_info_update(sdata->local, rx_status,
 					mgmt, skb->len, &elems,
 					freq, beacon);
-	ieee80211_rx_bss_put(sdata->local, bss);
+	if (bss)
+		ieee80211_rx_bss_put(sdata->local, bss);
 
 	dev_kfree_skb(skb);
 	return RX_QUEUED;

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH] mac80211: Fix scan RX processing oops

[Johannes Berg]

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

On Sat, 2008-10-11 at 03:29 +0300, Jouni Malinen wrote:
> ieee80211_bss_info_update() can return NULL. Verify that this is not the
> case before calling ieee802111_rx_bss_put() which would trigger an oops
> in interrupt context in atomic_dec_and_lock().
> 
> Signed-off-by: Jouni Malinen <jouni.malinen@atheros.com>

Acked-by: Johannes Berg <johannes@sipsolutions.net>

> 
> Index: wireless-testing/net/mac80211/scan.c
> ===================================================================
> --- wireless-testing.orig/net/mac80211/scan.c
> +++ wireless-testing/net/mac80211/scan.c
> @@ -388,7 +388,8 @@ ieee80211_scan_rx(struct ieee80211_sub_i
>  	bss = ieee80211_bss_info_update(sdata->local, rx_status,
>  					mgmt, skb->len, &elems,
>  					freq, beacon);
> -	ieee80211_rx_bss_put(sdata->local, bss);
> +	if (bss)
> +		ieee80211_rx_bss_put(sdata->local, bss);

I keep falling into that trap, maybe the put function should just handle
NULL instead...

johannes

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

Re: [PATCH] mac80211: Fix scan RX processing oops

[Jouni Malinen]

On Sat, Oct 11, 2008 at 02:38:06AM +0200, Johannes Berg wrote:
> > -	ieee80211_rx_bss_put(sdata->local, bss);
> > +	if (bss)
> > +		ieee80211_rx_bss_put(sdata->local, bss);
> 
> I keep falling into that trap, maybe the put function should just handle
> NULL instead...

I though about that for half a second or so ;-) and ended up doing this
instead after checking that other ieee80211_rx_bss_put() calls were only
passing in non-NULL values. Anyway, I would be fine with _put() being
able to handle NULL, too.


PS.

I don't know what exactly was triggering this oops (or well, what was
triggering ieee80211_bss_info_update() to return NULL to be more exact),
but it was happening very consistently in our office (but not anywhere
else I've been this week). It was kind of funny to see that oops at the
very moment when I was convincing people in a meeting that we can change
mac80211 and should do so if it is the best location for something and
makes it easier to implement something in a driver.. ;-)

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH] mac80211: Fix scan RX processing oops

[Johannes Berg]

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

On Sat, 2008-10-11 at 03:48 +0300, Jouni Malinen wrote:
> On Sat, Oct 11, 2008 at 02:38:06AM +0200, Johannes Berg wrote:
> > > -	ieee80211_rx_bss_put(sdata->local, bss);
> > > +	if (bss)
> > > +		ieee80211_rx_bss_put(sdata->local, bss);
> > 
> > I keep falling into that trap, maybe the put function should just handle
> > NULL instead...
> 
> I though about that for half a second or so ;-) and ended up doing this
> instead after checking that other ieee80211_rx_bss_put() calls were only
> passing in non-NULL values. Anyway, I would be fine with _put() being
> able to handle NULL, too.

I looked at it before and for some reason decided against it too. Let's
stick to this patch, I'll re-evaluate making it handle NULL.

> PS.
> 
> I don't know what exactly was triggering this oops (or well, what was
> triggering ieee80211_bss_info_update() to return NULL to be more exact),
> but it was happening very consistently in our office (but not anywhere
> else I've been this week).

Strange. You probably have a mesh network with bogus mesh config or mesh
ID IEs, I can't see the allocation fail consistently in your office ;)

> It was kind of funny to see that oops at the
> very moment when I was convincing people in a meeting that we can change
> mac80211 and should do so if it is the best location for something and
> makes it easier to implement something in a driver.. ;-)

Ouch, sorry :)

johannes

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

Re: [PATCH] mac80211: Fix scan RX processing oops

[Benoit PAPILLAULT]

Johannes Berg a =E9crit :
> On Sat, 2008-10-11 at 03:48 +0300, Jouni Malinen wrote:
>> On Sat, Oct 11, 2008 at 02:38:06AM +0200, Johannes Berg wrote:
>>>> -	ieee80211_rx_bss_put(sdata->local, bss);
>>>> +	if (bss)
>>>> +		ieee80211_rx_bss_put(sdata->local, bss);
>>> I keep falling into that trap, maybe the put function should just h=
andle
>>> NULL instead...
>> I though about that for half a second or so ;-) and ended up doing t=
his
>> instead after checking that other ieee80211_rx_bss_put() calls were =
only
>> passing in non-NULL values. Anyway, I would be fine with _put() bein=
g
>> able to handle NULL, too.
>=20
> I looked at it before and for some reason decided against it too. Let=
's
> stick to this patch, I'll re-evaluate making it handle NULL.
>=20
>> PS.
>>
>> I don't know what exactly was triggering this oops (or well, what wa=
s
>> triggering ieee80211_bss_info_update() to return NULL to be more exa=
ct),
>> but it was happening very consistently in our office (but not anywhe=
re
>> else I've been this week).
>=20
> Strange. You probably have a mesh network with bogus mesh config or m=
esh
> ID IEs, I can't see the allocation fail consistently in your office ;=
)
>=20
>> It was kind of funny to see that oops at the
>> very moment when I was convincing people in a meeting that we can ch=
ange
>> mac80211 and should do so if it is the best location for something a=
nd
>> makes it easier to implement something in a driver.. ;-)
>=20
> Ouch, sorry :)
>=20
> johannes

Acked-by: Benoit Papillault <benoit.papillault@free.fr>

BTW, This is indeed Mesh Config IE received with incorrect mesh_id_len
(8 bytes instead of 19 bytes as expected). I really think the current
patch is the correct one.

Regards,
Benoit
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html