Re: [PATCH] cfg80211: Fix race in bss timeout

[Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>]

On Wed, Dec 07, 2011 at 04:17:50PM +0100, Johannes Berg wrote:
> On Wed, 2011-12-07 at 20:41 +0530, Vasanthakumar Thiagarajan wrote:
> 
> > +	spin_lock_bh(&dev->bss_lock);
> > +	memcpy(bssid, bss->bssid, ETH_ALEN);
> > +	spin_unlock_bh(&dev->bss_lock);
> 
> I don't think this is necessary.

Right, i don't see any race either.

> 
> >  	nl80211_send_roamed(wiphy_to_dev(wdev->wiphy), wdev->netdev, bssid,
> >  			    req_ie, req_ie_len, resp_ie, resp_ie_len,
> >  			    GFP_KERNEL);
> > @@ -615,40 +612,65 @@ void __cfg80211_roamed(struct wireless_dev *wdev,
> >  	wdev->wext.prev_bssid_valid = true;
> >  	wireless_send_event(wdev->netdev, SIOCGIWAP, &wrqu, NULL);
> >  #endif
> > +
> > +	return;
> > +out:
> > +	if (bss)
> > +		cfg80211_put_bss(bss);
> >  }
> 
> Doesn't that leak the reference if you return? It'll also give you an
> smatch warning since the function assumes the "bss" pointer that was
> passed in is not NULL, no?

Oops, sorry, i'll fix it. I may need to run smatch as well.

> 
> > +static void cfg80211_roamed_bss(struct net_device *dev,
> > +				struct cfg80211_bss *bss, const u8 *req_ie,
> > +				size_t req_ie_len, const u8 *resp_ie,
> > +				size_t resp_ie_len, gfp_t gfp)
> >  {
> >  	struct wireless_dev *wdev = dev->ieee80211_ptr;
> >  	struct cfg80211_registered_device *rdev = wiphy_to_dev(wdev->wiphy);
> >  	struct cfg80211_event *ev;
> >  	unsigned long flags;
> >  
> > -	CFG80211_DEV_WARN_ON(wdev->sme_state != CFG80211_SME_CONNECTED);
> >  
> >  	ev = kzalloc(sizeof(*ev) + req_ie_len + resp_ie_len, gfp);
> 
> Why remove that warning? Also maybe do something like
> 
> 	if (WARN_ON(!bss))
> 		return;
> 
> (before allocating memory)

These warnings are added in cfg80211_roamed().

Thanks!

Vasanth