Re: [PATCH] mac80211: Allow sleeping in set_key op

[Michael Buesch <mb@bu3sch.de>]

On Monday 07 May 2007 18:52:24 Jiri Benc wrote:
> On Sun, 6 May 2007 20:37:34 +0200 Michael Buesch wrote:
> > [...]
> >  static void finish_sta_info_free(struct ieee80211_local *local,
> >  				 struct sta_info *sta)
> >  {
> > +	sta_info_key_disable(local, sta);
> > +
> >  #ifdef CONFIG_MAC80211_VERBOSE_DEBUG
> >  	printk(KERN_DEBUG "%s: Removed STA " MAC_FMT "\n",
> >  	       local->mdev->name, MAC_ARG(sta->addr));
> > @@ -213,6 +246,16 @@ static void finish_sta_info_free(struct 
> >  	sta_info_put(sta);
> >  }
> 
> There is a race here. You already removed the sta from sta_hash list
> and you're not protected by any lock. Thus, it is possible to add a new
> station with the same address before finish_sta_info_free is called.
> When this happens, you call the set_key handler for the new key and
> after that you call it again with DISABLE_KEY.
> 
> It's not easy to get this right. I remember also problems with
> dereferencing already freed key when I thought about possible ways to
> solve exactly this problem.

I'm not sure this race exists.
You assume that when a new key is added with the same mac as before
it overrides the old key. I think that's a bug in the driver then.
IMO the driver _must_ keep track of used key "slots" and don't
re-allocate them for new keys until disable-key is called.
And that's exactly what bcm43xx does.

IMO it's a bug in the driver, when it overrides a key that's not
DISABLEd.

Short: I don't think there is a race, and if, then it's a driver bug.


This is one of the showstopper bugs for bcm43xx to get merged.
(There are others, though).

-- 
Greetings Michael.