Re: Lots of confusion on bss refcounting.

[Ben Greear <greearb@candelatech.com>]

On 06/17/2013 12:02 PM, Johannes Berg wrote:
> On Mon, 2013-06-17 at 11:49 -0700, Ben Greear wrote:
>
>> We create an assoc_data, assign a bss pointer in ieee80211_mgd_assoc,
>> but do not claim a reference.
>
> Heh, yeah, I was actually looking at this code too but didn't have time
> today to finish my thoughts ...
>
>> Later, when deleting the assoc_data, the ref is not freed either,
>> except in one error path where it is explicitly freed:
>>
>> 		if (!ieee80211_assoc_success(sdata, *bss, mgmt, len)) {
>> 			/* oops -- internal error -- send timeout for now */
>> 			ieee80211_destroy_assoc_data(sdata, false);
>> 			cfg80211_put_bss(sdata->local->hw.wiphy, *bss);
>> 			return RX_MGMT_CFG80211_ASSOC_TIMEOUT;
>> 		}
>>
>> This seems ripe for bugs, if not already buggy.
>>
>> Maybe we should be more explicit about always grabbing a ref when
>> we take a reference to the pointer, and always put it when we
>> destroy the pointer?
>
> I think the reference is actually given to mac80211 by cfg80211 in
> cfg80211_mlme_assoc(), so we shouldn't need to grab a reference?
> Although I'm certainly willing to change this and make cfg80211 always
> put the reference after calling rdev_assoc() so that the driver/mac80211
> would be responsible for obtaining its own if it needs to hang on to the
> struct.
>
> This does seem broken though.

The bss reference is passed back, and through luck or careful programming,
it *seems* that all paths related to calling ieee80211_rx_mgmt_assoc_resp
managed to consume the bss.

I haven't figured out yet why this is not an erroneous put since I didn't
find the reference taken in the first place.

I'm going to work on making some changes to the ref counting scheme
a bit.  I'd rather have the code perhaps take and put a few refs
it might otherwise skip to keep the ownership cleaner and make
the code easier to debug and understand...

I'll post some for RFC when I make some progress.

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com