Lots of confusion on bss refcounting.

DISCUSSION Ben Greear 2 participants 10 messages Jun 17, 2013 – Jun 18, 2013

lore

Share & tools

lore

Ben Greear

author

2013-06-17 18:49 UTC #0 lore raw

More on looking for bss and ies leaks...

I am trying to understand the bss refcounting, but everywhere I look it seems like the code is weird at best.

For instance:

We create an assoc_data, assign a bss pointer in ieee80211_mgd_assoc, but do not claim a reference.

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'll be happy to cook up some patches if this seems like the right path to take.

Thanks,
Ben

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Johannes Berg

2013-06-17 19:03 UTC #1 lore raw

Quoted 2 lines · Ben Greear

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 ...

Quoted 10 lines

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;
		}

Quoted 5 lines

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.

johannes

Ben Greear

author

2013-06-17 19:09 UTC #2 lore raw

Quoted 24 lines · Johannes Berg

Quoted 2 lines · Ben Greear
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 ...
Quoted 10 lines
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;
		}
Quoted 5 lines
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?

Quoted 8 lines

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

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Ben Greear

author

2013-06-17 21:31 UTC #3 lore raw

Quoted 9 lines · Ben Greear

On 06/17/2013 12:02 PM, Johannes Berg wrote:
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.

Quoted 6 lines

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.

I think I found at least some of the leaks.

In places like ieee80211_mgd_stop, we were calling ieee80211_destroy_assoc_data, but it was not putting the bss reference.

I'll post some RFC patches in a minute or two...first is debugging logic, second attempts to fix bss ref counting. This needs more testing before it is applied...we will continue testing it....

Thanks,
Ben

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Ben Greear

author

2013-06-18 00:30 UTC #4 lore raw

Quoted 17 lines · Ben Greear

Quoted 9 lines · Ben Greear
On 06/17/2013 12:02 PM, Johannes Berg wrote:
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.
Quoted 6 lines
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.

Quoted 8 lines

I think I found at least some of the leaks.
In places like ieee80211_mgd_stop, we were calling ieee80211_destroy_assoc_data, but it was not putting the bss reference.
I'll post some RFC patches in a minute or two...first is debugging logic, second attempts to fix bss ref counting. This needs more testing before it is applied...we will continue testing it....

It seems that the wdev objects (struct wireless_dev) can also hold a reference to the bss.

Do you happen to know what code is responsible for destructing those objects? I want to check to make sure it properly puts its reference.

Even after the patches posted I still see a few leaked bss objects...

Thanks,
Ben

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Johannes Berg

2013-06-18 12:49 UTC #5 lore raw

Quoted 27 lines · Ben Greear

Quoted 17 lines · Ben Greear
Quoted 9 lines · Ben Greear
On 06/17/2013 12:02 PM, Johannes Berg wrote:
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.
Quoted 6 lines
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.
Quoted 8 lines
I think I found at least some of the leaks.
In places like ieee80211_mgd_stop, we were calling ieee80211_destroy_assoc_data, but it was not putting the bss reference.
I'll post some RFC patches in a minute or two...first is debugging logic, second attempts to fix bss ref counting. This needs more testing before it is applied...we will continue testing it....

Quoted 6 lines

It seems that the wdev objects (struct wireless_dev) can also hold a reference to the bss.
Do you happen to know what code is responsible for destructing those objects? I want to check to make sure it properly puts its reference.

You mean ->current_bss? That should be handled in all the callbacks in sme.c or so

johannes

Ben Greear

author

2013-06-18 15:47 UTC #6 lore raw

Quoted 31 lines · Johannes Berg

Quoted 30 lines · Ben Greear
Quoted 17 lines · Ben Greear
Quoted 9 lines · Ben Greear
On 06/17/2013 12:02 PM, Johannes Berg wrote:
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.
Quoted 6 lines
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.
Quoted 8 lines
I think I found at least some of the leaks.
In places like ieee80211_mgd_stop, we were calling ieee80211_destroy_assoc_data, but it was not putting the bss reference.
I'll post some RFC patches in a minute or two...first is debugging logic, second attempts to fix bss ref counting. This needs more testing before it is applied...we will continue testing it....
It seems that the wdev objects (struct wireless_dev) can also hold a reference to the bss.

Quoted 6 lines

Quoted 3 lines
Do you happen to know what code is responsible for destructing those objects? I want to check to make sure it properly puts its reference.
You mean ->current_bss? That should be handled in all the callbacks in sme.c or so

Looks like much of the action happens on work-queues. I'm wondering if we managed to delete wdev objects before we have completely cleaned up in some cases...

I was planning to add code to explicitly clean up current_bss if it is not NULL in whatever code actually does the final cleanup for wdev objects.

I didn't see an obvious cleanup method in sme.c, but will go look around some more...

Thanks,
Ben

Quoted 3 lines

johannes

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Johannes Berg

2013-06-18 15:52 UTC #7 lore raw

Quoted 7 lines

Quoted 3 lines
You mean ->current_bss? That should be handled in all the callbacks in sme.c or so
Looks like much of the action happens on work-queues. I'm wondering if we managed to delete wdev objects before we have completely cleaned up in some cases...

Don't we flush work structs appropriately?

johannes

Ben Greear

author

2013-06-18 15:59 UTC #8 lore raw

Quoted 9 lines · Johannes Berg

Quoted 7 lines
Quoted 3 lines
You mean ->current_bss? That should be handled in all the callbacks in sme.c or so
Looks like much of the action happens on work-queues. I'm wondering if we managed to delete wdev objects before we have completely cleaned up in some cases...
Don't we flush work structs appropriately?

Looks like it, from core.c in the netdev event handler:

		/*
		 * Ensure that all events have been processed and
		 * freed.
		 */
		cfg80211_process_wdev_events(wdev);


         /* I just added this to see if it helps... */
		if (WARN_ON(wdev->current_bss)) {
			cfg80211_unhold_bss(wdev->current_bss);
			cfg80211_put_bss(wdev->wiphy, &wdev->current_bss->pub);
			SET_BSS(wdev, NULL);
		}
		break;

Some of the unregister and similar sme.c calls that should be cleaning up the current_bss have some early returns if state does not match expected value. If the warning above hits, then probably we are hitting those somehow.

If not, then I'll keep looking :)

Thanks,
Ben

Quoted 3 lines

johannes

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Ben Greear

author

2013-06-18 21:36 UTC #9 lore raw

In reply to Johannes Berg

Quoted 9 lines · Johannes Berg

Quoted 7 lines
Quoted 3 lines
You mean ->current_bss? That should be handled in all the callbacks in sme.c or so
Looks like much of the action happens on work-queues. I'm wondering if we managed to delete wdev objects before we have completely cleaned up in some cases...
Don't we flush work structs appropriately?

I'm still seeing leaks after all those patches I posted...so I'm still looking. The leaked bss objects have a quite large refcount, in the hundreds after an hour or so of running, so this is probably more than a strange race somewhere.

This code looks questionable in wireless/mlme.c

int __cfg80211_mlme_assoc()
.....

It grabs a reference using cfg80211_get_bss, but it only does a put on the reference if there was an error code.

The __cfg80211_mlme_auth a bit above always does a put on the reference.

I'm thinking mlme_assoc should also always do put. Any reason you can think of otherwise?

Thanks,
Ben

Quoted 3 lines

johannes

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com