[PATCH] rt2x00: Fix NULL pointer error in adhoc/master mode

[PATCH] rt2x00: Fix NULL pointer error in adhoc/master mode

[Ivo van Doorn]

As soon as an interface is enabled, and that interface is in adhoc or master mode,
the device will start raising beacondone interrupts. But before the first interrupt is
raised, mac80211 will probably not have send any beacons to the device yet, which
results in a NULL pointer error when the skb is being freed.

Note that the "raise beacondone interrupts without a beacon" is also a bug,
and will be addressed later. The more important bug however is preventing
the NULL pointer failt itself, since there might be other conditions that could trigger
it as well.

Signed-off-by: Ivo van Doorn <IvDoorn@gmail.com>
---
diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c
index 8e86611..a9aa0d5 100644
--- a/drivers/net/wireless/rt2x00/rt2x00queue.c
+++ b/drivers/net/wireless/rt2x00/rt2x00queue.c
@@ -107,6 +107,9 @@ void rt2x00queue_unmap_skb(struct rt2x00_dev *rt2x00dev, struct sk_buff *skb)
 
 void rt2x00queue_free_skb(struct rt2x00_dev *rt2x00dev, struct sk_buff *skb)
 {
+	if (!skb)
+		return;
+
 	rt2x00queue_unmap_skb(rt2x00dev, skb);
 	dev_kfree_skb_any(skb);
 }

Re: [Rt2400-devel] [PATCH] rt2x00: Fix NULL pointer error in adhoc/master mode

[Bryan Batten]

Ivo van Doorn wrote:
...
> Note that the "raise beacondone interrupts without a beacon" is
> also a bug, and will be addressed later.
...

FWIW, I've never been able to figure out a way to shut it off in the 
legacy drivers. Unless there's some mechanism I've missed, I would 
consider this a bug, but its a hardware bug because - in the rt2500 
driver at least - even disabling it in the interrupt mask register has 
no effect.

I think it would be more accurately termed a target beacon 
transmission time interrupt. The rt61 driver's ISR simply does not 
test for the condition. The rt2400 and rt2500 drivers do nothing if 
not in adhoc mode. If in adhoc mode, they both assume a beacon frame 
is ready to go and send it. This relies on a canned frame that is set 
up during initialization.

Hope this is helpful,
-- 
Bryan
-
"Beyond the point of no return, there's no going back."

Narration to "Mars Rising" on the Science Channel

Re: [Rt2400-devel] [PATCH] rt2x00: Fix NULL pointer error in adhoc/master mode

[Ivo van Doorn]

On Saturday 05 July 2008, Bryan Batten wrote:
> Ivo van Doorn wrote:
> ...
> > Note that the "raise beacondone interrupts without a beacon" is
> > also a bug, and will be addressed later.
> ...
> 
> FWIW, I've never been able to figure out a way to shut it off in the 
> legacy drivers. Unless there's some mechanism I've missed, I would 
> consider this a bug, but its a hardware bug because - in the rt2500 
> driver at least - even disabling it in the interrupt mask register has 
> no effect.

That is interesting, have you tried controlling CSR14 and more specifically
the TBCN or BeaconGen fields?

> I think it would be more accurately termed a target beacon 
> transmission time interrupt. The rt61 driver's ISR simply does not 
> test for the condition. The rt2400 and rt2500 drivers do nothing if 
> not in adhoc mode. If in adhoc mode, they both assume a beacon frame 
> is ready to go and send it. This relies on a canned frame that is set 
> up during initialization.

Hmm, in that case I have to fix something in the beacondone handler
to prevent it requesting a beacon when mac80211 hasn't supplied anything
yet..

Thanks for the notice. :)

Ivo

Re: [Rt2400-devel] [PATCH] rt2x00: Fix NULL pointer error in adhoc/master mode

[Johannes Berg]

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


> Hmm, in that case I have to fix something in the beacondone handler
> to prevent it requesting a beacon when mac80211 hasn't supplied anything
> yet..

I was working in this area... Can you take a quick look at this patch
and let me know what you think?
http://johannes.sipsolutions.net/patches/kernel/all/2008-07-05-18%3a42/mac80211-beacon-revamp.patch

johannes

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

Re: [Rt2400-devel] [PATCH] rt2x00: Fix NULL pointer error in adhoc/master mode

[Ivo van Doorn]

On Saturday 05 July 2008, Johannes Berg wrote:
> 
> > Hmm, in that case I have to fix something in the beacondone handler
> > to prevent it requesting a beacon when mac80211 hasn't supplied anything
> > yet..
> 
> I was working in this area... Can you take a quick look at this patch
> and let me know what you think?
> http://johannes.sipsolutions.net/patches/kernel/all/2008-07-05-18%3a42/mac80211-beacon-revamp.patch

Patch looks good to me, it wouldn't change anything about the above since t2x00 would still
call ieee80211_beacon_get() when the beacon wasn't provided by mac80211 yet.

But the flags that mark what has changed is definately a good idea,
that will prevent some redundant register writes. :)

Ivo

Re: [Rt2400-devel] [PATCH] rt2x00: Fix NULL pointer error in adhoc/master mode

[Johannes Berg]

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

On Sat, 2008-07-05 at 21:26 +0200, Ivo van Doorn wrote:
> On Saturday 05 July 2008, Johannes Berg wrote:
> > 
> > > Hmm, in that case I have to fix something in the beacondone handler
> > > to prevent it requesting a beacon when mac80211 hasn't supplied anything
> > > yet..
> > 
> > I was working in this area... Can you take a quick look at this patch
> > and let me know what you think?
> > http://johannes.sipsolutions.net/patches/kernel/all/2008-07-05-18%3a42/mac80211-beacon-revamp.patch
> 
> Patch looks good to me, it wouldn't change anything about the above since t2x00 would still
> call ieee80211_beacon_get() when the beacon wasn't provided by mac80211 yet.

Oh, of course, I just was writing on the patch and read your mail and
replied instead of sending you another one :)

One thing I'm worried about is the sequence numbers, but that bug is
already there... IIRC you needed software sequence numbers for your
drivers, but what about the beacons? I haven't _changed_ anything, but
IBSS beacons seem to not get a software sequence number.

> But the flags that mark what has changed is definately a good idea,
> that will prevent some redundant register writes. :)

:)
Only once you use the flags. My patch is intentionally minimal.

johannes

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

Re: [Rt2400-devel] [PATCH] rt2x00: Fix NULL pointer error in adhoc/master mode

[Ivo van Doorn]

On Saturday 05 July 2008, Johannes Berg wrote:
> On Sat, 2008-07-05 at 21:26 +0200, Ivo van Doorn wrote:
> > On Saturday 05 July 2008, Johannes Berg wrote:
> > > 
> > > > Hmm, in that case I have to fix something in the beacondone handler
> > > > to prevent it requesting a beacon when mac80211 hasn't supplied anything
> > > > yet..
> > > 
> > > I was working in this area... Can you take a quick look at this patch
> > > and let me know what you think?
> > > http://johannes.sipsolutions.net/patches/kernel/all/2008-07-05-18%3a42/mac80211-beacon-revamp.patch
> > 
> > Patch looks good to me, it wouldn't change anything about the above since t2x00 would still
> > call ieee80211_beacon_get() when the beacon wasn't provided by mac80211 yet.
> 
> Oh, of course, I just was writing on the patch and read your mail and
> replied instead of sending you another one :)
> 
> One thing I'm worried about is the sequence numbers, but that bug is
> already there... IIRC you needed software sequence numbers for your
> drivers, but what about the beacons? I haven't _changed_ anything, but
> IBSS beacons seem to not get a software sequence number.

That problem is only in rt2400pci and rt2500pci who raise the beacon interrupt,
all other drivers have the template and auto sequence numbers.
But no sequence numbers for the beacons, I think that will definately cause
problems for those 2 drivers.

But the function ieee80211_include_sequence() is being called from within the
ieee80211_beacon_get() function. Only for the Mesh it doesn't seem to be called.

> > But the flags that mark what has changed is definately a good idea,
> > that will prevent some redundant register writes. :)
> 
> :)
> Only once you use the flags. My patch is intentionally minimal.

Not a problem, with that patch some other beaconing in the rt2x00 code
will change anyway. :)

Ivo

Re: [Rt2400-devel] [PATCH] rt2x00: Fix NULL pointer error in adhoc/master mode

[Johannes Berg]

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


> That problem is only in rt2400pci and rt2500pci who raise the beacon interrupt,
> all other drivers have the template and auto sequence numbers.

Ok.

> But no sequence numbers for the beacons, I think that will definately cause
> problems for those 2 drivers.
> 
> But the function ieee80211_include_sequence() is being called from within the
> ieee80211_beacon_get() function. Only for the Mesh it doesn't seem to be called.

Well it's not called from where the IBSS beacon is set either, and the
IBSS beacon isn't generated every time anyway. So I'm confused now. Do
rt2{4,5}00pci not support IBSS?

johannes

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

Re: [Rt2400-devel] [PATCH] rt2x00: Fix NULL pointer error in adhoc/master mode

[Ivo van Doorn]

On Saturday 05 July 2008, Johannes Berg wrote:
> 
> > That problem is only in rt2400pci and rt2500pci who raise the beacon interrupt,
> > all other drivers have the template and auto sequence numbers.
> 
> Ok.
> 
> > But no sequence numbers for the beacons, I think that will definately cause
> > problems for those 2 drivers.
> > 
> > But the function ieee80211_include_sequence() is being called from within the
> > ieee80211_beacon_get() function. Only for the Mesh it doesn't seem to be called.
> 
> Well it's not called from where the IBSS beacon is set either, and the
> IBSS beacon isn't generated every time anyway. So I'm confused now. Do
> rt2{4,5}00pci not support IBSS?

Well it does claim support for it, but it has been enabled only since 2.6.26, so the number
of people who have tested it is very limited. And so far little to no success stories...

The main trick in rt2x00lib is that it doesn't care if it is an IBSS or AP beacon, it will always
use the update_beacon() callback function to write the beacon to the hardware. It doesn't
need to differentiate between the beacon types (as far as I know, master mode isn't implemented
in the legacy drivers, and the only claim about if it is possible cones from Asus who owns the
closed source driver with AP functionality).

So for the Ralink hardware it just needs a completely valid beacon which it can transmit,
only for rt2400pci/rt2500pci it requires that the sequence number is set due to lacking
hardware sequence counting.
After that those 2 will raise an interrupt every beacon interval to request a new beacon,
which rt2x00lib fulfills by calling ieee80211_beacon_get().

Ivo

Re: [Rt2400-devel] [PATCH] rt2x00: Fix NULL pointer error in adhoc/master mode

[Bryan Batten]

Ivo van Doorn wrote:
...
> That is interesting, have you tried controlling CSR14 and more
> specifically the TBCN or BeaconGen fields?

No. It turns out that if I set the RT2500's CSR14 to zero (the default 
power on state according to the data sheet) at driver initialization, 
there are no beacon interrupts until enabled in AsicEnableIbssSync(). 
I deem this a bug, the fix for which in now in CVS.

Thanks,
-- 
Bryan
-
"Beyond the point of no return, there's no going back."

Narration to "Mars Rising" on the Science Channel

Re: [Rt2400-devel] [PATCH] rt2x00: Fix NULL pointer error in adhoc/master mode

[Ivo van Doorn]

On Saturday 05 July 2008, Bryan Batten wrote:
> Ivo van Doorn wrote:
> ...
> > That is interesting, have you tried controlling CSR14 and more
> > specifically the TBCN or BeaconGen fields?
> 
> No. It turns out that if I set the RT2500's CSR14 to zero (the default 
> power on state according to the data sheet) at driver initialization, 
> there are no beacon interrupts until enabled in AsicEnableIbssSync(). 
> I deem this a bug, the fix for which in now in CVS.

Heh, that will bring down the number of raised interrupts in the legacy driver. :)
You might want to add it to rt2400 as well, I expect the same behavior for that. ;)

I'll put the initialization into rt2x00 as well, since it doesn't set it to 0 by default either.

Ivo

Re: [Rt2400-devel] [PATCH] rt2x00: Fix NULL pointer error in adhoc/master mode

[Bryan Batten]

Ivo van Doorn wrote:
> On Saturday 05 July 2008, Bryan Batten wrote:
>> Ivo van Doorn wrote:
...
>>> Note that the "raise beacondone interrupts without a beacon" is
>>>  also a bug, and will be addressed later.
...
>> FWIW, I've never been able to figure out a way to shut it off in 
>> the legacy drivers. Unless there's some mechanism I've missed, I 
>> would consider this a bug, but its a hardware bug because - in 
>> the rt2500 driver at least - even disabling it in the interrupt 
>> mask register has no effect.

Looking thru the RT2400 and RT2500 datasheets, I notice that on pp. 12
of the 2400 datasheet, CSR8 bits 0 and 1 are described. On the same
page of the RT2500 datasheet, CSR8 bits 0 and 1 are not described. If
that is not a typo, that means to me that on the RT2400 you can
mask off beacon timer and wakeup timer interrupts with CSR8, but on
the RT2500 you cannot.

Would it be possible to verify with Ralink that this is not a typo in
the RT2500 datasheet?

Thanks,
-- 
Bryan
-
"Beyond the point of no return, there's no going back."

Narration to "Mars Rising" on the Science Channel

Re: [Rt2400-devel] [PATCH] rt2x00: Fix NULL pointer error in adhoc/master mode

[Ivo van Doorn]

On Wednesday 09 July 2008, Bryan Batten wrote:
> Ivo van Doorn wrote:
> > On Saturday 05 July 2008, Bryan Batten wrote:
> >> Ivo van Doorn wrote:
> ...
> >>> Note that the "raise beacondone interrupts without a beacon" is
> >>>  also a bug, and will be addressed later.
> ...
> >> FWIW, I've never been able to figure out a way to shut it off in 
> >> the legacy drivers. Unless there's some mechanism I've missed, I 
> >> would consider this a bug, but its a hardware bug because - in 
> >> the rt2500 driver at least - even disabling it in the interrupt 
> >> mask register has no effect.
> 
> Looking thru the RT2400 and RT2500 datasheets, I notice that on pp. 12
> of the 2400 datasheet, CSR8 bits 0 and 1 are described. On the same
> page of the RT2500 datasheet, CSR8 bits 0 and 1 are not described. If
> that is not a typo, that means to me that on the RT2400 you can
> mask off beacon timer and wakeup timer interrupts with CSR8, but on
> the RT2500 you cannot.

Yes, but if you look again you see that the legacy driver does define it,
and even includes the description of bits 9 -> 19
And the legacy driver source code has always been more reliable then
the datasheet, so I would put my money on the code. ;)

> Would it be possible to verify with Ralink that this is not a typo in
> the RT2500 datasheet?

I'll see what I can find out.

Ivo

Re: [Rt2400-devel] [PATCH] rt2x00: Fix NULL pointer error in adhoc/master mode

[Bryan Batten]

Ivo van Doorn wrote:
...
> Yes, but if you look again you see that the legacy driver does
> define it, and even includes the description of bits 9 -> 19 And
> the legacy driver source code has always been more reliable then 
> the datasheet, so I would put my money on the code. ;)

I know. But I suspect that in this case, the datasheet just *may* be 
correct.

>> Would it be possible to verify with Ralink that this is not a
>> typo in the RT2500 datasheet?
> 
> I'll see what I can find out.
> 

Thanks,
-- 
Bryan
-
"Beyond the point of no return, there's no going back."

Narration to "Mars Rising" on the Science Channel