Re: [panic] Race in IEEE802.11 layer towards device drivers

On Thursday 08 July 2010 05:12:37 Andrew Thompson wrote:
> On 8 July 2010 07:13, Hans Petter Selasky <hselasky_at_c2i.net> wrote:
> > Hi,
> >
> > When supplying wpa_supplicant.conf with incorrect passwords, but a valid
> > SSID, I have seen kernel panics several times when using USB based WLAN
> > dongles. When only supplying a valid password, no panic has been seen.
> >
> > How to reproduce:
> >
> > 1) configure invalid password
> > 2) wpa_cli: reconfigure
> > 3) configure valid password
> > 4) wpa_cli: reconfigure
> > 5) goto 1
> >
> > The USB commands which are executed inside the newstate callback usually
> > take very little time, but still not as little time as PCI read/writes.
> > I've forced slower operation in the newstate callback, and can reproduce
> > warning printouts from the IEEE802.11 layer in FreeBSD. Try to apply the
> > following patch to your USB code:
> >
> > http://p4web.freebsd.org/_at__at_180604?ac=10
> >
> > In my opinion the deferring of all states to a single task is wrong.
> > There should be at least one task per possible state, and the queuing
> > mechanism should follow the last-queued is last executed rule. This is
> > not the case with the task-queue mechanism in the kernel.
> 

Hi,

> You dont say why it should be this way, do you have an example of a
> problem this fixes?

Right.

The problem the way I see it, is that during execution of the newstate 
callback a new IEE802.11 state event comes along and alters the state of 
variables which we might refer.

If you look in this function, "ieee80211_new_state_locked()" you see that it 
is checking for:

if (vap->iv_flags_ext & IEEE80211_FEXT_STATEWAIT) {

But it still continues setting the new state, even though that flag is set. 

Another issue I see, is that the vap->iv_nstate and vap->iv_state variables 
are cached. Then vap->iv_newstate() is called, which can sleep. After that we 
are we are checking the cached version of the state variables, which possibly 
leads to making wrong decisions?

See:

ieee80211_newstate_cb():

        IEEE80211_LOCK(ic);
...
        nstate = vap->iv_nstate;
...
        ostate = vap->iv_state;

...

rc = vap->iv_newstate(vap, nstate, arg);

iv_newstate can drop the IC lock, and so the state can change!

...

If the state transitions are coming too fast, which is the case, the taskqueue 
mechanism used by the IEE802.11 layer is going to loose callbacks, and even 
risk out of order execution, for the newstate function.

I'm using 9-current as of one week ago on this computer. The same problem has 
also been seen on 8-stable. And I'm using 2-core CPU.

> I think the single state thread is correct. The whole thing works on
> state transitions, you dont just set a state.

Try my patch, and see that warnings that are printed when you leave 
wpa_supplicant configured with a wrong password.

You should be able to reproduce, just run a test for some time and it will 
panic.

> 
> > Description of panics. I didn't have core dump enabled on this box, so
> > please bear over with the following hand-written notes:
> >
> > 1) A vap->iv_bss == NULL, inside ratectl task in RUM driver.
> >
> > 2) A memcpy() fails inside the iee80211...newstate_cb()
> >
> > 3) This and similar printouts are seen:
> >
> > wlan0: ieee80211_new_state_locked: pending AUTH -> ASSOC transition lost
> 
> Can you see if you can get a core dump, or at least a DDB trace and
> the output from `show vap <addr>`

That might take some time. I'm very busy nowadays building a new house. I will 
check e-mails later today. Try to reproduce using the steps I've given. Maybe 
also try to insert some DELAY() around newstate execution, hence this problem 
does not occur N/N times.

Another suggestion. Add some debug code to WLAN:

while (to--) {
	set random state();
	if (random() & 1)
		pause("WDLY", 1);
}

In rum/run/... newstate function, add:

	if (random() & 1)
		pause("WDLY", 1); // OR DELAY(125 * (random() & 7))

Aswell at the beginning and end.

--HPS