Re: [RFC PATCH] cfg80211: fix duplicated scan entries after channel switch

[Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>]

...

> > To summarize, if BSS channel needs to be updated, then bss_tree should
> > be rebuilt in order to put updated BSS entry into a proper location.
> 
> Good catch!
> 
> > This commit suggests the following straightforward solution:
> > - if new entry has been already created for BSS after channel switch,
> >   then remove it completely
> 
> Shouldn't we prefer the new entry?
> 
> OTOH, the old entry will likely have a "hold", so it doesn't get removed
> while we're connected ... and the driver etc. might be referencing it.
> So I guess the old entry should be updated with info from the newer one?
> 
> > Finally, next scan operation will find BSS entry in expected location
> > in rb_tree. So all the IEs, including HT/VHT operation IEs,
> > will be properly updated.
> 
> Right. Although if it was there before, then it already has been updated
> in a sense... But I guess it's a corner case to even get there?
> 
> > 1. Tested using iwlwifi and qtnfmac drivers, looks good
> 
> Great.
> 
> > 2. Alternative approach: remove old BSS entry and keep new a one
> > This approach may have certain benefits for mac80211 drivers.
> > For instance, in this case HT/VHT operation IEs are going to be
> > valid from the start, no need to wait for the next scan.
> 
> > However the following procedure for replacing current_bss, protected
> > by wdev->mtx and rdev->bss_lock locks, seems to be insufficient:
> > 
> >   bss_ref_get(rdev, new);
> >   cfg80211_hold_bss(new);
> >   wdev->current_bss = new;
> > 
> >   cfg80211_unhold_bss(old);
> >   bss_ref_put(rdev, old);
> >   __cfg80211_unlink_bss(rdev, old);
> > 
> > When testing this alternative approach using iwlwifi driver,
> > occasional general protection fault crashes have been observed
> > on ieee80211_rx_mgmt_beacon/ieee80211_bss_info_update code paths.
> > So far I haven't yet root caused them.
> 
> At the very least you'd also have to update ifmgd->associated in
> mac80211, and that's basically not really possible? Well, I guess we
> could change the channel switch API to return the new one or something.
> 
> I guess the better thing would be to go update the old entry with the
> new one's data, before killing the new one.
> 
> Not sure it's worth the extra complexity though.

Hello Johannes,

Thanks for review! Summarizing your comments, here are the options
and some of their highlights:

1. replace old entry with the new entry
   - no easy way to update ifmgd->associated w/o rework of mac80211 csa

2. keep old entry, remove new entry
   - this is what suggested RFC patch does
   - works, but update of current_bss entry after csa may be delayed

3. keep old entry, update it using data from new entry, then remove new entry
   - this looks like a better approach

Unless I am missing something, the last option can be implemented on top of
the current RFC patch w/o extra complexity. The required bss entry update is
what cfg80211_bss_update function does when bss in question already exists.
So it should be possible to reuse that code.

I will post RFC patch v2 after more testing for both mac80211 and
fullmac cases.

Regards,
Sergey