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 <[email protected]>
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
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 <[email protected]>
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
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
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
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 <[email protected]>
Acked-by: Johannes Berg <[email protected]>
>
> 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